From 4dfe1e876b5ceeae90c57069df77f0f9f0f26456 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 5 Jul 2026 13:25:05 +0000 Subject: [PATCH] events: make addAbortListener resist stopImmediatePropagation 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. --- src/js/builtins.d.ts | 9 +++ src/js/builtins/BunBuiltinNames.h | 1 + src/js/internal/abort_listener.ts | 7 +- src/js/internal/shared.ts | 11 ++- src/js/internal/streams/operators.ts | 4 +- src/js/node/events.ts | 35 ++------ src/js/node/timers.promises.ts | 7 +- src/js/node/util.ts | 3 +- .../webcore/AddEventListenerOptions.h | 5 ++ src/jsc/bindings/webcore/EventTarget.cpp | 11 +-- .../webcore/JSAddEventListenerOptions.cpp | 11 +++ .../webcore/RegisteredEventListener.h | 7 +- test/js/node/events/event-emitter.test.ts | 79 +++++++++++++++++++ .../timers.promises/timers.promises.test.ts | 43 +++++++++- test/js/node/util/test-aborted.test.ts | 12 +++ 15 files changed, 198 insertions(+), 47 deletions(-) diff --git a/src/js/builtins.d.ts b/src/js/builtins.d.ts index 60cfb900918a..1407381f81fd 100644 --- a/src/js/builtins.d.ts +++ b/src/js/builtins.d.ts @@ -468,6 +468,15 @@ declare interface UnderlyingSource { $stream?: ReadableStream; } +declare interface AddEventListenerOptions { + /** + * Private symbol read by the native EventTarget. A listener registered with it still + * runs after another listener called `stopImmediatePropagation()`. Mirrors Node.js's + * internal `kResistStopPropagation`. + */ + $kResistStopPropagation?: boolean; +} + declare class OutOfMemoryError { constructor(); } diff --git a/src/js/builtins/BunBuiltinNames.h b/src/js/builtins/BunBuiltinNames.h index e7cce800edda..b6495c2eea2b 100644 --- a/src/js/builtins/BunBuiltinNames.h +++ b/src/js/builtins/BunBuiltinNames.h @@ -119,6 +119,7 @@ using namespace JSC; macro(isUntransferable) \ macro(join) \ macro(json) \ + macro(kResistStopPropagation) \ macro(key) \ macro(lazy) \ macro(lineText) \ diff --git a/src/js/internal/abort_listener.ts b/src/js/internal/abort_listener.ts index 89b6d0f00846..76127d89d6d2 100644 --- a/src/js/internal/abort_listener.ts +++ b/src/js/internal/abort_listener.ts @@ -1,5 +1,5 @@ const { validateAbortSignal, validateFunction } = require("internal/validators"); -const { kResistStopPropagation } = require("internal/shared"); +const { resistStopPropagation } = require("internal/shared"); function addAbortListener(signal: AbortSignal, listener: EventListener): Disposable { if (signal === undefined) { @@ -13,16 +13,17 @@ function addAbortListener(signal: AbortSignal, listener: EventListener): Disposa queueMicrotask(() => listener()); } else { // TODO(atlowChemi) add { subscription: true } and return directly - signal.addEventListener("abort", listener, { once: true, [kResistStopPropagation]: true }); + signal.addEventListener("abort", listener, resistStopPropagation({ __proto__: null, once: true })); removeEventListener = () => { signal.removeEventListener("abort", listener); }; } return { + __proto__: null, [Symbol.dispose]() { removeEventListener?.(); }, - }; + } as Disposable; } export default { diff --git a/src/js/internal/shared.ts b/src/js/internal/shared.ts index 749d7eb4dff2..8faf4a423884 100644 --- a/src/js/internal/shared.ts +++ b/src/js/internal/shared.ts @@ -144,6 +144,15 @@ function once(callback, { preserveReturnValue = false } = kEmptyObject) { const kEmptyObject = ObjectFreeze(Object.create(null)); +// Marks an addEventListener() options object so that dispatch still invokes the +// listener after an unrelated listener called event.stopImmediatePropagation(). +// `$kResistStopPropagation` is a private symbol the native EventTarget reads, so +// only these internal modules can reach it. +function resistStopPropagation(options: T): T { + (options as AddEventListenerOptions).$kResistStopPropagation = true; + return options; +} + function getLazy(initializer: () => T) { let value: T; let initialized = false; @@ -321,6 +330,7 @@ export default { ErrnoException, once, getLazy, + resistStopPropagation, hasObserver, startPerf, @@ -332,7 +342,6 @@ export default { kHandle: Symbol("kHandle"), kAutoDestroyed: Symbol("kAutoDestroyed"), - kResistStopPropagation: Symbol("kResistStopPropagation"), kWeakHandler: Symbol("kWeak"), kGetNativeReadableProto: Symbol("kGetNativeReadableProto"), kEmptyObject, diff --git a/src/js/internal/streams/operators.ts b/src/js/internal/streams/operators.ts index 1508a66480a9..23f72a3a0915 100644 --- a/src/js/internal/streams/operators.ts +++ b/src/js/internal/streams/operators.ts @@ -1,7 +1,7 @@ "use strict"; const { validateAbortSignal, validateFunction, validateInteger, validateObject } = require("internal/validators"); -const { kWeakHandler, kResistStopPropagation } = require("internal/shared"); +const { kWeakHandler, resistStopPropagation } = require("internal/shared"); const { finished } = require("internal/streams/end-of-stream"); const MathFloor = Math.floor; @@ -233,7 +233,7 @@ async function reduce(reducer, initialValue, options) { const ac = new AbortController(); const signal = ac.signal; if (options?.signal) { - const opts = { once: true, [kWeakHandler]: this, [kResistStopPropagation]: true }; + const opts = resistStopPropagation({ once: true, [kWeakHandler]: this }); options.signal.addEventListener("abort", () => ac.abort(), opts); } let gotAnyItemFromStream = false; diff --git a/src/js/node/events.ts b/src/js/node/events.ts index 309429417f54..a96011a3da69 100644 --- a/src/js/node/events.ts +++ b/src/js/node/events.ts @@ -32,6 +32,8 @@ const { validateFunction, validateString, } = require("internal/validators"); +const { addAbortListener } = require("internal/abort_listener"); +const { resistStopPropagation } = require("internal/shared"); const types = require("node:util/types"); let inspect: typeof import("node:util").inspect | undefined; @@ -574,7 +576,8 @@ async function once(emitter, type, options = kEmptyObject) { } resolve(args); }; - eventTargetAgnosticAddListener(emitter, type, resolver, { once: true }); + const opts = resistStopPropagation({ __proto__: null, once: true }); + eventTargetAgnosticAddListener(emitter, type, resolver, opts); if (type !== "error" && typeof emitter.once === "function") { // EventTarget does not have `error` event semantics like Node // EventEmitters, we listen to `error` events only on EventEmitters. @@ -586,7 +589,7 @@ async function once(emitter, type, options = kEmptyObject) { reject($makeAbortError(undefined, { cause: signal?.reason })); } if (signal != null) { - eventTargetAgnosticAddListener(signal, "abort", abortListener, { once: true }); + eventTargetAgnosticAddListener(signal, "abort", abortListener, opts); } return promise; @@ -874,34 +877,6 @@ function getMaxListeners(emitterOrTarget) { } Object.defineProperty(getMaxListeners, "name", { value: "getMaxListeners" }); -// Copy-pasta from Node.js source code -function addAbortListener(signal, listener) { - if (signal === undefined) { - throw $ERR_INVALID_ARG_TYPE("signal", "AbortSignal", signal); - } - - validateAbortSignal(signal, "signal"); - if (typeof listener !== "function") { - throw $ERR_INVALID_ARG_TYPE("listener", "function", listener); - } - - let removeEventListener; - if (signal.aborted) { - queueMicrotask(() => listener()); - } else { - signal.addEventListener("abort", listener, { __proto__: null, once: true }); - removeEventListener = () => { - signal.removeEventListener("abort", listener); - }; - } - return { - __proto__: null, - [Symbol.dispose]() { - removeEventListener?.(); - }, - }; -} - let EventEmitterReferencingAsyncResource; function lazyLoadAsyncResource() { if (!AsyncResource) { diff --git a/src/js/node/timers.promises.ts b/src/js/node/timers.promises.ts index 10926308c4c1..2074fb57ba1a 100644 --- a/src/js/node/timers.promises.ts +++ b/src/js/node/timers.promises.ts @@ -2,6 +2,7 @@ // https://github.com/niksy/isomorphic-timers-promises/blob/master/index.js const { validateBoolean, validateAbortSignal, validateObject, validateNumber } = require("internal/validators"); +const { resistStopPropagation } = require("internal/shared"); const symbolAsyncIterator = Symbol.asyncIterator; const setImmediateGlobal = globalThis.setImmediate; @@ -65,7 +66,7 @@ function setTimeout(after = 1, value, options = {}) { clearTimeout(timeout); reject($makeAbortError(undefined, { cause: signal.reason })); }; - signal.addEventListener("abort", onCancel); + signal.addEventListener("abort", onCancel, resistStopPropagation({ __proto__: null })); } }); return typeof onCancel !== "undefined" @@ -104,7 +105,7 @@ function setImmediate(value, options = {}) { clearImmediate(immediate); reject($makeAbortError(undefined, { cause: signal.reason })); }; - signal.addEventListener("abort", onCancel); + signal.addEventListener("abort", onCancel, resistStopPropagation({ __proto__: null })); } }); return typeof onCancel !== "undefined" @@ -187,7 +188,7 @@ function setInterval(after = 1, value, options = {}) { callback = undefined; } }; - signal.addEventListener("abort", onCancel); + signal.addEventListener("abort", onCancel, resistStopPropagation({ __proto__: null, once: true })); } return asyncIterator({ diff --git a/src/js/node/util.ts b/src/js/node/util.ts index 88b6cc4a0ceb..d70fcb2c56c0 100644 --- a/src/js/node/util.ts +++ b/src/js/node/util.ts @@ -4,6 +4,7 @@ const types = require("node:util/types"); const utl = require("internal/util/inspect"); const { promisify } = require("internal/promisify"); const { validateString, validateOneOf, validateBoolean } = require("internal/validators"); +const { resistStopPropagation } = require("internal/shared"); const { MIMEType, MIMEParams } = require("internal/util/mime"); const { deprecate } = require("internal/util/deprecate"); @@ -275,7 +276,7 @@ function aborted(signal: AbortSignal, resource: object) { // Do not leak the current scope into the listener. // Instead, create a new function. unregisterToken, - { once: true }, + resistStopPropagation({ __proto__: null, once: true }), ); if (!lazyAbortedRegistry) { diff --git a/src/jsc/bindings/webcore/AddEventListenerOptions.h b/src/jsc/bindings/webcore/AddEventListenerOptions.h index ddf74b529b30..377792966ecf 100644 --- a/src/jsc/bindings/webcore/AddEventListenerOptions.h +++ b/src/jsc/bindings/webcore/AddEventListenerOptions.h @@ -43,6 +43,11 @@ struct AddEventListenerOptions : EventListenerOptions { std::optional passive; bool once { false }; RefPtr signal; + + // Not part of the DOM standard. Set through a private symbol by Bun's internal + // modules, mirroring Node.js's kResistStopPropagation: a listener registered + // with it still runs after another listener called stopImmediatePropagation(). + bool resistStopPropagation { false }; }; } // namespace WebCore diff --git a/src/jsc/bindings/webcore/EventTarget.cpp b/src/jsc/bindings/webcore/EventTarget.cpp index 42c929f1b3a0..e3d74425215e 100644 --- a/src/jsc/bindings/webcore/EventTarget.cpp +++ b/src/jsc/bindings/webcore/EventTarget.cpp @@ -103,7 +103,7 @@ bool EventTarget::addEventListener(const AtomString& eventType, Refcallback(), registeredListener->useCapture())) // continue; - // If stopImmediatePropagation has been called, we just break out immediately, without - // handling any more events on this target. - if (event.immediatePropagationStopped()) - break; + // If stopImmediatePropagation has been called, skip the remaining listeners. Listeners + // registered with resistStopPropagation still run: they are how internal modules attach + // teardown that unrelated code sharing the event target must not be able to suppress. + if (event.immediatePropagationStopped() && !registeredListener->resistsStopPropagation()) + continue; // Make sure the JS wrapper and function stay alive until the end of this scope. Otherwise, // event listeners with 'once' flag may get collected as soon as they get unregistered below, diff --git a/src/jsc/bindings/webcore/JSAddEventListenerOptions.cpp b/src/jsc/bindings/webcore/JSAddEventListenerOptions.cpp index d3eea92f517c..9d7291182071 100644 --- a/src/jsc/bindings/webcore/JSAddEventListenerOptions.cpp +++ b/src/jsc/bindings/webcore/JSAddEventListenerOptions.cpp @@ -21,6 +21,7 @@ #include "config.h" #include "JSAddEventListenerOptions.h" +#include "BunClientData.h" #include "JSAbortSignal.h" #include "JSDOMConvertBoolean.h" #include "JSDOMConvertInterface.h" @@ -86,6 +87,16 @@ template<> AddEventListenerOptions convertDictionary(JS result.signal = convert>(lexicalGlobalObject, signalValue); RETURN_IF_EXCEPTION(throwScope, {}); } + // Bun extension, keyed off a private symbol that only internal modules can + // reach. See AddEventListenerOptions::resistStopPropagation. + if (!isNullOrUndefined) { + JSValue resistStopPropagationValue = object->get(&lexicalGlobalObject, builtinNames(vm).kResistStopPropagationPrivateName()); + RETURN_IF_EXCEPTION(throwScope, {}); + if (!resistStopPropagationValue.isUndefined()) { + result.resistStopPropagation = convert(lexicalGlobalObject, resistStopPropagationValue); + RETURN_IF_EXCEPTION(throwScope, {}); + } + } return result; } diff --git a/src/jsc/bindings/webcore/RegisteredEventListener.h b/src/jsc/bindings/webcore/RegisteredEventListener.h index cece5ce1a663..6b7d847595e9 100644 --- a/src/jsc/bindings/webcore/RegisteredEventListener.h +++ b/src/jsc/bindings/webcore/RegisteredEventListener.h @@ -36,16 +36,18 @@ class WeakPtrImplWithEventTargetData; class RegisteredEventListener : public RefCounted { public: struct Options { - Options(bool capture = false, bool passive = false, bool once = false) + Options(bool capture = false, bool passive = false, bool once = false, bool resistStopPropagation = false) : capture(capture) , passive(passive) , once(once) + , resistStopPropagation(resistStopPropagation) { } bool capture; bool passive; bool once; + bool resistStopPropagation; }; static Ref create(Ref&& listener, const Options& options) @@ -60,6 +62,7 @@ class RegisteredEventListener : public RefCounted { bool isPassive() const { return m_isPassive; } bool isOnce() const { return m_isOnce; } bool wasRemoved() const { return m_wasRemoved; } + bool resistsStopPropagation() const { return m_resistStopPropagation; } void markAsRemoved(); @@ -77,6 +80,7 @@ class RegisteredEventListener : public RefCounted { , m_isPassive(options.passive) , m_isOnce(options.once) , m_wasRemoved(false) + , m_resistStopPropagation(options.resistStopPropagation) , m_callback(WTF::move(listener)) { } @@ -85,6 +89,7 @@ class RegisteredEventListener : public RefCounted { bool m_isPassive : 1; bool m_isOnce : 1; bool m_wasRemoved : 1; + bool m_resistStopPropagation : 1; uint32_t m_abortAlgorithmIdentifier { 0 }; Ref m_callback; WeakPtr m_abortSignal; diff --git a/test/js/node/events/event-emitter.test.ts b/test/js/node/events/event-emitter.test.ts index 6c43069780b2..7c5d130ff7f5 100644 --- a/test/js/node/events/event-emitter.test.ts +++ b/test/js/node/events/event-emitter.test.ts @@ -877,6 +877,85 @@ test("using addAbortListener", async () => { expect(mocked).not.toHaveBeenCalled(); }); +describe("addAbortListener resists stopImmediatePropagation", () => { + test("runs after an earlier listener stopped propagation", () => { + const controller = new AbortController(); + const signal = controller.signal; + const order: string[] = []; + + signal.addEventListener("abort", e => { + order.push("stopper"); + e.stopImmediatePropagation(); + }); + EventEmitter.addAbortListener(signal, e => { + order.push(`cleanup:${(e as Event).target === signal}`); + }); + signal.addEventListener("abort", () => order.push("plain-after")); + + controller.abort(); + expect(order).toEqual(["stopper", "cleanup:true"]); + }); + + test("runs when it was registered before the listener that stops propagation", () => { + const controller = new AbortController(); + const signal = controller.signal; + const order: string[] = []; + + EventEmitter.addAbortListener(signal, () => order.push("cleanup")); + signal.addEventListener("abort", e => { + order.push("stopper"); + e.stopImmediatePropagation(); + }); + signal.addEventListener("abort", () => order.push("plain-after")); + + controller.abort(); + expect(order).toEqual(["cleanup", "stopper"]); + }); + + test("is not run once disposed", () => { + const controller = new AbortController(); + const signal = controller.signal; + const mocked = mock(); + + signal.addEventListener("abort", e => e.stopImmediatePropagation()); + { + using _ = EventEmitter.addAbortListener(signal, mocked); + } + + controller.abort(); + expect(mocked).not.toHaveBeenCalled(); + }); + + test("once(emitter, event, { signal }) still rejects on a suppressed signal", async () => { + const emitter = new EventEmitter(); + const controller = new AbortController(); + controller.signal.addEventListener("abort", e => e.stopImmediatePropagation()); + + const promise = EventEmitter.once(emitter, "never", { signal: controller.signal }); + expect(emitter.listenerCount("never")).toBe(1); + controller.abort(); + + // once()'s abort listener detaches the emitter listener and rejects, both synchronously. + expect(emitter.listenerCount("never")).toBe(0); + expect(await promise.catch(err => err.code)).toBe("ABORT_ERR"); + }); + + test("stopImmediatePropagation still suppresses ordinary listeners", () => { + const target = new EventTarget(); + const order: string[] = []; + + target.addEventListener("x", () => order.push("a")); + target.addEventListener("x", e => { + order.push("b"); + e.stopImmediatePropagation(); + }); + target.addEventListener("x", () => order.push("c")); + + target.dispatchEvent(new Event("x")); + expect(order).toEqual(["a", "b"]); + }); +}); + test("getMaxListeners", () => { const emitter = new EventEmitter(); expect(emitter.getMaxListeners()).toBe(10); diff --git a/test/js/node/timers.promises/timers.promises.test.ts b/test/js/node/timers.promises/timers.promises.test.ts index 9f79c2c369a7..06b210416413 100644 --- a/test/js/node/timers.promises/timers.promises.test.ts +++ b/test/js/node/timers.promises/timers.promises.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { setImmediate, setTimeout } from "node:timers/promises"; +import { setImmediate, setInterval, setTimeout } from "node:timers/promises"; describe("setTimeout", () => { it("abort() does not emit global error", async () => { @@ -37,6 +37,19 @@ describe("setTimeout", () => { await expect(promise).rejects.toThrow(expect.objectContaining({ name: "AbortError" })); expect(abortController.signal.aborted).toBe(true); }); + + // abort() runs the listener synchronously, so the short delay can never win the + // race: it only exists so a build that ignores the abort resolves and fails the + // assertion rather than leaving the promise pending forever. + it("rejects even when another listener stopped propagation", async () => { + const abortController = new AbortController(); + abortController.signal.addEventListener("abort", e => e.stopImmediatePropagation()); + + const promise = setTimeout(1, "not-aborted", { signal: abortController.signal }); + abortController.abort(); + + await expect(promise).rejects.toThrow(expect.objectContaining({ name: "AbortError" })); + }); }); describe("setImmediate", () => { @@ -62,4 +75,32 @@ describe("setImmediate", () => { expect(c.signal.aborted).toBe(true); expect(unhandledRejectionCaught).toBe(false); }); + + it("rejects even when another listener stopped propagation", async () => { + const abortController = new AbortController(); + abortController.signal.addEventListener("abort", e => e.stopImmediatePropagation()); + + const promise = setImmediate("not-aborted", { signal: abortController.signal }); + abortController.abort(); + + await expect(promise).rejects.toThrow(expect.objectContaining({ name: "AbortError" })); + }); +}); + +describe("setInterval", () => { + it("ends the iterator even when another listener stopped propagation", async () => { + const abortController = new AbortController(); + abortController.signal.addEventListener("abort", e => e.stopImmediatePropagation()); + + const iterator = setInterval(1, "tick", { signal: abortController.signal })[Symbol.asyncIterator](); + try { + const next = iterator.next(); + abortController.abort(); + + await expect(next).rejects.toThrow(expect.objectContaining({ name: "AbortError" })); + } finally { + // On a build that ignores the abort the interval is still armed; clear it. + await iterator.return!(); + } + }); }); diff --git a/test/js/node/util/test-aborted.test.ts b/test/js/node/util/test-aborted.test.ts index 11af6f00e04d..9e6e8b6100ec 100644 --- a/test/js/node/util/test-aborted.test.ts +++ b/test/js/node/util/test-aborted.test.ts @@ -15,6 +15,18 @@ test("aborted works when provided a resource that was already aborted", () => { return expect(abortedPromise).resolves.toBeUndefined(); }); +test("aborted resolves even when another listener stopped propagation", async () => { + const ac = new AbortController(); + ac.signal.addEventListener("abort", e => e.stopImmediatePropagation()); + const abortedPromise = aborted(ac.signal, {}); + ac.abort(); + + // aborted() resolves from a `once: true` listener, synchronously inside abort(), + // so by now it has been consumed and only the listener that stopped propagation is left. + expect(getEventListeners(ac.signal, "abort")).toHaveLength(1); + await expect(abortedPromise).resolves.toBeUndefined(); +}); + test("aborted works when provided a resource that was not already aborted", async () => { const ac = new AbortController(); var strong = {};