Conversation
…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.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 3 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 3 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 3 minutes for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughChangesThe scheduler now retains its VM through Deferred work VM ownership
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The deferred-work ownership update has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the lifetime of the raw clientData pointer each job now carries: queued jobs are released via release_unrun during close_and_wait (which only publishes Closed after the ticket count hits zero and every queued task is serviced), so Bun__deleteDeferredWorkTask runs before ~JSVMClientData on the normal teardown path. The refused-post path in Bun__queueJSCDeferredWorkTaskConcurrently dereferences job->clientData the same way the old code dereferenced the ticket's owner, so it is not a new exposure.
Extended reasoning...
Two confirmed findings are posted inline (the remaining stale target()->globalObject() read at JSCTaskScheduler.cpp:127 and the regression test not failing on a release build), so a human still needs to weigh in. This note records what else was examined: the new unowned JSVMClientData* on JSCDeferredWorkTask is live for every run/release path reachable via VmHandle::close_and_wait (src/jsc/VmHandle.rs:440-479), and the Posted::Refused branch in src/jsc/JSCScheduler.rs:62-69 was already dereferencing dead-realm memory before this change, so the PR does not widen that window. The refcount balance in runPendingWork and Bun__deleteDeferredWorkTask is unchanged by the refactor (same remove/dropPendingTicketLocked + refKeepAlive(-1) pairing).
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
|
Updated 7:37 AM PT - Sep 17th, 2026
✅ @robobun, your commit b0e8590d9daa8cb7dccd762a7586244624c84e3a passed in 🧪 To try this PR locally: bunx bun-pr 43085That installs a local version of the PR into your bun-43085 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the latest push and found no new bugs; the follow-up commits addressed the test-comment point from my earlier review. Since this is lifetime code in the JSC deferred-work glue and I could not build the debug binary in this environment to run the new fixture, a human look before merge is still worthwhile.
What was reviewed:
JSVMClientDatainitializer order:deferredWorkTimer(vm)sits betweencommonStringsandm_builtinNames, matching the member declaration order in BunClientData.h, so no reorder hazard.- The dropped
if (clientData)guard inBun__deleteDeferredWorkTask: the old path dereferencedjob->vm()(the dead owner) before the null check ever ran, so the guard protected nothing; the new rawjob->clientDatapointer has the same "released before ~VM" contract the Rustrelease_unrun/refused-post path already relies on, so no new lifetime assumption is introduced. runPendingWorknow reaches the VM viascheduler.vm()before thetarget()read, which stays behind the!isCancelled()check as on base.
Extended reasoning...
Overview
The change replaces JSCDeferredWorkTask::vm() (which walked ticket->scriptExecutionOwner()->vm(), a pointer into a realm that ~JSGlobalObject cancels but does not clear) with two stable handles: a WebCore::JSVMClientData* stored on each job at post time, and a JSC::VM& stored on JSCTaskScheduler at construction. Bun__runDeferredWork and Bun__deleteDeferredWorkTask now derive the VM handle, scheduler and VM from those. Since my prior review, the only diffs are shortened comments in the header/cpp and the test comment stating that only a debug build observes the assertion, which is what my optional inline note asked for.
Security risks
None of the usual kind (no user-controlled parsing, auth, or network surface). The relevant risk class is memory safety: a stale pointer read on the event-loop path. The fix removes one such read; the remaining target()->globalObject() read behind the !isCancelled() gate is unchanged from base and is explicitly scoped out to #39994 in the description, matching what I noted inline last time.
Level of scrutiny
Moderate-to-high. The diff is small (~40 lines of C++) but sits in the JSC/event-loop lifetime glue, which is the repo's most-blocked category. I checked the constructor initializer order against the header, confirmed WTF_MAKE_NONCOPYABLE resolves through the existing DeferredWorkTimer.h include, and traced the Rust release_unrun and Posted::Refused paths in src/jsc/JSCScheduler.rs and src/jsc/VmHandle.rs to confirm the new raw clientData pointer carries the same lifetime contract the old vm.clientData lookup did. The removed null guard in Bun__deleteDeferredWorkTask was unreachable-as-protection because the old code dereferenced the dead owner before reaching it.
Other factors
WebKit headers and a debug build are not present in this checkout, so I could not run the new vm.test.ts fixture or verify the claimed 3/3 debug-build failure on the unfixed base; the PR description's own verification note and CI are the evidence for that. The test itself is hermetic (spawned -e fixture, concurrent pipe drain, combined {stdout, stderr, exitCode} assertion, bounded poll). The bug hunt ran dry with no findings. Given the code-owner sensitivity of the JSC bindings and the pending #39994 interplay, a human confirmation before merge is the safer call rather than an automated approve.
This branch has not been deployed
1 participant
Problem
bun test --isolatesegfaults at exit after every test passed (bun test --isolate: intermittent segfault in Bun__runDeferredWork at exit after all tests pass (1.4.2, Linux x64, JIT off) #43056, Bun 1.4.2):Segmentation fault at address 0x9670inJSCTaskScheduler.cpp:31: vmunderBun__runDeferredWork.JSCDeferredWorkTask::vm()(src/jsc/bindings/JSCTaskScheduler.cpp:32) readticket->scriptExecutionOwner()->vm()before theisCancelled()check. The realm that owns the ticket can be destructed by then (a global that--isolateretired, a deadnode:vmcontext).~JSGlobalObjectcancels the ticket but leaves the owner pointer in place.Bun__deleteDeferredWorkTaskhad the same read.Fix
JSVMClientData, andJSCTaskSchedulerstores the VM it was created for.Bun__runDeferredWorkandBun__deleteDeferredWorkTaskreach the VM, the scheduler and the VM handle through that. The existingisCancelled()check inrunPendingWorkthen skips the job without a read of the ticket's owner or target.runPendingWork(target()->globalObject()). That is the Sentry face (takeDeadHoldingsValueon a destructed registry) and it stays with Let JSC's DeferredWorkTimer own the deferred-work tickets so a collection cancels a dead realm's work (WebKit bump) #39994.test/js/node/vm/vm.test.tsfails on an unfixed debug build (ASSERTION FAILED: !isCancelled()inTicket::scriptExecutionOwner(), 3 of 3 runs) and passes with the fix. A release build does not crash on that fixture: the sweptNodeVMGlobalObjectcell still sits in a live block, so the stale read returns the right VM. Also ran all ofvm.test.ts,test/cli/test/isolation.test.ts,test/js/web/atomics.test.ts,test/regression/issue/39900.test.ts,worker.test.tsandworker_threads.test.ts.Background
DeferredWorkTimercarries native completions (FinalizationRegistry cleanup, wasm compilation,Atomics.waitAsync) to the JS thread, oneTicketeach. A ticket records the realm that asked for the work as itsscriptExecutionOwner.onScheduleWorkSoonwraps the ticket and the task in aJSCDeferredWorkTaskand posts it to Bun's event loop. The job holds aRef<Ticket>, so the ticket outlives its realm.~JSGlobalObjectcallscancelPendingWorkSafe, which setsm_isCancelledon every ticket of that realm. It does not clear the owner pointer. JSC's ownscriptExecutionOwner()asserts!isCancelled()for that reason.Zig::GlobalObject(the main realm, a ShadowRealm, each--isolatefile) is larger thanMarkedSpace::largeCutoff, so it is a precise allocation. A collection destructs and frees it before it returns. That is why the--isolateface reads freed memory in release builds.Notes
The fixture: a
node:vmcontext creates aFinalizationRegistrywith four dead targets.Bun.gc(true)at GC end posts the registry's cleanup job. The context is then dropped andBun.gc(true)sweeps it before the event loop runs the job.releaseWeakRefs()clears theWeakRefthatcreateContext()holds on the sandbox, and one extracreateContext()unroots the previous context from the shared structure (#39990). Eight kept contexts fill the precise tier of the subspace so the contexts under test land in a block, and the context is created 500 frames deep so that no stale slot of the round's frame keeps it alive. The test needs at least one round to collect its context.I could not get the
--isolatetiming itself to fire with a constructed fixture (0 of 30 release runs). A ShadowRealm variant of the fixture also does not post a cleanup job for a dead realm, which I did not chase.Under a debug build the unfixed binary aborts with
ASSERTION FAILED: !isCancelled()atDeferredWorkTimer.h(150)on the current WebKit pin (000c48997255) and on the previous one. That assert is the same read as the issue'sHeapCellInlines.h: vmframe.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/vm/vm.test.ts