Repository navigation
Keep an aborted AbortSignal's wrapper alive while its controller is still reachable - #32777
Conversation
…still reach it Once a signal had aborted, JSAbortSignalOwner::isReachableFromOpaqueRoots returned false before checking visitor.containsOpaqueRoot, ignoring the opaque root added by a live JSAbortController. The signal's JS wrapper could then be collected while JS event listeners were still registered on the live native signal. JSEventListener only keeps its function alive through that wrapper (m_wrapper is weak), so later dispatches on the same signal were silently dropped in release builds and hit ASSERT(m_wrapper) in JSEventListener::ensureJSFunction in debug builds. Aborted signals now fall through to the opaque-root check like every other state, so the wrapper survives for as long as the controller's wrapper does.
|
Warning Review limit reached
More reviews will be available in 1 minute and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![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 and the fix looks correct, but since this changes GC reachability semantics for AbortSignal wrappers in the JSC bindings, it's worth a human sanity-check on the lifetime implications.
Extended reasoning...
Overview
This PR modifies JSAbortSignalOwner::isReachableFromOpaqueRoots in src/jsc/bindings/webcore/JSAbortSignalCustom.cpp. Previously, once a signal was aborted(), the function returned false immediately — bypassing the final visitor.containsOpaqueRoot(&abortSignal) check. Since JSAbortController::visitChildrenImpl registers root(&signal()) as an opaque root, a still-reachable controller could no longer keep its aborted signal's JS wrapper alive. Because JSEventListener::m_wrapper and m_jsFunction are both Weak<>, collecting the wrapper drops the listener functions, producing the ASSERTION FAILED: m_wrapper in debug and silent listener loss / expando loss in release.
The fix wraps the not-yet-aborted keep-alive heuristics in if (!abortSignal.aborted()) { ... } and lets all paths fall through to the existing opaque-root check. Net effect: an aborted signal's wrapper survives exactly as long as something (its controller) keeps the native signal in the opaque-root set; otherwise it's collectable as before. A new spawned-process test in abort-controller-gc-reason.test.ts covers both listener firing and expando retention across GC, and the PR notes pipeTo-signal-leak.test.ts (the opposite direction) still passes.
Security risks
None identified. This is a GC marking/reachability adjustment with no input parsing, auth, crypto, or external I/O involved.
Level of scrutiny
High. While the diff is mechanically tiny (an early return false becomes a guarded block), it changes object-lifetime semantics in WebCore-derived JSC bindings. Errors here manifest as either memory leaks under load or use-after-free / assertion crashes, and are notoriously hard to catch outside of targeted GC stress tests. The reasoning is sound and matches the existing opaque-root pattern, but this is exactly the area where a maintainer familiar with Bun's JSC GC integration should confirm there are no unintended retention edges (e.g., long-lived controllers now pinning large listener closures indefinitely is the intended behavior, but worth a human acknowledging the trade-off).
Other factors
- No CODEOWNERS entry covers this path.
- The bug-hunting pass found no issues.
- The added test follows the established subprocess-with-
Bun.gc(true)pattern already used in the same file. - Leak-direction coverage is delegated to an existing test rather than added here, which seems reasonable.
|
To make the lifetime trade-off explicit for whoever sanity-checks this: JSAbortController::visitChildrenImpl is the only site that adds an AbortSignal opaque root, so the added retention is exactly "an aborted signal's wrapper lives as long as its controller's wrapper does". Once the controller becomes unreachable, the signal, its wrapper, and the listener closures are collected as before; test/js/web/streams/pipeTo-signal-leak.test.ts covers that direction and still passes. Keeping the wrapper while the controller is reachable is required for correctness rather than a convenience: controller.signal always returns the same native object, and the JS functions of its registered listeners are only kept alive through that wrapper. |
There was a problem hiding this comment.
I didn't find any issues — the refactor correctly lets aborted signals fall through to the existing containsOpaqueRoot check, and JSAbortController::visitChildrenImpl is indeed the only opaque-root adder — but GC wrapper-lifetime changes in JSC bindings are subtle enough that a human should give this a quick sanity check.
Extended reasoning...
Overview
This PR modifies JSAbortSignalOwner::isReachableFromOpaqueRoots in src/jsc/bindings/webcore/JSAbortSignalCustom.cpp. Previously, an aborted signal returned false immediately, bypassing the trailing visitor.containsOpaqueRoot(&abortSignal) check. The change wraps the "can this still abort" keep-alive heuristics in if (!abortSignal.aborted()) so that every path now falls through to the opaque-root check. A new regression test in abort-controller-gc-reason.test.ts verifies that listeners and expando properties on controller.signal survive GC after abort() while the controller is still retained.
I verified the author's claim that retention is bounded: AbortController::opaqueRoot() (returning root(&signal())) is the only producer of an AbortSignal opaque root, and it's only consumed by JSAbortController::visitChildrenImpl. So the added lifetime is exactly "signal wrapper lives as long as its controller's wrapper", which is the correct user-observable semantics (controller.signal must hand back the same JS object). The opposite direction (controller dropped → aborted signal collectable) is covered by the existing pipeTo-signal-leak.test.ts.
Security risks
None identified. This is a GC reachability adjustment with no input parsing, auth, or privilege implications. The risk profile is memory correctness (leak vs. premature collection), not security.
Level of scrutiny
Medium-high. The diff is small and the transformation is mechanically straightforward (if (X) return false; …; return C → if (!X) { … } return C), but it lives in JSC GC-marking code where mistakes manifest as either hard-to-reproduce UAF/assertion crashes or slow leaks. The fix looks correct and is well-argued, but wrapper-lifetime logic in WebCore bindings is exactly the kind of thing a maintainer familiar with Bun's JSC fork should eyeball before merge.
Other factors
- No CODEOWNERS entry covers this path.
- The bug-hunting system found no issues.
- The PR fixes a concrete debug-build assertion (
ASSERTION FAILED: m_wrapperinJSEventListener::ensureJSFunction) and an observable release-build bug (lost listeners/expandos). - The new test follows the established pattern in the same file (spawn subprocess, force GC, assert).
| if (reason) [[unlikely]] | ||
| *reason = "Has Timeout And Abort Event Listener"_s; | ||
| *reason = "Is Following Signal"_s; | ||
| return true; |
There was a problem hiding this comment.
shouldn't this be if it contains the opaque root of the signal being followed?
There was a problem hiding this comment.
That check predates this PR and its behavior is unchanged here: the old if (aborted()) return false; sat above it, so it was already only reachable for non-aborted signals. The diff re-indents it into the if (!abortSignal.aborted()) block so that aborted signals can fall through to the containsOpaqueRoot(&abortSignal) check at the end, which is what the fix needs.
On the question itself: gating it on the followed signal's opaque root would be stricter, but today the only producer of an AbortSignal opaque root is JSAbortController::visitChildrenImpl(). A followed signal that is alive through its own JS wrapper, or through a native ref (timeout timer, request), is never in the opaque-root set, so containsOpaqueRoot(followedSignal) would report false and we would collect a follower whose source can still abort it, losing its listeners the same way this PR describes, one level up. Making that precise would also need JSAbortSignal to add itself as an opaque root during marking, so I left the clause as upstream WebCore has it (same unconditional check there). It also cannot pin a follower forever: m_followingSignal is a WeakPtr, so once the followed signal dies, the follower becomes collectable again.
For what it's worth, nothing in Bun calls AbortSignal::signalFollow() today (WebCore's fetch Request signals used it, ours does not), so isFollowingSignal() is always false at runtime and this branch is inert. I can remove the unused follow machinery, or tighten the check plus add the self opaque root, in a follow-up if you want.
|
CI status for the latest run (Buildkite build 65089): the red lanes are not related to this change.
The new test in |
…bility callback (#32785) ### Symptom `AbortSignal.any()` under GC pressure aborts debug/ASAN builds on a JSC parallel marker thread ("HeapHelper"), several threads at once: ``` ASSERTION FAILED: m_creationThread == currentThreadID() wtf/SingleThreadIntegralWrapper.h(57): WTF::SingleThreadIntegralWrapper<unsigned int>::assertThread() ``` ASAN-symbolized stack of the aborting helper thread: ``` WTF::WeakPtrImplBaseSingleThread<WebCore::WeakPtrImplWithEventTargetData>::deref() ~WeakPtr -> ~ListHashSetNode -> WTF::HashTable::clear WTF::WeakListHashSet<WebCore::AbortSignal, WebCore::WeakPtrImplWithEventTargetData>::isEmptyIgnoringNullReferences() WebCore::JSAbortSignalOwner::isReachableFromOpaqueRoots src/jsc/bindings/webcore/JSAbortSignalCustom.cpp:58 JSC::WeakBlock::specializedVisit JSC::MarkedSpace::forEachWeakInParallel ("Ws" marking constraint) JSC::SlotVisitor::drainFromShared (ParallelHelperPool "HeapHelper" thread) ``` Repro (crashes in about a second on a debug or ASAN build of main; release builds hit the same code path but have no assertion, so the race is silent there): ```js const noop = () => {}; function makeBatch(n) { const out = []; for (let i = 0; i < n; i++) { const a = new AbortController(), b = new AbortController(), c = new AbortController(); // dropped -> collected const dep = AbortSignal.any([a.signal, b.signal, c.signal]); dep.addEventListener("abort", noop); out.push(dep); } return out; } const keep = []; for (let round = 0; round < 400; round++) { keep.push(makeBatch(400)); if (keep.length > 12) keep.shift(); Bun.gc(true); if ((round & 15) === 0) await new Promise(r => setTimeout(r, 1)); } ``` ### Cause `JSAbortSignalOwner::isReachableFromOpaqueRoots()` runs on JSC's parallel marker threads. For a dependent signal with an abort listener it called `sourceSignals().isEmptyIgnoringNullReferences()`. The source set is a `WTF::WeakListHashSet`, and that method is not read-only: when every entry is dead it `const_cast`s and `clear()`s the set. Running that on a marker thread destroys `WeakPtr`s off their owning thread. `WeakPtrImplWithEventTargetData` uses a non-atomic, thread-asserted refcount (`SingleThreadIntegralWrapper`), so assert-enabled builds crash, and release builds get an unsynchronized cross-thread deref that can `delete` impl objects (which also host the signal's `EventTargetData`) and free hash-table nodes while the JS thread is still using them. (`WeakHashSet::isEmptyIgnoringNullReferences()` is read-only; the `WeakListHashSet` flavor used here is the one that prunes, which is easy to miss at the call site.) ### Fix - `AbortSignal::hasAliveSourceSignals()`: const, read-only emptiness probe (`begin() != end()` skips dead entries without destroying them). - `isReachableFromOpaqueRoots()` uses it instead of `isEmptyIgnoringNullReferences()`. The liveness rule is unchanged: a dependent signal with an abort listener stays alive while any source signal is alive. Dead entries are still pruned on the JS thread by the container's amortized cleanup on add/remove and by `markAborted()`'s `clear()`. No other GC visitor in `src/jsc/bindings` touches a weak container (grepped `isReachableFromOpaqueRoots` / `visitAdditionalChildren` implementations). ### Verification New test in `test/js/web/abort/abort-controller-gc-reason.test.ts` spawns the repro with `BUN_JSC_numberOfGCMarkers=8` (one-shot `bun -e` defaults to a single marker, which hides the bug by running the weak-handle visit on the JS thread). Without the fix (`bun bd test`, debug+ASAN): ``` (fail) AbortController GC > AbortSignal.any() dependent signals survive parallel GC after their sources are collected exitCode: 134 stderr: ASSERTION FAILED: m_creationThread == currentThreadID() wtf/SingleThreadIntegralWrapper.h(57) ... assertThread() ... ``` With the fix, all 4 tests in the file pass (new test 3/3 runs), and the larger repro above runs to completion. Note: #32777 (a different AbortSignal GC bug, wrapper liveness once aborted) edits the same function, so whichever lands second needs a trivial rebase.
2 participants
Problem
If an
AbortControllerstays reachable afterabort(), a GC can collect its signal's JS wrapper even though JS event listeners are still registered on the live native signal.JSEventListeneronly keeps the listener function alive through that wrapper (m_wrapperandm_jsFunctionare weak), so those listeners lose their functions:controller.signalsilently invoke nothing, and expando properties oncontroller.signaldisappearASSERTION FAILED: m_wrapperinWebCore::JSEventListener::ensureJSFunction(src/jsc/bindings/webcore/JSEventListener.h:157), SIGABRTOn bun 1.4.0 the 500 post-GC dispatches call no listeners; a debug build aborts with the assertion above.
Cause
JSAbortSignalOwner::isReachableFromOpaqueRootsreturnedfalseas soon asabortSignal.aborted(), before the finalvisitor.containsOpaqueRoot(&abortSignal)check.JSAbortController::visitChildrenImpldoes add the signal as an opaque root, but once the signal had fired that root was never consulted, so the wrapper (and with it every registered listener's function) was collected out from under the still reachable native signal.Fix
In
JSAbortSignalCustom.cpp,aborted()now only skips the "can this still abort" keep-alive conditions; every state falls through to the opaque-root check, so the wrapper lives exactly as long as something (itsAbortController's wrapper) keeps the native signal reachable from JS. Signals whose controller is also unreachable are still collected as before.Verification
New case in
test/js/web/abort/abort-controller-gc-reason.test.ts: register listeners, abort, GC, re-dispatch while only the controllers are retained.ASSERTION FAILED: m_wrapper, child exits with 134 (debug); listeners never fire again on release (1.4.0)bun bd test test/js/web/abort/passes, andtest/js/web/streams/pipeTo-signal-leak.test.tsstill passes (covers the opposite direction: dropping the controller must keep aborted signals collectable)