Derive the stack trace header from the error's name and message - #33437
Derive the stack trace header from the error's name and message#33437robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 17 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 (2)
Comment |
|
Updated 2:03 AM PT - Jul 14th, 2026
❌ @robobun, your commit 9d183d1 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33437That installs a local version of the PR into your bun-33437 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches GC-sensitive JSC binding code (new user-observable property reads on the error-info path, a changed assumption about what the finalizer can reach, and new MarkedArgumentBuffer rooting) — worth a human look from someone familiar with this file.
Extended reasoning...
Overview
This PR changes how the first line of a formatted stack trace is derived in src/jsc/bindings/FormatStackTraceForJS.cpp. It introduces getErrorNameAndMessage (reads name/message off arbitrary objects via getIfPropertyExists + toWTFString, falling back to the sanitized accessors for ErrorInstance), factors the header-append and the stack-frame GC-rooting loop into shared helpers, and roots captured frames in a MarkedArgumentBuffer at two additional sites in errorConstructorFuncCaptureStackTrace. Four new tests are added to test/js/node/v8/capture-stack-trace.test.js.
Security risks
None in the traditional sense (no auth, no untrusted input parsing beyond what already existed). The relevant risk class is memory safety: the change introduces user-triggerable getters and ToString coercion on a code path that holds unrooted StackFrame cells and that is also reachable (per the pre-existing comment) from a finalizer where allocation is forbidden. The PR addresses both — it adds MarkedArgumentBuffer rooting before formatting and asserts via comment that the finalizer only reaches computeErrorInfoWithoutPrepareStackTrace with a null errorInstance — but both of those are exactly the kind of invariant a JSC-familiar human should sign off on.
Level of scrutiny
High. Per this repo's own review guidance, JSC GC rooting, exception-scope discipline, and finalizer-path constraints are the most-blocked category of change. The diff is small and well-argued, but it (a) widens the set of callers that can run arbitrary user JS during stack formatting, (b) relies on a stated-but-not-mechanically-enforced invariant about the finalizer path, and (c) changes user-visible .stack output. That is not a mechanical change I can rubber-stamp.
Other factors
The PR description is thorough, the tests cover the V8 semantics matrix well (undefined vs null vs empty, coercion, prototype chain, accessors, prepareStackTrace), and the author verified the new tests fail on system Bun and pass on the debug build. No prior human reviews and no outstanding comments to address. Deferring so a maintainer can confirm the finalizer/rooting reasoning.
aacf895 to
55b7d4b
Compare
|
Pushed The CI failure was a stale base, not this diff14
Made the finalizer invariant machine-checkedThe review flagged the finalizer assumption as stated-but-not-enforced, which is fair. It is now asserted at the site that relies on it, rather than only described in a comment: // Reading these two properties can run user code. Only a mutator can reach here with a
// non-ErrorInstance target: the GC's finalizer path formats with a null errorObject, and
// materializeErrorInfoIfNeeded always passes an ErrorInstance, which returned above.
ASSERT(vm.heap.worldIsRunning());The two entry points, for the record:
So a plain object only reaches the property reads from Stressed with 40k |
There was a problem hiding this comment.
I didn't find any bugs, but this touches JSC bindings with GC-rooting and now runs user getters during stack formatting — worth a human look at the memory-safety and finalizer-path assumptions.
Extended reasoning...
Overview
This PR changes how the first line of a .stack string is composed in FormatStackTraceForJS.cpp. Previously, Error.captureStackTrace(obj) on a non-ErrorInstance hardcoded the header as "Error" (dropping both obj.name and obj.message), and formatStackTraceToJSValue hardcoded the name even for real TypeError instances. The fix introduces a shared getErrorNameAndMessage helper that follows V8's ErrorUtils::ToString semantics (undefined name → "Error", undefined message → empty, prototype-chain lookup with ToString coercion for plain objects; sanitizedNameString/sanitizedMessageString retained for ErrorInstance). It also extracts the existing header-join and MarkedArgumentBuffer frame-rooting blocks into appendErrorNameAndMessage / protectStackFrameCells, and adds frame rooting to the two eager-format paths in errorConstructorFuncCaptureStackTrace because reading name/message can now run a user getter that triggers GC while raw StackFrame cells are held. Four new tests in capture-stack-trace.test.js cover the header matrix, prototype-chain lookup, the pre-class custom-error idiom, and the prepareStackTrace default string.
Security risks
None identified. This is stack-trace string formatting; no auth, crypto, or filesystem/network surface. The new user-code-reachable path (invoking name/message getters on the target) was already partially present — the old formatStackTraceToJSValue did a full Get on message — and the eager model means a throwing getter propagates from captureStackTrace, which the PR description calls out as a known pre-existing difference from V8's lazy model.
Level of scrutiny
High. This is C++ JSC-bindings code on a hot, widely-used path (Error.captureStackTrace, .stack materialization) with explicit GC-safety implications. The repo's own review guidance flags exactly this class of change: running user JS while holding raw cells, rooting via MarkedArgumentBuffer, and RETURN_IF_EXCEPTION discipline after every call that can enter JS. The exception checks look complete and the new rooting mirrors the existing pattern in errorInstanceLazyStackCustomGetter, but the correctness of the finalizer-path guard (errorInstance is always null when called from finalizeUnconditionally, so getErrorNameAndMessage is unreachable there; asserted via vm.heap.worldIsRunning()) and whether every caller of computeErrorInfoWithoutPrepareStackTrace now has its frames rooted deserve a human eye.
Other factors
The bug-hunting system found nothing. Test coverage is thorough (11 header variants including null/undefined/numeric coercion, accessor properties, prototype chain, and the prepareStackTrace interaction), and the PR description documents a regression sweep across adjacent suites. The refactored helpers are behavior-preserving extractions of existing code. Still, this is not a mechanical change — it alters observable behavior of a core API and adds two new sites where user code runs during native formatting, so I'm deferring rather than approving.
Worth answering concretely, so here is every path that reaches the formatter holding a live
So every path either roots the frames, runs under The same table answers where user code can run. Happy to split the |
55b7d4b to
c2c0959
Compare
There was a problem hiding this comment.
I didn't find any issues, but this touches JSC GC rooting and introduces user-code execution (name/message getters) into the stack-trace formatting path — the finalizer-invariant and frame-rooting analysis look sound, but a maintainer familiar with JSC internals should confirm.
Extended reasoning...
Overview
This PR changes src/jsc/bindings/FormatStackTraceForJS.cpp so that the first line of a formatted stack trace derives from the target object's name and message properties (following V8's ErrorUtils::ToString), rather than a hardcoded "Error". It factors the header logic into getErrorNameAndMessage / appendErrorNameAndMessage, factors the pre-existing frame-cell rooting into protectStackFrameCells, and applies that rooting to two additional sites in errorConstructorFuncCaptureStackTrace now that formatting can run user getters. Four new tests in capture-stack-trace.test.js assert values taken from Node v26.3.0.
Security risks
None in the traditional sense (no auth, crypto, network, or filesystem surface). The relevant risk class here is memory safety: the change deliberately introduces property reads (getIfPropertyExists + toWTFString) on arbitrary user objects into a path that holds a raw Vector<StackFrame> whose cells the GC does not scan. The PR addresses this by rooting those cells in a MarkedArgumentBuffer before the reads, and by asserting vm.heap.worldIsRunning() at the one site where the reads occur. Exception scopes (RETURN_IF_EXCEPTION) are placed after every fallible call.
Level of scrutiny
High. This is C++ in the JSC bindings layer, and the repository's own review guidance calls out GC rooting, "anything that can run user JS can synchronously free your state", and finalizer-path constraints as the most-blocked category. The correctness of this change hinges on a non-local invariant — that the finalizer path always reaches computeErrorInfoWithoutPrepareStackTrace with errorInstance == nullptr, and that materializeErrorInfoIfNeeded always passes an ErrorInstance — which the author has traced and asserted but which a maintainer should independently confirm against current JSC.
Other factors
The PR description and follow-up comments are unusually thorough: every call path to the formatter is tabulated with what protects its frames, the change was stress-tested (40k iterations + Bun.gc() under debug+ASAN), and the bug-hunting system found nothing. The behavioral change itself (V8-compatible header formatting) is straightforward and well-tested. My hesitation is purely about the memory-safety delta in a subsystem where subtle mistakes become UAFs — that warrants a human sign-off rather than bot approval.
Status: rebased, diff is green, CI red is one slow darwin agentReady for a maintainer. Rebased onto main at Build #72736 ( The one not-passed lane is
The job log also shows This PR's own tests (four new ones + the updated #34104 assertion) pass on every lane including that one, which is why the 14 x64 run is 44/45 and not 40/45. I've already used one re-run on this PR (on the earlier darwin-aarch64 artifact-download timeout, which is now resolved), so I won't push another no-op; the linked build is the evidence. Happy to rebase or re-push if a fresh run is wanted. Review stateFour automated reviews, no findings. The GC-rooting and finalizer-path questions they flag for a human are answered in-thread: the invariant is |
The first line of a generated stack trace was composed from a hardcoded "Error" in two places: - computeErrorInfoWithoutPrepareStackTrace only read name/message when the target was an ErrorInstance, so Error.captureStackTrace(plainObject) always produced "Error" and dropped the object's message. - formatStackTraceToJSValue, which builds the default-formatted string handed to Error.prepareStackTrace, hardcoded the name, so a TypeError showed up as "Error: boom" there. Both now go through one helper that follows V8's ErrorUtils::ToString: an undefined name means "Error", an undefined message means the empty string, the two are joined with ": " only when both are non-empty, and for a non-ErrorInstance target they are read off the object itself. Reading those properties can run a getter, which can collect the cells a StackFrame holds, so the captured frames are rooted across formatting the same way the lazy .stack getter already does.
c2c0959 to
9d183d1
Compare
|
Rebased onto main (151 commits, #34104's test assertion is updated (to match Node)#34104 fixed a crash when the Error.prepareStackTrace = (e, s) => "custom-stack";
const e = new Error("x");
Object.defineProperty(e, "message", { get() { throw new TypeError("msg-boom"); } });
The reason: before this PR, The test's purpose is unchanged: it still spawns a subprocess and asserts 45/45 tests pass with the fix, 5 fail against main's |
There was a problem hiding this comment.
No bugs found, but this touches GC-sensitive JSC C++ (new user-code execution paths during stack formatting, MarkedArgumentBuffer rooting of StackFrame cells, finalizer-adjacent invariants) and rewrites an existing test's expected output — worth a maintainer's eyes.
What was reviewed:
getErrorNameAndMessage: exception checks after eachgetIfPropertyExists/toWTFString;ErrorInstancestill routes through sanitized (VMInquiry) accessors so the finalizer/materialize paths run no user JS.protectStackFrameCellsextraction is byte-equivalent to the pre-existing block inerrorInstanceLazyStackCustomGetter; the two new call sites incaptureStackTraceroot frames beforecomputeErrorInfoToJSValuecan allocate/call into JS.- The updated #34104 test still guards the original crash (
signalCode: null,exitCode: 0); only the stdout expectation changed to the Node-matching value.
Extended reasoning...
Overview
Two files: src/jsc/bindings/FormatStackTraceForJS.cpp (~100 lines net) and test/js/node/v8/capture-stack-trace.test.js (4 new tests, 1 updated assertion). The C++ change factors stack-header composition into getErrorNameAndMessage/appendErrorNameAndMessage, extends it to read name/message off non-ErrorInstance targets via full property lookups (matching V8's ErrorUtils::ToString), and factors the existing frame-cell rooting into protectStackFrameCells, applying it at two additional captureStackTrace sites now that formatting can run getters.
Security risks
None identified. The new property reads are on the caller's own object argument to Error.captureStackTrace, exception-checked, and don't widen any trust boundary.
Level of scrutiny
High. This is JSC bindings C++ in the exact category CLAUDE.md flags as most-blocked: it introduces a path where user JS can run (getIfPropertyExists + toWTFString on a plain object) while raw StackFrame cells are held, adds GC rooting to compensate, and relies on a finalizer-path invariant (the GC's onComputeErrorInfo never passes a non-ErrorInstance) now enforced only by a debug ASSERT. The author's entry-point table and 40k-iteration ASAN+GC stress run are convincing, but a maintainer familiar with ErrorInstance::finalizeUnconditionally / DeferGCForAWhile semantics should confirm the rooting story and the worldIsRunning() assertion.
Other factors
The PR also changes the expected output of #34104's regression test — the new value matches Node and the crash guard is preserved, but rewriting a just-merged test's assertions is exactly the kind of thing a human should sign off on. The author explicitly flagged this PR as "ready for a maintainer" in the thread.
1 participant
Repro
Error.captureStackTrace(obj)on a non-Errortarget is the canonical pre-classcustom-error idiom:
Under bun every such error's trace was labelled
Errorand its message was droppedfrom the stack string, so logged stacks lost both the error type and what went wrong.
Cause
The first line of a stack trace was composed from a hardcoded
"Error"in two places:computeErrorInfoWithoutPrepareStackTraceonly readname/messagewhen the targetwas an
ErrorInstance(viasanitizedNameString/sanitizedMessageString). A plainobject fell through to the
name = "Error"_sdefault with an empty message.formatStackTraceToJSValue, which builds the default-formatted string thatError.prepareStackTracereceives aserr.stack, readmessageoff the object buthardcoded the name, so even a real
TypeErrorwas labelledErrorthere.V8 composes that line with
ErrorUtils::ToString, which does aGet(target, "name")/Get(target, "message")on the object regardless of its type.Fix
Both sites now go through one
getErrorNameAndMessagehelper that followsErrorUtils::ToString: an undefinednamemeans"Error", an undefinedmessagemeansthe empty string, the two are joined with
": "only when both are non-empty, and for anon-
ErrorInstancetarget they are read off the object itself (prototype chain included,with
ToStringcoercion).ErrorInstancekeeps using the existing sanitized accessors,which also makes the
prepareStackTracedefault string agree with the non-prepareStackTraceone (what
test/js/node/v8/error-prepare-stack-default-fixture.jsasserts).Reading those two properties off the target can run a getter, and a
StackFrameholdscells the GC does not scan from the vector itself. The captured frames are now rooted in a
MarkedArgumentBufferacross formatting, the same way the lazy.stackgetter alreadydoes; that block is factored into
protectStackFrameCells.Known remaining difference
bun materializes
.stackeagerly insidecaptureStackTrace, V8 formats it lazily on firstaccess. So mutating
name/messageafter the call is still not reflected, and a throwingname/messagegetter throws fromcaptureStackTracerather than from the.stackread.Both are properties of the eager model, which predates this change (
formatStackTraceToJSValuealready did a full
Getonmessage); closing them means storing the frames for a plainobject, which is a larger change.
Rebase note: interaction with #34104
#34104 (merged) asserted that an accessor
.messagewhich throws whileError.prepareStackTraceis set makes the first.stackread throw and the second returnundefined. This PR routes anErrorInstancethrough the sanitized (VMInquiry) accessors for the header, which skip getters, so that specific repro now matches Node: the getter is never invoked,prepareStackTraceruns, and.stackis the string it returned. #34104's test assertion is updated accordingly; its regression guard (signalCode: null,exitCode: 0) is unchanged, and itsif (!result) return jsUndefined()safeguard stays.Verification
test/js/node/v8/capture-stack-trace.test.jsgains 4 tests, each asserting a value taken fromnode v26.3.0: the header cases above plus
{},{name: "", message: ""},{name: 42, message: 7},null/undefinedname and message, accessor-defined name and message, prototype-chain lookup,the pre-
classidiom, and theprepareStackTracedefault string forTypeError, anErrorsubclass, and a plain target.Regression sweep
test/js/node/v8/,test/js/bun/sourcemap/,test/js/bun/util/fuzzy-wuzzy.test.ts,test/js/deno/v8/error.test.ts,test/js/node/util/node-inspect-tests/parallel/util-format.test.js:8 fail before the diff (4 of them the new tests), 4 fail after. The 4 remaining
(
capture stack trace limit, the WebSocket call-sites test, the message-getter test,construct-subclass ReadStream) fail identically on unmodifiedmainwhen those files are runtogether, and all pass when each directory is run on its own.
test/js/bun/util/error-gc-test.test.jstimes out on unmodified
mainunder debug+ASAN too.[review] gate passed · iteration 3 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 3
evidence per changed file