fix(zone.js): store remove abort listener on the scheduled task (#56160)

arturovt · thePunderWoman · commit 4a3800a6a0ae · 2024-06-04T17:29:05.000Z
Prior to this commit, a memory leak occurred when the `abort` listener was not removed from the `AbortSignal`. We introduced a fix to remove the event listener, but it was erroneously stored on the `taskData`, which is a shared global object. Consequently, when something attempted to remove an event listener, it immediately removed the last stored abort listener. As a result, events would never be canceled. We have now rectified this by storing the remove abort listener function directly on the task itself. This adjustment ensures that the abort listener is tied only to the specific task. When the `abort` function is called, it cancels the task. Therefore, it is safe to associate the cleanup function directly with the task. Closes: #56148 PR Close #56160
diff --git a/packages/zone.js/lib/common/events.ts b/packages/zone.js/lib/common/events.ts
@@ -27,11 +27,10 @@ interface EventTaskData extends TaskData {
   // use global callback or not
   readonly useG?: boolean;
   taskData?: any;
-  removeAbortListener?: VoidFunction | null;
 }
 
 /** @internal **/
-interface InternalTaskData {
+interface InternalGlobalTaskData {
   // This is used internally to avoid duplicating event listeners on
   // the same target when the event name is the same, such as when
   // `addEventListener` is called multiple times on the `document`
@@ -46,6 +45,36 @@ interface InternalTaskData {
   options?: any; // boolean | AddEventListenerOptions
 }
 
+/**
+ * The `scheduleEventTask` function returns an `EventTask` object.
+ * However, we also store some task-related information on the task
+ * itself, such as the task target, for easy access when the task is
+ * manually canceled or for other purposes. This internal storage is
+ * used solely for enhancing our understanding of which properties are
+ * being assigned to the task.
+ *
+ * @internal
+ */
+interface InternalEventTask extends EventTask {
+  removeAbortListener?: VoidFunction | null;
+  // `target` is the actual event target on which `addEventListener`
+  // is being called for this specific task.
+  target?: any;
+  eventName?: string;
+  capture?: boolean;
+  // Not changing the type to avoid any regressions.
+  options?: any; // boolean | AddEventListenerOptions
+  // `isRemoved` is associated with a specific task and indicates whether
+  // that task was canceled and removed from the event target to prevent
+  // its invocation if dispatched later.
+  isRemoved?: boolean;
+  allRemoved?: boolean;
+}
+
+// Note that passive event listeners are now supported by most modern browsers,
+// including Chrome, Firefox, Safari, and Edge. There's a pending change that
+// would remove support for legacy browsers by zone.js. Removing `passiveSupported`
+// from the codebase will reduce the final code size for existing apps that still use zone.js.
 let passiveSupported = false;
 
 if (typeof window !== 'undefined') {
@@ -263,9 +292,15 @@ export function patchEventTarget(
 
     const eventNameToString = patchOptions && patchOptions.eventNameToString;
 
-    // a shared global taskData to pass data for scheduleEventTask
-    // so we do not need to create a new object just for pass some data
-    const taskData: InternalTaskData = {};
+    // We use a shared global `taskData` to pass data for `scheduleEventTask`,
+    // eliminating the need to create a new object solely for passing data.
+    // WARNING: This object has a static lifetime, meaning it is not created
+    // each time `addEventListener` is called. It is instantiated only once
+    // and captured by reference inside the `addEventListener` and
+    // `removeEventListener` functions. Do not add any new properties to this
+    // object, as doing so would necessitate maintaining the information
+    // between `addEventListener` calls.
+    const taskData: InternalGlobalTaskData = {};
 
     const nativeAddEventListener = (proto[zoneSymbolAddEventListener] = proto[ADD_EVENT_LISTENER]);
     const nativeRemoveEventListener = (proto[zoneSymbol(REMOVE_EVENT_LISTENER)] =
@@ -322,12 +357,18 @@ export function patchEventTarget(
       );
     };
 
-    const customCancelGlobal = function (task: any) {
+    /**
+     * In the context of events and listeners, this function will be
+     * called at the end by `cancelTask`, which, in turn, calls `task.cancelFn`.
+     * Cancelling a task is primarily used to remove event listeners from
+     * the task target.
+     */
+    const customCancelGlobal = function (task: InternalEventTask) {
       // if task is not marked as isRemoved, this call is directly
       // from Zone.prototype.cancelTask, we should remove the task
       // from tasksList of target first
       if (!task.isRemoved) {
-        const symbolEventNames = zoneSymbolEventNames[task.eventName];
+        const symbolEventNames = zoneSymbolEventNames[task.eventName!];
         let symbolEventName;
         if (symbolEventNames) {
           symbolEventName = symbolEventNames[task.capture ? TRUE_STR : FALSE_STR];
@@ -340,6 +381,10 @@ export function patchEventTarget(
               existingTasks.splice(i, 1);
               // set isRemoved to data for faster invokeTask check
               task.isRemoved = true;
+              if (task.removeAbortListener) {
+                task.removeAbortListener();
+                task.removeAbortListener = null;
+              }
               if (existingTasks.length === 0) {
                 // all tasks for the eventName + capture have gone,
                 // remove globalZoneAwareCallback and remove the task cache from target
@@ -563,7 +608,7 @@ export function patchEventTarget(
         // which in turn calls the native `addEventListener`. This is why `taskData.options`
         // is updated before scheduling the task, as `customScheduleGlobal` uses
         // `taskData.options` to pass it to the native `addEventListener`.
-        const task: any = zone.scheduleEventTask(
+        const task: InternalEventTask = zone.scheduleEventTask(
           source,
           delegate,
           data,
@@ -583,9 +628,7 @@ export function patchEventTarget(
           // as it creates a closure that captures `task`. This closure retains a reference to the
           // `task` object even after it goes out of scope, preventing `task` from being garbage
           // collected.
-          if (data) {
-            data.removeAbortListener = () => signal.removeEventListener('abort', onAbort);
-          }
+          task.removeAbortListener = () => signal.removeEventListener('abort', onAbort);
         }
 
         // should clear taskData.target to avoid memory leak
@@ -670,18 +713,22 @@ export function patchEventTarget(
       if (symbolEventNames) {
         symbolEventName = symbolEventNames[capture ? TRUE_STR : FALSE_STR];
       }
-      const existingTasks: Task[] = symbolEventName && target[symbolEventName];
+      const existingTasks: InternalEventTask[] = symbolEventName && target[symbolEventName];
+      // `existingTasks` may not exist if the `addEventListener` was called before
+      // it was patched by zone.js. Please refer to the attached issue for
+      // clarification, particularly after the `if` condition, before calling
+      // the native `removeEventListener`.
       if (existingTasks) {
         for (let i = 0; i < existingTasks.length; i++) {
           const existingTask = existingTasks[i];
           if (compare(existingTask, delegate)) {
             existingTasks.splice(i, 1);
             // set isRemoved to data for faster invokeTask check
-            (existingTask as any).isRemoved = true;
+            existingTask.isRemoved = true;
             if (existingTasks.length === 0) {
               // all tasks for the eventName + capture have gone,
               // remove globalZoneAwareCallback and remove the task cache from target
-              (existingTask as any).allRemoved = true;
+              existingTask.allRemoved = true;
               target[symbolEventName] = null;
               // in the target, we have an event listener which is added by on_property
               // such as target.onclick = function() {}, so we need to clear this internal
@@ -693,15 +740,11 @@ export function patchEventTarget(
                 target[onPropertySymbol] = null;
               }
             }
-
-            // Note that `removeAllListeners` would ultimately call `removeEventListener`,
-            // so we're safe to remove the abort listener only once here.
-            const taskData = existingTask.data as EventTaskData;
-            if (taskData?.removeAbortListener) {
-              taskData.removeAbortListener();
-              taskData.removeAbortListener = null;
-            }
-
+            // In all other conditions, when `addEventListener` is called after being
+            // patched by zone.js, we would always find an event task on the `EventTarget`.
+            // This will trigger `cancelFn` on the `existingTask`, leading to `customCancelGlobal`,
+            // which ultimately removes an event listener and cleans up the abort listener
+            // (if an `AbortSignal` was provided when scheduling a task).
             existingTask.zone.cancelTask(existingTask);
             if (returnTarget) {
               return target;
@@ -710,10 +753,12 @@ export function patchEventTarget(
           }
         }
       }
-      // issue 930, didn't find the event name or callback
-      // from zone kept existingTasks, the callback maybe
-      // added outside of zone, we need to call native removeEventListener
-      // to try to remove it.
+      // https://github.com/angular/zone.js/issues/930
+      // We may encounter a situation where the `addEventListener` was
+      // called on the event target before zone.js is loaded, resulting
+      // in no task being stored on the event target due to its invocation
+      // of the native implementation. In this scenario, we simply need to
+      // invoke the native `removeEventListener`.
       return nativeRemoveEventListener.apply(this, arguments);
     };
 
diff --git a/packages/zone.js/test/browser/browser.spec.ts b/packages/zone.js/test/browser/browser.spec.ts
@@ -2276,6 +2276,56 @@ describe('Zone', function () {
         expect(logs).toEqual(['click2']);
       });
 
+      // https://github.com/angular/angular/issues/56148
+      it('should store the remove abort listener on the task itself and not the task data', function () {
+        const logs: string[] = [];
+        const ac = new AbortController();
+        const onLoad = jasmine.createSpy();
+
+        // Note that we have to manually spy on the `removeEventListener`
+        // because zone.js calls the native `addEventListener` for an
+        // `AbortSignal`, which isn't patched by zone.js. Thus, calling
+        // `signal.eventListeners('abort')` would always return an empty list.
+        const removeEventListenerSpy = spyOn(
+          AbortSignal.prototype,
+          'removeEventListener',
+        ).and.callThrough();
+
+        window.addEventListener('load', onLoad, {once: true});
+
+        button.addEventListener(
+          'click',
+          function () {
+            logs.push('click');
+          },
+          {signal: ac.signal},
+        );
+
+        // In this scenario, we must dispatch the load event because
+        // `{once:true}` options are provided. This action will remove
+        // the event listener on the `window` after the task is executed,
+        // effectively cancelling it. Previously, we encountered a regression
+        // where `removeAbortListener` was stored on the task data instead of
+        // the task itself. Consequently, it would remove the `abort` listener
+        // from the signal. As a result, calling `abort()` on the controller wouldn't
+        // cancel any events because there was no `abort` listener present.
+        window.dispatchEvent(new Event('load'));
+        expect(onLoad).toHaveBeenCalled();
+
+        // Assert that the `abort` event listener has NOT been removed.
+        expect(removeEventListenerSpy).toHaveBeenCalledTimes(0);
+
+        // When calling abort, it should now remove the event listener
+        // from the button, thus dispatching events should not trigger the listener.
+        ac.abort();
+
+        button.dispatchEvent(clickEvent);
+
+        const listeners = button.eventListeners!('click');
+        expect(listeners.length).toBe(0);
+        expect(logs).toEqual([]);
+      });
+
       it('should not add event listeners with aborted signal', function () {
         let logs: string[] = [];
 
