nodejs/node · #64024
events: fix weak listener retention overwrite
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();