Error.prepareStackTrace: index source URLs by visible frame, not by JSC frame - #37450
Error.prepareStackTrace: index source URLs by visible frame, not by JSC frame#37450robobun wants to merge 4 commits into
Conversation
…SC frame computeErrorInfoWithPrepareStackTrace builds CallSites from JSCStackTrace::fromExisting, which skips private-visibility frames, but looked up each CallSite's source URL and global object in the unfiltered JSC::StackFrame vector at the same index. Whenever JSC kept a private frame in the vector (showPrivateScriptsInStackTraces, on by default in debug builds), every CallSite after it got the previous frame's source URL and was source-mapped against the wrong file. fromExisting now also returns the JSC::StackFrame behind each frame it kept, and the prepareStackTrace path reads from that list. Removes the unused JSCStackTrace::getStackTraceForThrownValue, the only other caller.
|
Status: reproduced on 1.4.0-canary.1 with |
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted index-alignment fix with a solid regression test.
What was reviewed
visibleFramespointer lifetime: points into caller-ownedstackFrames, which is not resized betweenfromExistingand the dereference loop; GC increateCallSitesFromFramesdoesn't touch the WTF::Vector storage.stackTrace.size() == visibleFrames.size()— both appended in lockstep, so*visibleFrames[i]is always valid fori < n.- Confirmed
fromExistinghas one caller (updated) andgetStackTraceForThrownValuehad none. - Test drains pipes concurrently, spreads
bunEnv, and asserts exact file/line for both env-var states.
Extended reasoning...
Overview
The PR fixes an off-by-one-list bug in computeErrorInfoWithPrepareStackTrace (src/jsc/bindings/FormatStackTraceForJS.cpp): CallSites were built from a filtered frame list (JSCStackTrace::fromExisting skips private-visibility frames) but their source URL and owning global object were then read from the unfiltered JSC::StackFrame vector at the same index. When showPrivateScriptsInStackTraces is on (debug builds default it on), the two lists diverge after the first private frame and every subsequent CallSite reports the wrong file. The fix has fromExisting also return a parallel vector of const JSC::StackFrame* pointing into the input vector at the kept indices, and the caller reads from that instead. getStackTraceForThrownValue (the only other fromExisting caller, itself unused) is deleted. A new test in capture-stack-trace.test.js runs a two-file bound-function + AsyncLocalStorage#run fixture under both env-var values and asserts identical CallSite file/line output.
Security risks
None. This is diagnostic output (CallSite getFileName()/getLineNumber()) and touches no auth, crypto, or untrusted-input parsing. No new allocations or refcounts.
Level of scrutiny
Moderate — C++ JSC bindings, but the change is mechanical index alignment with no new memory ownership, exception scopes, or GC interactions. The pointers stored in visibleFrames reference elements of the caller's Vector<StackFrame>& parameter, which is neither destroyed nor resized before the pointers are last dereferenced; the intervening createCallSitesFromFrames allocates JS objects but does not touch the stackFrames vector, and GC does not move WTF::Vector storage. The per-frame logic (Zig::sourceURL, callee-global-object lookup, sourcemap remap) is byte-identical to before — only which StackFrame it receives changed.
Other factors
- Verified via grep that
fromExistinghas exactly one caller (updated here) andgetStackTraceForThrownValuehas zero, so the deletion is safe and satisfies the "delete dead code in the same PR" rule. - The test follows harness conventions:
tempDir,{...bunEnv, ...}, concurrent pipe draining, exact-value assertions on structured output for bothBUN_JSC_showPrivateScriptsInStackTraces=0(pins release behavior) and=1(the previously-broken path). - No CODEOWNERS cover the touched files.
- PR description documents the root cause, the other stack-walking sites that were checked and why they're unaffected, and the
USE_SYSTEM_BUN=1failure /bun bdpass verification.
Pairs each CallSite with its JSC frame by construction rather than through a second index-aligned list. JSCStackFrame's StackVisitor constructor has had no callers since #19238, so it goes, along with the call-frame pointer only it populated (CallSite::getThis has returned undefined for every frame since then) and the now unused globalObject parameter of CallSite::finishCreation. fromExisting keeps its original signature.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
It built a JSCStackTrace over a vector owned by a JSC::Exception or ErrorInstance, which the frames now point into, and it has no callers.
|
Updated 5:34 AM PT - Aug 11th, 2026
✅ @robobun, your commit ba050a44eae7c6974af0b92348aa9133867c9c8e passed in 🧪 To try this PR locally: bunx bun-pr 37450That installs a local version of the PR into your bun-37450 --bun |
There was a problem hiding this comment.
I reviewed this PR and found no bugs; the earlier getStackTraceForThrownValue finding was addressed in ba050a4. Because it adds a raw-pointer field to JSCStackFrame and reworks the CallSite/StackFrame pairing in JSC bindings, a human look would still be worthwhile.
What was reviewed:
m_stackFramelifetime: solefromExistingcaller keepsstackFramesalive and unmutated across the loop;JSCStackTraceis stack-local andCallSitecopies data out rather than storing the pointer.- Dead-code removal: repo-wide grep confirms zero remaining references to
callFrame(), theStackVisitorctor, andgetStackTraceForThrownValue;isVisibleBuiltinFunctionis still used by the surviving ctor. CallSite::finishCreation: dropping thecallFrame->thisValue()branch is behavior-preserving sincem_callFramewas always null on the only remaining construction path.- Test: asserts exact file/line for named frames under both
BUN_JSC_showPrivateScriptsInStackTracesvalues, so the release-default path is pinned and the assertion cannot pass vacuously.
Extended reasoning...
Overview
The PR fixes an off-by-one indexing bug in computeErrorInfoWithPrepareStackTrace (FormatStackTraceForJS.cpp): it built CallSite objects from the private-frame-filtered JSCStackTrace but then read source URLs and per-frame global objects from the unfiltered Vector<JSC::StackFrame> at the same index. When showPrivateScriptsInStackTraces is on (all debug builds), a bound-function or ALS run frame in the raw vector shifted every subsequent CallSite onto the wrong file. The fix stores a const JSC::StackFrame* on each JSCStackFrame and reads through it, so the pairing is by construction. It also deletes dead code that the change made riskier to keep: the StackVisitor constructor, the m_callFrame field / callFrame() accessor, the now-always-null thisValue branch in CallSite::finishCreation (plus its unused globalObject param), and getStackTraceForThrownValue.
Security risks
None identified. This is error-stack formatting; no untrusted-input parsing, auth, or crypto. The new raw pointer is into a caller-owned WTF::Vector that is not GC-managed and is not mutated between construction and use, so there is no new UAF surface for the sole call site.
Level of scrutiny
High. This is C++ in src/jsc/bindings/ on a path that runs for every Error.prepareStackTrace invocation, and it introduces a raw-pointer field whose safety depends on the caller keeping the source vector alive. I traced the sole fromExisting caller and its two upstream entry points (errorInstanceLazyStackCustomGetter moves the vector into a local unique_ptr before the call; errorConstructorFuncCaptureStackTrace uses a stack-local vector) — neither reallocates or frees during computeErrorInfoWithPrepareStackTrace, and the JSCStackTrace holding the pointers is itself stack-local to that function. The new stackFrame() accessor has exactly one call site, inside that same scope. Still, raw-pointer lifetime invariants in JSC bindings are exactly where a maintainer familiar with ErrorInstance/StackFrame ownership should confirm the reasoning.
Other factors
All prior review threads are resolved: comment-cop's two long-comment flags were shortened in 22e4045, and my earlier note that getStackTraceForThrownValue was still present was addressed in ba050a4 (verified gone by grep). The new test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, exact-value toEqual on a structured result, both env-var states covered). The getThis() behavior change is nominal — the removed branch was unreachable since #19238 left m_callFrame always null — and existing getThis/getFunction tests in the same file continue to pass per the description.
1 participant
Repro
main.cjs:callers.cjs:outerandmainreport the file of the frame above them ([native code]is the bound function call), and because the remap then runs against the wrong file,outerkeeps its generated line.<anonymous>only looks right becausemainlives in the same file.The same happens with
AsyncLocalStorage#run(store, boundFn):runreports[native code]and the frame below it reportsnode:async_hooks. That is how the bake dev server hits it: react-server-dom calls components throughcomponentStorage.run(..., callComponentInDEV), a bound function, and builds the error's.stackthrough its ownError.prepareStackTrace, so a debug-build dev server printsCause
computeErrorInfoWithPrepareStackTrace(src/jsc/bindings/FormatStackTraceForJS.cpp) builds the CallSites fromJSCStackTrace::fromExisting, which skips frames with private implementation visibility (a bound function call is one,VM::getBoundFunctionmarks it private), but it then looked up each CallSite's source URL and owning global object in the unfilteredJSC::StackFramevector at the same index. Once one private frame is in that vector, every CallSite after it is paired with the previous JSC frame.JSC itself only keeps private frames in the vector while
Options::showPrivateScriptsInStackTraces()is on, which Bun turns on in debug builds (ZigGlobalObject.cpp) and whichBUN_JSC_showPrivateScriptsInStackTraces=1turns on in release builds. With it off,fromExistingfilters nothing and the two lists happen to line up, which is why the release default is unaffected. The mismatch dates back to #5802.Fix
JSCStackFramenow keeps a pointer to theJSC::StackFrameit was built from, and the prepareStackTrace loop reads the JSC frame through the sameJSCStackFramethe CallSite came from, so the pairing holds by construction instead of through a second index-aligned list. The per-frame logic (Zig::sourceURL, the node:vm global object check, the remap) is unchanged, so frames that were already lined up produce exactly the same CallSites as before, and the CallSite list now comes out the same whether or not JSC kept the private frames, which is whatfromExisting's filtering is for.Making that pointer unconditional means removing
JSCStackFrame's other constructor, the one taking aStackVisitor, which has had no callers since #19238. The call-frame pointer only that constructor populated goes with it: it has been null for every frame since then, soCallSite#getThis()already returnsundefinedfor every frame (aJSC::StackFramecarries no receiver), andCallSite::finishCreationnow says so directly and drops theglobalObjectparameter it only needed for that path. No behavior changes there.JSCStackTrace::getStackTraceForThrownValue, the only other user offromExisting, is deleted as well: it had no callers, and it built a trace over a vector owned by aJSC::ExceptionorErrorInstance, which the frames would now point into.The other code that walks these vectors (
formatStackTracefor the default.stackstring,populateStackTraceinZigException.cpp, andgetFramesForCaller, which filters the vector in place before it gets here) each works on one list, so this was the only site pairing two of them.Verification
New test in
test/js/node/v8/capture-stack-trace.test.js: a two-file fixture covering both the plain bound call andAsyncLocalStorage#runwith a bound callback, run withBUN_JSC_showPrivateScriptsInStackTracesset to0and to1, asserting both runs report the same file and line for every named CallSite. Before the fix the1run (and therefore any debug build) reportsouterat[native code],mainincallers.cjs,runat[native code]andviaAsyncLocalStorageinnode:async_hooks; the0run pins the release behavior that must not change.Also passing on the debug build:
test/js/bun/sourcemap,test/js/bun/util/reportError.test.ts,test/js/node/vm/vm.test.ts,test/js/node/util/util.test.js,test/js/web/workers/structured-clone.test.ts,test/regression/issue/{013880,fix-bindings-stack-trace,prepare-stack-trace-crash}.test.ts, and thenode:testprepareStackTrace / getCallSites / shadow realm parallel tests (these covergetThis/getFunctionon strict and sloppy frames).test/js/bun/util/inspect-error.test.js"Error inside minified file" fails on debug builds with and without this change (the privaterequireframe shows up in the default.stackpath, which this PR does not touch).Two nearby issues seen while reproducing are already owned by open PRs and are left alone here: #37388 (
new Error()minified toError()becomes a tail call in strict code and drops the creating frame) and #36602 (printing an error whose.stackwas already materialized source-maps the non-top frames a second time, #15859).[review] gate passed · iteration 0 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file