Skip to content

Commit

Permalink
fix(core): do not save point-in-time setTimeout and rAF references (
Browse files Browse the repository at this point in the history 
#55124)

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
  • Loading branch information
@atscott
atscott committed Apr 1, 2024
1 parent e18a0ed commit 840c375
Show file tree
Hide file tree
Showing 16 changed files with 87 additions and 64 deletions.
7 changes: 3 additions & 4 deletions packages/core/src/change_detection/scheduling/zoneless_scheduling_impl.ts
View file
Expand Up @@ -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';
Expand All @@ -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;
Expand Down Expand Up @@ -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);
});
}
Expand Down
38 changes: 18 additions & 20 deletions packages/core/src/util/callback_scheduler.ts
View file
Expand Up @@ -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
Expand All @@ -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;
};
}
4 changes: 2 additions & 2 deletions packages/core/src/zone/ng_zone.ts
View file
Expand Up @@ -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';

Expand Down Expand Up @@ -165,7 +165,7 @@ export class NgZone {
!shouldCoalesceRunChangeDetection && shouldCoalesceEventChangeDetection;
self.shouldCoalesceRunChangeDetection = shouldCoalesceRunChangeDetection;
self.callbackScheduled = false;
self.scheduleCallback = getCallbackScheduler();
self.scheduleCallback = scheduleCallback;
forkInnerZoneWithAngularBehavior(self);
}

Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json
View file
Expand Up @@ -899,9 +899,6 @@
{
"name": "getBindingsEnabled"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -1346,6 +1343,9 @@
{
"name": "saveNameToExportMap"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/animations/bundle.golden_symbols.json
View file
Expand Up @@ -962,9 +962,6 @@
{
"name": "getBindingsEnabled"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -1415,6 +1412,9 @@
{
"name": "saveNameToExportMap"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
View file
Expand Up @@ -734,9 +734,6 @@
{
"name": "getBindingsEnabled"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -1124,6 +1121,9 @@
{
"name": "saveNameToExportMap"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/defer/bundle.golden_symbols.json
View file
Expand Up @@ -836,9 +836,6 @@
{
"name": "getBindingsEnabled"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -2288,6 +2285,9 @@
{
"name": "saveResolvedLocalsInData"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
View file
Expand Up @@ -1022,9 +1022,6 @@
{
"name": "getBindingsEnabled"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -1631,6 +1628,9 @@
{
"name": "saveResolvedLocalsInData"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
View file
Expand Up @@ -992,9 +992,6 @@
{
"name": "getBindingsEnabled"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -1616,6 +1613,9 @@
{
"name": "saveResolvedLocalsInData"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/hello_world/bundle.golden_symbols.json
View file
Expand Up @@ -578,9 +578,6 @@
{
"name": "generateInitialInputs"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -893,6 +890,9 @@
{
"name": "saveNameToExportMap"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/hydration/bundle.golden_symbols.json
View file
Expand Up @@ -803,9 +803,6 @@
{
"name": "generateInitialInputs"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -1265,6 +1262,9 @@
{
"name": "scheduleAsyncIterable"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
View file
Expand Up @@ -1247,9 +1247,6 @@
{
"name": "getBootstrapListener"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getChildRouteGuards"
},
Expand Down Expand Up @@ -1913,6 +1910,9 @@
{
"name": "scheduleAsyncIterable"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json
View file
Expand Up @@ -656,9 +656,6 @@
{
"name": "generateInitialInputs"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -992,6 +989,9 @@
{
"name": "saveNameToExportMap"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
View file
Expand Up @@ -872,9 +872,6 @@
{
"name": "getBindingsEnabled"
},
{
"name": "getCallbackScheduler"
},
{
"name": "getClosureSafeProperty"
},
Expand Down Expand Up @@ -1352,6 +1349,9 @@
{
"name": "saveResolvedLocalsInData"
},
{
"name": "scheduleCallback"
},
{
"name": "searchTokensOnInjector"
},
Expand Down
31 changes: 30 additions & 1 deletion packages/core/test/change_detection_scheduler_spec.ts
View file
Expand Up @@ -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();
Expand Down Expand Up @@ -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', () => {
Expand Down

0 comments on commit 840c375

Please sign in to comment.