From 69763491c3ffb576822b179af3363ec666d43bce 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
 (#57882)

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.

PR Close #57882
---
 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 c22edea28fb..3b8d1a2a5b8 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 7e6c642972c..ca8b60761ab 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,