Repository navigation
Fix bug causing truncated stack traces - #22624
Conversation
|
Updated 3:24 AM PT - Sep 13th, 2025
❌ @Jarred-Sumner, your commit e7702e2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22624That installs a local version of the PR into your bun-22624 --bun |
WalkthroughAdds a frames_cap field to ZigStackTrace and initializes ZigException to track zero used frames with a separate capacity. Refactors C++ stack-trace assembly to use reference-based helpers and to bound population by frames_cap. Adds a test ensuring stack frames are preserved when inspecting errors. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests
Comment Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/ZigStackTrace.zig (1)
90-92: framesMutable should slice by capacity, not current lengthUsing frames_len here prevents writers from using the reserved capacity and can cause silent truncation when filling frames. Slice to frames_cap instead.
- pub fn framesMutable(this: *ZigStackTrace) []ZigStackFrame { - return this.frames_ptr[0..this.frames_len]; - } + pub fn framesMutable(this: *ZigStackTrace) []ZigStackFrame { + return this.frames_ptr[0..this.frames_cap]; + }
🧹 Nitpick comments (3)
src/bun.js/bindings/ZigStackTrace.zig (1)
25-31: Unify initialization semantics of frames_len with ZigExceptionHere frames_len is set to min(len, 255), while ZigException initializes frames_len = 0 and uses frames_cap to bound writes. Consider making fromFrames consistent (start “used” length at 0) unless callers depend on pre-filled length.
- .frames_len = @min(frames_slice.len, std.math.maxInt(u8)), + .frames_len = 0, .frames_cap = @min(frames_slice.len, std.math.maxInt(u8)),If there are call sites relying on non-zero frames_len from fromFrames, keep as-is; otherwise this change prevents accidental reads of uninitialized frames. Do you want me to scan the repo for fromFrames usage to confirm?
src/bun.js/bindings/headers-handwritten.h (1)
195-196: Guard the ABI: assert 1-byte fields for frames_len/frames_capAdd compile-time checks to ensure both remain uint8-sized and catch accidental type regressions.
typedef struct ZigStackTrace { BunString* source_lines_ptr; OrdinalNumber* source_lines_numbers; uint8_t source_lines_len; uint8_t source_lines_to_collect; ZigStackFrame* frames_ptr; uint8_t frames_len; uint8_t frames_cap; JSC::SourceProvider* referenced_source_provider; } ZigStackTrace; + +#ifdef __cplusplus +static_assert(sizeof(((ZigStackTrace*)0)->frames_len) == 1, "frames_len must be uint8_t"); +static_assert(sizeof(((ZigStackTrace*)0)->frames_cap) == 1, "frames_cap must be uint8_t"); +#endifsrc/bun.js/bindings/bindings.cpp (1)
4866-4915: Guard zero-capacity case in V8 stack parsing pathIf frames_cap is 0, the ASSERT would be compiled out in release and the write would still occur. Short-circuit when frame_count == 0.
- const uint8_t frame_count = except->stack.frames_cap; + const uint8_t frame_count = except->stack.frames_cap; + if (frame_count == 0) { + return; + }
📜 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 (4)
src/bun.js/bindings/ZigException.zig(1 hunks)src/bun.js/bindings/ZigStackTrace.zig(2 hunks)src/bun.js/bindings/bindings.cpp(12 hunks)src/bun.js/bindings/headers-handwritten.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/headers-handwritten.hsrc/bun.js/bindings/bindings.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/headers-handwritten.hsrc/bun.js/bindings/bindings.cpp
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/bindings/ZigStackTrace.zigsrc/bun.js/bindings/ZigException.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/bindings/ZigStackTrace.zigsrc/bun.js/bindings/ZigException.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/bindings/ZigStackTrace.zigsrc/bun.js/bindings/ZigException.zig
**/*.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/bindings.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/bindings.cpp
🧠 Learnings (13)
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/ZigStackTrace.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-02T17:14:46.924Z
Learnt from: taylordotfish
PR: oven-sh/bun#22227
File: src/safety/alloc.zig:93-95
Timestamp: 2025-09-02T17:14:46.924Z
Learning: In bun's Zig codebase, they use a custom extension of Zig that supports private field syntax with the `#` prefix (e.g., `#allocator`, `#trace`). This is not standard Zig syntax but is valid in their custom implementation. Fields prefixed with `#` are private fields that cannot be accessed from outside the defining struct.
Applied to files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/ZigStackTrace.zigsrc/bun.js/bindings/ZigException.zig
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/bindings.cpp
📚 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/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Applied to files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/ZigStackTrace.zigsrc/bun.js/bindings/ZigException.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JSC-bound classes
Applied to files:
src/bun.js/bindings/headers-handwritten.hsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/ZigException.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/ZigException.zigsrc/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/bindings.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/bindings.cpp (2)
src/bun.js/bindings/ErrorStackTrace.cpp (24)
functionName(391-398)functionName(391-391)functionName(589-608)functionName(589-589)functionName(610-666)functionName(610-610)functionName(668-810)functionName(668-668)sourceURL(382-389)sourceURL(382-382)sourceURL(490-497)sourceURL(490-490)sourceURL(499-517)sourceURL(499-499)sourceURL(519-522)sourceURL(519-519)sourceURL(524-532)sourceURL(524-524)sourceURL(534-541)sourceURL(534-534)sourceURL(543-554)sourceURL(543-543)sourceURL(556-572)sourceURL(556-556)src/bun.js/bindings/BunString.cpp (8)
toStringRef(210-225)toStringRef(210-210)toStringRef(249-256)toStringRef(249-249)toStringRef(257-264)toStringRef(257-257)toStringRef(265-273)toStringRef(265-265)
🔇 Additional comments (3)
src/bun.js/bindings/ZigStackTrace.zig (1)
12-13: ABI add: frames_cap looks good and matches u8 intentNo issues with placement after frames_len; aligns with the C header addition.
src/bun.js/bindings/ZigException.zig (1)
106-114: Init stack with len=0 and cap=frames.len — LGTMThis avoids reading uninitialized frames and lets the producer respect capacity.
src/bun.js/bindings/bindings.cpp (1)
4434-4529: Position population logic looks correctComputes positions safely (line/column or bytecode path), refs SourceProvider only when needed, and copies ZigStackFramePosition by value. No issues.
| static void populateStackTrace(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& frames, ZigStackTrace& trace, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags) | ||
| { | ||
| uint8_t frame_i = 0; | ||
| size_t stack_frame_i = 0; | ||
| const size_t total_frame_count = frames.size(); | ||
| const uint8_t frame_count = total_frame_count < trace->frames_len ? total_frame_count : trace->frames_len; | ||
| const uint8_t frame_count = total_frame_count < trace.frames_cap ? total_frame_count : trace.frames_cap; | ||
|
|
||
| while (frame_i < frame_count && stack_frame_i < total_frame_count) { | ||
| // Skip native frames | ||
| while (stack_frame_i < total_frame_count && !(&frames.at(stack_frame_i))->hasLineAndColumnInfo() && !(&frames.at(stack_frame_i))->isWasmFrame()) { | ||
| while (stack_frame_i < total_frame_count && !(frames.at(stack_frame_i).hasLineAndColumnInfo()) && !(frames.at(stack_frame_i).isWasmFrame())) { | ||
| stack_frame_i++; | ||
| } | ||
| if (stack_frame_i >= total_frame_count) | ||
| break; | ||
|
|
||
| ZigStackFrame* frame = &trace->frames_ptr[frame_i]; | ||
| populateStackFrame(vm, trace, &frames[stack_frame_i], frame, frame_i == 0, &trace->referenced_source_provider, globalObject, flags); | ||
| ZigStackFrame& frame = trace.frames_ptr[frame_i]; | ||
| populateStackFrame(vm, trace, frames[stack_frame_i], frame, frame_i == 0, &trace.referenced_source_provider, globalObject, flags); | ||
| stack_frame_i++; | ||
| frame_i++; | ||
| } | ||
| trace->frames_len = frame_i; | ||
| trace.frames_len = frame_i; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Zero-initialize each destination frame before populating to avoid stale BunString refs
populateStackTrace writes selected fields; without clearing, previous contents (function_name/source_url/code_type) may linger and leak or corrupt output when reusing the buffer.
static void populateStackTrace(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& frames, ZigStackTrace& trace, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags)
{
uint8_t frame_i = 0;
size_t stack_frame_i = 0;
const size_t total_frame_count = frames.size();
const uint8_t frame_count = total_frame_count < trace.frames_cap ? total_frame_count : trace.frames_cap;
while (frame_i < frame_count && stack_frame_i < total_frame_count) {
// Skip native frames
while (stack_frame_i < total_frame_count && !(frames.at(stack_frame_i).hasLineAndColumnInfo()) && !(frames.at(stack_frame_i).isWasmFrame())) {
stack_frame_i++;
}
if (stack_frame_i >= total_frame_count)
break;
- ZigStackFrame& frame = trace.frames_ptr[frame_i];
+ ZigStackFrame& frame = trace.frames_ptr[frame_i];
+ frame = {}; // clear stale refs/fields
populateStackFrame(vm, trace, frames[stack_frame_i], frame, frame_i == 0, &trace.referenced_source_provider, globalObject, flags);
stack_frame_i++;
frame_i++;
}
trace.frames_len = frame_i;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void populateStackTrace(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& frames, ZigStackTrace& trace, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags) | |
| { | |
| uint8_t frame_i = 0; | |
| size_t stack_frame_i = 0; | |
| const size_t total_frame_count = frames.size(); | |
| const uint8_t frame_count = total_frame_count < trace->frames_len ? total_frame_count : trace->frames_len; | |
| const uint8_t frame_count = total_frame_count < trace.frames_cap ? total_frame_count : trace.frames_cap; | |
| while (frame_i < frame_count && stack_frame_i < total_frame_count) { | |
| // Skip native frames | |
| while (stack_frame_i < total_frame_count && !(&frames.at(stack_frame_i))->hasLineAndColumnInfo() && !(&frames.at(stack_frame_i))->isWasmFrame()) { | |
| while (stack_frame_i < total_frame_count && !(frames.at(stack_frame_i).hasLineAndColumnInfo()) && !(frames.at(stack_frame_i).isWasmFrame())) { | |
| stack_frame_i++; | |
| } | |
| if (stack_frame_i >= total_frame_count) | |
| break; | |
| ZigStackFrame* frame = &trace->frames_ptr[frame_i]; | |
| populateStackFrame(vm, trace, &frames[stack_frame_i], frame, frame_i == 0, &trace->referenced_source_provider, globalObject, flags); | |
| ZigStackFrame& frame = trace.frames_ptr[frame_i]; | |
| populateStackFrame(vm, trace, frames[stack_frame_i], frame, frame_i == 0, &trace.referenced_source_provider, globalObject, flags); | |
| stack_frame_i++; | |
| frame_i++; | |
| } | |
| trace->frames_len = frame_i; | |
| trace.frames_len = frame_i; | |
| } | |
| static void populateStackTrace(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& frames, ZigStackTrace& trace, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags) | |
| { | |
| uint8_t frame_i = 0; | |
| size_t stack_frame_i = 0; | |
| const size_t total_frame_count = frames.size(); | |
| const uint8_t frame_count = total_frame_count < trace.frames_cap ? total_frame_count : trace.frames_cap; | |
| while (frame_i < frame_count && stack_frame_i < total_frame_count) { | |
| // Skip native frames | |
| while (stack_frame_i < total_frame_count && !(frames.at(stack_frame_i).hasLineAndColumnInfo()) && !(frames.at(stack_frame_i).isWasmFrame())) { | |
| stack_frame_i++; | |
| } | |
| if (stack_frame_i >= total_frame_count) | |
| break; | |
| ZigStackFrame& frame = trace.frames_ptr[frame_i]; | |
| frame = {}; // clear stale refs/fields | |
| populateStackFrame(vm, trace, frames[stack_frame_i], frame, frame_i == 0, &trace.referenced_source_provider, globalObject, flags); | |
| stack_frame_i++; | |
| frame_i++; | |
| } | |
| trace.frames_len = frame_i; | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/bindings/bindings.cpp around lines 4719 to 4741,
populateStackTrace currently writes only selected fields into trace.frames_ptr
entries which can leave stale BunString references (function_name, source_url,
code_type) from prior uses; before calling populateStackFrame for each
destination ZigStackFrame, zero-initialize or otherwise clear the entire
ZigStackFrame (e.g., memset the struct to 0 or explicitly reset its string
fields) to ensure no leftover data or dangling references remain, then proceed
to call populateStackFrame and update trace.frames_len as before.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/bun.js/bindings/bindings.cpp (1)
4719-4741: Add nullptr check and clear stale frame data before population.The refactored code now correctly uses reference types and bounds checking with
frames_cap. However, there are two important safety improvements needed:
- Validate that
trace.frames_ptris not null before dereferencing- Clear stale frame data to prevent leaking old BunString references
Apply this diff to add safety checks and clear stale data:
static void populateStackTrace(JSC::VM& vm, const WTF::Vector<JSC::StackFrame>& frames, ZigStackTrace& trace, JSC::JSGlobalObject* globalObject, PopulateStackTraceFlags flags) { + // Ensure frames_ptr is valid + if (!trace.frames_ptr) [[unlikely]] { + trace.frames_len = 0; + return; + } + uint8_t frame_i = 0; size_t stack_frame_i = 0; const size_t total_frame_count = frames.size(); const uint8_t frame_count = total_frame_count < trace.frames_cap ? total_frame_count : trace.frames_cap; while (frame_i < frame_count && stack_frame_i < total_frame_count) { // Skip native frames while (stack_frame_i < total_frame_count && !(frames.at(stack_frame_i).hasLineAndColumnInfo()) && !(frames.at(stack_frame_i).isWasmFrame())) { stack_frame_i++; } if (stack_frame_i >= total_frame_count) break; ZigStackFrame& frame = trace.frames_ptr[frame_i]; + // Clear stale frame data to prevent BunString reference leaks + frame = {}; populateStackFrame(vm, trace, frames[stack_frame_i], frame, frame_i == 0, &trace.referenced_source_provider, globalObject, flags); stack_frame_i++; frame_i++; } trace.frames_len = frame_i; }
🧹 Nitpick comments (1)
src/bun.js/bindings/bindings.cpp (1)
4434-4529: Consider adding bounds check for source lines array.The function correctly uses references and handles position data. However, when populating source lines, it should verify that
source_lines_countdoesn't exceed the actual capacity of the source lines buffer.Add a bounds check before the source lines population:
static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunString* source_lines, OrdinalNumber* source_line_numbers, uint8_t source_lines_count, ZigStackFramePosition& position, JSC::SourceProvider** referenced_source_provider, PopulateStackTraceFlags flags) { // ... existing code ... if (source_lines_count > 1 && source_lines != nullptr && sourceString.is8Bit()) { + // Ensure we don't overflow the source lines buffer + ASSERT(source_lines_count <= 5); // Or whatever the actual max is // Search for the beginning of the line unsigned int lineStart = location.byte_position; // ... rest of the function
📜 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 (2)
src/bun.js/bindings/bindings.cpp(22 hunks)test/cli/inspect/inspect.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/cli/inspect/inspect.test.ts
test/cli/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/cli/**/*.{js,ts}: Place CLI command tests (e.g., bun install, bun init) under test/cli/
When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Files:
test/cli/inspect/inspect.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/cli/inspect/inspect.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/cli/inspect/inspect.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/cli/inspect/inspect.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/cli/inspect/inspect.test.ts
**/*.{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/bindings.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/bindings.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/bindings.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/bindings.cpp
🧠 Learnings (21)
📚 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/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/cli/inspect/inspect.test.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, use normalizeBunSnapshot when asserting snapshots
Applied to files:
test/cli/inspect/inspect.test.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to test/**/*.test.{ts,tsx} : Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Applied to files:
test/cli/inspect/inspect.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errors
Applied to files:
test/cli/inspect/inspect.test.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Cache structures in ZigGlobalObject for JSC-bound classes
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS<ClassName> and re-export toJS/fromJS/fromJSDirect
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 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/napi/napi.zig : Add new V8 API method mangled symbols to the V8API struct in src/napi/napi.zig for both GCC/Clang and MSVC
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Add iso subspaces for classes with C++ fields in JSC bindings
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 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/bindings.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/**/js_*.zig : Handle reference counting correctly with ref()/deref() in JS-facing Zig code
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.{h,cpp} : If only a class (no public constructor/prototype), use JSC::LazyProperty<JSGlobalObject, Structure>, initialize it in finishCreation, and visit it in visitChildren
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.cpp : Initialize the LazyClassStructure in GlobalObject::finishCreation and visit it in GlobalObject::visitChildrenImpl
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : In finalize() for objects holding JS references, release them using .deref() before destroy
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to src/bun.js/bindings/ZigGlobalObject.h : When there is a class, prototype, and constructor, add a JSC::LazyClassStructure field for the class to ZigGlobalObject.h
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 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/**/*.{h,cpp} : Use JSC::WriteBarrier for heap-allocated references and implement visitChildren() for custom heap objects
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/bindings/bindings.cpp
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/bindings/bindings.cpp
🧬 Code graph analysis (1)
src/bun.js/bindings/bindings.cpp (2)
src/bun.js/bindings/ErrorStackTrace.cpp (24)
functionName(391-398)functionName(391-391)functionName(589-608)functionName(589-589)functionName(610-666)functionName(610-610)functionName(668-810)functionName(668-668)sourceURL(382-389)sourceURL(382-382)sourceURL(490-497)sourceURL(490-490)sourceURL(499-517)sourceURL(499-499)sourceURL(519-522)sourceURL(519-519)sourceURL(524-532)sourceURL(524-524)sourceURL(534-541)sourceURL(534-534)sourceURL(543-554)sourceURL(543-543)sourceURL(556-572)sourceURL(556-556)src/bun.js/bindings/BunString.cpp (8)
toStringRef(210-225)toStringRef(210-210)toStringRef(249-256)toStringRef(249-249)toStringRef(257-264)toStringRef(257-257)toStringRef(265-273)toStringRef(265-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (4)
test/cli/inspect/inspect.test.ts (1)
452-520: LGTM! Well-structured test for stack frame preservation.The test effectively validates that error.stack doesn't lose frames by comparing two scenarios - one accessing
e.stackand one not. The use of inline snapshots and normalization ensures consistent testing across platforms.src/bun.js/bindings/bindings.cpp (3)
4372-4432: LGTM! Clean refactoring to use references.The conversion from pointers to references in
populateStackFrameMetadataimproves type safety and readability. The dot notation access is more idiomatic C++.
4761-4977: LGTM! Comprehensive error instance handling with proper reference semantics.The refactored
fromErrorInstancefunction properly uses references throughout and correctly initializes the stack frames. The handling of both JSC stack traces and string-based stack traces is thorough.
4979-5104: LGTM! Proper fallback handling for non-ErrorInstance exceptions.The
exceptionFromStringfunction correctly handles edge cases where exceptions don't inherit from ErrorInstance, with proper initialization of the stack frames.
What does this PR do?
Fixes #21593 (very likely)
How did you verify your code works?
Regression test