From 0ddf2e7a5b8dab8fa15da18ba43bff0654c5cba2 Mon Sep 17 00:00:00 2001 From: Sam Julien Date: Tue, 23 Apr 2019 11:13:50 +0300 Subject: [PATCH] fix(upgrade): do not break if `onMicrotaskEmpty` emits while a `$digest` is in progress (#29794) Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could emit while a `$digest` was in progress, thus triggering another `$digest`, which in turn would throw a `$digest already in progress` error. Furthermore, throwing an error from inside the `onMicrotaskEmpty` subscription would result in unsubscribing and stop triggering further `$digest`s, when `onMicrotaskEmpty` emitted. Usually, emitting while a `$digest` was already in progress was a result of unintentionally running some part of AngularJS outside the Angular zone, but there are valid (if rare) usecases where this can happen (see #24680 for details). This commit addresses the issue as follows: - If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not trigger another `$digest` (to avoid the error). `$evalAsync()` is used instead, to ensure that the bindings are evaluated at least once more. - Since there is still a high probability that the situation is a result of programming error (i.e. some AngularJS part running outside the Angular Zone), a warning will be logged, but only if the app is in [dev mode][1]. [1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12 Fixes #24680 PR Close #29794 --- .../src/dynamic/src/upgrade_adapter.ts | 18 +++++- .../upgrade/src/dynamic/test/upgrade_spec.ts | 42 +++++++++++++- packages/upgrade/static/src/upgrade_module.ts | 16 +++++- .../test/integration/change_detection_spec.ts | 57 ++++++++++++++++++- 4 files changed, 124 insertions(+), 9 deletions(-) diff --git a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts index 31571a14ac..ff35f0a1c2 100644 --- a/packages/upgrade/src/dynamic/src/upgrade_adapter.ts +++ b/packages/upgrade/src/dynamic/src/upgrade_adapter.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Compiler, CompilerOptions, Injector, NgModule, NgModuleRef, NgZone, StaticProvider, Testability, Type, resolveForwardRef} from '@angular/core'; +import {Compiler, CompilerOptions, Injector, NgModule, NgModuleRef, NgZone, StaticProvider, Testability, Type, isDevMode, resolveForwardRef} from '@angular/core'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; import {IAngularBootstrapConfig, IAugmentedJQuery, IInjectorService, IModule, IProvideService, IRootScopeService, ITestabilityService, bootstrap, element as angularElement, module_ as angularModule} from '../../common/src/angular1'; @@ -598,8 +598,20 @@ export class UpgradeAdapter { }) .then(() => this.ng2BootstrapDeferred.resolve(ng1Injector), onError) .then(() => { - let subscription = - this.ngZone.onMicrotaskEmpty.subscribe({next: () => rootScope.$digest()}); + let subscription = this.ngZone.onMicrotaskEmpty.subscribe({ + next: () => { + if (rootScope.$$phase) { + if (isDevMode()) { + console.warn( + 'A digest was triggered while one was already in progress. This may mean that something is triggering digests outside the Angular zone.'); + } + + return rootScope.$evalAsync(() => {}); + } + + return rootScope.$digest(); + } + }); rootScope.$on('$destroy', () => { subscription.unsubscribe(); }); }); }) diff --git a/packages/upgrade/src/dynamic/test/upgrade_spec.ts b/packages/upgrade/src/dynamic/test/upgrade_spec.ts index 879db4b188..acb2892d5a 100644 --- a/packages/upgrade/src/dynamic/test/upgrade_spec.ts +++ b/packages/upgrade/src/dynamic/test/upgrade_spec.ts @@ -169,7 +169,46 @@ withEachNg1Version(() => { })); }); - describe('scope/component change-detection', () => { + describe('change-detection', () => { + it('should not break if a $digest is already in progress', async(() => { + @Component({selector: 'my-app', template: ''}) + class AppComponent { + } + + @NgModule({declarations: [AppComponent], imports: [BrowserModule]}) + class Ng2Module { + } + + const ng1Module = angular.module('ng1', []); + const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); + const element = html(''); + + adapter.bootstrap(element, [ng1Module.name]).ready((ref) => { + const $rootScope: any = ref.ng1RootScope; + const ngZone: NgZone = ref.ng2ModuleRef.injector.get(NgZone); + const digestSpy = spyOn($rootScope, '$digest').and.callThrough(); + + // Step 1: Ensure `$digest` is run on `onMicrotaskEmpty`. + ngZone.onMicrotaskEmpty.emit(null); + expect(digestSpy).toHaveBeenCalledTimes(1); + + digestSpy.calls.reset(); + + // Step 2: Cause the issue. + $rootScope.$apply(() => ngZone.onMicrotaskEmpty.emit(null)); + + // With the fix, `$digest` will only be run once (for `$apply()`). + // Without the fix, `$digest()` would have been run an extra time (`onMicrotaskEmpty`). + expect(digestSpy).toHaveBeenCalledTimes(1); + + digestSpy.calls.reset(); + + // Step 3: Ensure that `$digest()` is still executed on `onMicrotaskEmpty`. + ngZone.onMicrotaskEmpty.emit(null); + expect(digestSpy).toHaveBeenCalledTimes(1); + }); + })); + it('should interleave scope and component expressions', async(() => { const adapter: UpgradeAdapter = new UpgradeAdapter(forwardRef(() => Ng2Module)); const ng1Module = angular.module_('ng1', []); @@ -215,7 +254,6 @@ withEachNg1Version(() => { }); })); - it('should propagate changes to a downgraded component inside the ngZone', async(() => { let appComponent: AppComponent; let upgradeRef: UpgradeAdapterRef; diff --git a/packages/upgrade/static/src/upgrade_module.ts b/packages/upgrade/static/src/upgrade_module.ts index d14aabc32b..5301f8748d 100644 --- a/packages/upgrade/static/src/upgrade_module.ts +++ b/packages/upgrade/static/src/upgrade_module.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Injector, NgModule, NgZone, Testability} from '@angular/core'; +import {Injector, NgModule, NgZone, Testability, isDevMode} from '@angular/core'; import {IInjectorService, IIntervalService, IProvideService, ITestabilityService, bootstrap, element as angularElement, module_ as angularModule} from '../../src/common/src/angular1'; import {$$TESTABILITY, $DELEGATE, $INJECTOR, $INTERVAL, $PROVIDE, INJECTOR_KEY, LAZY_MODULE_REF, UPGRADE_APP_TYPE_KEY, UPGRADE_MODULE_NAME} from '../../src/common/src/constants'; @@ -255,8 +255,18 @@ export class UpgradeModule { // stabilizing setTimeout(() => { const $rootScope = $injector.get('$rootScope'); - const subscription = - this.ngZone.onMicrotaskEmpty.subscribe(() => $rootScope.$digest()); + const subscription = this.ngZone.onMicrotaskEmpty.subscribe(() => { + if ($rootScope.$$phase) { + if (isDevMode()) { + console.warn( + 'A digest was triggered while one was already in progress. This may mean that something is triggering digests outside the Angular zone.'); + } + + return $rootScope.$evalAsync(); + } + + return $rootScope.$digest(); + }); $rootScope.$on('$destroy', () => { subscription.unsubscribe(); }); }, 0); } diff --git a/packages/upgrade/static/test/integration/change_detection_spec.ts b/packages/upgrade/static/test/integration/change_detection_spec.ts index cf65fddde3..cacb7cc806 100644 --- a/packages/upgrade/static/test/integration/change_detection_spec.ts +++ b/packages/upgrade/static/test/integration/change_detection_spec.ts @@ -18,10 +18,65 @@ import {html, withEachNg1Version} from '../../../src/common/test/helpers/common_ import {bootstrap} from './static_test_helpers'; withEachNg1Version(() => { - describe('scope/component change-detection', () => { + describe('change-detection', () => { beforeEach(() => destroyPlatform()); afterEach(() => destroyPlatform()); + it('should not break if a $digest is already in progress', async(() => { + const element = html(''); + + @Component({selector: 'my-app', template: ''}) + class AppComponent { + } + + @NgModule({ + declarations: [AppComponent], + entryComponents: [AppComponent], + imports: [BrowserModule, UpgradeModule] + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const ng1Module = angular.module('ng1', []).directive( + 'myApp', downgradeComponent({component: AppComponent})); + + bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then((upgrade) => { + const $rootScope = upgrade.$injector.get('$rootScope') as angular.IRootScopeService; + const ngZone: NgZone = upgrade.ngZone; + + // Wrap in a setTimeout to ensure all boostrap operations have completed. + setTimeout( + // Run inside the Angular zone, so that operations such as emitting + // `onMicrotaskEmpty` do not trigger entering/existing the zone (and thus another + // `$digest`). This also closer simulates what would happen in a real app. + () => ngZone.run(() => { + const digestSpy = spyOn($rootScope, '$digest').and.callThrough(); + + // Step 1: Ensure `$digest` is run on `onMicrotaskEmpty`. + ngZone.onMicrotaskEmpty.emit(null); + expect(digestSpy).toHaveBeenCalledTimes(1); + + digestSpy.calls.reset(); + + // Step 2: Cause the issue. + $rootScope.$apply(() => ngZone.onMicrotaskEmpty.emit(null)); + + // With the fix, `$digest` will only be run once (for `$apply()`). + // Without the fix, `$digest()` would have been run an extra time + // (`onMicrotaskEmpty`). + expect(digestSpy).toHaveBeenCalledTimes(1); + + digestSpy.calls.reset(); + + // Step 3: Ensure that `$digest()` is still executed on `onMicrotaskEmpty`. + ngZone.onMicrotaskEmpty.emit(null); + expect(digestSpy).toHaveBeenCalledTimes(1); + }), + 0); + }); + })); + it('should interleave scope and component expressions', async(() => { const log: string[] = []; const l = (value: string) => {