Error.captureStackTrace: lazily compute .stack header on non-Error targets - #35640
Error.captureStackTrace: lazily compute .stack header on non-Error targets#35640robobun wants to merge 6 commits into
Conversation
…rgets When the target of Error.captureStackTrace is not a native JSC ErrorInstance (a plain object, or a function-based Error subclass whose prototype is Object.create(Error.prototype), as used by jsonwebtoken), Bun eagerly formatted the stack string at capture time with a hardcoded "Error" header, ignoring the target's own .name/.message. V8 installs a lazy accessor and reads name/message at first .stack access, so setting them after capture is observable. This makes the non-ErrorInstance path match the existing ErrorInstance path: build the sourcemapped CallSite array at capture time, stash it under a private name on the target, and install a lazy custom getter that reads name/message (via Error.prototype.toString's algorithm) and consults Error.prepareStackTrace at first access. Also fixes formatStackTraceToJSValue's header to read .name instead of hardcoding "Error: ", so the header inside prepareStackTrace callbacks is correct for any target. Fixes the .stack property for the JsonWebTokenError pattern in #13904.
|
Warning Review limit reached
Next review available in: 13 seconds 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 (7)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Updated 11:23 AM PT - Jul 25th, 2026
❌ @autofix-ci[bot], your commit 644df87 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35640That installs a local version of the PR into your bun-35640 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re the duplicate flags: #33437 and #34868 both fix the hardcoded This PR installs a lazy accessor instead, so the header and |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…e re-entry, proto-chain lookup, dedicated setter CallSite::formatAsString now emits "async " for await-chain frames, matching V8's CallSite.prototype.toString() and preserving the prefix that Bun::formatStackTrace already rendered on the pre-lazy path. The non-ErrorInstance lazy getter now clears the private CallSite slot before reading name/message so a getter that reads this.stack terminates at the !callSites guard instead of recursing to stack overflow, walks the prototype chain so Object.create(target).stack resolves, and is paired with a dedicated setter that releases the CallSite array when .stack is overwritten before first read.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
All four issues from my earlier review are addressed in 51978f9 (detach-before-reentry, async prefix in CallSite::formatAsString, proto-chain walk in the getter, dedicated setter that clears the private slot), each with a regression test, and the bug hunter found nothing new on this revision. Deferring to a human because this is ~200 lines of new JSC binding code on the error/stack-trace path — new CustomAccessor pair, private-name GC root on arbitrary targets, user-code re-entry ordering — and the comment-cop flags on the three multi-line comments are still open.
What was reviewed:
- Verified the
isInsideErrorPrepareStackTraceCallbackflag is reset beforeRETURN_IF_EXCEPTION, so a throwing name/message getter or prepareStackTrace doesn't leave it stuck. - Checked the new setter's receiver-based
putDirect—child.stack = xon an inheriting object shadows on the child and leaves the parent's lazy state intact, which matches V8's [[Set]] semantics. - Confirmed
m_nonErrorLazyStackCustomGetterSetteris registered inFOR_EACH_GLOBALOBJECT_GC_MEMBERso it's visited.
Extended reasoning...
Overview
The PR changes Error.captureStackTrace(target) for non-ErrorInstance targets from eagerly formatting .stack at capture time to installing a lazy CustomAccessor, so the header (.name/.message) and Error.prepareStackTrace are consulted at first access — matching V8 and fixing #16418 and the jsonwebtoken half of #13904. It touches FormatStackTraceForJS.cpp (new getter/setter, factored-out buildSourceMappedCallSitesArray, header now reads .name via Error.prototype.toString semantics), CallSite.cpp (emit async prefix), ZigGlobalObject.{h,cpp} (new lazy CustomGetterSetter property), BunBuiltinNames.h (capturedStackTrace private name), and adds seven tests to capture-stack-trace.test.js.
Security risks
None identified. The change only affects string formatting of stack traces. It does invoke user getters (.name/.message) and prepareStackTrace from native code, but that surface already existed on the ErrorInstance path and the eager path when prepareStackTrace was set; the re-entrancy guard (detach private slot before formatting) now prevents unbounded recursion.
Level of scrutiny
High. This is C++ in the JSC bindings on a path exercised by essentially every error in the runtime. It introduces a new GC-visible slot (a JSArray of CallSite objects rooted on arbitrary user objects via a private name), a new CustomAccessor pair, prototype-chain walking with getPrototypeDirect, and ordering constraints around user-code invocation. The REVIEW.md memory-safety section ("Anything that can run user JS can synchronously free your state") applies directly. The four issues my first pass found — re-entrancy stack overflow, dropped async prefix, retained CallSite array on setter, proto-chain miss — were all real regressions in the initial revision, which is a signal that this area is subtle enough to want maintainer eyes.
Other factors
- All four prior findings were fixed in 51978f9 and each has a dedicated regression test in the same file.
- The refactor of
computeErrorInfoWithPrepareStackTraceintobuildSourceMappedCallSitesArray+ wrapper is a mechanical extraction; I diffed the moved body and it's identical apart from the return point. - The new
LazyPropertyOfGlobalObject<CustomGetterSetter>is added toFOR_EACH_GLOBALOBJECT_GC_MEMBER, so it's visited — no missing write-barrier wiring. - The three open comment-cop flags (lines ~790/801/819) are on comments that document ordering invariants and the reason for the proto-chain walk; they read as legitimate invariant comments per REVIEW.md rather than narration, but that's a maintainer call.
- Two potentially-duplicating open PRs (#33437, #34868) take the eager-header approach; this PR supersedes them for the lazy-access cases and will conflict on
formatStackTraceToJSValue's header block.
The sentinel that breaks re-entry is restored to the CallSite array if user code (name/message getter, prepareStackTrace) throws during formatting, so a later .stack read retries instead of returning undefined. Matches Node, which re-invokes the getter on the next access. Also trim the multi-line comments flagged by comment-cop.
…Site array while the accessor is still installed The isNull sentinel now halts the prototype walk so a re-entrant read on a child cannot materialize an ancestor's capture. On the exception path, the CallSite array is restored only when .stack is still the custom accessor; if prepareStackTrace already replaced it with the default-formatted string, the array is released instead of being orphaned.
43b0e1a to
644df87
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/internal/inspector/cdp.ts:1-11— This PR also addssrc/js/internal/inspector/cdp.ts(702 lines) andsrc/jsc/bindings/BunDebugger.h, which are unrelated in-progressnode:inspectorwork — nothing in the tree importsinspector/cdpor#includesBunDebugger.h, and the fourjsFunction_*NodeInspector*host functions it declares have no definitions anywhere. Both files landed in commit 102103d whose message only mentions the proto-chain/sentinel fix, so this looks like an accidental inclusion; please drop them from this PR and land them with the inspector work.Extended reasoning...
What the issue is
The PR's stated scope is the lazy
.stackheader forError.captureStackTraceon non-Error targets — changes toFormatStackTraceForJS.{cpp,h},CallSite.cpp,ZigGlobalObject.{cpp,h},BunBuiltinNames.h, andcapture-stack-trace.test.js. However, the diff also adds two entirely unrelated new files:src/js/internal/inspector/cdp.ts(702 lines) — anInspectorCDPAdapterclass that translates between the V8 Chrome DevTools Protocol and JSC's inspector protocolsrc/jsc/bindings/BunDebugger.h(15 lines) — declares four host functions:jsFunction_openNodeInspector,jsFunction_waitForNodeInspectorConnection,jsFunction_postNodeInspectorControl,jsFunction_closeNodeInspector
Neither file has anything to do with
Error.captureStackTraceor the.stackheader.Step-by-step proof that these are dead / accidental
- Nothing references them.
rg 'InspectorCDPAdapter|inspector/cdp|jsFunction_openNodeInspector|jsFunction_waitForNodeInspectorConnection|jsFunction_postNodeInspectorControl|jsFunction_closeNodeInspector|BunDebugger\.h'over the whole tree matches only the two files themselves. No.cppfile#includesBunDebugger.h; no module importsinternal/inspector/cdp. - The declared host functions have no definitions.
BunDebugger.husesJSC_DECLARE_HOST_FUNCTIONfor four symbols, but there is no correspondingJSC_DEFINE_HOST_FUNCTIONanywhere in the tree. These are header-only declarations with no backing implementation. - They arrived in an unrelated commit.
git log -- src/js/internal/inspector/cdp.ts src/jsc/bindings/BunDebugger.hshows both files were added in102103d6, whose message is "stop proto-chain walk at the re-entry sentinel; only restore the CallSite array while the accessor is still installed" — a captureStackTrace follow-up with no mention of inspector/CDP work. - The PR description doesn't mention them. The description lists only the
FormatStackTraceForJS/CallSite/ZigGlobalObject/test changes. - The comment-cop bot flagged
cdp.ts18 times andBunDebugger.honce in the PR timeline — a strong signal these files weren't reviewed as part of this change.
Why nothing prevents it
The build passes because dead code compiles:
JSC_DECLARE_HOST_FUNCTIONis a forward declaration and doesn't require a definition unless the symbol is referenced (it isn't), andcdp.tsbundles cleanly as an internal module even though nothing imports it. So CI is green despite 717 lines of unreachable in-progress feature work riding along.Impact
No runtime failure — the code is unreachable, which is why this is filed as a nit rather than blocking. But merging as-is has real costs:
- Misleading git history.
git blameon these files will forever point at a captureStackTrace PR, and when the actualnode:inspectorPR lands later it will show as a diff against half-finished code that was already merged. - Dead declarations.
BunDebugger.hdeclares four host functions that don't exist — anyone grepping for them later will find a header with no implementation. - Review scope violation. Per REVIEW.md, focused bugfix PRs shouldn't carry unrelated feature work ("don't ride file-wide standardization on a focused bugfix"), and dead code shouldn't land (the "delete dead code in the same PR that makes it dead" rule; here it's the inverse — adding dead code).
How to fix
git rm src/js/internal/inspector/cdp.ts src/jsc/bindings/BunDebugger.hand amend/push. These files should land in their ownnode:inspectorPR alongside the.cppthat defines the four host functions and whatever importsInspectorCDPAdapter.
|
Those two files were an accidental inclusion from a stale worktree and are already removed in 644df87; the diff is back to the intended 7 files. |
|
CI status on 644df87 (build 80956): |
1 participant
What does this PR do?
Fixes #16418. Fixes the
.stackheader for the jsonwebtokenJsonWebTokenErrorpattern reported in #13904.When
Error.captureStackTrace(target)is called with a target that is not a nativeErrorInstance(a plain object, or a function-based Error subclass whose prototype isObject.create(Error.prototype)), Bun eagerly formatted the stack string at capture time and hardcoded"Error"as the header, ignoring the target's own.name/.message. V8 installs a lazy accessor and derives the header from.name/.messageat first.stackaccess, so setting them after capture (as jsonwebtoken does) must be observable:Before:
After (matches Node):
The fix
The non-
ErrorInstancebranch oferrorConstructorFuncCaptureStackTracenow mirrors what theErrorInstancebranch already does: build the source-mappedCallSitearray at capture time, stash it on the target under a private name, and install a lazy custom accessor on.stack. The new getter reads.name/.messagevia theError.prototype.toStringalgorithm (so{name: undefined}→"Error",{name: ""}→ message only, etc.) and consultsError.prepareStackTraceat first access instead of at capture time, both matching V8.Also fixes the header in
formatStackTraceToJSValueto read.namefrom the error object instead of hardcoding"Error: ", so the temporary string seen inside aprepareStackTracecallback has the correct header too.Note: #13904 accumulated two distinct reports. This PR fixes the original JsonWebTokenError
.stackheader; #32136 addresses the separate zod/tail-call frame-clearing symptom from the later comment.How did you verify your code works?
New tests in
test/js/node/v8/capture-stack-trace.test.js:captureStackTrace on a non-Error object reads name/message lazily for the stack header(the jsonwebtoken shape)captureStackTrace on a non-Error object installs a lazy accessor(descriptor matches Node's{get, set, enumerable: false, configurable: true})captureStackTrace header on a non-Error object matches V8's Error.prototype.toString algorithm(11 header edge cases verified byte-for-byte against Node)captureStackTrace on a non-Error object invokes Error.prepareStackTrace at access time(theError.prepareStackTraceis called onError.captureStackTracewhen it shouldn't be #16418 repro)All four fail on the unfixed build; all 47 tests in the file pass with the fix. The existing
"Error.captureStackTrace installs .stack as non-enumerable"test is updated to assert thatprepareStackTraceruns at.stackaccess time for plain-object targets (verified against Node), since it previously asserted on the eager behavior this PR removes.[review] gate passed · iteration 0 · 7 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