fix(core): do not save point-in-time setTimeout and rAF references (#55124)

atscott · atscott · commit 840c375255dc · 2024-04-01T11:00:11.000-07:00
Grabbing and saving the references to global `setTimeout` and `rAF` implementations at a certain point in time can be problematic, espcially in tests. Tests might call something like `jasmine.clock().install();` and `jasmine.clock().uninstall();`. If the install happens before we grab the implementation and then the uninstall happens after, our scheduling function will be broken because it would have saved a reference to the jasmine `setTimeout` implementation, which would have since been cleaned up and will throw an error when attempting to access `delayedFunctionScheduler`. There are other scenarios that may apply, not even just for tests, when patches are applied and removed to the globals. PR Close #55124
diff --git a/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts b/packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts
@@ -12,7 +12,7 @@ import {inject} from '../../di/injector_compatibility';
 import {EnvironmentProviders} from '../../di/interface/provider';
 import {makeEnvironmentProviders} from '../../di/provider_collection';
 import {PendingTasks} from '../../pending_tasks';
-import {getCallbackScheduler} from '../../util/callback_scheduler';
+import {scheduleCallback} from '../../util/callback_scheduler';
 import {NgZone, NoopNgZone} from '../../zone/ng_zone';
 
 import {ChangeDetectionScheduler, NotificationType, ZONELESS_ENABLED, ZONELESS_SCHEDULER_DISABLED} from './zoneless_scheduling';
@@ -23,7 +23,6 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
   private taskService = inject(PendingTasks);
   private pendingRenderTaskId: number|null = null;
   private shouldRefreshViews = false;
-  private readonly schedule = getCallbackScheduler();
   private readonly ngZone = inject(NgZone);
   private runningTick = false;
   private cancelScheduledCallback: null|(() => void) = null;
@@ -56,12 +55,12 @@ export class ChangeDetectionSchedulerImpl implements ChangeDetectionScheduler {
     // effectively get the unpatched versions of setTimeout and rAF (#55092)
     if (typeof Zone !== 'undefined' && Zone.root?.run) {
       Zone.root.run(() => {
-        this.cancelScheduledCallback = this.schedule(() => {
+        this.cancelScheduledCallback = scheduleCallback(() => {
           this.tick(this.shouldRefreshViews);
         });
       });
     } else {
-      this.cancelScheduledCallback = this.schedule(() => {
+      this.cancelScheduledCallback = scheduleCallback(() => {
         this.tick(this.shouldRefreshViews);
       });
     }
diff --git a/packages/core/src/util/callback_scheduler.ts b/packages/core/src/util/callback_scheduler.ts
@@ -34,7 +34,7 @@ import {global} from './global';
  *
  * @returns a function to cancel the scheduled callback
  */
-export function getCallbackScheduler(): (callback: Function) => () => void {
+export function scheduleCallback(callback: Function): () => void {
   // Note: the `getNativeRequestAnimationFrame` is used in the `NgZone` class, but we cannot use the
   // `inject` function. The `NgZone` instance may be created manually, and thus the injection
   // context will be unavailable. This might be enough to check whether `requestAnimationFrame` is
@@ -60,25 +60,23 @@ export function getCallbackScheduler(): (callback: Function) => () => void {
     nativeSetTimeout = (nativeSetTimeout as any)[ORIGINAL_DELEGATE_SYMBOL] ?? nativeSetTimeout;
   }
 
-  return (callback: Function) => {
-    let executeCallback = true;
-    nativeSetTimeout(() => {
-      if (!executeCallback) {
-        return;
-      }
-      executeCallback = false;
-      callback();
-    });
-    nativeRequestAnimationFrame?.(() => {
-      if (!executeCallback) {
-        return;
-      }
-      executeCallback = false;
-      callback();
-    });
+  let executeCallback = true;
+  nativeSetTimeout(() => {
+    if (!executeCallback) {
+      return;
+    }
+    executeCallback = false;
+    callback();
+  });
+  nativeRequestAnimationFrame?.(() => {
+    if (!executeCallback) {
+      return;
+    }
+    executeCallback = false;
+    callback();
+  });
 
-    return () => {
-      executeCallback = false;
-    };
+  return () => {
+    executeCallback = false;
   };
 }
diff --git a/packages/core/src/zone/ng_zone.ts b/packages/core/src/zone/ng_zone.ts
@@ -9,7 +9,7 @@
 
 import {RuntimeError, RuntimeErrorCode} from '../errors';
 import {EventEmitter} from '../event_emitter';
-import {getCallbackScheduler} from '../util/callback_scheduler';
+import {scheduleCallback} from '../util/callback_scheduler';
 import {global} from '../util/global';
 import {noop} from '../util/noop';
 
@@ -165,7 +165,7 @@ export class NgZone {
         !shouldCoalesceRunChangeDetection && shouldCoalesceEventChangeDetection;
     self.shouldCoalesceRunChangeDetection = shouldCoalesceRunChangeDetection;
     self.callbackScheduled = false;
-    self.scheduleCallback = getCallbackScheduler();
+    self.scheduleCallback = scheduleCallback;
     forkInnerZoneWithAngularBehavior(self);
   }
 
diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json
@@ -899,9 +899,6 @@
   {
     "name": "getBindingsEnabled"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -1346,6 +1343,9 @@
   {
     "name": "saveNameToExportMap"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json
@@ -962,9 +962,6 @@
   {
     "name": "getBindingsEnabled"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -1415,6 +1412,9 @@
   {
     "name": "saveNameToExportMap"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
@@ -734,9 +734,6 @@
   {
     "name": "getBindingsEnabled"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -1124,6 +1121,9 @@
   {
     "name": "saveNameToExportMap"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json
@@ -836,9 +836,6 @@
   {
     "name": "getBindingsEnabled"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -2288,6 +2285,9 @@
   {
     "name": "saveResolvedLocalsInData"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -1022,9 +1022,6 @@
   {
     "name": "getBindingsEnabled"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -1631,6 +1628,9 @@
   {
     "name": "saveResolvedLocalsInData"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
@@ -992,9 +992,6 @@
   {
     "name": "getBindingsEnabled"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -1616,6 +1613,9 @@
   {
     "name": "saveResolvedLocalsInData"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json
@@ -578,9 +578,6 @@
   {
     "name": "generateInitialInputs"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -893,6 +890,9 @@
   {
     "name": "saveNameToExportMap"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json
@@ -803,9 +803,6 @@
   {
     "name": "generateInitialInputs"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -1265,6 +1262,9 @@
   {
     "name": "scheduleAsyncIterable"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json
@@ -1247,9 +1247,6 @@
   {
     "name": "getBootstrapListener"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getChildRouteGuards"
   },
@@ -1913,6 +1910,9 @@
   {
     "name": "scheduleAsyncIterable"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json
@@ -656,9 +656,6 @@
   {
     "name": "generateInitialInputs"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -992,6 +989,9 @@
   {
     "name": "saveNameToExportMap"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -872,9 +872,6 @@
   {
     "name": "getBindingsEnabled"
   },
-  {
-    "name": "getCallbackScheduler"
-  },
   {
     "name": "getClosureSafeProperty"
   },
@@ -1352,6 +1349,9 @@
   {
     "name": "saveResolvedLocalsInData"
   },
+  {
+    "name": "scheduleCallback"
+  },
   {
     "name": "searchTokensOnInjector"
   },
diff --git a/packages/core/test/change_detection_scheduler_spec.ts b/packages/core/test/change_detection_scheduler_spec.ts
@@ -16,11 +16,13 @@ import {withBody} from '@angular/private/testing';
 import {BehaviorSubject, firstValueFrom} from 'rxjs';
 import {filter, take, tap} from 'rxjs/operators';
 
+import {global} from '../src/util/global';
+
 function isStable(injector = TestBed.inject(EnvironmentInjector)): boolean {
   return toSignal(injector.get(ApplicationRef).isStable, {requireSync: true, injector})();
 }
 
-describe('Angular with NoopNgZone', () => {
+describe('Angular with zoneless enabled', () => {
   async function createFixture<T>(type: Type<T>): Promise<ComponentFixture<T>> {
     const fixture = TestBed.createComponent(type);
     await fixture.whenStable();
@@ -512,6 +514,33 @@ describe('Angular with NoopNgZone', () => {
         await fixture.whenStable();
         expect(embeddedViewRef.rootNodes[0].innerHTML).toContain('new');
       });
+
+  it('does not fail when global timing functions are patched and unpatched', async () => {
+    @Component({template: '', standalone: true})
+    class App {
+      cdr = inject(ChangeDetectorRef);
+    }
+
+    let patched = false;
+    const originalSetTimeout = global.setTimeout;
+    global.setTimeout = (handler: any) => {
+      if (!patched) {
+        throw new Error('no longer patched');
+      }
+      originalSetTimeout(handler);
+    };
+    patched = true;
+    const fixture = TestBed.createComponent(App);
+    await fixture.whenStable();
+    global.setTimeout = originalSetTimeout;
+    patched = false;
+    expect(() => {
+      // cause another scheduler notification. This should not fail due to `setTimeout` being
+      // unpatched.
+      fixture.componentInstance.cdr.markForCheck();
+    }).not.toThrow();
+    await fixture.whenStable();
+  });
 });
 
 describe('Angular with scheduler and ZoneJS', () => {
diff --git a/packages/core/test/zone/ng_zone_spec.ts b/packages/core/test/zone/ng_zone_spec.ts

Original file line number	Diff line number	Diff line change
`@@ -899,9 +899,6 @@`
`899`	`899`	`{`
`900`	`900`	`"name": "getBindingsEnabled"`
`901`	`901`	`},`
`902`		`- {`
`903`		`- "name": "getCallbackScheduler"`
`904`		`- },`
`905`	`902`	`{`
`906`	`903`	`"name": "getClosureSafeProperty"`
`907`	`904`	`},`
`@@ -1346,6 +1343,9 @@`
`1346`	`1343`	`{`
`1347`	`1344`	`"name": "saveNameToExportMap"`
`1348`	`1345`	`},`
	`1346`	`+ {`
	`1347`	`+ "name": "scheduleCallback"`
	`1348`	`+ },`
`1349`	`1349`	`{`
`1350`	`1350`	`"name": "searchTokensOnInjector"`
`1351`	`1351`	`},`
Original file line number	Diff line number	Diff line change
`@@ -962,9 +962,6 @@`
`962`	`962`	`{`
`963`	`963`	`"name": "getBindingsEnabled"`
`964`	`964`	`},`
`965`		`- {`
`966`		`- "name": "getCallbackScheduler"`
`967`		`- },`
`968`	`965`	`{`
`969`	`966`	`"name": "getClosureSafeProperty"`
`970`	`967`	`},`
`@@ -1415,6 +1412,9 @@`
`1415`	`1412`	`{`
`1416`	`1413`	`"name": "saveNameToExportMap"`
`1417`	`1414`	`},`
	`1415`	`+ {`
	`1416`	`+ "name": "scheduleCallback"`
	`1417`	`+ },`
`1418`	`1418`	`{`
`1419`	`1419`	`"name": "searchTokensOnInjector"`
`1420`	`1420`	`},`
Original file line number	Diff line number	Diff line change
`@@ -734,9 +734,6 @@`
`734`	`734`	`{`
`735`	`735`	`"name": "getBindingsEnabled"`
`736`	`736`	`},`
`737`		`- {`
`738`		`- "name": "getCallbackScheduler"`
`739`		`- },`
`740`	`737`	`{`
`741`	`738`	`"name": "getClosureSafeProperty"`
`742`	`739`	`},`
`@@ -1124,6 +1121,9 @@`
`1124`	`1121`	`{`
`1125`	`1122`	`"name": "saveNameToExportMap"`
`1126`	`1123`	`},`
	`1124`	`+ {`
	`1125`	`+ "name": "scheduleCallback"`
	`1126`	`+ },`
`1127`	`1127`	`{`
`1128`	`1128`	`"name": "searchTokensOnInjector"`
`1129`	`1129`	`},`
Original file line number	Diff line number	Diff line change
`@@ -836,9 +836,6 @@`
`836`	`836`	`{`
`837`	`837`	`"name": "getBindingsEnabled"`
`838`	`838`	`},`
`839`		`- {`
`840`		`- "name": "getCallbackScheduler"`
`841`		`- },`
`842`	`839`	`{`
`843`	`840`	`"name": "getClosureSafeProperty"`
`844`	`841`	`},`
`@@ -2288,6 +2285,9 @@`
`2288`	`2285`	`{`
`2289`	`2286`	`"name": "saveResolvedLocalsInData"`
`2290`	`2287`	`},`
	`2288`	`+ {`
	`2289`	`+ "name": "scheduleCallback"`
	`2290`	`+ },`
`2291`	`2291`	`{`
`2292`	`2292`	`"name": "searchTokensOnInjector"`
`2293`	`2293`	`},`
Original file line number	Diff line number	Diff line change
`@@ -1022,9 +1022,6 @@`
`1022`	`1022`	`{`
`1023`	`1023`	`"name": "getBindingsEnabled"`
`1024`	`1024`	`},`
`1025`		`- {`
`1026`		`- "name": "getCallbackScheduler"`
`1027`		`- },`
`1028`	`1025`	`{`
`1029`	`1026`	`"name": "getClosureSafeProperty"`
`1030`	`1027`	`},`
`@@ -1631,6 +1628,9 @@`
`1631`	`1628`	`{`
`1632`	`1629`	`"name": "saveResolvedLocalsInData"`
`1633`	`1630`	`},`
	`1631`	`+ {`
	`1632`	`+ "name": "scheduleCallback"`
	`1633`	`+ },`
`1634`	`1634`	`{`
`1635`	`1635`	`"name": "searchTokensOnInjector"`
`1636`	`1636`	`},`
Original file line number	Diff line number	Diff line change
`@@ -992,9 +992,6 @@`
`992`	`992`	`{`
`993`	`993`	`"name": "getBindingsEnabled"`
`994`	`994`	`},`
`995`		`- {`
`996`		`- "name": "getCallbackScheduler"`
`997`		`- },`
`998`	`995`	`{`
`999`	`996`	`"name": "getClosureSafeProperty"`
`1000`	`997`	`},`
`@@ -1616,6 +1613,9 @@`
`1616`	`1613`	`{`
`1617`	`1614`	`"name": "saveResolvedLocalsInData"`
`1618`	`1615`	`},`
	`1616`	`+ {`
	`1617`	`+ "name": "scheduleCallback"`
	`1618`	`+ },`
`1619`	`1619`	`{`
`1620`	`1620`	`"name": "searchTokensOnInjector"`
`1621`	`1621`	`},`
Original file line number	Diff line number	Diff line change
`@@ -578,9 +578,6 @@`
`578`	`578`	`{`
`579`	`579`	`"name": "generateInitialInputs"`
`580`	`580`	`},`
`581`		`- {`
`582`		`- "name": "getCallbackScheduler"`
`583`		`- },`
`584`	`581`	`{`
`585`	`582`	`"name": "getClosureSafeProperty"`
`586`	`583`	`},`
`@@ -893,6 +890,9 @@`
`893`	`890`	`{`
`894`	`891`	`"name": "saveNameToExportMap"`
`895`	`892`	`},`
	`893`	`+ {`
	`894`	`+ "name": "scheduleCallback"`
	`895`	`+ },`
`896`	`896`	`{`
`897`	`897`	`"name": "searchTokensOnInjector"`
`898`	`898`	`},`