HandleSet: assert the VM's API lock is held when mutating handles - #387
HandleSet: assert the VM's API lock is held when mutating handles#387robobun wants to merge 1 commit into
Conversation
HandleSet::allocate, deallocate, and writeBarrier mutate m_strongList, which the GC's strong-handles marking constraint scans. The API lock is what orders those mutations with the scan, so mutating from a thread that does not hold it is a data race. Heap::protect/unprotect already assert exactly this for the protect set; this adds the same assertion to the Strong handle set. The scheduling-dependent crash this turns deterministic: a Strong captured by value in a cross-thread lambda, destroyed on the other thread, corrupts the owner VM's m_strongList (oven-sh/bun#30185). That class of bug previously surfaced only as a rare segfault or livelock in the marking constraint; with this assertion any debug build reports the violating call site on the first mutation. Debug-only: assertMayMutate() compiles to an empty inline in release builds.
![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 issues, but this adds an assertion to every Strong handle allocate/deallocate/writeBarrier — one of the most fundamental paths in JSC — so a maintainer should confirm the invariant is truly universal before it lands.
What was reviewed:
- Confirmed
Heap::protect/unprotectalready assert the identical invariant, andShouldStrongDestructorGrabLock::Yestakes aJSLockHolderbefore deallocating — the invariant is already acknowledged in-tree. - Verified the header-cycle rationale (
Heap.hincludesHandleSet.h) and that the out-of-line shape mirrors the existingisLiveNodehelper; release builds get an empty inline. - Checked the teardown-safety claim:
m_apiLock(VM.h:439) is declared beforeheap(VM.h:470), so the lock outlives the HandleSet.
Extended reasoning...
Overview
This PR adds a debug-only assertion (assertMayMutate()) to HandleSet::allocate, deallocate, and writeBarrier, requiring that the owning VM's API lock is held by the current thread. The implementation is out-of-line in HandleSet.cpp under #if ASSERT_ENABLED (because HandleSet.h cannot include VM.h — Heap.h includes HandleSet.h), with an empty inline stub in release builds. Two files touched, ~20 lines added, no logic changed.
Security risks
None. This is a debug-only diagnostic that reads thread/lock state and calls ASSERT_WITH_MESSAGE. It is compiled out entirely in release builds. It cannot introduce injection, auth, or data-exposure issues.
Level of scrutiny
Higher than the diff size suggests. HandleSet::allocate/deallocate/writeBarrier sit underneath every Strong<T> construction, destruction, and assignment in the engine. Asserting an invariant here is asserting it across the entire JSC codebase and every downstream consumer (Bun, and any WebCore paths this fork retains). If any legitimate call site — VM construction edge cases, compiler-thread teardown, inspector paths, etc. — mutates a handle without the API lock held, every debug build breaks. The PR description addresses teardown ordering and reports 486 passing tests in Bun's worker suites, which is good evidence, but Bun's test surface is not the same as JSC's own stress/test262 surface.
Other factors
- The pattern is well-precedented:
Heap::protect/unprotectalreadyASSERT(vm().currentThreadIsHoldingAPILock())for the analogous protect set, andStrong'sShouldStrongDestructorGrabLock::Yesvariant explicitly takes aJSLockHolderbefore callingdeallocate. So the invariant being asserted is not novel — it is already documented behaviorally elsewhere in the tree. - The out-of-line-under-ASSERT / inline-empty-otherwise shape exactly mirrors the existing
isLiveNodehelper in the same file, so it is idiomatic for this code. - The PR description notes further suite coverage is still being run via a pinned Bun PR — results from that broader run would be useful signal.
Given the guideline that changes touching critical code paths should get human eyes even when they look correct, I'm deferring rather than approving.
WalkthroughSummary
ChangesHandleSet mutation safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Preview Builds
|
|
Validation of this change inside bun is done in oven-sh/bun#36958, which pins this PR's preview build (
Once bun's CI is green over the preview build, this is ready to merge; the bun PR then flips its pin to the merge commit. |
|
Heads-up for the next upstream merge: WebKit/WebKit@ff64aee116 (Aug 3, "[JSC] Introduce StrongBlock") deletes HandleSet.{h,cpp} in favor of StrongSet/StrongBlock, so this change will surface as a delete/modify conflict and needs porting:
bun-side enforcement exists as of oven-sh/bun#36958: a debug-lane test deliberately violates the contract from another thread and asserts the child dies with this assertion's message, so a bump that drops the port fails bun's debug CI instead of silently losing the detector. |
1 participant
What does this PR do?
Adds a debug-only assertion to
HandleSet::allocate,deallocate, andwriteBarrier: the owning VM's API lock must be held by the current thread.These three functions mutate
m_strongList, which the GC's strong-handles marking constraint scans. The API lock is what orders those mutations with the scan, so a mutation from a thread that does not hold it is a data race.Heap::protect/unprotectalready assert exactly this for the protect set; the Strong handle set never got the same assertion.Strong's ownShouldStrongDestructorGrabLock::Yesvariant (which takes aJSLockHolderbefore deallocating) is existing acknowledgement of the invariant.The motivating bug class is a
Strongcaptured by value in a lambda that another thread destroys: the destructor then unlinks a node from the owner VM'sm_strongListwith no synchronization against that VM's GC. oven-sh/bun#30185 was exactly this (worker thread destroying a parent-VMStrong<JSPromise>captured in a cross-thread task), and it surfaced only as a rare, scheduling-dependent segfault at 0x10 (or a livelock) in the "Sh" marking constraint. Measurements in oven-sh/bun#36952 show that after bun's GC scheduling changed, the same reintroduced bug produced 0 crashes in 18,000 iterations of the old probabilistic stress guard, i.e. the bug class had no effective detector left.With this assertion, any debug build reports the violating call site deterministically on the first mutation.
Implementation notes
assertMayMutate()is out-of-line in HandleSet.cpp because HandleSet.h cannot include VM.h (Heap.h includes HandleSet.h). Release builds get an empty inline and compile it out. Same shape as the existingisLiveNodedebug helper.m_apiLockis declared beforeheap, so memberStrongs and the Heap's HandleSet are destroyed while the lock object is still alive, andVM::~VMalready asserts the lock is held.Verification
Validated in a bun debug+ASAN build linked against this JSC (linux x64):
Strong<JSPromise>capture inWorker.getHeapSnapshot()'s cross-thread lambda: the assertion fires on the first round-trip, every run, naming this invariant (previously: 0 detections in 18,000 iterations of the stress workload).