From ccd6e48f78a0f64639d135536de5abd35cbd0280 Mon Sep 17 00:00:00 2001
From: arturovt <arthurandrosovich@gmail.com>
Date: Thu, 19 Sep 2024 20:22:46 +0300
Subject: [PATCH] fix(zone.js): remove `abort` listener once fetch is settled

This commit updates the `fetch` patch for zone.js. Currently, we're attaching an
`abort` event listener on the signal (when it's provided) and never removing it.
We should be good citizens and remove event listeners whenever objects need to be
properly collected. In Firefox, when saving a heap snapshot and running it through
`fxsnapshot`, querying `AbortSignal` will print a so-called "CaptureMap" with a list
of "lambdas," indicating that the signal is not garbage collected because of the event
listener lambda function.
---
 packages/zone.js/lib/common/fetch.ts       | 27 ++++++++++++++--------
 packages/zone.js/test/common/fetch.spec.ts |  9 ++++++++
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/packages/zone.js/lib/common/fetch.ts b/packages/zone.js/lib/common/fetch.ts
index d2892d6ad2d3..cd95d46884a7 100644
--- a/packages/zone.js/lib/common/fetch.ts
+++ b/packages/zone.js/lib/common/fetch.ts
@@ -99,22 +99,31 @@ export function patchFetch(Zone: ZoneType): void {
       options.signal = fetchSignal;
       args[1] = options;
 
+      let onAbort: () => void;
       if (signal) {
         const nativeAddEventListener =
           signal[Zone.__symbol__('addEventListener') as 'addEventListener'] ||
           signal.addEventListener;
 
-        nativeAddEventListener.call(
-          signal,
-          'abort',
-          function () {
-            ac!.abort();
-          },
-          {once: true},
-        );
+        onAbort = () => ac!.abort();
+        nativeAddEventListener.call(signal, 'abort', onAbort, {once: true});
       }
 
-      return createFetchTask('fetch', {fetchArgs: args} as FetchTaskData, fetch, this, args, ac);
+      return createFetchTask(
+        'fetch',
+        {fetchArgs: args} as FetchTaskData,
+        fetch,
+        this,
+        args,
+        ac,
+      ).finally(() => {
+        // We need to be good citizens and remove the `abort` listener once
+        // the fetch is settled. The `abort` listener may not be called at all,
+        // which means the event listener closure would retain a reference to
+        // the `ac` object even if it goes out of scope. Since browser's garbage
+        // collectors work differently, some may not be smart enough to collect a signal.
+        signal?.removeEventListener('abort', onAbort);
+      });
     };
 
     if (OriginalResponse?.prototype) {
diff --git a/packages/zone.js/test/common/fetch.spec.ts b/packages/zone.js/test/common/fetch.spec.ts
index d6228e1dc980..d4cf8d8baf37 100644
--- a/packages/zone.js/test/common/fetch.spec.ts
+++ b/packages/zone.js/test/common/fetch.spec.ts
@@ -169,6 +169,10 @@ describe(
                       'invokeTask:fetch:macroTask',
                       'scheduleTask:Promise.then:microTask',
                       'invokeTask:Promise.then:microTask',
+
+                      // This is the `finally` task, which is used for cleanup.
+                      'scheduleTask:Promise.then:microTask',
+                      'invokeTask:Promise.then:microTask',
                     ]);
                     done();
                   },
@@ -194,6 +198,11 @@ describe(
                       'invokeTask:fetch:macroTask',
                       'scheduleTask:Promise.then:microTask',
                       'invokeTask:Promise.then:microTask',
+
+                      // This is the `finally` task, which is used for cleanup.
+                      'scheduleTask:Promise.then:microTask',
+                      'invokeTask:Promise.then:microTask',
+
                       // Please refer to the issue link above. Previously, `Response` methods were not
                       // patched by zone.js, and their return values were considered only as
                       // microtasks (not macrotasks). The Angular zone stabilized prematurely,