diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js index 9948d3f1868e32..a8c2b2d42e93d5 100644 --- a/lib/internal/event_target.js +++ b/lib/internal/event_target.js @@ -14,6 +14,7 @@ const { ReflectApply, SafeFinalizationRegistry, SafeMap, + SafeSet, SafeWeakMap, SafeWeakRef, SafeWeakSet, @@ -491,8 +492,17 @@ class Listener { listener: this, eventType, }, this); - // Make the retainer retain the listener in a WeakMap - weakListeners().map.set(weak, listener); + // Store only a WeakRef to the retainer here. Listener instances are + // Strongly reachable from the EventTarget's listener list for as long + // as they're registered, so a plain strong reference would keep the + // retainer (and listener) alive forever, defeating weak retention + this.weakKeyRef = new SafeWeakRef(weak); + let retained = weakListeners().map.get(weak); + if (retained === undefined) { + retained = new SafeSet(); + weakListeners().map.set(weak, retained); + } + retained.add(listener); this.listener = this.callback; } else if (typeof listener === 'function') { this.callback = listener; @@ -545,8 +555,19 @@ class Listener { if (this.next !== undefined) this.next.previous = this.previous; this.removed = true; - if (this.weak) + if (this.weak) { weakListeners().registry.unregister(this); + const weakKey = this.weakKeyRef?.deref(); + const listener = this.callback?.deref(); + if (weakKey !== undefined && listener !== undefined) { + const retained = weakListeners().map.get(weakKey); + if (retained !== undefined) { + retained.delete(listener); + if (retained.size === 0) + weakListeners().map.delete(weakKey); + } + } + } } } diff --git a/test/parallel/test-abortcontroller-internal.js b/test/parallel/test-abortcontroller-internal.js index 63dd26c36e32a0..cd6dfb1a74d8e6 100644 --- a/test/parallel/test-abortcontroller-internal.js +++ b/test/parallel/test-abortcontroller-internal.js @@ -30,3 +30,22 @@ test('A weak event listener should not prevent gc', async () => { globalThis.gc(); assert.strictEqual(ref.deref(), undefined); }); + +test('two weak listeners with the same retainer should both run on abort', async () => { + // Regression test for https://github.com/nodejs/node/issues/63954 + const ac = new AbortController(); + let aRan = false; + let bRan = false; + + ac.signal.addEventListener('a', () => { aRan = true; }, { [kWeakHandler]: ac }); + ac.signal.addEventListener('b', () => { bRan = true; }, { [kWeakHandler]: ac }); + + await sleep(10); + globalThis.gc(); + + ac.signal.dispatchEvent(new Event('a')); + ac.signal.dispatchEvent(new Event('b')); + + assert.strictEqual(aRan, true); + assert.strictEqual(bRan, true); +}); diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js index d0b3ad03a1df6a..fbfdcf586f6ed1 100644 --- a/test/parallel/test-eventtarget.js +++ b/test/parallel/test-eventtarget.js @@ -685,7 +685,21 @@ let asyncTest = Promise.resolve(); et.dispatchEvent(new Event('foo')); }); } - +{ + // Two listeners sharing the same retainer key must NOT evict each + // other from the weak retention map — both must survive a GC cycle + // and both must be removable independently. + // Regression test for https://github.com/nodejs/node/issues/63954 + const et = new EventTarget(); + const aCalled = common.mustNotCall(); + const bCalled = common.mustCall(); + et.addEventListener('a', aCalled, { [kWeakHandler]: et }); + et.addEventListener('b', bCalled, { [kWeakHandler]: et }); + globalThis.gc(); + et.removeEventListener('a', aCalled); + et.dispatchEvent(new Event('a')); + et.dispatchEvent(new Event('b')); +} { const et = new EventTarget();