From 142dfed0859d1571b17f9a473cfd98a0d38f9295 Mon Sep 17 00:00:00 2001
From: aryan7905 <aryansrivastava354@gmail.com>
Date: Sat, 20 Jun 2026 16:07:17 +0530
Subject: [PATCH] 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: https://github.com/nodejs/node/issues/63954
Signed-off-by: aryan7905 <aryansrivastava354@gmail.com>
---
 lib/internal/event_target.js                  | 27 ++++++++++++++++---
 .../parallel/test-abortcontroller-internal.js | 19 +++++++++++++
 test/parallel/test-eventtarget.js             | 16 ++++++++++-
 3 files changed, 58 insertions(+), 4 deletions(-)

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();