fix(zone.js): remove abort listener once fetch is settled (#57882)

arturovt · AndrewKushnir · commit 02de7fc73167 · 2024-10-07T08:27:54.000-07:00
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
diff --git 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
@@ -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,
