From 9d0bbcad8c1c94b3cce71f447ac66a36ee37b458 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 17 Sep 2026 13:43:36 +0000 Subject: [PATCH 1/4] Reach the VM of a deferred-work job through its client data, not the ticket's realm Bun__runDeferredWork and Bun__deleteDeferredWorkTask read ticket->scriptExecutionOwner()->vm() before they checked the ticket. The realm that owns the ticket can be destructed by then: a global that bun test --isolate retired, or a dead node:vm context. ~JSGlobalObject cancels the ticket but leaves the owner pointer in place, so the read hit a destructed cell. This is the crash in #43056. The job now carries its JSVMClientData, and JSCTaskScheduler stores the VM it was created for. Both entry points use that. The existing isCancelled() check then skips the job. This is the Bun-only part of #39994. A ticket whose realm died but was not swept yet is still not cancelled, and its job still reads the dead target. That needs the WebKit change in #39994. --- src/jsc/bindings/BunClientData.cpp | 1 + src/jsc/bindings/JSCTaskScheduler.cpp | 43 ++++++++++--------- src/jsc/bindings/JSCTaskScheduler.h | 17 ++++++-- test/js/node/vm/vm.test.ts | 60 +++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 25 deletions(-) diff --git a/src/jsc/bindings/BunClientData.cpp b/src/jsc/bindings/BunClientData.cpp index 4defb040a138..0044755e5a2b 100644 --- a/src/jsc/bindings/BunClientData.cpp +++ b/src/jsc/bindings/BunClientData.cpp @@ -55,6 +55,7 @@ JSHeapData::~JSHeapData() = default; JSVMClientData::JSVMClientData(VM& vm, RefPtr sourceProvider) : commonStrings(vm) + , deferredWorkTimer(vm) , m_builtinNames(vm) , m_builtinFunctions(makeUnique(vm, sourceProvider, m_builtinNames)) , m_heapData(JSHeapData::ensureHeapData(vm.heap)) diff --git a/src/jsc/bindings/JSCTaskScheduler.cpp b/src/jsc/bindings/JSCTaskScheduler.cpp index 55460ce2dfd1..0597dca3791d 100644 --- a/src/jsc/bindings/JSCTaskScheduler.cpp +++ b/src/jsc/bindings/JSCTaskScheduler.cpp @@ -17,19 +17,18 @@ extern "C" void Bun__queueJSCDeferredWorkTaskConcurrently(const ::BunVmHandleRef class JSCDeferredWorkTask { public: - JSCDeferredWorkTask(Ref ticket, Task&& task) - : ticket(WTF::move(ticket)) + JSCDeferredWorkTask(WebCore::JSVMClientData* clientData, Ref ticket, Task&& task) + : clientData(clientData) + , ticket(WTF::move(ticket)) , task(WTF::move(task)) { } + // The ticket's realm can be dead by the time the job runs. The VM and the + // scheduler come from here, not from the ticket (JSCTaskScheduler::vm()). + WebCore::JSVMClientData* clientData; Ref ticket; Task task; - ~JSCDeferredWorkTask() - { - } - - JSC::VM& vm() const { return ticket->scriptExecutionOwner()->vm(); } WTF_MAKE_TZONE_ALLOCATED(JSCDeferredWorkTask); }; @@ -88,7 +87,7 @@ void JSCTaskScheduler::onScheduleWorkSoon(WebCore::JSVMClientData* clientData, R // Outside m_lock (markShuttingDown, on the VM's thread, needs it): a post that // still races the shutdown lands on the VM handle, which either queues it for // the teardown to release unrun or refuses it and runs the job's release path. - auto* job = new JSCDeferredWorkTask(WTF::move(ticket), WTF::move(task)); + auto* job = new JSCDeferredWorkTask(clientData, WTF::move(ticket), WTF::move(task)); Bun__queueJSCDeferredWorkTaskConcurrently(clientData->vmHandle, job, loopKind); } @@ -104,8 +103,12 @@ void JSCTaskScheduler::onCancelPendingWork(WebCore::JSVMClientData* clientData, Bun__VmHandle__refKeepAlive(vmHandle, BunLoopKind::Regular, -1); } -static void runPendingWork(const ::BunVmHandleRef* vmHandle, Bun::JSCTaskScheduler& scheduler, JSCDeferredWorkTask* job) +static void runPendingWork(JSCDeferredWorkTask* job) { + auto* clientData = job->clientData; + auto* vmHandle = clientData->vmHandle; + auto& scheduler = clientData->deferredWorkTimer; + Locker holder { scheduler.m_lock }; bool wasPending = scheduler.m_pendingTicketsKeepingEventLoopAlive.remove(job->ticket.ptr()); if (!wasPending) { @@ -120,7 +123,7 @@ static void runPendingWork(const ::BunVmHandleRef* vmHandle, Bun::JSCTaskSchedul // event-loop callback boundary, an exception a task lets escape is // reported as uncaught here rather than left on the VM for the next entry. if (wasPending && !job->ticket->isCancelled() && Bun__VmHandle__scriptAllowed(vmHandle)) { - auto& vm = job->vm(); + auto& vm = scheduler.vm(); auto* globalObject = job->ticket->target()->globalObject(); // The realm's own status, as DeferredWorkTimer::doWork asks it before it runs a // task. A realm that `bun test --isolate` retired reports Stopped, so the @@ -143,10 +146,7 @@ static void runPendingWork(const ::BunVmHandleRef* vmHandle, Bun::JSCTaskSchedul extern "C" void Bun__runDeferredWork(Bun::JSCDeferredWorkTask* job) { - auto& vm = job->vm(); - auto clientData = WebCore::clientData(vm); - - runPendingWork(clientData->vmHandle, clientData->deferredWorkTimer, job); + runPendingWork(job); } // Reclaim a queued-but-never-dispatched job during shutdown. Called while the @@ -155,14 +155,13 @@ extern "C" void Bun__runDeferredWork(Bun::JSCDeferredWorkTask* job) // ticket take() so the pending set and event-loop ref stay balanced. extern "C" void Bun__deleteDeferredWorkTask(Bun::JSCDeferredWorkTask* job) { - if (auto* clientData = WebCore::clientData(job->vm())) { - auto& scheduler = clientData->deferredWorkTimer; - Locker holder { scheduler.m_lock }; - bool wasKeepingAlive = dropPendingTicketLocked(scheduler, job->ticket.ptr()); - holder.unlockEarly(); - if (wasKeepingAlive) - Bun__VmHandle__refKeepAlive(clientData->vmHandle, BunLoopKind::Regular, -1); - } + auto* clientData = job->clientData; + auto& scheduler = clientData->deferredWorkTimer; + Locker holder { scheduler.m_lock }; + bool wasKeepingAlive = dropPendingTicketLocked(scheduler, job->ticket.ptr()); + holder.unlockEarly(); + if (wasKeepingAlive) + Bun__VmHandle__refKeepAlive(clientData->vmHandle, BunLoopKind::Regular, -1); delete job; } diff --git a/src/jsc/bindings/JSCTaskScheduler.h b/src/jsc/bindings/JSCTaskScheduler.h index a89a6dd3048c..2399da77b101 100644 --- a/src/jsc/bindings/JSCTaskScheduler.h +++ b/src/jsc/bindings/JSCTaskScheduler.h @@ -10,13 +10,21 @@ class JSVMClientData; namespace Bun { class JSCTaskScheduler { + WTF_MAKE_NONCOPYABLE(JSCTaskScheduler); + public: - JSCTaskScheduler() - : m_pendingTicketsKeepingEventLoopAlive() - , m_pendingTicketsOther() + explicit JSCTaskScheduler(JSC::VM& vm) + : m_vm(vm) { } + // A posted job reaches its VM through here, never through its ticket. The + // realm that owns the ticket can be destructed before the job runs (a global + // that `bun test --isolate` retired, a dead node:vm context). ~JSGlobalObject + // only cancels the ticket; its scriptExecutionOwner() still points at the + // destructed realm. + JSC::VM& vm() const { return m_vm; } + static void onAddPendingWork(WebCore::JSVMClientData* clientData, Ref&& ticket, JSC::DeferredWorkTimer::WorkType kind); static void onScheduleWorkSoon(WebCore::JSVMClientData* clientData, Ref&& ticket, JSC::DeferredWorkTimer::Task&& task); static void onCancelPendingWork(WebCore::JSVMClientData* clientData, JSC::DeferredWorkTimer::Ticket& ticket); @@ -38,6 +46,9 @@ class JSCTaskScheduler { // Value: the loop that was current when JSC registered the work; its completion is posted there. UncheckedKeyHashMap, BunLoopKind> m_pendingTicketsKeepingEventLoopAlive; UncheckedKeyHashMap, BunLoopKind> m_pendingTicketsOther; + +private: + JSC::VM& m_vm; }; } diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index 71da4f554818..93a58ff59514 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -2551,3 +2551,63 @@ test.skipIf(memoryForLongStrings < 10 * 1024 ** 3)( }, 30_000, ); + +// A FinalizationRegistry cleanup job is posted to the event loop for a context. +// The context then dies and is swept before the job runs. ~JSGlobalObject only +// cancels the job's ticket; the job must not reach the VM through the +// ticket's destructed realm. On a debug build this was the assertion +// `!isCancelled()` in DeferredWorkTimer::Ticket::scriptExecutionOwner(). +test("a FinalizationRegistry cleanup job of a context that was destructed before it ran does not read the dead context", async () => { + const fixture = ` + const vm = require("node:vm"); + const { releaseWeakRefs } = require("bun:jsc"); + + // The first cells of the context subspace are precise allocations. Keep 8 + // contexts alive so the contexts under test land in a block. + const keep = []; + for (let i = 0; i < 8; i++) keep.push(vm.createContext({})); + + const collectedRounds = new Set(); + const observer = new FinalizationRegistry(round => collectedRounds.add(round)); + + let round = 0; + function deep(depth, fn) { + if (depth === 0) return fn(); + const r = deep(depth - 1, fn); + return r; + } + function setup() { + const sandbox = {}; + observer.register(sandbox, round); + const context = vm.createContext(sandbox); + vm.runInContext( + "globalThis.registry = new FinalizationRegistry(() => {}); for (let i = 0; i < 4; i++) registry.register({}, i);", + context, + ); + // The targets are dead and the registry is alive: its cleanup job is posted. + Bun.gc(true); + } + + for (round = 0; round < 5; round++) { + // Create the context well below the frames the rest of the round runs in. + deep(500, setup); + // The shared context structure roots the most recently created context. + vm.createContext({}); + // createContext() holds the sandbox in a WeakRef until the turn ends. + releaseWeakRefs(); + // The context dies and is swept here, before the posted job runs. + Bun.gc(true); + for (let i = 0; !collectedRounds.has(round) && i < 50; i++) await Bun.sleep(1); + } + if (collectedRounds.size === 0) throw new Error("no round collected its context"); + console.log("done"); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ stdout: "done\n", stderr: "", exitCode: 0 }); +}); From 899452f957946e30cd6bf90c380ca29b996d6eb5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:02:34 +0000 Subject: [PATCH 2/4] test: say that only a debug build observes the dead-owner read --- test/js/node/vm/vm.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index 93a58ff59514..eee6b6620948 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -2555,8 +2555,10 @@ test.skipIf(memoryForLongStrings < 10 * 1024 ** 3)( // A FinalizationRegistry cleanup job is posted to the event loop for a context. // The context then dies and is swept before the job runs. ~JSGlobalObject only // cancels the job's ticket; the job must not reach the VM through the -// ticket's destructed realm. On a debug build this was the assertion -// `!isCancelled()` in DeferredWorkTimer::Ticket::scriptExecutionOwner(). +// ticket's destructed realm. Only a debug build observes that read: it trips +// the assertion `!isCancelled()` in DeferredWorkTimer::Ticket::scriptExecutionOwner(). +// A release build reads the swept cell's block header, which still names the +// right VM, so a green release run is not coverage for this read. test("a FinalizationRegistry cleanup job of a context that was destructed before it ran does not read the dead context", async () => { const fixture = ` const vm = require("node:vm"); From fe07020edb1fb0cbb8ae947a39f9a9c04181cab8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:04:50 +0000 Subject: [PATCH 3/4] Shorten the scheduler comments --- src/jsc/bindings/JSCTaskScheduler.cpp | 2 -- src/jsc/bindings/JSCTaskScheduler.h | 7 ++----- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/jsc/bindings/JSCTaskScheduler.cpp b/src/jsc/bindings/JSCTaskScheduler.cpp index 0597dca3791d..f00feea34987 100644 --- a/src/jsc/bindings/JSCTaskScheduler.cpp +++ b/src/jsc/bindings/JSCTaskScheduler.cpp @@ -24,8 +24,6 @@ class JSCDeferredWorkTask { { } - // The ticket's realm can be dead by the time the job runs. The VM and the - // scheduler come from here, not from the ticket (JSCTaskScheduler::vm()). WebCore::JSVMClientData* clientData; Ref ticket; Task task; diff --git a/src/jsc/bindings/JSCTaskScheduler.h b/src/jsc/bindings/JSCTaskScheduler.h index 2399da77b101..92f0a492f4d8 100644 --- a/src/jsc/bindings/JSCTaskScheduler.h +++ b/src/jsc/bindings/JSCTaskScheduler.h @@ -18,11 +18,8 @@ class JSCTaskScheduler { { } - // A posted job reaches its VM through here, never through its ticket. The - // realm that owns the ticket can be destructed before the job runs (a global - // that `bun test --isolate` retired, a dead node:vm context). ~JSGlobalObject - // only cancels the ticket; its scriptExecutionOwner() still points at the - // destructed realm. + // A job's ticket can outlive its realm; ~JSGlobalObject cancels the ticket + // but leaves scriptExecutionOwner() dangling. Jobs reach the VM through here. JSC::VM& vm() const { return m_vm; } static void onAddPendingWork(WebCore::JSVMClientData* clientData, Ref&& ticket, JSC::DeferredWorkTimer::WorkType kind); From b0e8590d9daa8cb7dccd762a7586244624c84e3a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:06:51 +0000 Subject: [PATCH 4/4] Shorten the vm() comment to one line --- src/jsc/bindings/JSCTaskScheduler.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/jsc/bindings/JSCTaskScheduler.h b/src/jsc/bindings/JSCTaskScheduler.h index 92f0a492f4d8..4341a0cf55ad 100644 --- a/src/jsc/bindings/JSCTaskScheduler.h +++ b/src/jsc/bindings/JSCTaskScheduler.h @@ -18,8 +18,7 @@ class JSCTaskScheduler { { } - // A job's ticket can outlive its realm; ~JSGlobalObject cancels the ticket - // but leaves scriptExecutionOwner() dangling. Jobs reach the VM through here. + // Jobs take the VM from here, not from their ticket's scriptExecutionOwner(). JSC::VM& vm() const { return m_vm; } static void onAddPendingWork(WebCore::JSVMClientData* clientData, Ref&& ticket, JSC::DeferredWorkTimer::WorkType kind);