Fix cross-thread WeakPtr destruction in AbortSignal.any()'s GC reachability callback by robobun · Pull Request #32785 · oven-sh/bun

robobun · 2026-06-26T19:59:46Z

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):

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_casts and clear()s the set.

Running that on a marker thread destroys WeakPtrs 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.

JSAbortSignalOwner::isReachableFromOpaqueRoots() runs on JSC's parallel marker threads. For a dependent (AbortSignal.any) signal with an abort listener it probed the source set with WeakListHashSet::isEmptyIgnoringNullReferences(), which clear()s the set once every source signal has died. That destroys WeakPtrs whose impls use a single-threaded refcount, so assert-enabled builds crash with "m_creationThread == currentThreadID()" and release builds race the JS thread (node frees and WeakPtrImpl derefs off-thread). Replace the probe with a read-only one (begin() != end()); pruning keeps happening on the JS thread via the container's amortized cleanup.

robobun · 2026-06-26T19:59:56Z

^{Updated 4:40 PM PT - Jun 26th, 2026}

❌ @robobun, your commit 5d86042 has 4 failures in Build #65144 (All Failures):

test/js/web/websocket/autobahn.test.ts - code 1 on 🍎 14 aarch64
test/js/bun/http/fetch-file-upload.test.ts - timeout on 🍎 14 aarch64
test/js/bun/http/bun-serve-file.test.ts - timeout on 🍎 14 aarch64
test/js/web/workers/structured-clone.test.ts - code 1 on 🐧 25.04 x64-baseline
test/js/web/workers/structured-clone.test.ts - code 1 on 🐧 13 x64-baseline
test/js/web/workers/structured-clone.test.ts - code 1 on 🐧 25.04 x64
test/js/web/workers/structured-clone.test.ts - code 1 on 🐧 13 x64

🧪 To try this PR locally:

bunx bun-pr 32785

That installs a local version of the PR into your bun-32785 executable, so you can run:

bun-32785 --bun

robobun · 2026-06-26T19:59:57Z

Status: reproduced on main with a debug build (ASSERTION FAILED: m_creationThread == currentThreadID() on HeapHelper threads, 3/3 runs); fix verified both ways with bun bd test test/js/web/abort/abort-controller-gc-reason.test.ts (the new test fails with that assertion on unfixed src, passes with the fix). Re-verified after merging main to resolve the #32777 conflict: all 5 tests in the file pass on the merged debug build.

CI on the merged head (build 65144): nothing red involves this change or its test.

2x darwin 26 aarch64 test-bun: buildkite-agent artifact download timed out after 120s, the runner aborted before any test ran (infra).
Windows 2019 x64 / x64-baseline: failures in test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts (truncated trailing UTF-8 cases). That file landed on main in Don't panic generating a sourcemap for a source ending in a truncated UTF-8 sequence #32774 (990be52) and reached this branch through the merge; unrelated to AbortSignal.
Known-flaky Windows hot.test.ts / bun-install.test.ts noted by the flake annotation.

These need a job retry or a main-side fix rather than changes here.

coderabbitai · 2026-06-26T20:00:20Z

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 3 minutes and 2 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: af75a77e-a19e-43c2-a769-cae08559c3d8

📥 Commits

Reviewing files that changed from the base of the PR and between c34c700 and 5d86042.

📒 Files selected for processing (2)

src/jsc/bindings/webcore/JSAbortSignalCustom.cpp
test/js/web/abort/abort-controller-gc-reason.test.ts

Walkthrough

AbortSignal now exposes a GC-thread probe for alive source signals, and the opaque-root reachability check uses it. A regression test exercises AbortSignal.any() dependent signals under repeated parallel GC after their sources are collected.

Changes

AbortSignal GC reachability

Layer / File(s)	Summary
Helper and reachability check `src/jsc/bindings/webcore/AbortSignal.h`, `src/jsc/bindings/webcore/JSAbortSignalCustom.cpp`	`AbortSignal::hasAliveSourceSignals()` is added and used in `JSAbortSignalOwner::isReachableFromOpaqueRoots()` for dependent-signal reachability.
Parallel GC regression test `test/js/web/abort/abort-controller-gc-reason.test.ts`	A new regression test spawns a separate `bun -e` process, creates `AbortSignal.any()` dependents from short-lived controllers, forces repeated GC, and asserts `PASS`.

Possibly related PRs

oven-sh/bun#32747: Updates GC-related AbortSignal/AbortController handling, including write-barrier storage and signal visitation, around the same area of the abort signal GC logic.

Suggested reviewers

Jarred-Sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main fix in AbortSignal.any() and the GC reachability callback.
Description check	✅ Passed	It clearly explains the bug, fix, and verification, even though it uses custom headings instead of the template.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-06-26T20:05:02Z

Found 2 issues this PR may fix:

Flaky SIGILL crash in JSC garbage collector during bun test on Linux x64 (1.3.7 and 1.3.9) #27312 - Flaky SIGILL/SIGSEGV crash in SlotVisitor::drain on JSC's parallel GC marker threads — same crash site (SlotVisitor::drain → drainFromShared → ParallelHelperPool::Thread::work) that this PR's fix targets
Random crash #20429 - Random crash on GC HeapHelper thread in SlotVisitor::drain — same parallel GC marker thread crash pattern, possibly caused by heap corruption from the non-atomic WeakPtr destruction this PR fixes

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #27312
Fixes #20429

🤖 Generated with Claude Code

robobun · 2026-06-26T20:07:24Z

Checked both issues before adding Fixes lines, and I don't think either can be claimed by this PR, so I'm not adding them.

The crash fixed here needs a specific trigger: a dependent signal created by AbortSignal.any(), with an abort listener, whose source signals have all been collected. In assert-enabled builds it dies with ASSERTION FAILED: m_creationThread == currentThreadID(); in release builds it's silent corruption, so it could surface as unrelated-looking GC crashes, but "same crash site" is not evidence by itself: SlotVisitor::drain is where any marking-time corruption shows up.

Random crash #20429: floating point exception in JSString::visitChildren on the concurrent collector thread, Bun v1.2.9, no reproduction. Nothing points at AbortSignal.any().
Flaky SIGILL crash in JSC garbage collector during bun test on Linux x64 (1.3.7 and 1.3.9) #27312: flaky SIGILL in SlotVisitor::drain during bun test on 1.3.7/1.3.9. The described workload (temp dirs, spyOn(Bun, "spawn"), stdout/stderr patching) doesn't obviously create dependent signals. They are reachable without calling AbortSignal.any() directly (the node:stream operator helpers create one when given a signal), but the report doesn't show that path either.

If #27312's suite does use AbortSignal.any() or node:stream operators with a signal, retesting on a build with this fix would be a useful data point. Leaving both issues open rather than auto-closing them on merge.

claude

Thanks for condensing the test comment. I didn't find any correctness issues, but this is a concurrency fix in a GC marker-thread callback whose safety hinges on WeakListHashSet::begin() being truly read-only under parallel marking — worth a quick look from someone who knows the WTF/JSC weak-container internals.

Extended reasoning...

Overview

Three files touched: a new const helper AbortSignal::hasAliveSourceSignals() in AbortSignal.h, a one-line swap in JSAbortSignalOwner::isReachableFromOpaqueRoots() (JSAbortSignalCustom.cpp) to call it instead of sourceSignals().isEmptyIgnoringNullReferences(), and a new regression test in abort-controller-gc-reason.test.ts that forces parallel GC markers via BUN_JSC_numberOfGCMarkers=8. The boolean semantics are preserved (!isEmpty… ↔ begin() != end()), and the only behavioral change is that the marker-thread probe no longer prunes dead WeakPtr entries.

Security risks

None in the conventional sense (no auth/input handling). The change removes a cross-thread data race / use-after-free vector rather than introducing one. The residual question is whether the replacement probe is itself fully safe on a non-owning thread — i.e., that WeakListHashSet's const iterator skip-dead path only reads the impl pointer and never touches the single-threaded refcount or amortized-cleanup counters. The PR description asserts this and it matches my understanding of WTF's weak iterators, but I couldn't verify against the vendored WTF header from this repo.

Level of scrutiny

High. The diff is tiny, but isReachableFromOpaqueRoots runs on JSC's parallel HeapHelper threads during weak-handle visiting, and the correctness argument depends on internal guarantees of WTF::WeakListHashSet iteration. Concurrent-GC liveness callbacks are exactly the kind of code where a subtle misread of container semantics produces silent corruption only in release builds. This deserves sign-off from someone familiar with WebCore/WTF weak containers, not bot auto-approval.

Other factors

The bug-hunting pass found no issues.
My only prior feedback (a style nit on the 5-line test comment) was addressed in c34c700 and the thread is resolved.
The PR includes a targeted regression test that the author reports fails on unfixed source and passes with the fix; CI is still building.
No CODEOWNERS entry covers these paths.
The author flags an expected trivial conflict with #32777 in the same function — whoever merges should be aware.

robobun · 2026-06-26T20:35:42Z

On the one open question (is the replacement probe actually read-only on a marker thread): I traced it through the WTF headers before making the change. References are to vendor/WebKit/Source/WTF/wtf/; the prebuilt WebKit the build links against ships the same headers.

What hasAliveSourceSignals() executes on the marker thread:

WeakListHashSet::begin() const / end() const (WeakListHashSet.h:165-166) construct const iterators around ListHashSet::begin()/end(); the iterator base constructor only stores positions.
ListHashSetConstIterator's constructor runs skipEmptyBuckets() (ListHashSet.h:444-452), which walks m_next links and evaluates isHashTraitsWeakNullValue(node()->m_value), i.e. WeakPtr::isWeakNullValue() -> !*m_impl, a plain load of WeakPtrImpl::m_ptr. RefPtr::operator* does not ref/deref, so no refcount is touched.
The final == is a position comparison.

What it cannot reach:

The mutable operation counter (increaseOperationCountSinceLastCleanup, WeakListHashSet.h:394) is only bumped by iterator operator++ (advance(), WeakListHashSet.h:78-82), find, and contains; the probe uses none of them.
The pruning paths (amortizedCleanupIfNeeded, removeNullReferences, and the const_cast<WeakListHashSet&>(*this).clear() inside isEmptyIgnoringNullReferences(), WeakListHashSet.h:300-307) are only reachable from the old call. The new probe has no write path.

Reading WeakPtrImpl::m_ptr from a marker thread is the part WTF explicitly permits for GC threads (WeakPtr::canSafelyBeUsed(), WeakPtr.h:217-223 carves out currentThreadMayBeGCThread()), and it is the same read the old probe already did while skipping dead entries. The crash stack in the description shows the write half that this PR removes: isEmptyIgnoringNullReferences -> clear -> ~ListHashSetNode -> ~WeakPtr -> WeakPtrImplBaseSingleThread::deref -> SingleThreadIntegralWrapper::assertThread on a HeapHelper thread.

One pre-existing exposure is unchanged by this PR: a marker thread can read the set while the JS thread structurally mutates it (markAborted() calls m_sourceSignals.clear()). That window exists with the old probe too and matches what upstream WebCore does in this callback; closing it would need a lock shared by markAborted() and the GC callback, which I kept out of scope for the demonstrated bug.

Jarred-Sumner · 2026-06-26T20:59:05Z

@robobun fix merge conflict

…tion

robobun · 2026-06-26T21:08:22Z

Merged main and resolved the conflict in 5d86042. #32777 rewrote JSAbortSignalOwner::isReachableFromOpaqueRoots, so I kept its structure (the !aborted() wrapper and the opaque-root fallthrough) and applied this PR's change inside the isDependent() block: the GC-thread probe is now hasAliveSourceSignals() instead of sourceSignals().isEmptyIgnoringNullReferences(). The test file auto-merged with both PRs' tests. All 5 tests in test/js/web/abort/abort-controller-gc-reason.test.ts pass on the merged debug build, including #32777's new one.

github-actions Bot added the claude label Jun 26, 2026

claude Bot reviewed Jun 26, 2026

View reviewed changes

Comment thread test/js/web/abort/abort-controller-gc-reason.test.ts Outdated

test: condense regression test comment to three lines

c34c700

claude Bot reviewed Jun 26, 2026

View reviewed changes

Merge branch 'main' into farm/c9b8f45a/abortsignal-any-gc-thread-muta…

5d86042

…tion

Jarred-Sumner merged commit be99161 into main Jun 26, 2026
69 of 76 checks passed

Jarred-Sumner deleted the farm/c9b8f45a/abortsignal-any-gc-thread-mutation branch June 26, 2026 22:13

coderabbitai Bot mentioned this pull request Jun 27, 2026

Request: dependent AbortSignal per spec; async_hooks: fix ALS disable/run leak and AsyncResource.bind #32860

Closed

robobun mentioned this pull request Jul 2, 2026

Claude code (most recent) 2.1.150 embeds Bun v1.3.14 521eedd6 panic: deallocation did fail at 0xffff ... #31409

Closed

coderabbitai Bot mentioned this pull request Jul 4, 2026

test: spec coverage for AbortSignal/AbortController and structuredClone #33324

Closed

coderabbitai Bot mentioned this pull request Aug 10, 2026

Add a self-hosted Mastra Studio example neon-solutions/mastra-studio#1

Merged

robobun mentioned this pull request Aug 13, 2026

bun test 1.3.13+ SIGTRAP (JSC GC recursive-mark crash) with accumulated React/happy-dom state in shared-global mode — regression from 1.3.12 #38211

Closed

dylan-conway mentioned this pull request Sep 17, 2026

AbortSignal.any(): keep a dependent's listeners only while a source can abort #43129

Closed

Navigation Menu

Repository navigation

Fix cross-thread WeakPtr destruction in AbortSignal.any()'s GC reachability callback - #32785

Fix cross-thread WeakPtr destruction in AbortSignal.any()'s GC reachability callback#32785
Jarred-Sumner merged 3 commits into
mainfrom
farm/c9b8f45a/abortsignal-any-gc-thread-mutation

robobun commented Jun 26, 2026

Uh oh!

robobun commented Jun 26, 2026 •

edited

Loading

Uh oh!

robobun commented Jun 26, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

Review limit reached

Uh oh!

github-actions Bot commented Jun 26, 2026

Uh oh!

robobun commented Jun 26, 2026

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

robobun commented Jun 26, 2026

Uh oh!

Jarred-Sumner commented Jun 26, 2026

Uh oh!

robobun commented Jun 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented Jun 26, 2026

Symptom

Cause

Fix

Verification

Uh oh!

robobun commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Possibly related PRs

Suggested reviewers

Uh oh!

github-actions Bot commented Jun 26, 2026

Uh oh!

robobun commented Jun 26, 2026

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

robobun commented Jun 26, 2026

Uh oh!

Jarred-Sumner commented Jun 26, 2026

Uh oh!

robobun commented Jun 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Jun 26, 2026 •

edited

Loading

robobun commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading