events: fix weak listener retention overwrite · nodejs/node@f6b4793 · GitHub
Skip to content

Commit f6b4793

Browse files
aryansavesaduh95
authored andcommitted
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>
1 parent e3f52b8 commit f6b4793

3 files changed

Lines changed: 58 additions & 4 deletions

File tree

‎lib/internal/event_target.js‎

Lines changed: 24 additions & 3 deletions

‎test/parallel/test-abortcontroller-internal.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,22 @@ test('A weak event listener should not prevent gc', async () => {
3030
globalThis.gc();
3131
assert.strictEqual(ref.deref(), undefined);
3232
});
33+
34+
test('two weak listeners with the same retainer should both run on abort', async () => {
35+
// Regression test for https://github.com/nodejs/node/issues/63954
36+
const ac = new AbortController();
37+
let aRan = false;
38+
let bRan = false;
39+
40+
ac.signal.addEventListener('a', () => { aRan = true; }, { [kWeakHandler]: ac });
41+
ac.signal.addEventListener('b', () => { bRan = true; }, { [kWeakHandler]: ac });
42+
43+
await sleep(10);
44+
globalThis.gc();
45+
46+
ac.signal.dispatchEvent(new Event('a'));
47+
ac.signal.dispatchEvent(new Event('b'));
48+
49+
assert.strictEqual(aRan, true);
50+
assert.strictEqual(bRan, true);
51+
});

‎test/parallel/test-eventtarget.js‎

Lines changed: 15 additions & 1 deletion

0 commit comments

Comments
 (0)