Reach the VM of a deferred-work job through its client data, not the ticket's realm

src/jsc/bindings/BunClientData.cpp

@@ -55,6 +55,7 @@ JSHeapData::~JSHeapData() = default;
 
 JSVMClientData::JSVMClientData(VM& vm, RefPtr<JSC::SourceProvider> sourceProvider)
     : commonStrings(vm)
+    , deferredWorkTimer(vm)
     , m_builtinNames(vm)
     , m_builtinFunctions(makeUnique<JSBuiltinFunctions>(vm, sourceProvider, m_builtinNames))
     , m_heapData(JSHeapData::ensureHeapData(vm.heap))

src/jsc/bindings/JSCTaskScheduler.cpp

@@ -17,19 +17,16 @@ extern "C" void Bun__queueJSCDeferredWorkTaskConcurrently(const ::BunVmHandleRef
 
 class JSCDeferredWorkTask {
 public:
-    JSCDeferredWorkTask(Ref<Ticket> ticket, Task&& task)
-        : ticket(WTF::move(ticket))
+    JSCDeferredWorkTask(WebCore::JSVMClientData* clientData, Ref<Ticket> ticket, Task&& task)
+        : clientData(clientData)
+        , ticket(WTF::move(ticket))
         , task(WTF::move(task))
     {
     }
 
+    WebCore::JSVMClientData* clientData;
     Ref<Ticket> ticket;
     Task task;
-    ~JSCDeferredWorkTask()
-    {
-    }
-
-    JSC::VM& vm() const { return ticket->scriptExecutionOwner()->vm(); }
 
     WTF_MAKE_TZONE_ALLOCATED(JSCDeferredWorkTask);
 };
@@ -88,7 +85,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 +101,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<Lock> holder { scheduler.m_lock };
     bool wasPending = scheduler.m_pendingTicketsKeepingEventLoopAlive.remove(job->ticket.ptr());
     if (!wasPending) {
@@ -120,7 +121,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 +144,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 +153,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<Lock> 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<Lock> 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;
 }
 

src/jsc/bindings/JSCTaskScheduler.h

@@ -10,13 +10,17 @@ class JSVMClientData;
 namespace Bun {
 
 class JSCTaskScheduler {
+    WTF_MAKE_NONCOPYABLE(JSCTaskScheduler);
+
 public:
-    JSCTaskScheduler()
-        : m_pendingTicketsKeepingEventLoopAlive()
-        , m_pendingTicketsOther()
+    explicit JSCTaskScheduler(JSC::VM& vm)
+        : m_vm(vm)
     {
     }
 
+    // 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<JSC::DeferredWorkTimer::Ticket>&& ticket, JSC::DeferredWorkTimer::WorkType kind);
     static void onScheduleWorkSoon(WebCore::JSVMClientData* clientData, Ref<JSC::DeferredWorkTimer::Ticket>&& ticket, JSC::DeferredWorkTimer::Task&& task);
     static void onCancelPendingWork(WebCore::JSVMClientData* clientData, JSC::DeferredWorkTimer::Ticket& ticket);
@@ -38,6 +42,9 @@ class JSCTaskScheduler {
     // Value: the loop that was current when JSC registered the work; its completion is posted there.
     UncheckedKeyHashMap<Ref<JSC::DeferredWorkTimer::Ticket>, BunLoopKind> m_pendingTicketsKeepingEventLoopAlive;
     UncheckedKeyHashMap<Ref<JSC::DeferredWorkTimer::Ticket>, BunLoopKind> m_pendingTicketsOther;
+
+private:
+    JSC::VM& m_vm;
 };
 
 }

test/js/node/vm/vm.test.ts

@@ -2551,3 +2551,65 @@ 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. 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");
+    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 });
+});