error printer: excerpt the frame picked for the code frame, not frame 0, for sources without a source map - #38356
Conversation
remap_zig_exception picks the first frame that is in one of the user's sources (skipping JS builtins and bun's own modules) and shows that frame's line as the code frame. When that source has no source map (node:vm scripts, eval, new Function) the lines are collected by ZigException__collectSourceLines, which always excerpted frame 0. With a builtin on top of the stack this printed the builtin's own source text, e.g. Array.prototype.reduce's, above "reduce of empty array with no initial value". Pass the index of the picked frame through to C++ and collect the lines of that frame only. The other frames' positions were only being recomputed by that pass, which nothing needed.
|
Status: reproduced on 1.4.0 ( Fail-before / pass-after: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughSource-line collection now receives the selected stack-frame index. Native collection validates and resolves that frame. VM tests cover code frames from scripts, dynamic execution, built-in methods, events, and uncaught errors. ChangesException source-frame collection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Cross-reference from going over the open PRs in this area: #38324 rewrites the same collector ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the excerpt now comes from the same frame the printer already picked for the code frame's location.
What was reviewed:
top as u8cast:topindexes intoframes_buf[..frames_len]whereframes_lenfits inu8(Holder::FRAME_COUNT = 32), and C++ bounds-checks it againsttrace.frames_lenbefore use.- Dropped
OnlySourceLinesloop over non-top frames: it only re-ranpopulateStackFramePositionwithnullptrsource-line buffers, which memcpy'd the same position bytes theOnlyPositionpass had already stored — nothing consumed the rewrite. referenced_source_providerref balance: still exactly oneprovider->ref()percollect_source_linescall (only the picked frame reaches the ref path, same as before when onlyis_topdid), released inZigException::deinit.- FFI signature: Rust extern declaration, both call sites, and the C++ definition all take the new
uint8_t frame_index; the defaultsource_lines_frame_index = 0onpopulateStackTracekeeps theOnlyPositioncallers (fromErrorInstance,JSC__Exception__getStackTrace) unchanged.
Extended reasoning...
Overview
This PR fixes the code-frame excerpt shown above errors thrown inside a JS builtin (or a bun node:* module) when called from a source without a source map (node:vm scripts, eval, new Function). remap_zig_exception already picks the first user frame (top) and uses its source_url/position for the code frame's label; the fix threads top through to ZigException__collectSourceLines → populateStackTrace(OnlySourceLines) so the excerpted text comes from that same frame's SourceProvider instead of unconditionally from frames[0]. The now-redundant loop over the remaining frames (which only rewrote positions with values the OnlyPosition pass had already stored) and the now-constant is_top parameter are removed. Six new tests in test/js/node/vm/vm.test.ts cover vm.Script, runInNewContext, eval, new Function, a node:events frame on top, and the uncaught-error path via Bun.spawn.
Security risks
None. This is presentation-only (error-printer output). No new inputs are parsed, no allocation sizes derive from untrusted data, and the new frame_index is a scalar produced by Rust from an in-bounds enumeration and re-bounds-checked in C++ (>= trace.frames_len and jsc_stack_frame_index < 0 || >= frames.size() both early-return). The SourceProvider ref taken for the excerpt is the same one that was taken before, just for a different frame, and is released in ZigException::deinit as before.
Level of scrutiny
Medium. It touches the JSC↔Rust FFI boundary and drops a loop, so the review checked (a) the FFI signature is consistent on both sides, (b) the cast top as u8 cannot truncate (top indexes a frames_len: u8-bounded slice, capped at 32), (c) the dropped loop had no side effect anyone consumed — it re-memcpy'd positions from the same JSC frame the first pass had already stored, and (d) the referenced_source_provider ref/deref balance is unchanged (exactly one ref per call, still released in deinit). All check out. The behavioral surface is limited to which line of text appears in an error message.
Other factors
The tests are well-constructed: they call the builtin from line 2 so both the excerpt text and its line-number label must come from the user frame; they assert against Bun.inspect and the uncaught-error stderr; the subprocess test drains stdout/stderr/exited concurrently and asserts exitCode last. The PR description documents fail-before/pass-after with USE_SYSTEM_BUN=1 and an ASAN RSS check over 30k iterations. The caret-column question is explicitly out of scope (handled by #38335/#38349) and the tests only assert a caret line exists, not its column, so they won't conflict.
|
Updated 2:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 86c92d2 has some failures in 🧪 To try this PR locally: bunx bun-pr 38356That installs a local version of the PR into your bun-38356 --bun |
1 participant
Problem
node:*modules) that was called from a source bun did not transpile (anode:vmscript,eval,new Function), the code frame above the error shows the builtin's own source instead of the user's line. Affects the uncaught-error output,Bun.inspect(err)andconsole.error(err). Reproduces on 1.4.0 and on main:Array.prototype.reducebuiltin; withemitter.emit("error")and no listener it prints bun'snode:eventssource). Expected, and what this PR prints:1 | [].reduce((a, b) => a).remap_zig_exception(src/jsc/VirtualMachine.rs, thetoploop around line 5599) picks the first frame that is in a user source, skipping builtin andbun:/node:frames. When the source map lookup for that frame finds nothing it callsexception.collect_source_lines(), andpopulateStackTraceinsrc/jsc/bindings/ZigException.cpp(theOnlySourceLinesbranch, line 448 on main) excerptedframes[0]unconditionally (is_top = i == 0); it was never told which frame had been picked (Fix segmentation fault during building stack traces string #22902 split the two passes and kept frame 0 for this one).Fix
ZigException__collectSourceLinestakes the index of the picked frame; theOnlySourceLinespass excerpts that frame only. Both Rust call sites passtop.topis already the frame whosesource_urland position are shown as the code frame's location; the excerpt now comes from the same frame, which is what the transpiled-file path does when it readsframes[top].source_url. When nothing is skippedtopis 0, so the common case is unchanged.populateStackFramePositionon every other frame, which only rewrote positions with the values the first pass had already stored; nothing consumed that, so the loop is gone together with the now-constantis_topparameter.collect_source_lines()calls, not which frame it reads. The tests here therefore check the excerpt line and only that a caret line follows it.SourceProvider;collectSourceLinesslices from it) but still slicesframes_ptr[0], so the bug in this PR survives it as-is. On top of worker errors: carry the parse error's location to the parent; let parse/resolve diagnostics cross structured clone #38324 this change reduces to passingtopthrough to itscollectSourceLinesand indexing with it; I will rebase this PR to that shape if worker errors: carry the parse error's location to the parent; let parse/resolve diagnostics cross structured clone #38324 lands first.test/js/node/vm/vm.test.ts("code frame of an error thrown inside a builtin called from a source without a source map":vm.Script,runInNewContext,eval,new Function, anode:eventsframe on top, and the uncaught-error output). All 6 fail on 1.4.0 with the builtin's text in the diff and pass with this change.test/js/node/vm/*.test.ts,test/js/bun/test/stack.test.ts,test/js/bun/util/reportError.test.tspass;test/js/bun/util/inspect-error.test.jshas two debug-build-only failures that reproduce without this change (tracked separately).Bun.inspect()calls on this shape of error hold steady in RSS under ASAN with a small quarantine (the source provider ref taken for the excerpt is released inZigException::deinitas before).Background
N | source textline plus^that bun prints above an error'sname: message. It is built in two steps.JSC__JSValue__toZigException(OnlyPositionpass) copies JSC's stack frames intoZigStackTrace.frames, recording each one's index into JSC's frame vector injsc_stack_frame_index.remap_zig_exceptionthen filters those frames, pickstop, and either rebuilds the lines itself from the original file (sources bun transpiled: it has them on disk or in the source map) or, when there is no source map for the frame, asks C++ to cut the lines out of theJSC::SourceProviderof that frame (OnlySourceLinespass). The second case is the only one that can see anode:vm/eval/new Functionsource, which exists nowhere but inside JSC.Array.prototype.reduceis itself JavaScript inside JSC, so it appears as a frame with an empty source URL and a position inside the builtin's text; that text is what was being excerpted. Bun'snode:*modules appear the same way with anode:URL.frame_indexindexesZigStackTrace.framesafter Rust's filtering; the filtering swaps entries but each entry keeps its ownjsc_stack_frame_index, so C++ still reaches the right JSC frame. Frames parsed out of anerror.stackstring carry-1there and get no excerpt, as before.