Preserve a pending exception across the GC stack-trace finalizer

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<StackFrame>& 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();
     }

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,
+  });
+});

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,
+);

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,
+);

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,
+);