Replace direct GCOwnedDataScope member access with explicit String copy in ErrorStackTrace
#22730
Conversation
WalkthroughStandardized string handling in ErrorStackTrace.cpp by returning/assigning String objects instead of raw data pointers for function name retrieval via "name" and "displayName" properties. Control flow and isEmpty() checks remain unchanged. No exported/public API changes. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bun.js/bindings/ErrorStackTrace.cpp (1)
6-7: Include root.h first to satisfy bindings lints.Bindings .cpp files should include
root.hat the top.Apply:
+#include "root.h" #include "config.h"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/bindings/ErrorStackTrace.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/ErrorStackTrace.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/ErrorStackTrace.cpp
**/*.{cpp,cc,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format C/C++ sources/headers with clang-format (bun run clang-format)
Files:
src/bun.js/bindings/ErrorStackTrace.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: For JS classes with a public constructor, implement Foo, FooPrototype, and FooConstructor (JSC patterns)
Define class properties using HashTableValue arrays in JSC bindings
Add iso subspaces for classes with C++ fields in JSC bindings
Cache structures in ZigGlobalObject for JSC-bound classes
Files:
src/bun.js/bindings/ErrorStackTrace.cpp
🧠 Learnings (1)
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add new V8 API mangled symbols (without leading underscore) to src/symbols.txt
Applied to files:
src/bun.js/bindings/ErrorStackTrace.cpp
🔇 Additional comments (2)
src/bun.js/bindings/ErrorStackTrace.cpp (2)
695-701: Solid fix: copy out of GCOwnedDataScope (name path).Returning
strensures a realStringcopy and avoids use-after-free fromstr.data. LGTM.
712-719: Consistent lifetime fix (displayName path).Assigning
functionName = strcorrectly copies; prevents dangling access. LGTM.
There was a problem hiding this comment.
While we probably shouldn't access str.data directly, it will end up calling String(const String&) which increments the reference count for the underlying StringImpl, which means this PR ends up making no changes.
I think the next step here is to figure out a way to reproduce the crash
What does this PR do?
This change fixes direct access to the data member of GCOwnedDataScope<const String&> in the functionName
function within ErrorStackTrace.cpp. The code now uses implicit conversion through GCOwnedDataScope's operator
const T() instead of direct member access.
The segmentation fault occurs in StringImpl::destroy during reference count operations when processing stack
traces. The suspected issue is that direct access to str.data creates a potential use-after-free condition where
the String copy constructor may operate on memory from a GCOwnedDataScope that has already been destroyed.
This change follows the same pattern used elsewhere in JSC codebase, such as getCalculatedDisplayName
How did you verify your code works?
The is issue flaky, so I wasn't able to create reproduction...