nodejs/node · #64024

events: fix weak listener retention overwrite

aryansaves · merged Aug 28, 20263 files · 58 + / 4
lib/internal/event_target.js24 + / 3
@@ -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.js19 + / 0
@@ -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.js15 + / 1
@@ -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();