fix(upgrade): do not break if onMicrotaskEmpty emits while a `$dige…

…st` is in progress (#29794) (#30107)

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

PR Close #30107

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 * as angular 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(); });
                   });
             })

packages/upgrade/src/dynamic/test/upgrade_spec.ts

@@ -171,7 +171,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('<my-app></my-app>');
+
+           adapter.bootstrap(element, [ng1Module.name]).ready((ref) => {
+             const $rootScope: any = ref.ng1RootScope;
+             const ngZone: NgZone = ref.ng2ModuleRef.injector.get<NgZone>(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', []);
@@ -217,7 +256,6 @@ withEachNg1Version(() => {
            });
          }));
 
-
       fixmeIvy(
           'FW-712: Rendering is being run on next "animation frame" rather than "Zone.microTaskEmpty" trigger')
           .it('should propagate changes to a downgraded component inside the ngZone', async(() => {

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 * as angular 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);
               }

packages/upgrade/static/test/integration/change_detection_spec.ts

@@ -19,10 +19,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('<my-app></my-app>');
+
+         @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) => {