Repository navigation
Fix AbortSignal leak in ReadableStream.pipeTo with signal option - #28491
Merged
Jarred-Sumner merged 1 commit intoMar 24, 2026
Merged
Conversation
JSAbortAlgorithm held its callback via JSCallbackDataStrong, creating a GC root. When the callback closure captured the signal (via pipeState), this formed an uncollectable cycle: AbortSignal -> m_algorithms -> Strong<callback> -> closure -> JSAbortSignal -> Ref<AbortSignal>. Switch to JSCallbackDataWeak and keep the callback alive by visiting it from JSAbortSignal::visitAdditionalChildrenInGCThread while the signal wrapper is reachable. Store JS abort algorithms in a separate vector so the GC thread can iterate them (erasing Ref<AbortAlgorithm> into a lambda would hide it from the GC).
Collaborator
Contributor
|
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 (6)
WalkthroughThe changes refactor Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
310
to
313
| size_t AbortSignal::memoryCost() const | ||
| { | ||
| return sizeof(AbortSignal) + m_native_callbacks.sizeInBytes() + m_algorithms.sizeInBytes() + m_sourceSignals.capacity() + m_dependentSignals.capacity(); | ||
| return sizeof(AbortSignal) + m_native_callbacks.sizeInBytes() + m_algorithms.sizeInBytes() + m_abortAlgorithms.sizeInBytes() + m_sourceSignals.capacity() + m_dependentSignals.capacity(); | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 AbortSignal::memoryCost() reads m_abortAlgorithms.sizeInBytes() without holding m_abortAlgorithmsLock, violating the WTF_GUARDED_BY_LOCK(m_abortAlgorithmsLock) annotation that this PR itself introduces. Since memoryCost() is called via JSAbortSignal::estimatedSize() during GC heap accounting (potentially from a concurrent GC thread), this is a C++ data race with main-thread calls to addAbortAlgorithmToSignal() or removeAbortAlgorithmFromSignal(). Fix: acquire m_abortAlgorithmsLock before the sizeInBytes() call.
Extended reasoning...
Bug: memoryCost() violates its own lock annotation
What the bug is
AbortSignal::memoryCost() is a const method computing heap size for JSC memory accounting. This PR adds a new field m_abortAlgorithms annotated with WTF_GUARDED_BY_LOCK(m_abortAlgorithmsLock). Every other accessor of this field properly acquires the lock: addAbortAlgorithmToSignal() (before append()), removeAbortAlgorithmFromSignal() (before removeFirstMatching()), runAbortSteps() (before std::exchange()), and visitAbortAlgorithms() (before iterating). memoryCost() is the sole exception, calling m_abortAlgorithms.sizeInBytes() without holding the lock.
Concrete code path
JSAbortSignal::estimatedSize() calls wrapped().memoryCost(). With concurrent GC enabled (BUN_JSC_useConcurrentGC=1, explicitly mentioned in the PR description's stress tests), JSC invokes estimatedSize() from a GC thread. If the main thread is simultaneously inside addAbortAlgorithmToSignal() holding m_abortAlgorithmsLock while appending to the vector (updating internal m_size and potentially reallocating), the GC thread reading m_size via sizeInBytes() is an unsynchronized read-write data race.
Why existing code does not prevent it
The const qualifier on memoryCost() provides no thread-safety guarantee. WTF_GUARDED_BY_LOCK is a static-analysis annotation (Clang thread-safety analysis), not a runtime enforcement mechanism. There is no other synchronization primitive guarding this read path.
Step-by-step proof
- Main thread calls
addAbortAlgorithmToSignal(), acquiresm_abortAlgorithmsLock, appends tom_abortAlgorithms(updatesm_size, may reallocate buffer). - Simultaneously, GC thread calls
JSAbortSignal::estimatedSize()->wrapped().memoryCost()->m_abortAlgorithms.sizeInBytes()with no lock held. - Step 2 races on
m_sizewhile step 1 writes it -- undefined behavior per the C++ memory model.
Note that visitAbortAlgorithms() correctly holds the lock during its iteration, but memoryCost() does not follow the same discipline.
Impact
The practical consequence is an inaccurate memory cost estimate rather than a crash or incorrect abort logic. However, it is technically undefined behavior in C++ and violates the explicit WTF_GUARDED_BY_LOCK contract introduced by this very PR. Concurrent GC is explicitly exercised in the PR stress test.
Fix
Acquire m_abortAlgorithmsLock inside memoryCost() before calling sizeInBytes():
size_t AbortSignal::memoryCost() const
{
size_t abortAlgorithmsCost;
{
Locker locker { m_abortAlgorithmsLock };
abortAlgorithmsCost = m_abortAlgorithms.sizeInBytes();
}
return sizeof(AbortSignal) + m_native_callbacks.sizeInBytes() + m_algorithms.sizeInBytes()
+ abortAlgorithmsCost + m_sourceSignals.capacity() + m_dependentSignals.capacity();
}
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
Collaborator
There was a problem hiding this comment.
Interestingly, this does not fix #26392
sosukesuzuki
added a commit
that referenced
this pull request
Mar 25, 2026
Follow-up to #28494. Backports WebKit 276563 which removed the Strong/Weak split in favor of a single Weak-only JSCallbackData class, making it structurally impossible to introduce Strong callback handles (a recurring source of GC reference cycles). The prerequisite AbortSignal::m_algorithms restructuring that #28494 deferred this work for was already completed in #28491, so this is now a pure rename + dead code removal with no behavior change. Also deletes ErrorCallback/JSErrorCallback (FileSystemEntry API callback, never used in Bun).
Jarred-Sumner
added a commit
that referenced
this pull request
Mar 25, 2026
…28539) ## What Follow-up to #28494. Backports WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563) ("Make all callbacks Weak handles"), which removed `JSCallbackDataStrong`/`JSCallbackDataWeak` in favor of a single Weak-only `JSCallbackData` class. ## Why `JSC::Strong` handles in callback wrappers have been a recurring source of GC reference cycles (see #28491 for the `pipeTo + signal` leak). Upstream WebKit solved this structurally in 2024-07 by deleting the Strong variant entirely, making such leaks impossible to introduce by construction. \#28494 deferred this backport because it "requires restructuring `AbortSignal::m_algorithms` to allow GC visitation." That restructuring was already done in #28491 (`m_abortAlgorithms` + `visitAbortAlgorithms()` + Lock), so the prerequisite is satisfied and this is now a pure rename + dead code removal. ## Changes ### `JSCallbackData.h` / `.cpp` - Merged `JSCallbackDataWeak` into the base `JSCallbackData` class - Deleted `JSCallbackDataStrong` - Replaced `Strong.h`/`StrongInlines.h` includes with `Weak.h`/`WeakInlines.h` - Kept Bun's existing `invokeCallback(VM&, ...)` signature and `visitJSFunction` method name (not upstream's `visitJSFunctionInGCThread`) to avoid touching base class overrides — this is a structural sync, not a full API sync ### `JSAbortAlgorithm`, `JSPerformanceObserverCallback` - Type rename only: `JSCallbackDataWeak*` → `JSCallbackData*` - No semantic change (both were already Weak) ### Deleted: `ErrorCallback.{h,cpp,idl}`, `JSErrorCallback.{h,cpp}` - FileSystemEntry API callback (`webkitGetAsEntry()` etc.), never implemented in Bun - Zero `create()` call sites, zero `#include` references outside itself - Was the only remaining `JSCallbackDataStrong` user - Upstream still has `ErrorCallback` under `Modules/entriesapi/`, but with `[GenerateIsReachable]` which generates a Weak callback — our copy predated that and was a stale Strong version ## Verification - `nm build/debug/bun-debug | grep -cE "JSCallbackData(Strong|Weak)"` → `0` - `test/js/web/abort/` — pass - `test/js/web/streams/pipeTo-signal-leak.test.ts` — pass (regression check for #28491) - `test/js/web/timers/performance-entries.test.ts` — pass - `BUN_JSC_validateExceptionChecks=1` — pass - GC stress: 100 AbortSignal callbacks + 50× `Bun.gc(true)` → all 100 fire (opaque-root reachability preserved) - GC stress: 50 PerformanceObserver callbacks + 30× `Bun.gc(true)` → all 50 fire --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
structwafel
pushed a commit
to structwafel/bun
that referenced
this pull request
Apr 25, 2026
…n-sh#28491) ## What does this PR do? Fixes a 100% leak of `AbortSignal` when `ReadableStream.prototype.pipeTo` is called with a `signal` option and the pipe never completes. ## The cycle ``` AbortSignal (RefCounted) → m_algorithms[i] (lambda capturing Ref<JSAbortAlgorithm>) → JSCallbackDataStrong::m_callback (JSC::Strong = GC root) → callback closure → pipeState.signal (JSAbortSignal wrapper) → Ref<AbortSignal> ← cycle ``` `JSAbortAlgorithm` held its JS callback via `JSCallbackDataStrong`, which creates a `JSC::Strong` handle (a GC root). The abort algorithm closure registered by `pipeTo` captures `pipeState`, which in turn holds `pipeState.signal` (the `JSAbortSignal` wrapper). Since the wrapper holds a `Ref<AbortSignal>`, the cycle is complete and nothing can be collected even after all user-side references are dropped. ### Before / After | | Bun (before) | Bun (after) | Node.js | |---|---|---|---| | 200 iterations, pipe never completes | 201 AbortSignals retained | 1 | ~0 | ## The fix Switch `JSAbortAlgorithm` from `JSCallbackDataStrong` to `JSCallbackDataWeak`, and keep the callback alive by visiting it from `JSAbortSignal::visitAdditionalChildrenInGCThread` while the signal wrapper is reachable. This follows two existing patterns in the codebase: - **`JSCallbackDataWeak` + `visitJSFunction` override** — same as `JSPerformanceObserverCallback` (`src/bun.js/bindings/webcore/JSPerformanceObserverCallback.{h,cpp}`) - **Lock + Vector + GC-thread visit** — same as `EventListenerMap::visitJSEventListeners` (`src/bun.js/bindings/webcore/EventListenerMap.h:77-85`), which already protects every `addEventListener`/`removeEventListener` against concurrent GC iteration ### Why a separate `m_abortAlgorithms` vector? The existing `m_algorithms` stores type-erased `Function<void(JSValue)>` lambdas. Erasing `Ref<AbortAlgorithm>` into a lambda would hide it from the GC thread — there would be no way to call `visitJSFunction` on it. Storing `Ref<AbortAlgorithm>` directly in a separate vector lets `visitAbortAlgorithms` iterate and mark the weak callbacks. The `Locker` in `runAbortSteps` moves the vector out under the lock and releases before calling `handleEvent`, so we never hold the lock during JS re-entry. ## How did you verify your code works? - New regression test `test/js/web/streams/pipeTo-signal-leak.test.ts` fails on main (201 leaked) and passes with this fix - Existing `test/js/web/abort/abort.test.ts`, `test/js/deno/abort/abort-controller.test.ts`, `test/js/node/util/test-aborted.test.ts`, `test/js/web/streams/streams.test.js` all pass - Concurrent GC stress test (`BUN_JSC_useConcurrentGC=1`, 500 controllers × 10 pipes) — no crash, no deadlock - 5000-iteration heap stress — AbortSignal count stable (growth: -2) - JSC exception scope validation (`BUN_JSC_validateExceptionChecks=1`) — clean
structwafel
pushed a commit
to structwafel/bun
that referenced
this pull request
Apr 25, 2026
…ven-sh#28539) ## What Follow-up to oven-sh#28494. Backports WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563) ("Make all callbacks Weak handles"), which removed `JSCallbackDataStrong`/`JSCallbackDataWeak` in favor of a single Weak-only `JSCallbackData` class. ## Why `JSC::Strong` handles in callback wrappers have been a recurring source of GC reference cycles (see oven-sh#28491 for the `pipeTo + signal` leak). Upstream WebKit solved this structurally in 2024-07 by deleting the Strong variant entirely, making such leaks impossible to introduce by construction. \oven-sh#28494 deferred this backport because it "requires restructuring `AbortSignal::m_algorithms` to allow GC visitation." That restructuring was already done in oven-sh#28491 (`m_abortAlgorithms` + `visitAbortAlgorithms()` + Lock), so the prerequisite is satisfied and this is now a pure rename + dead code removal. ## Changes ### `JSCallbackData.h` / `.cpp` - Merged `JSCallbackDataWeak` into the base `JSCallbackData` class - Deleted `JSCallbackDataStrong` - Replaced `Strong.h`/`StrongInlines.h` includes with `Weak.h`/`WeakInlines.h` - Kept Bun's existing `invokeCallback(VM&, ...)` signature and `visitJSFunction` method name (not upstream's `visitJSFunctionInGCThread`) to avoid touching base class overrides — this is a structural sync, not a full API sync ### `JSAbortAlgorithm`, `JSPerformanceObserverCallback` - Type rename only: `JSCallbackDataWeak*` → `JSCallbackData*` - No semantic change (both were already Weak) ### Deleted: `ErrorCallback.{h,cpp,idl}`, `JSErrorCallback.{h,cpp}` - FileSystemEntry API callback (`webkitGetAsEntry()` etc.), never implemented in Bun - Zero `create()` call sites, zero `#include` references outside itself - Was the only remaining `JSCallbackDataStrong` user - Upstream still has `ErrorCallback` under `Modules/entriesapi/`, but with `[GenerateIsReachable]` which generates a Weak callback — our copy predated that and was a stale Strong version ## Verification - `nm build/debug/bun-debug | grep -cE "JSCallbackData(Strong|Weak)"` → `0` - `test/js/web/abort/` — pass - `test/js/web/streams/pipeTo-signal-leak.test.ts` — pass (regression check for oven-sh#28491) - `test/js/web/timers/performance-entries.test.ts` — pass - `BUN_JSC_validateExceptionChecks=1` — pass - GC stress: 100 AbortSignal callbacks + 50× `Bun.gc(true)` → all 100 fire (opaque-root reachability preserved) - GC stress: 50 PerformanceObserver callbacks + 30× `Bun.gc(true)` → all 50 fire --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…n-sh#28491) ## What does this PR do? Fixes a 100% leak of `AbortSignal` when `ReadableStream.prototype.pipeTo` is called with a `signal` option and the pipe never completes. ## The cycle ``` AbortSignal (RefCounted) → m_algorithms[i] (lambda capturing Ref<JSAbortAlgorithm>) → JSCallbackDataStrong::m_callback (JSC::Strong = GC root) → callback closure → pipeState.signal (JSAbortSignal wrapper) → Ref<AbortSignal> ← cycle ``` `JSAbortAlgorithm` held its JS callback via `JSCallbackDataStrong`, which creates a `JSC::Strong` handle (a GC root). The abort algorithm closure registered by `pipeTo` captures `pipeState`, which in turn holds `pipeState.signal` (the `JSAbortSignal` wrapper). Since the wrapper holds a `Ref<AbortSignal>`, the cycle is complete and nothing can be collected even after all user-side references are dropped. ### Before / After | | Bun (before) | Bun (after) | Node.js | |---|---|---|---| | 200 iterations, pipe never completes | 201 AbortSignals retained | 1 | ~0 | ## The fix Switch `JSAbortAlgorithm` from `JSCallbackDataStrong` to `JSCallbackDataWeak`, and keep the callback alive by visiting it from `JSAbortSignal::visitAdditionalChildrenInGCThread` while the signal wrapper is reachable. This follows two existing patterns in the codebase: - **`JSCallbackDataWeak` + `visitJSFunction` override** — same as `JSPerformanceObserverCallback` (`src/bun.js/bindings/webcore/JSPerformanceObserverCallback.{h,cpp}`) - **Lock + Vector + GC-thread visit** — same as `EventListenerMap::visitJSEventListeners` (`src/bun.js/bindings/webcore/EventListenerMap.h:77-85`), which already protects every `addEventListener`/`removeEventListener` against concurrent GC iteration ### Why a separate `m_abortAlgorithms` vector? The existing `m_algorithms` stores type-erased `Function<void(JSValue)>` lambdas. Erasing `Ref<AbortAlgorithm>` into a lambda would hide it from the GC thread — there would be no way to call `visitJSFunction` on it. Storing `Ref<AbortAlgorithm>` directly in a separate vector lets `visitAbortAlgorithms` iterate and mark the weak callbacks. The `Locker` in `runAbortSteps` moves the vector out under the lock and releases before calling `handleEvent`, so we never hold the lock during JS re-entry. ## How did you verify your code works? - New regression test `test/js/web/streams/pipeTo-signal-leak.test.ts` fails on main (201 leaked) and passes with this fix - Existing `test/js/web/abort/abort.test.ts`, `test/js/deno/abort/abort-controller.test.ts`, `test/js/node/util/test-aborted.test.ts`, `test/js/web/streams/streams.test.js` all pass - Concurrent GC stress test (`BUN_JSC_useConcurrentGC=1`, 500 controllers × 10 pipes) — no crash, no deadlock - 5000-iteration heap stress — AbortSignal count stable (growth: -2) - JSC exception scope validation (`BUN_JSC_validateExceptionChecks=1`) — clean
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…ven-sh#28539) ## What Follow-up to oven-sh#28494. Backports WebKit [276563](https://bugs.webkit.org/show_bug.cgi?id=276563) ("Make all callbacks Weak handles"), which removed `JSCallbackDataStrong`/`JSCallbackDataWeak` in favor of a single Weak-only `JSCallbackData` class. ## Why `JSC::Strong` handles in callback wrappers have been a recurring source of GC reference cycles (see oven-sh#28491 for the `pipeTo + signal` leak). Upstream WebKit solved this structurally in 2024-07 by deleting the Strong variant entirely, making such leaks impossible to introduce by construction. \oven-sh#28494 deferred this backport because it "requires restructuring `AbortSignal::m_algorithms` to allow GC visitation." That restructuring was already done in oven-sh#28491 (`m_abortAlgorithms` + `visitAbortAlgorithms()` + Lock), so the prerequisite is satisfied and this is now a pure rename + dead code removal. ## Changes ### `JSCallbackData.h` / `.cpp` - Merged `JSCallbackDataWeak` into the base `JSCallbackData` class - Deleted `JSCallbackDataStrong` - Replaced `Strong.h`/`StrongInlines.h` includes with `Weak.h`/`WeakInlines.h` - Kept Bun's existing `invokeCallback(VM&, ...)` signature and `visitJSFunction` method name (not upstream's `visitJSFunctionInGCThread`) to avoid touching base class overrides — this is a structural sync, not a full API sync ### `JSAbortAlgorithm`, `JSPerformanceObserverCallback` - Type rename only: `JSCallbackDataWeak*` → `JSCallbackData*` - No semantic change (both were already Weak) ### Deleted: `ErrorCallback.{h,cpp,idl}`, `JSErrorCallback.{h,cpp}` - FileSystemEntry API callback (`webkitGetAsEntry()` etc.), never implemented in Bun - Zero `create()` call sites, zero `#include` references outside itself - Was the only remaining `JSCallbackDataStrong` user - Upstream still has `ErrorCallback` under `Modules/entriesapi/`, but with `[GenerateIsReachable]` which generates a Weak callback — our copy predated that and was a stale Strong version ## Verification - `nm build/debug/bun-debug | grep -cE "JSCallbackData(Strong|Weak)"` → `0` - `test/js/web/abort/` — pass - `test/js/web/streams/pipeTo-signal-leak.test.ts` — pass (regression check for oven-sh#28491) - `test/js/web/timers/performance-entries.test.ts` — pass - `BUN_JSC_validateExceptionChecks=1` — pass - GC stress: 100 AbortSignal callbacks + 50× `Bun.gc(true)` → all 100 fire (opaque-root reachability preserved) - GC stress: 50 PerformanceObserver callbacks + 30× `Bun.gc(true)` → all 50 fire --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a 100% leak of
AbortSignalwhenReadableStream.prototype.pipeTois called with asignaloption and the pipe never completes.The cycle
JSAbortAlgorithmheld its JS callback viaJSCallbackDataStrong, which creates aJSC::Stronghandle (a GC root). The abort algorithm closure registered bypipeTocapturespipeState, which in turn holdspipeState.signal(theJSAbortSignalwrapper). Since the wrapper holds aRef<AbortSignal>, the cycle is complete and nothing can be collected even after all user-side references are dropped.Before / After
The fix
Switch
JSAbortAlgorithmfromJSCallbackDataStrongtoJSCallbackDataWeak, and keep the callback alive by visiting it fromJSAbortSignal::visitAdditionalChildrenInGCThreadwhile the signal wrapper is reachable.This follows two existing patterns in the codebase:
JSCallbackDataWeak+visitJSFunctionoverride — same asJSPerformanceObserverCallback(src/bun.js/bindings/webcore/JSPerformanceObserverCallback.{h,cpp})EventListenerMap::visitJSEventListeners(src/bun.js/bindings/webcore/EventListenerMap.h:77-85), which already protects everyaddEventListener/removeEventListeneragainst concurrent GC iterationWhy a separate
m_abortAlgorithmsvector?The existing
m_algorithmsstores type-erasedFunction<void(JSValue)>lambdas. ErasingRef<AbortAlgorithm>into a lambda would hide it from the GC thread — there would be no way to callvisitJSFunctionon it. StoringRef<AbortAlgorithm>directly in a separate vector letsvisitAbortAlgorithmsiterate and mark the weak callbacks.The
LockerinrunAbortStepsmoves the vector out under the lock and releases before callinghandleEvent, so we never hold the lock during JS re-entry.How did you verify your code works?
test/js/web/streams/pipeTo-signal-leak.test.tsfails on main (201 leaked) and passes with this fixtest/js/web/abort/abort.test.ts,test/js/deno/abort/abort-controller.test.ts,test/js/node/util/test-aborted.test.ts,test/js/web/streams/streams.test.jsall passBUN_JSC_useConcurrentGC=1, 500 controllers × 10 pipes) — no crash, no deadlockBUN_JSC_validateExceptionChecks=1) — clean