events: fix weak listener retention overwrite

The weakListeners() retention map in EventTarget stored a single
listener per retainer key. Multiple addEventListener() calls sharing
the same retainer (e.g. two listeners on one target using the same
AbortSignal) silently evicted each other's strong reference, allowing
the evicted listener to be garbage collected before its signal
aborted, so only the most recently registered listener was ever
actually removed on abort.

Changed the retention map to store a Set of listeners per retainer
key instead of a single listener, and added matching cleanup in
remove() so entries are released once their retainer's set is empty.

This also fixes the same class of bug in events.aborted() and the
streams kWeakHandler usage, since both go through the same
EventTarget.prototype.addEventListener() code path.

Fixes: #63954
Signed-off-by: aryan7905 <aryansrivastava354@gmail.com>
PR-URL: #64024
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Aviv Keller <me@aviv.sh>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Author: aryansaves

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);
+        }
+      }
+    }
   }
 }
 

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);
+});

test/parallel/test-eventtarget.js

@@ -699,7 +699,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();