diff --git a/src/jsc/bindings/FormatStackTraceForJS.cpp b/src/jsc/bindings/FormatStackTraceForJS.cpp index b74b3a61a0ab..5fc9bab2686d 100644 --- a/src/jsc/bindings/FormatStackTraceForJS.cpp +++ b/src/jsc/bindings/FormatStackTraceForJS.cpp @@ -6,6 +6,8 @@ #include "JavaScriptCore/ArgList.h" #include "JavaScriptCore/CallData.h" +#include "JavaScriptCore/DeferTermination.h" +#include "JavaScriptCore/FrameTracers.h" #include "JavaScriptCore/TopExceptionScope.h" #include "JavaScriptCore/Error.h" #include "JavaScriptCore/ErrorInstance.h" @@ -597,11 +599,15 @@ WTF::String computeErrorInfoWrapperToString(JSC::VM& vm, Vector& sta OrdinalNumber line = OrdinalNumber::fromOneBasedInt(line_in); OrdinalNumber column = OrdinalNumber::fromOneBasedInt(column_in); + // A termination thrown in here would survive the clear below and be lost to the restore. + JSC::DeferTerminationForAWhile deferTermination(vm); + // Runs from the GC end phase while the mutator may have its own pending exception. + JSC::SuspendExceptionScope suspendExceptionScope(vm); + auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); WTF::String result = computeErrorInfoToString(vm, stackTrace, line, column, sourceURL); if (scope.exception()) { - // TODO: is this correct? vm.setOnComputeErrorInfo doesnt appear to properly handle a function that can throw - // test/js/node/test/parallel/test-stream-writable-write-writev-finish.js is the one that trips the exception checker + // The onComputeErrorInfo hook cannot propagate a throw. (void)scope.tryClearException(); result = WTF::emptyString(); } diff --git a/test/js/bun/gc/error-stack-finalizer-exception.test.ts b/test/js/bun/gc/error-stack-finalizer-exception.test.ts new file mode 100644 index 000000000000..45318d5cdf43 --- /dev/null +++ b/test/js/bun/gc/error-stack-finalizer-exception.test.ts @@ -0,0 +1,45 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +// computeErrorInfoWrapperToString (src/jsc/bindings/FormatStackTraceForJS.cpp, +// the vm.setOnComputeErrorInfo hook) runs from +// ErrorInstance::reconcileWeakReferencesAtGCEnd during Heap::runEndPhase, to +// materialize the stack of a live Error whose frames have died. It used to +// clear whatever exception was pending on the VM. The entry module's +// evaluation promise is rejected with the caught exception after an allocation +// safepoint, so a collection ending in that window dropped the error: +// +// assert build: ASSERTION FAILED: exception JSPromise.cpp, rejectWithCaughtException +// release build: panic(main thread): Segmentation fault at address 0x8 +// +// Unlike the concurrent-GC reproducers (sourcetextmodule-link-gc.test.ts, +// dynamic-import-evaluation-error-gc.test.ts, the Bun.resolve() case in +// resolve-error.test.ts) this one is deterministic: slowPathAllocsBetweenGCs +// collects every N slow-path allocations, so the end phase lands in the same +// place every run. The Error is built inside an eval'd arrow so its frames are +// garbage by the time the throw propagates, which is what gives the end phase a +// stack to materialize. +// +// It is allocation-count sensitive, as that option is: on the unfixed debug +// build N of 3, 4 and 7 abort every run while 1, 2, 5, 6 and >= 8 print the +// error normally. Check the values that fire rather than one, since the counts +// shift with the build. A value that does not fire still has to print the +// error, so this cannot flake on a fixed build. +const ALLOCS_BETWEEN_GCS = [3, 4, 7]; + +test.each(ALLOCS_BETWEEN_GCS)("an uncaught Error survives a GC end phase (every %i allocations)", async n => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", `const e = eval("(() => Object.assign(new Error('c'), { code: 1 }))")(); throw e`], + env: { ...bunEnv, BUN_JSC_slowPathAllocsBetweenGCs: String(n) }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Bun prints `error: c` plus the `code: 1` property and a frame. The crash + // produces an assertion or a segfault instead, and a nonzero-but-not-1 code. + expect({ stdout, startsWithError: stderr.startsWith("error: c"), exitCode }).toEqual({ + stdout: "", + startsWithError: true, + exitCode: 1, + }); +}); diff --git a/test/js/bun/resolve/dynamic-import-evaluation-error-gc.test.ts b/test/js/bun/resolve/dynamic-import-evaluation-error-gc.test.ts new file mode 100644 index 000000000000..2249180322ca --- /dev/null +++ b/test/js/bun/resolve/dynamic-import-evaluation-error-gc.test.ts @@ -0,0 +1,72 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isWindows, tempDir } from "harness"; + +// computeErrorInfoWrapperToString (src/jsc/bindings/FormatStackTraceForJS.cpp, +// installed via vm.setOnComputeErrorInfo) runs from +// ErrorInstance::reconcileWeakReferencesAtGCEnd during Heap::runEndPhase, for +// any live Error whose stack frames now reference dead code. It used to clear +// whatever exception was pending after computing the stack string. With +// concurrent GC that end phase can land while the mutator is parked at a +// safepoint inside CyclicModuleRecord::evaluate, between step 9 (the module's +// evaluation error is the pending exception) and step 9.d +// (rejectWithCaughtException), which then finds no exception and crashes +// (`ASSERTION FAILED: exception` at JSPromise.cpp on assert builds, a SEGV at +// address 0x8 on release: Sentry BUN-4R6E). +// +// This is the plain import() face of the bug. Each iteration writes a fresh +// ESM graph with one member that throws at top level and one that uses +// top-level await. import(a) fails on the throwing member; import(b) reaches +// that already-errored member, so its Evaluate() rethrows the stored error and +// runs the step 9 -> 9.d window again. The race needs the accumulated graphs +// and GC cycles of a long loop; a single iteration does not fire, 100 fire on +// about 1 run in 6, 300 on essentially every run of the unfixed assert build. +// The fixture runs twice and a single crash fails the test. +// +// Skipped on Windows: two runs of a 300-iteration loop that writes 2100 files +// and evaluates 1200 module graphs are several times slower under +// Windows + ASAN in CI, and the fixed C++ path is not platform-specific. +test.skipIf(isWindows)( + "import() of a module whose evaluation throws survives a GC stack-trace finalizer", + async () => { + const fixture = ` + import { mkdirSync, writeFileSync } from "node:fs"; + import { join } from "node:path"; + + const root = join(import.meta.dir, "graphs"); + for (let it = 0; it < 300; it++) { + const d = join(root, "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(() => {}); + } + console.log("ok"); + `; + + using dir = tempDir("dynamic-import-eval-error-gc", { "fixture.mjs": fixture }); + + for (let i = 0; i < 2; i++) { + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.mjs"], + cwd: String(dir), + env: bunEnv, + stdio: ["ignore", "pipe", "pipe"], + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ok", exitCode: 0 }); + void stderr; + } + }, + // Two runs of a 300-iteration loop on a debug+ASAN build take well over the + // default 5s. + 120_000, +); diff --git a/test/js/bun/resolve/resolve-error.test.ts b/test/js/bun/resolve/resolve-error.test.ts index dd8be583fe5a..978f4947d126 100644 --- a/test/js/bun/resolve/resolve-error.test.ts +++ b/test/js/bun/resolve/resolve-error.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; +import { bunEnv, bunExe, isWindows, tempDir } from "harness"; import path from "node:path"; describe("ResolveMessage", () => { @@ -327,3 +327,54 @@ describe.concurrent("Bun.resolve() rejections are tracked", () => { expect(exitCode).toBe(0); }); }); + +// Bun.resolve() allocates its rejected promise while the exception the resolve +// threw is still pending on the VM. That allocation is a GC safepoint, and the +// concurrent collector's end phase materializes the stack of every live Error +// whose frames died, through the onComputeErrorInfo hook. The hook used to +// clear whatever exception was pending, so the rejection then found nothing: +// `panic: A JavaScript exception was thrown, but it was cleared before it could be read.` +// +// Each call comes from a fresh closure and the reasons are kept in a ring, so +// every collection has live Errors with a dead top frame to materialize. The +// calls have to be awaited one at a time (64 per tick never fires), and +// collectContinuously keeps an end phase always imminent. The unfixed debug +// build panics after 2500 to 8500 iterations. Not concurrent with the tests +// above: a busy machine starves the collector and hides the race. +// +// Skipped on Windows: collectContinuously is several times slower under +// Windows + ASAN in CI, and the fixed C++ path is not platform-specific. +it.skipIf(isWindows)( + "Bun.resolve() rejection survives a GC stack-trace finalizer", + async () => { + using dir = tempDir("bun-resolve-rejection-gc", { + "fixture.mjs": ` + const keep = []; + let rejections = 0; + for (let i = 0; i < 20000; i++) { + const call = () => Bun.resolve(); + await call().catch(e => { + rejections++; + keep.push(e); + if (keep.length > 1024) keep.shift(); + }); + } + console.log("ok rejections=" + rejections); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.mjs"], + env: { ...bunEnv, BUN_JSC_collectContinuously: "1" }, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ok rejections=20000", exitCode: 0 }); + void stderr; + }, + // 20000 awaited rejections under collectContinuously on a debug+ASAN build + // take well over the default 5s. + 120_000, +); diff --git a/test/js/node/vm/sourcetextmodule-link-gc.test.ts b/test/js/node/vm/sourcetextmodule-link-gc.test.ts index fab4c87bdd45..f10bcbfaf512 100644 --- a/test/js/node/vm/sourcetextmodule-link-gc.test.ts +++ b/test/js/node/vm/sourcetextmodule-link-gc.test.ts @@ -12,7 +12,7 @@ // ~9× per link() call. import { expect, test } from "bun:test"; -import { bunEnv, bunExe, isWindows } from "harness"; +import { bunEnv, bunExe, isWindows, tempDir } from "harness"; // collectContinuously is very slow under Windows + ASAN in CI; the code path // is identical on Linux/macOS, so skip Windows to keep duration reasonable. @@ -70,3 +70,147 @@ test.skipIf(isWindows)( }, 120_000, ); + +// computeErrorInfoWrapperToString (src/jsc/bindings/FormatStackTraceForJS.cpp, +// installed via vm.setOnComputeErrorInfo) runs from ErrorInstance:: +// finalizeUnconditionally during Heap::runEndPhase. It used to clear 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 and crashed +// (`ASSERTION FAILED: exception` at JSPromise.cpp, or a SEGV at 0x8 on release). +// The fix only swallows an exception the stack computation itself raised. +// +// The race depends on concurrent GC landing in that window: --smol shrinks the +// heap so collections run often enough to hit it on essentially every run of +// the unfixed assert build, while collectContinuously moves the window and +// hides it. The fixture is a real file (a -e eval script has no source URL, so +// the stack-trace finalizer takes a different path and doesn't fire). The +// processes run one at a time (concurrency starves the collector and hides the +// race); a single crash fails the test. +// +// Skipped on Windows for the same reason as the test above: four runs of a +// 60-iteration GC-churn loop are several times slower under Windows + ASAN in +// CI, and the fixed C++ path is not platform-specific. +// +// The fixture is shared with the terminate() test below, which only needs it to +// keep the finalizer busy for as long as the worker lives. +function evaluationErrorFixture(iterations: number) { + return ` + import * as vm from "node:vm"; + // Separate, pre-existing bug: the throwing top-level-await module below + // (shared by two importers) reports one unhandled rejection per graph even + // though every evaluate() is awaited and caught. Tolerate exactly that one + // so the crash under test stays the only way this process exits nonzero. + process.on("unhandledRejection", e => { + if (e instanceof URIError && e.message === "tla") return; + console.error("unexpected unhandled rejection:", e); + process.exit(3); + }); + const sl = async () => { await new Promise(r => setTimeout(r, 12)); for (let i = 0; i < 8; i++) await null; }; + const CA = async f => { try { return await f(); } catch (e) { return e; } }; + const ni = () => { throw new Error("noimports"); }; + const M = (src, id, o) => new vm.SourceTextModule(src, { identifier: id, ...o }); + + async function fam(sd) { + { const bad = M("throw new TypeError('boom')", "s1bad" + sd), a = M("import 'bad'; export const x=1;", "s1a" + sd), b = M("import 'bad'; export const y=2;", "s1b" + sd); + await a.link(() => bad); await b.link(() => bad); + try { await a.evaluate(); } catch {} try { await b.evaluate(); } catch {} try { await a.evaluate(); } catch {} + await CA(() => bad.evaluate()); } + { const ctx = vm.createContext({ log: [] }); + const leaf = M("log.push('L0'); export const v = await Promise.resolve(7); log.push('L1');", "s2l" + sd, { context: ctx }); + const mid = M("import {v} from 'l'; export const m=v+1;", "s2m" + sd, { context: ctx }); + const root = M("import {m} from 'm'; export const r=m+1;", "s2r" + sd, { context: ctx }); + const map = { l: leaf, m: mid }; await root.link(s => map[s]); + const ep = root.evaluate(); await CA(async () => { await ep; }); await CA(() => root.evaluate()); } + { const leaf = M("await 0; throw new URIError('tla');", "s3l" + sd), a = M("import 'l'; export const x=1;", "s3a" + sd), b = M("import 'l'; export const y=1;", "s3b" + sd); + await a.link(() => leaf); await b.link(() => leaf); + try { await a.evaluate(); } catch {} try { await b.evaluate(); } catch {} try { await leaf.evaluate(); } catch {} } + { const m = M("export const a=1;", "s5" + sd); await m.link(ni); await m.evaluate(); + const bad = M("null.x", "s5b" + sd); await bad.link(ni); await bad.evaluate().catch(() => {}); } + { const val = ["str", "num", "null", "undef", "obj", "sym"][sd % 6]; + const src = { str: "throw 'x'", num: "throw 42", null: "throw null", undef: "throw undefined", obj: "throw {a:1}", sym: "throw Symbol.iterator" }[val]; + const m = M(src, "s6" + sd); await m.link(ni); await CA(() => m.evaluate()); } + await sl(); + } + for (let sd = 0; sd < ${iterations}; sd++) await fam(sd); + console.log("ok"); + `; +} + +test.skipIf(isWindows)( + "vm.SourceTextModule evaluation error survives a concurrent-GC stack-trace finalizer", + async () => { + using dir = tempDir("vm-module-eval-error-gc", { "fixture.mjs": evaluationErrorFixture(60) }); + + async function run() { + await using proc = Bun.spawn({ + cmd: [bunExe(), "--smol", "--experimental-vm-modules", "fixture.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + stdout: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout: stdout.trim(), stderr, exitCode }; + } + + for (let i = 0; i < 4; i++) { + const { stdout, stderr, exitCode } = await run(); + expect({ stdout, exitCode }).toEqual({ stdout: "ok", exitCode: 0 }); + void stderr; + } + }, + 120_000, +); + +// The finalizer now sets the mutator's pending exception aside with a +// SuspendExceptionScope. That scope restores the slot blindly on exit, so a +// termination exception thrown inside the window (a RETURN_IF_EXCEPTION inside +// the stack computation services the worker's pending terminate() request) +// survived the clear and was then overwritten, leaving the NeedExceptionHandling +// trap bit set with no exception behind it. The next exception check asserted +// `!!exception == needHandling(NeedExceptionHandling)`. Termination is now +// deferred for the duration of the window (DeferTerminationForAWhile). +// +// Run the GC-churn fixture inside a Worker and terminate it mid-run, 20 times. +// Every run of the build with the suspend scope but without the deferral +// asserted; this guards that pairing rather than the original crash (it passes +// on a build with neither). Windows: same reason as above. +test.skipIf(isWindows)( + "terminate() while the stack-trace finalizer runs keeps the exception and trap state in sync", + async () => { + using dir = tempDir("vm-module-eval-error-gc-terminate", { + "fixture.mjs": evaluationErrorFixture(600), + "stress.mjs": ` + import { readFileSync } from "node:fs"; + const src = readFileSync(new URL("./fixture.mjs", import.meta.url), "utf8"); + const url = URL.createObjectURL(new Blob([src], { type: "text/javascript" })); + for (let r = 0; r < 20; r++) { + // smol: the worker VM gets its own heap; --smol on the parent does not reach it. + const w = new Worker(url, { type: "module", smol: true }); + // Listen before yielding: close is only dispatched if a listener exists + // when the worker goes away, so a worker that dies early must not hang us. + const closed = new Promise(res => w.addEventListener("close", res, { once: true })); + await new Promise(res => setTimeout(res, 150 + ((r * 37) % 400))); + w.terminate(); + await closed; + } + console.log("ok"); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "--smol", "--experimental-vm-modules", "stress.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + stdout: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), exitCode }).toEqual({ stdout: "ok", exitCode: 0 }); + void stderr; + }, + 120_000, +);