Skip to content

Keep an aborted AbortSignal's wrapper alive while its controller is still reachable #32777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 2 commits into from
Jun 26, 2026
Merged

Keep an aborted AbortSignal's wrapper alive while its controller is still reachable #32777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7575fd6
Keep an aborted AbortSignal's wrapper alive while its controller can …
robobun Jun 26, 2026
f2923cc
ci: retrigger
robobun Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 23 additions & 21 deletions src/jsc/bindings/webcore/JSAbortSignalCustom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,37 +39,39 @@ bool JSAbortSignalOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> ha
return true;
}

if (abortSignal.aborted())
return false;

if (abortSignal.isFollowingSignal()) {
if (reason) [[unlikely]]
*reason = "Is Following Signal"_s;
return true;
}

if (abortSignal.hasAbortEventListener()) {
if (abortSignal.hasActiveTimeoutTimer()) {
if (!abortSignal.aborted()) {
if (abortSignal.isFollowingSignal()) {
if (reason) [[unlikely]]
*reason = "Has Timeout And Abort Event Listener"_s;
*reason = "Is Following Signal"_s;
return true;

@Jarred-Sumner Jarred-Sumner Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be if it contains the opaque root of the signal being followed?

@robobun robobun Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (abortSignal.isDependent()) {
if (!abortSignal.sourceSignals().isEmptyIgnoringNullReferences()) {

if (abortSignal.hasAbortEventListener()) {
if (abortSignal.hasActiveTimeoutTimer()) {
if (reason) [[unlikely]]
*reason = "Has Source Signals And Abort Event Listener"_s;
*reason = "Has Timeout And Abort Event Listener"_s;
return true;
}
}
if (abortSignal.isDependent()) {
if (!abortSignal.sourceSignals().isEmptyIgnoringNullReferences()) {
if (reason) [[unlikely]]
*reason = "Has Source Signals And Abort Event Listener"_s;
return true;
}
}

// https://github.com/oven-sh/bun/issues/4517
if (abortSignal.hasPendingActivity()) {
if (reason) [[unlikely]]
*reason = "Has Pending Activity"_s;
return true;
// https://github.com/oven-sh/bun/issues/4517
if (abortSignal.hasPendingActivity()) {
if (reason) [[unlikely]]
*reason = "Has Pending Activity"_s;
return true;
}
}
}

// Even when aborted, the wrapper must stay alive while an opaque root (e.g. the wrapper of
// the AbortController that owns the signal) keeps it reachable: the JS listener functions
// registered on the native signal are only kept alive through it (JSEventListener::m_wrapper).
return visitor.containsOpaqueRoot(&abortSignal);
}

Expand Down
53 changes: 53 additions & 0 deletions test/js/web/abort/abort-controller-gc-reason.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,57 @@ describe("AbortController GC", () => {
expect(stdout.trim()).toBe("PASS");
expect(exitCode).toBe(0);
});

// The signal's wrapper owns the JS functions of its event listeners
// (JSEventListener::m_wrapper). It must not be collected once aborted while the
// retained controller still exposes the signal, or those listeners are lost.
test("abort event listeners survive GC when only controller is retained", async () => {
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`
let fired = 0;
const controllers = [];
for (let i = 0; i < 200; i++) {
const ac = new AbortController();
ac.signal.addEventListener("abort", () => {
fired++;
});
ac.signal.tag = i;
ac.abort();
controllers.push(ac);
}
if (fired !== 200) {
console.error("FAIL: expected 200 abort events before GC, got", fired);
process.exit(1);
}

for (let i = 0; i < 10; i++) {
Bun.gc(true);
}

let lostTags = 0;
for (let i = 0; i < controllers.length; i++) {
if (controllers[i].signal.tag !== i) lostTags++;
}
for (const ac of controllers) {
ac.signal.dispatchEvent(new Event("abort"));
}
if (lostTags !== 0 || fired !== 400) {
console.error("FAIL: lostTags =", lostTags, "fired =", fired);
process.exit(1);
}
console.log("PASS");
`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect({ stdout: stdout.trim(), exitCode, stderr }).toEqual({ stdout: "PASS", exitCode: 0, stderr });
});
});
Loading