events: remove weak listener for event target

Fixes: #48951 PR-URL: #48952 Reviewed-By: Chemi Atlow <[email protected]>
nodejs · Aug 17, 2023 · d568600 · d568600
1 parent dca8678
commit d568600
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 8 deletions.
diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js
@@ -62,6 +62,7 @@ const kWeakHandler = Symbol('kWeak');
 const kResistStopPropagation = Symbol('kResistStopPropagation');
 
 const kHybridDispatch = SymbolFor('nodejs.internal.kHybridDispatch');
+const kRemoveWeakListenerHelper = Symbol('nodejs.internal.removeWeakListenerHelper');
 const kCreateEvent = Symbol('kCreateEvent');
 const kNewListener = Symbol('kNewListener');
 const kRemoveListener = Symbol('kRemoveListener');
@@ -410,7 +411,7 @@ let weakListenersState = null;
 let objectToWeakListenerMap = null;
 function weakListeners() {
   weakListenersState ??= new SafeFinalizationRegistry(
-    (listener) => listener.remove(),
+    ({ eventTarget, listener, eventType }) => eventTarget.deref()?.[kRemoveWeakListenerHelper](eventType, listener),
   );
   objectToWeakListenerMap ??= new SafeWeakMap();
   return { registry: weakListenersState, map: objectToWeakListenerMap };
@@ -432,7 +433,7 @@ const kFlagResistStopPropagation = 1 << 6;
 // the linked list makes dispatching faster, even if adding/removing is
 // slower.
 class Listener {
-  constructor(previous, listener, once, capture, passive,
+  constructor(eventTarget, eventType, previous, listener, once, capture, passive,
               isNodeStyleListener, weak, resistStopPropagation) {
     this.next = undefined;
     if (previous !== undefined)
@@ -459,7 +460,13 @@ class Listener {
 
     if (this.weak) {
       this.callback = new SafeWeakRef(listener);
-      weakListeners().registry.register(listener, this, this);
+      weakListeners().registry.register(listener, {
+        __proto__: null,
+        // Weak ref so the listener won't hold the eventTarget alive
+        eventTarget: new SafeWeakRef(eventTarget),
+        listener: this,
+        eventType,
+      }, this);
       // Make the retainer retain the listener in a WeakMap
       weakListeners().map.set(weak, listener);
       this.listener = this.callback;
@@ -625,7 +632,7 @@ class EventTarget {
     if (root === undefined) {
       root = { size: 1, next: undefined, resistStopPropagation: Boolean(resistStopPropagation) };
       // This is the first handler in our linked list.
-      new Listener(root, listener, once, capture, passive,
+      new Listener(this, type, root, listener, once, capture, passive,
                    isNodeStyleListener, weak, resistStopPropagation);
       this[kNewListener](
         root.size,
@@ -652,7 +659,7 @@ class EventTarget {
       return;
     }
 
-    new Listener(previous, listener, once, capture, passive,
+    new Listener(this, type, previous, listener, once, capture, passive,
                  isNodeStyleListener, weak, resistStopPropagation);
     root.size++;
     root.resistStopPropagation ||= Boolean(resistStopPropagation);
@@ -695,6 +702,28 @@ class EventTarget {
     }
   }
 
+  [kRemoveWeakListenerHelper](type, listener) {
+    const root = this[kEvents].get(type);
+    if (root === undefined || root.next === undefined)
+      return;
+
+    const capture = listener.capture === true;
+
+    let handler = root.next;
+    while (handler !== undefined) {
+      if (handler === listener) {
+        handler.remove();
+        root.size--;
+        if (root.size === 0)
+          this[kEvents].delete(type);
+        // Undefined is passed as the listener as the listener was GCed
+        this[kRemoveListener](root.size, type, undefined, capture);
+        break;
+      }
+      handler = handler.next;
+    }
+  }
+
   /**
    * @param {Event} event
    */

diff --git a/test/parallel/test-eventtarget-memoryleakwarning.js b/test/parallel/test-eventtarget-memoryleakwarning.js
@@ -1,11 +1,13 @@
-// Flags: --no-warnings
+// Flags: --expose-internals --no-warnings --expose-gc
 'use strict';
 const common = require('../common');
 const {
   setMaxListeners,
   EventEmitter,
 } = require('events');
 const assert = require('assert');
+const { kWeakHandler } = require('internal/event_target');
+const { setTimeout } = require('timers/promises');
 
 common.expectWarning({
   MaxListenersExceededWarning: [
@@ -73,3 +75,20 @@ common.expectWarning({
   setMaxListeners(2, ee);
   assert.strictEqual(ee.getMaxListeners(), 2);
 }
+
+{
+  (async () => {
+    // Test that EventTarget listener don't emit MaxListenersExceededWarning for weak listeners that GCed
+    const et = new EventTarget();
+    setMaxListeners(2, et);
+
+    for (let i = 0; i <= 3; i++) {
+      et.addEventListener('foo', () => {}, {
+        [kWeakHandler]: {},
+      });
+
+      await setTimeout(0);
+      global.gc();
+    }
+  })().then(common.mustCall(), common.mustNotCall());
+}
diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js
@@ -16,8 +16,8 @@ const {
 
 const { once } = require('events');
 
-const { promisify, inspect } = require('util');
-const delay = promisify(setTimeout);
+const { inspect } = require('util');
+const { setTimeout: delay } = require('timers/promises');
 
 // The globals are defined.
 ok(Event);