StrongRootBlock: relink blocks without the cell-validating write barrier - #38975
StrongRootBlock: relink blocks without the cell-validating write barrier#38975robobun wants to merge 4 commits into
Conversation
StrongRootBlock::setNext went through WriteBarrier::setMayBeNull, whose
GC-validation path calls classInfo() on the block being linked. The list
is relinked from Bun__StrongRef__delete, which a wrapper's finalizer
reaches while JSC is sweeping (Listener::deinit dropping its `data`
Strong, for example), and JSC asserts that classInfo() is not read during
a sweep. Releasing the last slot of a block that sits between two live
blocks therefore aborted debug builds with
ASSERTION FAILED: vm().currentThreadIsHoldingAPILock() =>
vm().heap.mutatorState() != MutatorState::Sweeping
Store the link with setWithoutWriteBarrier and run vm.writeBarrier()
explicitly. The generational barrier is still needed: acquire() links the
reused old spare ahead of blocks allocated since the last collection, and
the slot store that follows does not remember the block when the value is
not a cell.
Tests: releasing a finalizer-held Strong out of a middle block (fails on
the unfixed debug build), and an eden GC after an old block is relinked
ahead of new ones (fails if the barrier is dropped).
|
Warning Review limit reached
Next review available in: 56 seconds 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 (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesStrongRootBlock GC behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:13 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
|
Status: fix and tests pushed, review in progress. Reproduced on an unfixed debug build ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes write-barrier semantics on a GC-rooted linked list during sweep — where correctness depends on JSC-internal guarantees about Heap::writeBarrier being sweep-safe and sufficient for the generational remembered set — a human familiar with JSC's GC should confirm.
What was reviewed:
setNextdecomposition is behaviorally equivalent tosetMayBeNullminus the debug-onlyvalidateCell— the store and barrier are unchanged, andvm.writeBarrier(this, nullptr)(fromrelease's trailingsetNext(vm, nullptr)) is a no-op in JSC.- Traced
acquire/releasein StrongRootBlock.cpp: the "old spare linked ahead of new blocks" scenario the barrier protects is real, and the second test's block arithmetic checks out. - Both new tests follow harness conventions (port 0, concurrent pipe drain, stderr-first assertions); the exact-layout dependency on
CAP=960and zero startup Strongs is self-checked via the printed block counts.
Extended reasoning...
Overview
The PR changes one method in src/jsc/bindings/StrongRootBlock.h: setNext now uses m_next.setWithoutWriteBarrier(next) + vm.writeBarrier(this, next) instead of m_next.setMayBeNull(vm, this, next). This drops only the debug-build validateCell type check on next, which calls classInfo() — forbidden while JSC is in MutatorState::Sweeping. The path is reached when a wrapper's finalizer (running during sweep) drops the last bun_jsc::Strong in a block that sits between two other live blocks. Two new subprocess tests are added to test/js/web/timers/timer-gc-roots.test.ts: one reproduces the debug assertion by arranging a middle-block unlink during sweep, and one guards against dropping the barrier entirely by proving an eden GC would collect new blocks linked behind a re-used old spare.
Security risks
None. This is a debug-assertion fix in GC bookkeeping; release builds already compile validateCell to nothing, so the store + barrier sequence is byte-identical there. No user-controlled input reaches this path.
Level of scrutiny
High. StrongRootBlock is the backing store for every bun_jsc::Strong handle in the runtime, and the change reasons about generational GC invariants (remembered set, eden vs full collection, sweep-time restrictions). Getting the barrier wrong in either direction means either a debug crash (status quo) or a use-after-free of everything a block roots (if the barrier were dropped). The PR description is unusually thorough — it names the exact assertion, stack, mechanism, and cites #37008 as precedent for the same pattern — and the second test empirically proves the barrier is load-bearing. But confirming that Heap::writeBarrier(from, to) itself is safe to call during sweep (the PR asserts it "only reads structures and cell states") requires JSC-internal knowledge I'd want a maintainer to sign off on.
Other factors
I verified the decomposition against what WriteBarrierBase<T>::setMayBeNull does: it is exactly validateCell (debug, non-null only) + setEarlyValue (= setWithoutWriteBarrier) + vm.writeBarrier(owner, value), so the new code is semantically identical in release and drops only the problematic debug check. release() calls setNext(vm, nullptr) at the end, and Heap::writeBarrier early-returns on a null to, so that path is unchanged. The test block-count arithmetic (6 blocks armed, 3*CAP + CAP+1 protected Timeouts, blocksOld: 2 / blocksNew: 5) all checks out against capacity = 960 and the acquire/release logic in StrongRootBlock.cpp. The tests are intricate and will break if startup ever allocates a Strong before user code or if capacity changes, but the blocksAtStart: 0 / blocksArmed: 6 assertions make that failure loud rather than silent — which is the right trade-off for a test that needs exact layout control. #37842 is noted as removing this code entirely on a future WebKit bump, so this is a targeted stopgap.
|
On the open question (whether
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The 5-line setNext change decomposes setMayBeNull into its raw store + explicit vm.writeBarrier, dropping only the debug-time validateCell, and the two new tests pin both the sweep-time relink and the eden-GC barrier requirement. Because this sits in JSC GC write-barrier semantics — and the author flagged whether Heap::writeBarrier is safe mid-sweep as an open question for confirmation — a human look would still be worthwhile.
What was reviewed:
- Confirmed the new sequence matches what
setMayBeNullalready executed in release builds (store +vm.writeBarrier(owner, value)), so no behavior change outside dropping the debugclassInfo()read. - Checked all three
setNextcall sites inStrongRootBlock.cpp(acquire prepend, release middle-unlink, release null-out) — the null case already went throughvm.writeBarrier(this, nullptr)on main, so no new edge. - Tests: subprocess pipes drained concurrently,
port: 0, assertions on exact block counts self-check theCAP == capacityand no-Strongs-at-start assumptions rather than silently passing.
Extended reasoning...
Overview
The PR changes one method in src/jsc/bindings/StrongRootBlock.h: setNext previously called m_next.setMayBeNull(vm, this, next); it now does m_next.setWithoutWriteBarrier(next) followed by vm.writeBarrier(this, next). This drops the debug-only validateCell<StrongRootBlock*> step (which reads classInfo() and trips validateIsNotSweeping when a finalizer releases the last Strong in a middle block during a sweep) while keeping the identical raw store and generational barrier that release builds already execute. Two regression tests are added to the existing test/js/web/timers/timer-gc-roots.test.ts: one that reproduces the debug assertion via a middle-block unlink from a Listener finalizer, and one that guards the barrier by verifying an old spare block re-linked ahead of new blocks keeps them alive across an eden GC.
Security risks
None. This is a debug-build assertion fix in the GC-root bookkeeping for bun_jsc::Strong; no user-controlled input, parsing, auth, or network surface is touched. The write barrier is preserved, so the generational-GC invariant that prevents old→new pointers from being missed is unchanged.
Level of scrutiny
High. This is JSC garbage-collector write-barrier code, where a mistake produces silent use-after-free rather than a crash. The change is tiny and the PR description traces the exact JSC call paths (WriteBarrierInlines.h, HeapInlines.h, Heap::writeBarrierSlowPath, addToRememberedSet) to argue safety, but the author explicitly left "whether Heap::writeBarrier is safe to run during a sweep" as an open question for a reviewer to confirm. The repo's review guidance treats GC/write-barrier reasoning as the most-blocked category, and #37008 is cited as precedent for the same pattern — a human familiar with that precedent should sign off.
Other factors
- The three call sites of
setNextinStrongRootBlock.cpp(acquire line 88, release lines 107 and 112) all pass either a live listed block, the parked spare, the current head, ornullptr; the dropped type check was redundant for all of them since the "Srb" marking constraint roots every block involved. - The tests hard-code
CAP = 960to matchStrongRootBlock::capacityand assume a fresh process starts with zero Strongs. Both assumptions are asserted through the printedblocksArmed/blocksAtStart/blocksOld/blocksNewcounts, so a future capacity change or a startup-time Strong will fail loudly rather than pass vacuously. - The comment-cop feedback about the paragraph-long code comment was addressed (reduced to one line in c123264); those threads are resolved.
- No CODEOWNERS entry covers these files.
- #37842 is noted as removing
StrongRootBlockentirely on a future WebKit pin, so this fix is a bridge for the current pin — small blast radius either way, but still worth a human confirming the sweep-time barrier reasoning before merge.
1 participant
Problem
ASSERTION FAILED: vm().currentThreadIsHoldingAPILock() => vm().heap.mutatorState() != MutatorState::Sweeping(JSCell::validateIsNotSweeping, JSCell.cpp:179). Stack:JSCell::classInfo<-validateCell<Bun::StrongRootBlock*><-WriteBarrierBase<StrongRootBlock>::setMayBeNull<-StrongRootBlock::setNext<-StrongRootBlock::release<-Bun__StrongRef__delete<-Listener::deinit<-Listener::finalize<-MarkedBlock::Handle::specializedSweep.bun_jsc::Strong(Listener::deinitdrops thedataslot thatstop()only emptied;ByteStream/FileReaderkeep theirpending_valueslot the same way). When that was the last occupied slot of a block,StrongRootBlock::release(src/jsc/bindings/StrongRootBlock.cpp:107) unlinks the block withprev->setNext(vm, block->next()).setNext(src/jsc/bindings/StrongRootBlock.h:86) usedWriteBarrier::setMayBeNull, whose GC-validation path (validateCell->ASSERT_GC_OBJECT_INHERITS->classInfo()) is what JSC forbids during a sweep. It only fires when the successor is non-null, i.e. when the emptied block sits between two live blocks, which is why it is timing dependent in practice (seen on Windows x64 running several socket test files in one debugbun testprocess; not platform specific).bun bd) are affected; release builds compilevalidateCellto nothing.Fix
setNextstores the link withsetWithoutWriteBarrierand then callsvm.writeBarrier(this, next)itself. This drops only the debug-time type check ofnext; the store and the generational barrier are the same onessetMayBeNullperformed. Same approach as Unpin ArrayBuffers without classInfo() so blob finalizers are safe during GC sweep #37008 takes for a different sweep-timeclassInfo()caller.nextis always a block taken from this list or the parked spare, and every cell touched (prev,block,next) is live, since the list and spare are rooted by the "Srb" marking constraint.setMayBeNullisvalidateCellfollowed by the same raw store and the samevm.writeBarrier(owner, value)(WriteBarrierInlines.h:43-55), so release builds on main already execute exactly this sequence fromrelease()inside the sweep.Heap::writeBarrierreads structure pointers and the cell state and, on the slow path, pushes the cell ontom_mutatorMarkStack, whichMarkStackMergingConstraintfolds into the next collection; it never readsclassInfo(). The first test below drives that slow path under a debug JSC (prevwas marked by the collection being swept) with JSC's own assertions on.acquire()links the reused spare, which survived a collection and so is old, ahead of blocks allocated since the last collection. The slot store that follows only remembers the block when the value is a cell, so for a non-cell value (listener.data = 1) them_nextbarrier is the only thing that keeps the new blocks, and everything they root, alive across the next eden collection. A build with the barrier removed loses all 2880 Timeouts in the second test below.bun bd test test/js/web/timers/timer-gc-roots.test.ts(the existing StrongRootBlock test file):Bun.listenwhosedataslot opens a fourth block, fills two more blocks, clears the timers sharing the listener's block, thenBun.gc(true). Unfixed debug build: aborts with the assertion above (5/5 runs); fixed: passes, andprotectedObjectTypeCountsstill reaches every block on both sides of the unlinked one. With the listener's block at the head of the list instead, the unfixed build does not assert, confirming the middle-block relink is the trigger.afterEden: 0on a build whosesetNextomits the barrier.test/js/bun/net/socket-retention.test.ts,test/js/bun/jsc/bun-jsc.test.ts,test/js/web/timers/clearImmediate-gc.test.ts).StrongRootBlockaltogether but depends on a WebKit bump; this keeps debug builds usable on the current pin and is independent of it.Background
bun_jsc::Strongis the Rust handle that roots a JS value. Since Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC #35849 each handle is one slot in aStrongRootBlock, a JS cell holding 960 slots. Active blocks form a singly linked list throughm_next; the head, the one parked spare block and the blocks' structure are rooted by a marking constraint, so blocks and everything in their slots stay alive. Releasing the last slot of a block unlinks it (release), and a new handle that finds every block full re-links the spare at the head (acquire).finalize/deinit) run with the heap inMutatorState::Sweeping. Because a Structure may already have been swept by then,JSCell::classInfo()asserts it is not called in that state.WriteBarrier<T>::set/setMayBeNullcallclassInfo()on the value in builds withENABLE(GC_VALIDATION)(debug);WriteBarrier<Unknown>(the slots themselves) does not, which is why only them_nextlink was affected.