Keep the error name and message in stack strings cached by the GC finalizer - #34408
Keep the error name and message in stack strings cached by the GC finalizer#34408robobun wants to merge 3 commits into
Conversation
…alizer When a GC runs between an error being thrown and the first .stack access, ErrorInstance::finalizeUnconditionally caches the stack string eagerly so the captured frames can be released. That path went through a callback that never received the ErrorInstance, so the cached string began with a bare "Error" instead of "Name: message". Async-thrown errors hit this reliably because nothing else keeps their resumed frames marked. JSC now exposes onComputeErrorInfoWithInstance (oven-sh/WebKit#302), which also passes the instance. Register that callback instead and read name/message there. Inside the finalizer this must not allocate in the JS heap or run user code, so the properties are read with Structure::getConcurrently (a plain get could re-materialize a property table the GC just cleared) and the instance is not forwarded to formatStackTrace's SyntaxError branch, which does a putDirect. WEBKIT_VERSION points at the preview build of oven-sh/WebKit#302 and should move to the merged commit once that lands. Fixes #34398
|
Warning Review limit reached
Next review available in: 9 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 (1)
WalkthroughChangesThe PR adds finalizer-safe lazy Error stack GC formatting
WebKit build version
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:53 PM PT - Jul 16th, 2026
❌ @robobun, your commit e6da701 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34408That installs a local version of the PR into your bun-34408 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding, I also checked whether the one-level prototype walk in getDirectPropertyInFinalizer misses multi-level Error subclasses — it matches sanitizedNameString's own-then-direct-prototype behavior, so no regression there. Also confirmed the old computeErrorInfoWrapperToString symbol has no remaining live callers (only a commented-out reference in BakeGlobalObject.cpp).
Extended reasoning...
This touches the GC end-phase finalizer path in JSC bindings and depends on an unmerged oven-sh/WebKit change (WEBKIT_VERSION is pinned to a preview-PR tag), so it needs human review regardless. The inline nit about tryGetValue() vs tryGetValueWithoutGC() covers the one substantive concern found; the note above records what else was examined so it isn't re-explored.
The pre-existing stress tests in error-gc-test.test.js are too slow for debug+ASAN runs with the default per-test timeout, so the new tests get their own file. In the finalizer branch, read name/message with tryGetValueWithoutGC: plain tryGetValue resolves ropes through a path that calls heap.reportExtraMemoryAllocated, which must not run during the GC end phase. This matches the MustNotTriggerGC lookup in ErrorStackTrace.cpp.
|
Addressed the review: the finalizer branch now uses tryGetValueWithoutGC for both the name and message reads (17fe187), consistent with the MustNotTriggerGC lookup in ErrorStackTrace.cpp. Plain tryGetValue would resolve ropes through heap.reportExtraMemoryAllocated during the GC end phase. Also moved the new tests into test/js/bun/util/error-stack-gc.test.js: the pre-existing stress tests in error-gc-test.test.js exceed the default per-test timeout under debug+ASAN, independent of this change. I tried to add a reassigned-rope-message case, but every shape that assigns a property on the error before the throw keeps the captured frames marked, so the finalizer path never fires and the test passes even without the fix. Left it out rather than ship a test that cannot fail. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/util/error-stack-gc.test.js`:
- Around line 63-76: Replace the manual cases loop around the subprocess test
with a describe.each(cases) parameterized block, preserving each case’s label,
script, expected value, and concurrent test execution. Keep the existing
Bun.spawn, output parsing, exit-code validation, and expectation logic inside
the parameterized test body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33c03473-40e9-4c01-a650-c86340189f06
📒 Files selected for processing (5)
scripts/build/deps/webkit.tssrc/jsc/bindings/FormatStackTraceForJS.cppsrc/jsc/bindings/FormatStackTraceForJS.hsrc/jsc/bindings/ZigGlobalObject.cpptest/js/bun/util/error-stack-gc.test.js
|
CI status: everything related to this diff is green; the new tests pass on all lanes. The remaining red is test/js/web/timers/timer-heap-race.test.ts on the x64-asan lane, a pre-existing failure on main (also red in builds without this change) that is being fixed separately. The other red lanes passed on retry. Merge order: oven-sh/WebKit#302 should land first, then WEBKIT_VERSION here moves from the preview tag to the merged commit. |
|
This came up again independently (worker_threads tests under ASAN, see below), so adding what that second look turned up rather than opening another PR for the same bug. The trigger is broader than async throws. The finalizer path runs whenever any callee or code block captured in the trace is collected before the first const errors = [];
[1].forEach(x => { errors.push(new TypeError("bad " + x)); });
Bun.gc(true);
console.log(errors[0].stack.split("\n")[0]); // bun 1.4.0: "Error" node: "TypeError: bad 1"
console.log(require("util").inspect(errors[0]).split("\n")[0]); // "Error" as well (improveStack only rewrites headers that start with err.name)Same result on 1.4.0 for errors created by On the WebKit side, oven-sh/WebKit#302 has a changes requested review: the instance must not be read from the finalizer. Unpinned property tables are dropped during marking, so by the time There is no bun-only fix: the string callback never sees the error object, and |
|
Another sighting of this bug, in case it helps get it over the line: the debug-build flake in I came at it independently and ended up with the same design (closed my duplicate of oven-sh/WebKit#302 as oven-sh/WebKit#445), so no competing PR. Two things from that attempt that may be worth folding in here:
That branch pins a preview build of the now-closed WebKit PR, so it only builds against something carrying #302; it is there for cherry-picking, not as an alternative. |
1 participant
Fixes #34398
Repro
Before:
{"message":"the message","stackHead":"Error"}(the message is dropped from the stack string). Expected, and what Node prints:"stackHead":"Error: the message". Sync-thrown errors were unaffected; async-thrown errors hit this reliably because nothing keeps the resumed async frames marked.Cause
Two paths materialize the stack string:
.stackaccess:vm.onComputeErrorInfoJSValue()receives theErrorInstance, reads name/message, and formatsError: the message\n at ....ErrorInstance::finalizeUnconditionallycaches the stack string eagerly (so the frames can be released) viavm.onComputeErrorInfo(), which did NOT receive the instance. Bun's callback fell back toname = "Error", empty message, and that string was cached and later served verbatim bymaterializeErrorInfoIfNeeded.Fix
VM::onComputeErrorInfoWithInstancecallback (same shape asonComputeErrorInfoplus theJSObject*), preferred byErrorInstance::computeErrorInfowhen set. The existing callback is untouched, so the change is backward compatible.Structure::getConcurrently+getDirect(a regular lookup can re-materialize a property table the GC just cleared, which allocates a GC cell during the end phase), mirroringsanitizedNameString's own-then-prototype lookup;formatStackTrace, whose SyntaxError branch does aputDirect.WEBKIT_VERSIONpoints at the preview build of Pass the error instance to the stack string callback WebKit#302 (autobuild-preview-pr-302-4abb9e38). It should be bumped to the merged oven-sh/WebKit commit once that PR lands.Verification
New tests in
test/js/bun/util/error-gc-test.test.js(spawned subprocesses, so the GC/marking conditions match the issue): async-thrownError,TypeError(name from the prototype), and a subclass with an ownnameproperty all keepName: messageas the stack head afterBun.gc(true); sync-thrown and primed-stack variants stay correct. All three bug cases fail on the unfixed build (bareError" head) and pass with this change. Related suites (capture-stack-trace,circular-error-stack,inspect-error,error-prepare-stack-trace`) show no new failures.[decide:webkit] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file