events: make addAbortListener resist stopImmediatePropagation - #33377
Conversation
WalkthroughThis PR introduces a ChangesResistStopPropagation feature
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:12 AM PT - Jul 18th, 2026
❌ @robobun, your commit 4dfe1e8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33377That installs a local version of the PR into your bun-33377 --bun |
![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 bugs, but this changes native EventTarget dispatch semantics (the break → continue in innerInvokeEventListeners) and threads a new private-symbol option through the C++ dictionary converter, so it's worth a human look before merging.
Extended reasoning...
Overview
This PR makes events.addAbortListener (and the other Node APIs that use kResistStopPropagation internally — events.once, util.aborted, timers/promises, stream reduce) resistant to stopImmediatePropagation() on shared AbortSignals, matching Node.js. It does so by adding a resistStopPropagation flag to the native AddEventListenerOptions/RegisteredEventListener, keyed off a new JSC private symbol $kResistStopPropagation, and changing EventTarget::innerInvokeEventListeners to continue past suppressed listeners rather than break. On the JS side it consolidates the duplicate addAbortListener into internal/abort_listener, replaces the plain-Symbol kResistStopPropagation export with a resistStopPropagation() helper that sets the private symbol, and wires it through six internal modules. 15 files touched across C++ bindings, builtin names, internal JS, and tests.
Security risks
Low. The new option is gated behind a JSC private name (builtinNames(vm).kResistStopPropagationPrivateName()), so userland cannot set it via string keys, own Symbols, Symbol.for, or prototype pollution. The extra object->get() in convertDictionary follows the same exception-checked pattern as the existing option reads. No auth/crypto/permission code is involved.
Level of scrutiny
Medium-high. The break → continue change in innerInvokeEventListeners is on the hot path for every event dispatch in the runtime, and it subtly changes iteration behavior (the loop now walks the remaining listeners after stopImmediatePropagation instead of exiting early). The tests confirm ordinary listeners are still suppressed and once: true semantics are preserved, but this is core WebCore-derived dispatch logic and deserves a maintainer's eyes. The dictionary converter also now performs an extra property get on every addEventListener options object.
Other factors
- The removed
kResistStopPropagationSymbol export frominternal/sharedhas no remaining consumers (grep-verified). - Test coverage is solid: six new tests exercising the positive case, ordering, disposal, and the negative contract (ordinary listeners still suppressed), plus the PR description reports the broader event/stream suites still pass.
- No prior human reviews or outstanding comments; CI build was just kicked off.
- No CODEOWNERS entries cover these paths.
|
Both of the things flagged for a human look are fair, so here is the evidence I gathered on each. 1.
|
| case | node | this PR |
|---|---|---|
| capture-phase stopper, bubble-phase resist listener | capture-stopper,resist |
capture-stopper,resist |
resist listener is once: true, runs after a stop |
consumed (1 listener left) | consumed (1 listener left) |
removeEventListener called from a listener after a stop |
a |
a |
plain once: true listener skipped by a stop |
stays registered | stays registered |
| passive listener after a stop | does not run | does not run |
stopPropagation() (not Immediate) |
a,b |
a,b |
resist listener itself calls stopImmediatePropagation() |
resist |
resist |
On the cost: the loop only walks past listeners once stopImmediatePropagation() has actually been called, and for each one it skips, the body is wasRemoved() plus the two phase checks plus the stopped check. No allocation, no JS call, no side effect, so nothing in it is observable. Node does the same walk (its #dispatch keeps iterating when root.resistStopPropagation is set and skips per-listener on event[kStop] && !handler.resistStopPropagation); it only short-circuits when the list has no resisting listener, which is an optimization on a path that is already rare.
2. Extra object->get() in convertDictionary<AddEventListenerOptions>
It only runs when the third argument is an object. addEventListener(type, fn) and addEventListener(type, fn, true) never reach convertDictionary at all.
When it does run, it is the cheapest of the five reads: it uses the Identifier cached in BunBuiltinNames, so unlike the four existing reads it skips the Identifier::fromString(vm, "capture"_s) AtomString lookup.
A/B of addEventListener(type, fn, { once: true }) + removeEventListener, same debug+ASAN build, best of 3 rounds x 100k iterations:
main 99.2 us/op, 101.5 us/op
this PR 101.3 us/op, 101.6 us/op
The difference is inside the run-to-run spread of an unchanged binary (main alone varies by 2.3 us). Absolute numbers are debug+ASAN, not meaningful on their own, but the delta is what matters here.
3. The private symbol
Userland cannot set it. A string key "$kResistStopPropagation", an own Symbol("kResistStopPropagation"), Symbol.for("kResistStopPropagation") and Object.prototype pollution all fail to make a listener resist.
It also cannot be harvested. Even when an internal options object is handed straight to userland (by patching AbortSignal.prototype.addEventListener), JSC filters private names out of enumeration:
ownKeys: once
getOwnPropertySymbols: 0
JSON: {"once":true}
proto: null
There was a problem hiding this comment.
I didn't find any issues, but this changes the core EventTarget::innerInvokeEventListeners dispatch loop (break → continue) and introduces a private-symbol channel from internal JS into the native dictionary converter — both worth a human sanity-check on the approach.
Extended reasoning...
Overview
This PR makes events.addAbortListener (and the other Node APIs that use kResistStopPropagation internally — events.once, util.aborted, timers/promises, stream reduce) resist stopImmediatePropagation() on shared AbortSignals, matching Node.js semantics. It touches 15 files: the native WebCore EventTarget dispatch loop and options plumbing (EventTarget.cpp, AddEventListenerOptions.h, RegisteredEventListener.h, JSAddEventListenerOptions.cpp), a new private builtin name (BunBuiltinNames.h, builtins.d.ts), six internal JS modules, and three test files. It also deduplicates addAbortListener (removing the copy in node:events in favor of internal/abort_listener) and removes the now-dead kResistStopPropagation plain-Symbol export from internal/shared.
Security risks
The main security-relevant surface is the new private-symbol option: a listener carrying $kResistStopPropagation bypasses stopImmediatePropagation(). The PR keys it off a JSC private name (builtinNames(vm).kResistStopPropagationPrivateName()) so userland cannot set it via string key, own Symbol, Symbol.for, or prototype pollution; the follow-up comment also verifies it isn't enumerable when the internal options object is handed to a patched addEventListener. That reasoning looks sound to me, but since this is effectively a new privileged capability gated on a private symbol, it deserves a human eye.
Level of scrutiny
Medium-high. The break → continue change in innerInvokeEventListeners affects every EventTarget dispatch in the runtime, not just AbortSignal. The PR argues (with a 7-case differential vs. Node) that the only observable difference is the intended one, and a regression test confirms ordinary listeners are still suppressed. The extra object->get() in convertDictionary<AddEventListenerOptions> only runs when an options object is passed and uses a cached Identifier; the author's A/B shows it's within noise. Still, this is a deliberate divergence from the DOM spec's dispatch algorithm and a design decision (private-symbol channel from builtin JS into a native IDL dictionary) that a maintainer should sign off on.
Other factors
- No bugs surfaced from the multi-agent review.
- I confirmed the removed
kResistStopPropagationSymbol export has no remaining consumers insrc/. - Test coverage is good: six new targeted tests plus a negative test that
stopImmediatePropagationstill works for ordinary listeners; the description shows they fail on the old build. events.oncenow passes the sameoptsobject (with the private symbol) as theflagsargument througheventTargetAgnosticAddListenereven whenemitteris anEventEmitter— harmless since onlyflags.onceis read on that path, but worth noting.- No prior human or bot review comments to address; CI build is in progress.
|
Two notes on the points raised, since both are design questions worth settling on the record.
// node lib/events.js, inside once()
const opts = { __proto__: null, once: true, [kResistStopPropagation]: true };
eventTargetAgnosticAddListener(emitter, name, resolver, opts);The only difference here is that I reuse that one object for the signal listener, where Node allocates a second identical literal. The divergence from the DOM spec's dispatch algorithm. This is narrower than it looks, and I think that is the thing to check rather than the
So the spec divergence is confined to exactly the internal Node APIs where Node's own |
Status: diff is green, the two red lanes are a break on main and a tart-host capacity errorRebased onto Build #75420 finished: 284 passed, 2 failed.
The Ready for review. |
3d6bc70 to
34b1c52
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this touches native EventTarget dispatch (innerInvokeEventListeners break → continue), adds a JSC private name, and threads a new flag through RegisteredEventListener / AddEventListenerOptions — a deliberate DOM-spec divergence gated behind an internal-only symbol. That's a cross-layer design decision worth a maintainer's eyes rather than a bot approval.
What was reviewed
break→continueininnerInvokeEventListeners: skipped listeners hit onlywasRemoved()+ phase checks, no side effects;once: truelisteners past a stop stay registered as before.kResistStopPropagationsymbol removal frominternal/shared: no remaining consumers insrc/js.- Private-name read in
convertDictionary: exception-checked, only reached when the third arg is an object; userland cannot forge the key.
Extended reasoning...
Overview
Implements Node's kResistStopPropagation semantics on Bun's native EventTarget so events.addAbortListener, events.once({signal}), util.aborted, timers/promises, and stream reduce still fire their abort teardown when another listener on the shared AbortSignal calls stopImmediatePropagation(). Spans four WebCore C++ files (dispatch loop, listener struct, options dict, dictionary converter), BunBuiltinNames.h, one shared TS helper, five internal JS modules, and three test files. The duplicate addAbortListener in node:events is deleted in favor of internal/abort_listener.
Security risks
The new option is keyed on a JSC private name, so userland cannot set it via string key, own Symbol, Symbol.for, or prototype pollution (author verified; private names are filtered from ownKeys/getOwnPropertySymbols). The extra object->get() in convertDictionary uses the cached private Identifier and is RETURN_IF_EXCEPTION-guarded. No auth/crypto/permission surface touched.
Level of scrutiny
High. EventTarget::innerInvokeEventListeners is the core event dispatch path shared by every native EventTarget (AbortSignal, WebSocket, MessagePort, etc.), and this change intentionally diverges from the DOM spec's "stop immediate → break" step. The divergence is scoped (only listeners carrying the internal flag ever run past a stop, and no user code can set that flag), but confirming that scoping — and that the continue walk has no observable side effect on skipped listeners — is the kind of architectural call the repo guidance reserves for a maintainer.
Other factors
CI is fully green (286/286), the author provided differential testing against Node v26 across seven dispatch edge cases, and BUN_JSC_validateExceptionChecks=1 is clean. No remaining consumers of the removed kResistStopPropagation symbol export. The bug-hunting system's only candidate findings were comment-length style nits, both refuted. Deferring solely on scope/criticality, not on any identified defect.
34b1c52 to
2af6361
Compare
There was a problem hiding this comment.
No issues found, but this touches native EventTarget::innerInvokeEventListeners dispatch and introduces a new private-symbol → C++ flag mechanism, so it's worth a human look at the design.
What was reviewed:
break→continuein the dispatch loop: skipped listeners hit no side-effecting code (once/passive/callback all come after the guard), so ordinarystopImmediatePropagation()semantics are preserved.- Removal of the
kResistStopPropagationsymbol export frominternal/shared: greppedsrc/js— every prior consumer was migrated to the new helper; no orphans. convertDictionaryaddition follows the neighboring reads' exception-check pattern; the private name is looked up viabuiltinNames(vm)so userland can't set it.
Extended reasoning...
Overview
The PR wires Node's internal kResistStopPropagation semantic through Bun's native EventTarget. It adds a resistStopPropagation bit to AddEventListenerOptions/RegisteredEventListener, reads it from a JSC private name in convertDictionary<AddEventListenerOptions>, and changes innerInvokeEventListeners to skip (rather than break on) suppressed listeners so those with the bit still run. On the JS side it replaces the dead kResistStopPropagation Symbol() export with a resistStopPropagation(opts) helper, deletes the duplicate addAbortListener in node:events in favor of internal/abort_listener, and threads the flag through events.once, util.aborted, timers/promises setTimeout/setImmediate/setInterval, and stream reduce. Six new tests cover the fixed paths plus a regression guard that ordinary stopImmediatePropagation() still suppresses.
Security risks
None identified. The new flag is gated behind a JSC private name that only bundled internal modules can reference; userland cannot forge it via string key, own Symbol, Symbol.for, or prototype pollution (the PR author verified enumeration filters it out). The dispatch-loop change only alters control flow when a listener already carries the internal-only bit, so no user listener can start running where it previously would not.
Level of scrutiny
High. EventTarget::innerInvokeEventListeners is the spec-defined dispatch path for every native EventTarget (AbortSignal, MessagePort, Worker, WebSocket, etc.), and the change is a deliberate divergence from the DOM spec's inner-invoke algorithm. The divergence is narrow and well-argued (mirrors Node's own EventTarget), but it plus the new cross-layer private-symbol mechanism are the kind of architectural choices a maintainer should sign off on rather than land on bot approval alone.
Other factors
The implementation looks correct end-to-end. I confirmed the removed kResistStopPropagation symbol has no remaining consumers in src/js; the continue placement means skipped listeners never reach the isOnce() removal or handleEvent call, so a suppressed once: true listener stays registered as before; the new C++ get() follows the same RETURN_IF_EXCEPTION pattern as the four existing reads and the author ran BUN_JSC_validateExceptionChecks=1 clean. Tests are placed in the existing per-module files, assert exact ordering/codes, and the gate evidence shows fail-before/pass-after on both debug+ASAN and release. Deferring solely on scope, not on any correctness concern.
AbortSignal is a native EventTarget, and innerInvokeEventListeners broke
out of its dispatch loop as soon as a listener called
stopImmediatePropagation(). Listeners that Node marks with its internal
kResistStopPropagation symbol were therefore silently skipped: any code
sharing the signal could suppress another consumer's teardown.
Add resistStopPropagation to AddEventListenerOptions, parsed from a JSC
private symbol that only internal modules can reach, carry it on
RegisteredEventListener, and skip suppressed listeners during dispatch
instead of breaking.
Wire it into the listeners Node marks the same way: addAbortListener,
events.once(emitter, type, { signal }), util.aborted(), timers/promises
setTimeout/setImmediate/setInterval, and stream reduce(). On those paths
a suppressed signal left the promise pending forever rather than merely
skipping cleanup.
Also drops the duplicate addAbortListener in node:events in favor of
internal/abort_listener.
2af6361 to
4dfe1e8
Compare
There was a problem hiding this comment.
No bugs found, but this is a cross-layer change to native EventTarget dispatch semantics (the break → continue in innerInvokeEventListeners is a deliberate DOM-spec divergence gated on a new private symbol), so it's worth a human sign-off on the design.
What was reviewed:
- Checked that the removed
kResistStopPropagationexport frominternal/sharedhas no remaining consumers insrc/. - Verified
node:events→internal/abort_listenerrequire does not introduce a cycle. - The
break→continueloop is observably identical for user-registered listeners (flag only reachable via a JSC private name); a skippedonce: truelistener stays registered because the check runs before the once-removal, matching the oldbreak. - Exception handling in the new
convertDictionaryget()matches the surrounding pattern withRETURN_IF_EXCEPTION.
Extended reasoning...
Overview
The PR wires Node's kResistStopPropagation semantics into Bun's native EventTarget. It adds a bool resistStopPropagation to AddEventListenerOptions / RegisteredEventListener, reads it from a JSC private symbol in convertDictionary<AddEventListenerOptions>, and changes innerInvokeEventListeners to continue past suppressed listeners instead of breaking when a resisting listener is present. On the JS side it registers a new builtin private name, replaces the exported kResistStopPropagation symbol with a resistStopPropagation(opts) helper, deduplicates addAbortListener (events.ts now imports the internal one), and applies the flag in events.once, util.aborted, timers/promises and stream reduce(). Six new tests cover each surface plus a negative test that ordinary stopImmediatePropagation() still suppresses.
Security risks
The flag is keyed on a JSC private name (BunBuiltinNames), not a user-reachable string or well-known symbol; the author verified prototype pollution, Symbol.for, string keys, and enumeration harvesting all fail to reach it. Userland cannot register a listener that survives stopImmediatePropagation(), so there is no new privilege surface. The extra object->get() in convertDictionary can invoke user getters/Proxy traps, but that was already true of the four preceding reads and it is guarded by RETURN_IF_EXCEPTION. I don't see security concerns here.
Level of scrutiny
This warrants a human look: EventTarget::innerInvokeEventListeners is the dispatch core for every web API in Bun, and changing it from break to continue is a deliberate divergence from the DOM "inner invoke" algorithm. The author's evidence (7 differential edge cases against Node v26, perf A/B, non-forgeability of the symbol) is thorough, and the divergence is scoped to internal-only listeners — but confirming that this is the right layer (native dispatch vs. e.g. addAbortAlgorithmToSignal, which fires before dispatch and is already used elsewhere) is a design call a maintainer should make.
Other factors
- Removed
kResistStopPropagationfrominternal/sharedexports: grep confirms no remaining importers. - The 4-line comment in
shared.tswas flagged and ruled out as a nit. - The
RegisteredEventListenerbitfield addition (5thbool : 1) fits within existing padding and doesn't change layout meaningfully. - Tests use synchronous
abort()so there is no timing race; the setTimeout(1) is only a fallback settlement path for a broken build, as the comment explains. - No outstanding human reviewer comments; the robobun follow-ups are the author's own evidence posts.
|
One new point worth answering on the record: why the dispatch loop rather than
const events = require("node:events");
const target = new EventTarget(); // not an AbortSignal
target.addEventListener("foo", e => e.stopImmediatePropagation());
const p = events.once(target, "foo"); // resolver registered with kResistStopPropagation
target.dispatchEvent(new Event("foo"));
// node: resolves
// bun 1.3.14 / 1.4.0: never settles (resolver left registered, 3 listeners)
Two smaller reasons it is also the wrong layer for the
So |
2 participants
Repro
Node prints
cleanup. Bun prints nothing.addAbortListeneris the API Node tells resource owners to use specifically so that another consumer of a sharedAbortSignalcannot suppress their teardown. On Bun it degraded into a plain listener, so it was skipped in exactly the composed-signal scenarios it exists for.Cause
AbortSignalis a nativeEventTarget, andEventTarget::innerInvokeEventListenersleft the dispatch loop the momentstopImmediatePropagation()had been called:Node's
EventTargetis JS, so it can record a per-listenerresistStopPropagationflag (from its internalkResistStopPropagationsymbol) and skip only the listeners that lack it.internal/abort_listener.tsalready passed[kResistStopPropagation]: true, but it was a plain JS symbol that the native dictionary converter never read, so it did nothing.node:eventsshipped a second copy ofaddAbortListenerthat did not even pass it.Fix
AddEventListenerOptionsgainsresistStopPropagation, parsed inconvertDictionaryfrom$kResistStopPropagation, a JSC private symbol only Bun's internal modules can reach. Userland cannot forge it: a string key, an ownSymbol(),Symbol.for()andObject.prototypepollution all fail.RegisteredEventListenercarries the flag, andinnerInvokeEventListenersskips suppressed listeners instead of breaking, so a listener that set it still runs.addAbortListenerinnode:eventsis gone; it now usesinternal/abort_listener.kResistStopPropagation:events.once(emitter, type, { signal })(both listeners),util.aborted(),timers/promisessetTimeout/setImmediate/setInterval, and streamreduce().On those last paths, a suppressed signal did not merely skip cleanup, it left the promise pending forever:
stopImmediatePropagation()listener on the signalevents.addAbortListener(signal, fn)fnnot calledevents.once(emitter, type, { signal })AbortErrorAbortErrorutil.aborted(signal, resource)timers/promises.setTimeout(ms, v, { signal })AbortErrorAbortErrorstopImmediatePropagation()still suppresses ordinary listeners, and it still leaves a lateronce: truelistener registered.fs.promises.watch's JS abort listener is deliberately left alone: Bun's nativefs.watchregisters its own abort algorithm, which runs before event dispatch, so marking the redundant JS listener would change nothing.Verification
Six new tests across
event-emitter.test.ts,timers.promises.test.tsandtest-aborted.test.ts. Each fails in under 60ms on a build without thesrc/changes and passes with them:before / after
After: all three files pass, plus
test/js/web/events/,test/js/node/events/,test/js/node/stream/,test/js/deno/event/, and thetest-eventtarget.js/test-events-once.js/test-events-add-abort-listener.mjsNode suites.BUN_JSC_validateExceptionChecks=1reports no violations for the newget()inconvertDictionary.[review] gate passed · iteration 5 · 15 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 5
evidence per changed file