fix(zone.js): setTimeout patch should clean tasksByHandleId cache. (#40586)

JiaLiPassion · alxhub · commit 0652b29f6288 · 2021-02-09T10:43:05.000-08:00
Close #40387 Currently zone.js patches `setTimeout` and keeps a `tasksByHandleId` map to keep `timerId` <-> `ZoneTask` relationship. This is needed so that when `clearTimeout(timerId)` is called, zone.js can find the associated `ZoneTask`. Now zone.js set the `tasksByHandleId` map in the `scheduleTask` function, but if the `setTimeout` is running in the `FakeAsyncZoneSpec` or any other `ZoneSpec` with `onScheduleTask` hooks. The `scheduleTask` in `timer` patch may not be invoked. For example: ``` fakeAsync(() => { setTimeout(() => {}); tick(); }); ``` In this case, the `timerId` kept in the `tasksByHandleId` map is not cleared. This is because the `FakeAsyncZoneSpec` in the `onScheduleTask` hook looks like this. ``` onScheduleTask(delegate, ..., task) { fakeAsyncScheduler.setTimeout(task); return task; } ``` Because `FakeAsyncZoneSpec` handles the task itself and it doesn't call `parentDelegate.onScheduleTask`, therefore the default `scheduleTask` in the `timer` patch is not invoked. In this commit, the cleanup logic is moved from `scheduleTask` to `setTimeout` patch entry to avoid the memory leak. PR Close #40586
diff --git a/packages/zone.js/lib/common/timers.ts b/packages/zone.js/lib/common/timers.ts
@@ -29,27 +29,9 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
 
   function scheduleTask(task: Task) {
     const data = <TimerOptions>task.data;
-    function timer(this: unknown) {
-      try {
-        task.invoke.apply(this, arguments);
-      } finally {
-        // issue-934, task will be cancelled
-        // even it is a periodic task such as
-        // setInterval
-        if (!(task.data && task.data.isPeriodic)) {
-          if (typeof data.handleId === 'number') {
-            // in non-nodejs env, we remove timerId
-            // from local cache
-            delete tasksByHandleId[data.handleId];
-          } else if (data.handleId) {
-            // Node returns complex objects as handleIds
-            // we remove task reference from timer object
-            (data.handleId as any)[taskSymbol] = null;
-          }
-        }
-      }
-    }
-    data.args[0] = timer;
+    data.args[0] = function() {
+      return task.invoke.apply(this, arguments);
+    };
     data.handleId = setNative!.apply(window, data.args);
     return task;
   }
@@ -67,6 +49,32 @@ export function patchTimer(window: any, setName: string, cancelName: string, nam
                                                                              undefined,
             args: args
           };
+          const callback = args[0];
+          args[0] = function timer(this: unknown) {
+            try {
+              return callback.apply(this, arguments);
+            } finally {
+              // issue-934, task will be cancelled
+              // even it is a periodic task such as
+              // setInterval
+
+              // https://github.com/angular/angular/issues/40387
+              // Cleanup tasksByHandleId should be handled before scheduleTask
+              // Since some zoneSpec may intercept and doesn't trigger
+              // scheduleFn(scheduleTask) provided here.
+              if (!(options.isPeriodic)) {
+                if (typeof options.handleId === 'number') {
+                  // in non-nodejs env, we remove timerId
+                  // from local cache
+                  delete tasksByHandleId[options.handleId];
+                } else if (options.handleId) {
+                  // Node returns complex objects as handleIds
+                  // we remove task reference from timer object
+                  (options.handleId as any)[taskSymbol] = null;
+                }
+              }
+            }
+          };
           const task =
               scheduleMacroTaskWithCurrentZone(setName, args[0], options, scheduleTask, clearTask);
           if (!task) {
diff --git a/packages/zone.js/test/zone-spec/fake-async-test.spec.ts b/packages/zone.js/test/zone-spec/fake-async-test.spec.ts
@@ -289,6 +289,40 @@ describe('FakeAsyncTestZoneSpec', () => {
       });
     });
 
+    it('should clear internal timerId cache', () => {
+      let taskSpy: jasmine.Spy = jasmine.createSpy('taskGetState');
+      fakeAsyncTestZone
+          .fork({
+            name: 'scheduleZone',
+            onScheduleTask: (delegate: ZoneDelegate, curr: Zone, target: Zone, task: Task) => {
+              (task as any)._state = task.state;
+              Object.defineProperty(task, 'state', {
+                configurable: true,
+                enumerable: true,
+                get: () => {
+                  taskSpy();
+                  return (task as any)._state;
+                },
+                set: (newState: string) => {
+                  (task as any)._state = newState;
+                }
+              });
+              return delegate.scheduleTask(target, task);
+            }
+          })
+          .run(() => {
+            const id = setTimeout(() => {}, 0);
+            testZoneSpec.tick();
+            clearTimeout(id);
+            // This is a hack way to test the timerId cache is cleaned or not
+            // since the tasksByHandleId cache is an internal variable held by
+            // zone.js timer patch, if the cache is not cleared, the code in `timer.ts`
+            // will call `task.state` one more time to check whether to clear the
+            // task or not, so here we use this count to check the issue is fixed or not
+            // For details, please refer to https://github.com/angular/angular/issues/40387
+            expect(taskSpy.calls.count()).toEqual(5);
+          });
+    });
     it('should pass arguments to setImmediate', ifEnvSupports('setImmediate', () => {
          fakeAsyncTestZone.run(() => {
            let value = 'genuine value';
