Preserve a pending exception across the GC stack-trace finalizer - #33584
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughChangesException-safe stack trace finalization
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change preserves a pending module-evaluation exception during stack-trace finalization, preventing the reported crash path; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:58 AM PT - Sep 14th, 2026
✅ @robobun, your commit afb2e9ce6c5e3e6683659524b34cb648bb4b086d passed in 🧪 To try this PR locally: bunx bun-pr 33584That installs a local version of the PR into your bun-33584 --bun |
![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 JSC exception-scope handling inside a GC finalizer (concurrent-GC safepoint interleave with a pending mutator exception), so it's worth a human familiar with JSC internals confirming this is the right layer for the fix vs. e.g. a suspend-exception pattern.
Extended reasoning...
Overview
The PR changes one function in src/jsc/bindings/FormatStackTraceForJS.cpp: computeErrorInfoWrapperToString, which is installed via vm.setOnComputeErrorInfo and invoked from ErrorInstance::finalizeUnconditionally during Heap::runEndPhase. Previously it unconditionally cleared any pending exception after computing the stack string; the fix captures scope.exception() before the computation and only clears if a different exception is now pending, preserving a mutator-owned exception that happened to be on the VM at the GC safepoint. A regression test is added to test/js/node/vm/sourcetextmodule-link-gc.test.ts that spawns a --smol fixture exercising vm.SourceTextModule evaluation errors under concurrent GC.
Security risks
None identified. This is an internal exception-handling adjustment in a GC finalizer path; no user-controlled input parsing, auth, crypto, or permission surface is touched.
Level of scrutiny
High. Although the diff is tiny (one captured pointer + one added comparison), it sits at the intersection of JSC's exception-scope machinery and concurrent-GC finalization — an area CLAUDE.md explicitly flags as the most-blocked review category. The root-cause analysis is thorough and the fix is strictly narrowing (it clears in fewer cases than before, never more), so it's unlikely to regress the original test-stream-writable-write-writev-finish.js case. But whether "snapshot the prior exception and compare" is the idiomatic JSC pattern here — versus, say, suspending the exception across the call, or addressing why the finalizer is entered at all with a pending VM exception — is a design call a JSC-familiar maintainer should make.
Other factors
- No CODEOWNER covers this file.
- The bug-hunting pass found no issues.
- The test follows harness conventions (
tempDir,bunEnv/bunExe, concurrent pipe drain, asserts{stdout, exitCode}object) and is skipped on Windows consistent with the sibling test in the same file. - In the finalizer path
errorInstanceisnullptr, so theRETURN_IF_EXCEPTIONguards insidecomputeErrorInfoWithoutPrepareStackTraceare not reached on entry with a pre-existing exception; and JSC'sthrowExceptiondoes not overwrite an already-pending exception, so the!= priorExceptioncomparison should hold in the intended case. Still, given the subtlety, deferring to a human.
|
Good call on the suspend-exception pattern. JSC has Switched to it in 6ff6ce9. It is strictly more correct than the compare-snapshot approach: the stack computation now runs against a clean exception slot (no early returns on a pre-existing exception), and both Re-verified locally:
|
There was a problem hiding this comment.
The fix looks well-reasoned and uses JSC's own SuspendExceptionScope primitive, but the correctness argument hinges on subtle concurrent-GC / finalizer / VM-exception-state interactions that deserve a look from someone who knows the JSC exception model well.
Extended reasoning...
Overview
This PR adds a single JSC::SuspendExceptionScope RAII guard to computeErrorInfoWrapperToString in src/jsc/bindings/FormatStackTraceForJS.cpp, plus a new regression test in test/js/node/vm/sourcetextmodule-link-gc.test.ts. The functional change is one line (plus an include and comment updates): suspend any pre-existing VM exception before computing an error's stack string in the GC finalizer, so the existing tryClearException() only ever swallows exceptions raised by the stack computation itself, not one belonging to the suspended mutator.
Security risks
None. This is a crash fix in error-info finalization; there is no attacker-controlled input surface, auth, or data-exposure angle.
Level of scrutiny
High. The diff is tiny, but it sits at the intersection of concurrent GC (Heap::runEndPhase → ErrorInstance::finalizeUnconditionally), JSC's VM-global exception slot, and CyclicModuleRecord::evaluate's reject-with-caught-exception path. The root-cause narrative is detailed and plausible, and SuspendExceptionScope is the upstream JSC primitive for exactly this (cited prior art: TypeProfilerLog::processLogEntries). Destruction order is correct (the suspend scope outlives the TopExceptionScope, so the mutator's exception is restored last). Still, whether it is truly safe for this finalizer to run against a nulled exception slot while the mutator is suspended mid-evaluation — and whether restoring m_lastException / the trap bit here has no other side effects — is the kind of JSC-internals question I'd want a human familiar with the GC/exception model to confirm rather than rubber-stamp.
Other factors
- No CODEOWNERS coverage for these paths.
- The bug-hunting system found no issues.
- The regression test is thorough (real file fixture,
--smolto force frequent collections, 4 sequential runs, well-commented rationale) and follows harness conventions. - The PR author already iterated once (snapshot-compare →
SuspendExceptionScope) and re-verified both the crash repro and the exception-checker test that motivated the originaltryClearException.
Given the subtlety of the code path, I'm deferring rather than approving.
computeErrorInfoWrapperToString (installed via vm.setOnComputeErrorInfo) runs from ErrorInstance::finalizeUnconditionally during Heap::runEndPhase. It cleared whatever exception was pending after computing a stack string. With concurrent GC that finalizer lands at an arbitrary safepoint: if it fires while the mutator is mid vm.SourceTextModule evaluation with a module's evaluation error pending, it nulled that exception. CyclicModuleRecord::evaluate step 9.d then reached rejectWithCaughtException with no pending exception, aborting on assert builds (ASSERTION FAILED: exception, JSPromise.cpp) and segfaulting at 0x8 on release. Only swallow an exception the stack computation itself raised; leave a pre-existing one untouched so it survives the finalizer.
JSC has a dedicated RAII helper for exactly this case (a callee that must run transparently to a pre-existing pending exception). It saves and nulls m_exception, m_lastException and the NeedExceptionHandling trap, then restores all three on destruction. TypeProfilerLog::processLogEntries uses the same pattern for the same reason.
A fresh on-disk ESM graph per iteration, one member throwing at top level and one using top-level await. import(a) fails on the throwing member; import(b) reaches that already-errored member, so Evaluate() rethrows the stored error and runs the step 9 -> 9.d window again. This is the path in the release crash reports (JSPromise::reject <- rejectWithCaughtException <- CyclicModuleRecord::evaluate <- dynamicImportLoadSettled, SEGV at 0x8).
6ff6ce9 to
91b4605
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
… the vm fixture The vm fixture's unhandledRejection handler counted rejections that nothing asserted. It was also hiding a separate, pre-existing bug: a throwing top-level-await SourceTextModule shared by two importers reports one unhandled rejection per graph even though every evaluate() is awaited and caught. Tolerate exactly that rejection and exit nonzero on any other, so the crash under test stays the only failure mode. Both Windows skips now say why: the fixtures are long GC-churn loops that run several times slower under Windows + ASAN, and the fixed path is not platform-specific.
There was a problem hiding this comment.
Both earlier nits are addressed — the Windows skips now carry a stated reason, and the unhandledRejection handler in the vm.SourceTextModule fixture now fails the process (exit 3) on any rejection other than the known pre-existing URIError('tla'), so a regression that turns the crash into a stray unhandled rejection can no longer pass. No further findings from this run; a human look is still worthwhile given this is a concurrent-GC/exception-handling change in the JSC bindings.
What was reviewed
SuspendExceptionScopeplacement: constructed before theTopExceptionScopeand destructed after it, so the mutator's exception is restored after the local clear — ordering is correct.- Confirmed the inner
tryClearException()now only sees exceptions raised bycomputeErrorInfoToStringitself, matching the stated intent. - Re-read both test fixtures against the updated harness conventions (tempDir, bunEnv, concurrent pipe drain, stdout-before-exitCode assertion shape).
Extended reasoning...
Overview
The PR is a 3-line native fix in src/jsc/bindings/FormatStackTraceForJS.cpp: it wraps the onComputeErrorInfo hook body in JSC::SuspendExceptionScope so a mutator's pending exception is stashed and restored across the GC-end-phase stack-trace materialization, and only exceptions raised by the computation itself are cleared. Two stress tests exercise the plain import() and vm.SourceTextModule faces of the crash. Since the previous review, commit 4edbfd4 addressed both inline nits: skip-reason comments were added for the Windows gates, and the unhandledRejection handler in the SourceTextModule fixture was rewritten to whitelist only the known pre-existing URIError('tla') and hard-exit(3) on anything else.
Security risks
None. This is an internal JSC exception-bookkeeping fix with no user-facing input surface, auth, or data-handling change.
Level of scrutiny
Medium-high. The source change is tiny and follows an upstream JSC pattern (SuspendExceptionScope, as used in TypeProfilerLog::processLogEntries), and the destruction ordering relative to the inner TopExceptionScope is correct. But it sits in a concurrent-GC finalizer path where subtle exception-state and safepoint interactions live, so a maintainer familiar with the JSC bindings should still sign off rather than an automated approval.
Other factors
The new tests are heavy (120s per-test timeouts, 2× and 4× serial subprocess runs) — the PR justifies the iteration counts and serial execution as necessary to hit the race reliably on the unfixed build, and both are Windows-skipped with stated CI-runtime reasons. The rewritten unhandledRejection handler now fails closed on unexpected rejections, resolving the earlier concern that it could mask a regression. No outstanding third-party CHANGES_REQUESTED reviews are visible in the timeline.
SuspendExceptionScope restores the saved exception slot blindly on exit. If a termination exception is thrown inside the window (a RETURN_IF_EXCEPTION in the stack computation servicing a pending terminate()), tryClearException refuses to clear it, and the restore then overwrites it with the saved value (null when nothing was pending) while the NeedExceptionHandling trap bit stays set. The next exception check asserts the exception/trap invariant: ASSERTION FAILED: !!(*scope).exception() == vm.traps().needHandling(NeedExceptionHandling) TopExceptionScope__exceptionIncludingTraps Hold termination off for the duration of the window with DeferTerminationForAWhile, which re-arms the NeedTermination trap on exit so the mutator throws it at its next check. A termination already pending on entry is suspended and rethrown by the same scope. Regression test: run the GC-churn fixture in a Worker and terminate it mid-run. Every run of the suspend-only build asserted.
…ose before yielding --smol on the parent does not reach a Worker's VM; pass smol: true so the worker's heap is the one that collects often. Register the close listener before the timer so a worker that dies early still dispatches close instead of hanging the loop until the test timeout.
|
CI status at d661d2b (build 108433): every lane is green except |
Bun.resolve() allocates its rejected promise while the exception the resolve threw is still pending (JSPromise::rejected_promise_with_caught_exception). That allocation is a GC safepoint. When the concurrent collector's end phase ran there and materialized a live Error's stack, the hook cleared the pending exception and the rejection then found nothing: panic: A JavaScript exception was thrown, but it was cleared before it could be read. take_exception <- JSPromise::reject <- rejected_promise_with_caught_exception <- bun_object::resolve The unfixed debug build panics after 2500 to 8500 iterations under collectContinuously; with the finalizer fix 80000 iterations complete.
robobun
changed the title
|
One more face of this cause, measured on release builds. It is on main and not in a release yet. Symptom. A failing Path. #41796 (merged 2026-09-07) makes Script. The const keep = [];
let rejected = 0;
for (let i = 0; i < 3000; i++) {
const f = new Function("return new Error('e" + i + "')");
keep.push(f());
if (keep.length > 400) keep.shift();
try {
await Bun.resolve("./does-not-exist-" + i, import.meta.dir);
} catch (e) {
rejected++;
}
}
console.log("rejected", rejected);Runs. Linux x64,
Without The same helper serves |
slowPathAllocsBetweenGCs collects every N slow-path allocations, so the end phase lands in the same place every run. With the Error built inside an eval'd arrow its frames are dead by the time the uncaught throw propagates, which gives the end phase a stack to materialize in the window where the entry module's evaluation promise is about to be rejected with the caught exception. Unlike the three concurrent-GC reproducers this needs no iteration count and finishes in about 6s. On the unfixed build all three N values abort every run; with the fix all print `error: c` and exit 1.
…ing length limit (#42314) ### Problem - Reading `.stack` of an error with a very long message aborts the process: `panic(main thread): abort() called`, exit code 134. Example: `bun -e 'new Error("q".repeat(2**31-10)).stack'`. The default `Error.prepareStackTrace`, which also runs before a user-defined one, aborts the same way. - Both formatters in `src/jsc/bindings/FormatStackTraceForJS.cpp` (`formatStackTrace` at :154, `formatStackTraceToJSValue` at :42) use a default-constructed `WTF::StringBuilder`. It calls `CRASH()` on the append that passes `String::MaxLength`. The message and each frame's source URL come from JS. ### Fix - Both builders record the overflow (`OverflowPolicy::RecordOverflow`). A trace that did not fit becomes the `name: message` header without the frames, or the message alone if the header does not fit either. - It degrades and does not throw, because `formatStackTrace` also runs from `ErrorInstance::finalizeUnconditionally`, inside a GC, where nothing can throw. `.stack` is then the same whether the GC or the getter computes it. - A trace under the limit is unchanged. A user `Error.prepareStackTrace` still runs and still gets the call sites. - Verified: `test/js/node/v8/capture-stack-trace.test.js`, one new test with both paths in one child. Each path aborts alone on 1.4.3. Also `stack.test.ts`, three stack-trace regression tests and `test-error-prepare-stack-trace.js`. ### Background - `String::MaxLength` is 2^31 - 1 code units. An error message can be that long. - `.stack` is computed lazily: `ErrorInstance::materializeErrorInfoIfNeeded` calls Bun's hook on the first read. If the GC finds dead frames first, `finalizeUnconditionally` calls the string variant of the same hook. - `RecordOverflow` makes the builder set a flag that `hasOverflowed()` reports. Later appends do nothing. <details><summary>Notes</summary> This is split out of #42202, which is now only about error messages. Other policies for a trace that does not fit, and why this PR does not take them: - Throw, like Node. Node v26.3.0 throws `RangeError: Invalid string length` from the `.stack` read for the same input. In Bun the same text is also produced inside a GC, where a throw is not possible, so the two paths would differ. The getter runs under many operations on the error object (`Object.keys`, a property write), which would all start to throw. - Keep the frames and cut the message. That keeps more information, because the frames are small. It needs the header and the frames built apart, which is a larger change to a function that #40354 and #33584 also edit. The change is small on purpose: the builder policy and three return sites. It does not touch the parts of the file that #40354 and #33584 change, so any merge order works. Seen while testing, not caused by this change: `stack.test.ts > Async functions frame should be included in stack trace` fails when it runs in one process after the three regression files (two extra frames). It fails the same way with `src/` at `origin/main`, and it passes alone. Cost of the test: the length is what is under test, so the child needs a string of about 2 GiB. Both paths run in one child, so the string is allocated once. Each path copies the header once, so the child touches about 6 GB of pages. It takes about 5 s in a debug ASAN build, so it carries a 30 s ceiling. It skips below 10 GiB of total memory, the gate `test/js/bun/transpiler/source-too-large.test.ts` uses. </details>
…n-sh#33584) ### Problem - A garbage collection can drop the VM's pending exception. A module whose evaluation throws then hits `ASSERTION FAILED: exception` in `JSPromise::rejectWithCaughtException` (assert builds) or segfaults at `0x8` in `JSPromise::reject <- rejectWithCaughtException <- CyclicModuleRecord::evaluate <- dynamicImportLoadSettled` (release, Sentry BUN-4R6E). `Bun.resolve()` hits `panic: A JavaScript exception was thrown, but it was cleared before it could be read.` in `rejected_promise_with_caught_exception`. - Cause: `computeErrorInfoWrapperToString` (`src/jsc/bindings/FormatStackTraceForJS.cpp`), the `onComputeErrorInfo` hook, runs in `Heap::runEndPhase` for live Errors whose frames died, and cleared whatever exception was pending. With concurrent GC it lands while the mutator is parked at an allocation with an exception pending. ### Fix - Wrap the hook body in JSC's `SuspendExceptionScope` (as `TypeProfilerLog::processLogEntries` does). The mutator's exception and trap bit are set aside and restored, so `tryClearException()` only swallows what the computation itself raised. - Add `DeferTerminationForAWhile`. A `terminate()` thrown inside the window survives the clear, and the restore then overwrites it with the trap bit still set. - Verified: `error-stack-finalizer-exception.test.ts` is the deterministic one (`slowPathAllocsBetweenGCs` fixes where the end phase lands): all three N values abort every run unfixed, all pass fixed, in about 6s. `dynamic-import-evaluation-error-gc.test.ts`, `resolve-error.test.ts` and `sourcetextmodule-link-gc.test.ts` cover the concurrent-GC faces and fail 3/3 unfixed. The terminate test fails 3/3 on a suspend-only build. `test-stream-writable-write-writev-finish.js` passes under `BUN_JSC_validateExceptionChecks=1`. ### Background - JSC keeps one pending exception per VM (`VM::m_exception`), mirrored by a trap bit. A callee that clears it drops the caller's exception. - `CyclicModuleRecord::evaluate` step 9 sees the module's error as the pending exception and step 9.d rejects by re-reading the slot. `Bun.resolve()` allocates its promise before it takes the exception. - With concurrent GC the end phase runs while the mutator is stopped at a safepoint, which is any allocation slow path. `ErrorInstance` keeps weak stack frames. When a frame's code dies, the end phase materializes the stack string through this hook. <details><summary>Notes</summary> Deterministic door (no concurrent collector): `BUN_JSC_slowPathAllocsBetweenGCs=7 bun -e 'const e = eval("(() => Object.assign(new Error(\"c\"), { code: 1 }))")(); throw e'`. Reported as 5/5 `Segmentation fault at address 0x8` on release 1.4.2, canary and a release build of main, and 5/5 `ASSERTION FAILED: exception` on an ASan build. Measured here on a debug+ASAN build of main: N of 3, 4 and 7 abort every run; 1, 2, 5, 6 and >= 8 print the error normally; with the fix all of 3, 4, 7 print `error: c` and exit 1 (9/9). The eval'd arrow matters: it makes the Error's frames garbage before the throw propagates, so the end phase has a stack to materialize. Reproduction (plain `import()`, no `node:vm`, no plugin), about 4 of 6 runs abort at 200 iterations and 6 of 6 at 300 on the unfixed assert build: ```js import { mkdirSync, writeFileSync } from "node:fs"; import { join } from "node:path"; for (let it = 0; it < 300; it++) { const d = join(import.meta.dir, "graphs", "g" + it); mkdirSync(d, { recursive: true }); writeFileSync(join(d, "bad.mjs"), `export const x = ${it};\nthrow new Error("boom ${it}");\n`); writeFileSync(join(d, "tla.mjs"), `await new Promise(r => setTimeout(r, 0));\nexport const t = ${it};\n`); writeFileSync(join(d, "leaf.mjs"), `export const l = ${it};\n`); writeFileSync(join(d, "mid.mjs"), `import { l } from "./leaf.mjs";\nimport "./bad.mjs";\nexport const m = l + 1;\n`); writeFileSync(join(d, "a.mjs"), `import { m } from "./mid.mjs";\nimport { t } from "./tla.mjs";\nexport const a = m + t;\n`); writeFileSync(join(d, "b.mjs"), `import { t } from "./tla.mjs";\nimport "./bad.mjs";\nexport const b = t;\n`); writeFileSync(join(d, "c.mjs"), `import { m } from "./mid.mjs";\nexport const c = m;\n`); await import(join(d, "a.mjs")).catch(() => {}); await import(join(d, "b.mjs")).catch(() => {}); await import(join(d, "c.mjs")).catch(() => {}); await import(`data:text/javascript,throw new Error('d${it}')`).catch(() => {}); } ``` `import(b)` reaches the already-errored `bad.mjs`, so `InnerModuleEvaluation` step 2 rethrows the stored error and the step 9 to 9.d window runs again. A data:-URL-only loop does not fire. A single iteration does not fire. State at the assert (gdb, conditional break on `exception == 0` at the assert line): the `Exception*` local captured at step 9 is non-null while `vm.m_exception` is null. The window between the two is straight-line C++ (`attachErrorInfo`, `setStatus`, `setEvaluationError`) with no JS and no microtask checkpoint, so the slot is cleared out of band. `BUN_JSC_useConcurrentGC=false`: 0/16 aborts (baseline 12/16 on the `vm.SourceTextModule` reproducer). `collectContinuously` also hides it (it moves the window). The `vm.SourceTextModule` reproducer (the first one found) needs `--smol` to fire reliably and a file-backed fixture (a `-e` script has no source URL, so the stack materializer takes a different path). Earlier iteration of this PR compared the pending exception before and after the computation and cleared only a new one. `SuspendExceptionScope` replaces that: it is the upstream pattern, it also restores `m_lastException` and the trap bit, and the computation runs against a clean slot. The first reports of this assert named a `Bun.plugin` `onResolve` recipe. That recipe cannot reach the state (its filter never matched), and the plugin is not part of the cause. The first push used `SuspendExceptionScope` alone. CI caught the termination interaction on the debian x64-asan lane (`worker.test.ts`, "terminate() while importing every builtin module"): `ASSERTION FAILED: !!(*scope).exception() == vm.traps().needHandling(JSC::VMTraps::NeedExceptionHandling)` in `TopExceptionScope__exceptionIncludingTraps`. A stress (the vm fixture inside a Worker terminated mid-run, 20 rounds) reproduced it 6/6 on that build and 0/6 with the deferral; it is now the third test in the vm file. `Bun.resolve()` face (the helper from oven-sh#41796): `Bun.resolve()` with no arguments from a fresh closure per iteration, rejection reasons kept in a 1024-entry ring, awaited one at a time, `BUN_JSC_collectContinuously=1`. Unfixed main 09bb546 (debug+ASAN): 9/9 panic after 2500 to 8500 iterations, trace `take_exception <- JSPromise::reject <- rejected_promise_with_caught_exception <- bun_object::resolve`. With the fix: 5/5 complete 80000 iterations. Batching 64 calls per tick never fires. Reported on a release build of main as well (panic within 0.2 s under collectContinuously). The same helper serves the early rejections of `fetch()`, `Bun.write()` and `server.fetch()`. The module-evaluation standalone above also segfaults shipped release builds on demand under `BUN_JSC_collectContinuously=1` (reported: 1.4.2 4/5, canary 4/5, release main 5/5 at N=400, SEGV at 0x8). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/vm/sourcetextmodule-link-gc.test.ts, test/js/bun/resolve/resolve-error.test.ts, test/js/bun/resolve/dynamic-import-evaluation-error-gc.test.ts <!-- robobun:evidence:end -->
…ing length limit (oven-sh#42314) ### Problem - Reading `.stack` of an error with a very long message aborts the process: `panic(main thread): abort() called`, exit code 134. Example: `bun -e 'new Error("q".repeat(2**31-10)).stack'`. The default `Error.prepareStackTrace`, which also runs before a user-defined one, aborts the same way. - Both formatters in `src/jsc/bindings/FormatStackTraceForJS.cpp` (`formatStackTrace` at :154, `formatStackTraceToJSValue` at :42) use a default-constructed `WTF::StringBuilder`. It calls `CRASH()` on the append that passes `String::MaxLength`. The message and each frame's source URL come from JS. ### Fix - Both builders record the overflow (`OverflowPolicy::RecordOverflow`). A trace that did not fit becomes the `name: message` header without the frames, or the message alone if the header does not fit either. - It degrades and does not throw, because `formatStackTrace` also runs from `ErrorInstance::finalizeUnconditionally`, inside a GC, where nothing can throw. `.stack` is then the same whether the GC or the getter computes it. - A trace under the limit is unchanged. A user `Error.prepareStackTrace` still runs and still gets the call sites. - Verified: `test/js/node/v8/capture-stack-trace.test.js`, one new test with both paths in one child. Each path aborts alone on 1.4.3. Also `stack.test.ts`, three stack-trace regression tests and `test-error-prepare-stack-trace.js`. ### Background - `String::MaxLength` is 2^31 - 1 code units. An error message can be that long. - `.stack` is computed lazily: `ErrorInstance::materializeErrorInfoIfNeeded` calls Bun's hook on the first read. If the GC finds dead frames first, `finalizeUnconditionally` calls the string variant of the same hook. - `RecordOverflow` makes the builder set a flag that `hasOverflowed()` reports. Later appends do nothing. <details><summary>Notes</summary> This is split out of oven-sh#42202, which is now only about error messages. Other policies for a trace that does not fit, and why this PR does not take them: - Throw, like Node. Node v26.3.0 throws `RangeError: Invalid string length` from the `.stack` read for the same input. In Bun the same text is also produced inside a GC, where a throw is not possible, so the two paths would differ. The getter runs under many operations on the error object (`Object.keys`, a property write), which would all start to throw. - Keep the frames and cut the message. That keeps more information, because the frames are small. It needs the header and the frames built apart, which is a larger change to a function that oven-sh#40354 and oven-sh#33584 also edit. The change is small on purpose: the builder policy and three return sites. It does not touch the parts of the file that oven-sh#40354 and oven-sh#33584 change, so any merge order works. Seen while testing, not caused by this change: `stack.test.ts > Async functions frame should be included in stack trace` fails when it runs in one process after the three regression files (two extra frames). It fails the same way with `src/` at `origin/main`, and it passes alone. Cost of the test: the length is what is under test, so the child needs a string of about 2 GiB. Both paths run in one child, so the string is allocated once. Each path copies the header once, so the child touches about 6 GB of pages. It takes about 5 s in a debug ASAN build, so it carries a 30 s ceiling. It skips below 10 GiB of total memory, the gate `test/js/bun/transpiler/source-too-large.test.ts` uses. </details>
2 participants
Problem
ASSERTION FAILED: exceptioninJSPromise::rejectWithCaughtException(assert builds) or segfaults at0x8inJSPromise::reject <- rejectWithCaughtException <- CyclicModuleRecord::evaluate <- dynamicImportLoadSettled(release, Sentry BUN-4R6E).Bun.resolve()hitspanic: A JavaScript exception was thrown, but it was cleared before it could be read.inrejected_promise_with_caught_exception.computeErrorInfoWrapperToString(src/jsc/bindings/FormatStackTraceForJS.cpp), theonComputeErrorInfohook, runs inHeap::runEndPhasefor live Errors whose frames died, and cleared whatever exception was pending. With concurrent GC it lands while the mutator is parked at an allocation with an exception pending.Fix
SuspendExceptionScope(asTypeProfilerLog::processLogEntriesdoes). The mutator's exception and trap bit are set aside and restored, sotryClearException()only swallows what the computation itself raised.DeferTerminationForAWhile. Aterminate()thrown inside the window survives the clear, and the restore then overwrites it with the trap bit still set.error-stack-finalizer-exception.test.tsis the deterministic one (slowPathAllocsBetweenGCsfixes where the end phase lands): all three N values abort every run unfixed, all pass fixed, in about 6s.dynamic-import-evaluation-error-gc.test.ts,resolve-error.test.tsandsourcetextmodule-link-gc.test.tscover the concurrent-GC faces and fail 3/3 unfixed. The terminate test fails 3/3 on a suspend-only build.test-stream-writable-write-writev-finish.jspasses underBUN_JSC_validateExceptionChecks=1.Background
VM::m_exception), mirrored by a trap bit. A callee that clears it drops the caller's exception.CyclicModuleRecord::evaluatestep 9 sees the module's error as the pending exception and step 9.d rejects by re-reading the slot.Bun.resolve()allocates its promise before it takes the exception.ErrorInstancekeeps weak stack frames. When a frame's code dies, the end phase materializes the stack string through this hook.Notes
Deterministic door (no concurrent collector): `BUN_JSC_slowPathAllocsBetweenGCs=7 bun -e 'const e = eval("(() => Object.assign(new Error(\"c\"), { code: 1 }))")(); throw e'`. Reported as 5/5 `Segmentation fault at address 0x8` on release 1.4.2, canary and a release build of main, and 5/5 `ASSERTION FAILED: exception` on an ASan build. Measured here on a debug+ASAN build of main: N of 3, 4 and 7 abort every run; 1, 2, 5, 6 and >= 8 print the error normally; with the fix all of 3, 4, 7 print `error: c` and exit 1 (9/9). The eval'd arrow matters: it makes the Error's frames garbage before the throw propagates, so the end phase has a stack to materialize.Reproduction (plain
import(), nonode:vm, no plugin), about 4 of 6 runs abort at 200 iterations and 6 of 6 at 300 on the unfixed assert build:import(b)reaches the already-erroredbad.mjs, soInnerModuleEvaluationstep 2 rethrows the stored error and the step 9 to 9.d window runs again. A data:-URL-only loop does not fire. A single iteration does not fire.State at the assert (gdb, conditional break on
exception == 0at the assert line): theException*local captured at step 9 is non-null whilevm.m_exceptionis null. The window between the two is straight-line C++ (attachErrorInfo,setStatus,setEvaluationError) with no JS and no microtask checkpoint, so the slot is cleared out of band.BUN_JSC_useConcurrentGC=false: 0/16 aborts (baseline 12/16 on thevm.SourceTextModulereproducer).collectContinuouslyalso hides it (it moves the window).The
vm.SourceTextModulereproducer (the first one found) needs--smolto fire reliably and a file-backed fixture (a-escript has no source URL, so the stack materializer takes a different path).Earlier iteration of this PR compared the pending exception before and after the computation and cleared only a new one.
SuspendExceptionScopereplaces that: it is the upstream pattern, it also restoresm_lastExceptionand the trap bit, and the computation runs against a clean slot.The first reports of this assert named a
Bun.pluginonResolverecipe. That recipe cannot reach the state (its filter never matched), and the plugin is not part of the cause.The first push used
SuspendExceptionScopealone. CI caught the termination interaction on the debian x64-asan lane (worker.test.ts, "terminate() while importing every builtin module"):ASSERTION FAILED: !!(*scope).exception() == vm.traps().needHandling(JSC::VMTraps::NeedExceptionHandling)inTopExceptionScope__exceptionIncludingTraps. A stress (the vm fixture inside a Worker terminated mid-run, 20 rounds) reproduced it 6/6 on that build and 0/6 with the deferral; it is now the third test in the vm file.Bun.resolve()face (the helper from #41796):Bun.resolve()with no arguments from a fresh closure per iteration, rejection reasons kept in a 1024-entry ring, awaited one at a time,BUN_JSC_collectContinuously=1. Unfixed main 09bb546 (debug+ASAN): 9/9 panic after 2500 to 8500 iterations, tracetake_exception <- JSPromise::reject <- rejected_promise_with_caught_exception <- bun_object::resolve. With the fix: 5/5 complete 80000 iterations. Batching 64 calls per tick never fires. Reported on a release build of main as well (panic within 0.2 s under collectContinuously). The same helper serves the early rejections offetch(),Bun.write()andserver.fetch().The module-evaluation standalone above also segfaults shipped release builds on demand under
BUN_JSC_collectContinuously=1(reported: 1.4.2 4/5, canary 4/5, release main 5/5 at N=400, SEGV at 0x8).no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/vm/sourcetextmodule-link-gc.test.ts, test/js/bun/resolve/resolve-error.test.ts, test/js/bun/resolve/dynamic-import-evaluation-error-gc.test.ts