destructOnExit: release nested JSLockHolder VM refs so ~VM runs - #34448
destructOnExit: release nested JSLockHolder VM refs so ~VM runs#34448robobun wants to merge 1 commit into
Conversation
When process.exit() is reached from inside a JSEventListener (e.g. worker.onmessage), JSEventListener::handleEvent's JSLockHolder holds a RefPtr<VM> on the stack. destructOnExit only released the two creation refs, so the VM stayed at refcount>=1, ~VM() never ran, and Heap::lastChanceToFinalize() was skipped. Any wrapper that survived collectNow() as conservatively reachable then leaked its native m_ctx. timer-heap-race.test.ts was flakily hitting this on the asan lane: the fixture calls process.exit(0) from worker.onmessage while a few just-fired Timeout wrappers are still conservatively live on the stack. Release every remaining VM ref. global_exit is noreturn so the stacked holders' destructors never run, and workers were joined by terminate_all_workers_and_wait, so no other thread uses the VM. Co-authored-by: Ciro Spaciari <ciro.spaciari@gmail.com>
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 1:10 AM PT - Jul 17th, 2026
❌ @robobun, your commit fafaab4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34448That installs a local version of the PR into your bun-34448 --bun |
|
Reproduced the original flake locally (~20/30 runs of the fixture under the asan-lane env). The new deterministic test ( Self-review clean (no surviving concerns). CI build 74417: The |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Shouldn't we instead rely on ContextDestructionObserver to release the lock? Can we do that?
I looked at that first. The extra ref isn't held by anything that registers with the context: it's To use the observer we'd have to change every call site that can be on the stack at The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. The refcount-loop change looks correct and is confined to the BUN_DESTRUCT_VM_ON_EXIT=1 asan lane, but it's a VM-lifetime change in JSC bindings (last loop iteration runs ~VM() under a live vm&), so worth a maintainer glance.
What was reviewed:
- Loop captures
refCount()once into a local, sovmisn't touched after the final deref frees it;runLoopis already held viaRef<>(line 4114) sothreadWillExit()after~VMis safe. - Confirmed
terminate_all_workers_and_waitruns beforedestructOnExitinVirtualMachine.rs:1564, matching the comment's "workers were already joined" claim. - New test mirrors the neighboring asan test's structure/env exactly; fixture pins the
TimeoutonglobalThisso the leak is deterministic rather than stack-layout-dependent.
Extended reasoning...
Overview
Fixes a flaky LSan leak in timer-heap-race.test.ts on the x64-asan lane. The root cause: when process.exit() is called from inside worker.onmessage, JSEventListener::handleEvent has a JSLockHolder (holding a RefPtr<VM>) on the stack, so the VM refcount at destructOnExit is ≥3. The old code released exactly two refs (the creation pair), so ~VM() — and therefore Heap::lastChanceToFinalize() — never ran, and any wrapper that collectNow() left conservatively reachable leaked its native m_ctx.
The fix replaces the two fixed derefSuppressingSaferCPPChecking() calls with for (unsigned refs = vm.refCount(); refs > 0; --refs) vm.deref...(). A new asan-only test with a fixture that pins a Timeout on globalThis before exiting from onmessage makes the leak deterministic.
Security risks
None. This is exit-path teardown code that only runs under BUN_DESTRUCT_VM_ON_EXIT=1 (the asan/leak CI lane). global_exit is noreturn, so nothing after this observes the freed VM. Release builds don't reach this path.
Level of scrutiny
Medium-high. Although the diff is tiny (~2 lines of logic), it's VM lifetime management in the JSC C++ bindings — the repo's most-scrutinized memory-safety category. I checked:
- The loop reads
refCount()once into a local before iterating, so it doesn't dereferencevmafter the lastderef()frees it. This is the same UAF shape the original two-deref code already had (second deref freed the VM when refcount was exactly 2), so no new hazard. runLoop->threadWillExit()after~VMis safe becauserunLoopis aRef<WTF::RunLoop>captured beforehand (line 4113-4114 already documents this).- The "workers were already joined" claim in the new comment is accurate —
terminate_all_workers_and_waitruns atVirtualMachine.rs:1564, beforedestructOnExitat line 1613. - The stack-resident
JSLockHolderRefPtr<VM>s become dangling after~VM, butglobal_exitis noreturn so their destructors never run — this is stated in both the PR description and the code comment.
Other factors
The PR description is unusually thorough: it names the exact mechanism, cites prior art on ciro/repl-node-tests (commit 19d8c87), and reports 0/50 vs ~20/30 flake rates plus 369 passing worker tests. The new test follows the neighboring asan test's pattern byte-for-byte (same env, same assertion shape via runFixture). The fixture wires worker.onerror to fail loudly. Blast radius is limited to the asan CI lane.
Deferring rather than approving because VM teardown ref-balancing is exactly the class of change the repo's review rules flag for maintainer eyes, even when it looks correct.
2 participants
What
test/js/web/timers/timer-heap-race.test.tswent flakily red on the x64-asan lane (seen in #34422's build 74213 and reported against build 74295):The test itself (added in #33131) is a crash-freedom stress of the timer heap under cross-thread
Atomics.waitAsynccancellation; the leak it is tripping over is a pre-existing main-thread VM teardown gap that the fixture happens to reach.Cause
The fixture calls
process.exit(0)fromworker.onmessage. That handler runs insideJSEventListener::handleEvent, which takes its ownJSLockHolder(holding aRefPtr<VM>). Whenglobal_exitreachesZig__GlobalObject__destructOnExit, the VM refcount is therefore at least 3 (two creation refs plus the handler's lock holder), butdestructOnExitonly released the two creation refs.~VM()never ran, soHeap::lastChanceToFinalize()was skipped.collectNow()still runs first, so mostJSTimeoutwrappers are swept and theirTimeoutObjectboxes freed viafinalize(). The flaky single leak is whichever wrapper the conservative stack scan happened to keep reachable (a stale encoded pointer from an earlierfire()frame): withoutlastChanceToFinalize(), that wrapper's destructor never runs and itsm_ctxbox (one 480-byteTimeoutObject) is reported by LSan.Fix
Release every remaining VM ref in
destructOnExitso~VM()always runs. The two creation refs are constant; anything beyond that is aRefPtr<VM>in aJSLockHolderstill on the stack.global_exitis noreturn so those holders' destructors never run, andterminate_all_workers_and_waithas already joined every worker thread, so no other thread can observe the freed VM.The same observation and a closely related fix already exist on the unmerged
ciro/repl-node-testsbranch (commit 19d8c87, for the REPL's mid-evalprocess.exit()). This PR takes the simplerrefCount()loop so the full~VM()teardown path runs rather than callinglastChanceToFinalize()directly.Verification
process.exit() from worker.onmessage finalizes the JSC heapreproduces the leak deterministically by pinning aTimeoutonglobalThisbefore exiting fromonmessage: fails on main (480-byteTimeoutObject), passes with the fix.bun bd test test/js/web/timers/timer-heap-race.test.ts: 4 pass.bun bd test test/js/web/workers/: 369 pass.destructOnExitonly runs underBUN_DESTRUCT_VM_ON_EXIT=1(the asan/leak lane), so release builds are unaffected.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · no surviving concerns
29 concerns were raised and did not survive verification.