fix(core): Ensure backwards-referenced transplanted views are refreshed

goldens/public-api/core/errors.md

@@ -57,6 +57,8 @@ export const enum RuntimeErrorCode {
     // (undocumented)
     IMPORT_PROVIDERS_FROM_STANDALONE = 800,
     // (undocumented)
+    INFINITE_CHANGE_DETECTION = 103,
+    // (undocumented)
     INJECTOR_ALREADY_DESTROYED = 205,
     // (undocumented)
     INVALID_DIFFER_INPUT = 900,

packages/core/src/errors.ts

@@ -31,6 +31,7 @@ export const enum RuntimeErrorCode {
   EXPRESSION_CHANGED_AFTER_CHECKED = -100,
   RECURSIVE_APPLICATION_REF_TICK = 101,
   RECURSIVE_APPLICATION_RENDER = 102,
+  INFINITE_CHANGE_DETECTION = 103,
 
   // Dependency Injection Errors
   CYCLIC_DI_DEPENDENCY = -200,

packages/core/src/render3/instructions/change_detection.ts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {RuntimeError, RuntimeErrorCode} from '../../errors';
 import {assertDefined, assertEqual} from '../../util/assert';
 import {assertLContainer} from '../assert';
 import {getComponentViewByInstance} from '../context_discovery';
@@ -19,6 +20,11 @@ import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, mar
 
 import {executeTemplate, executeViewQueryFn, handleError, processHostBindingOpCodes, refreshContentQueries} from './shared';
 
+/**
+ * The maximum number of times the change detection traversal will rerun before throwing an error.
+ */
+const MAXIMUM_REFRESH_RERUNS = 100;
+
 export function detectChangesInternal<T>(
     tView: TView, lView: LView, context: T, notifyErrorHandler = true) {
   const environment = lView[ENVIRONMENT];
@@ -37,6 +43,25 @@ export function detectChangesInternal<T>(
 
   try {
     refreshView(tView, lView, tView.template, context);
+    let retries = 0;
+    // If after running change detection, this view still needs to be refreshed or there are
+    // descendants views that need to be refreshed due to re-dirtying during the change detection
+    // run, detect changes on the view again. We run change detection in `Targeted` mode to only
+    // refresh views with the `RefreshView` flag.
+    while (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh)) {
+      if (retries === MAXIMUM_REFRESH_RERUNS) {
+        throw new RuntimeError(
+            RuntimeErrorCode.INFINITE_CHANGE_DETECTION,
+            ngDevMode &&
+                'Infinite change detection while trying to refresh views. ' +
+                    'There may be components which each cause the other to require a refresh, ' +
+                    'causing an infinite loop.');
+      }
+      retries++;
+      // Even if this view is detached, we still detect changes in targeted mode because this was
+      // the root of the change detection run.
+      detectChangesInView(lView, ChangeDetectionMode.Targeted);
+    }
   } catch (error) {
     if (notifyErrorHandler) {
       handleError(lView, error);
@@ -228,7 +253,6 @@ export function refreshView<T>(
     if (!isInCheckNoChangesPass) {
       lView[FLAGS] &= ~(LViewFlags.Dirty | LViewFlags.FirstLViewPass);
     }
-    lView[FLAGS] &= ~LViewFlags.RefreshView;
   } catch (e) {
     // If refreshing a view causes an error, we need to remark the ancestors as needing traversal
     // because the error might have caused a situation where views below the current location are
@@ -319,7 +343,7 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) {
 
   // Flag cleared before change detection runs so that the view can be re-marked for traversal if
   // necessary.
-  lView[FLAGS] &= ~LViewFlags.HasChildViewsToRefresh;
+  lView[FLAGS] &= ~(LViewFlags.HasChildViewsToRefresh | LViewFlags.RefreshView);
 
   if ((flags & (LViewFlags.CheckAlways | LViewFlags.Dirty) &&
        mode === ChangeDetectionMode.Global) ||

packages/core/test/acceptance/change_detection_transplanted_view_spec.ts

@@ -410,35 +410,25 @@ describe('change detection for transplanted views', () => {
          expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1);
        });
 
-    it('updates only the declaration view when there is a change to declaration and declaration is marked dirty',
-       () => {
-         appComponent.declaration.name = 'new name';
-         appComponent.declaration.changeDetectorRef.markForCheck();
-         fixture.detectChanges(false);
-
-         const expectedContent =
-             'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(new name)';
-         expect(fixture.nativeElement.textContent).toEqual(expectedContent);
-         expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0);
+    it('updates the declaration view when there is a change to either declaration or insertion', () => {
+      appComponent.declaration.name = 'new name';
+      appComponent.declaration.changeDetectorRef.markForCheck();
+      fixture.detectChanges(false);
 
-         // Note here that this second change detection should not be necessary, but is because of
-         // the backwards reference not being fully supported. The assertions below should be true
-         // after the first CD.
-         fixture.detectChanges(false);
-         expect(fixture.nativeElement.textContent)
-             .toEqual(
-                 'Insertion(initial)TemplateDeclaration(new name)TemplateContext(initial)Declaration(new name)');
-         expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1);
-       });
+      const expectedContent =
+          'Insertion(initial)TemplateDeclaration(new name)TemplateContext(initial)Declaration(new name)';
+      expect(fixture.nativeElement.textContent).toEqual(expectedContent);
+      expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1);
+    });
 
-    it('should not update when there is a change to insertion and declaration is marked dirty', () => {
+    it('should update when there is a change to insertion and declaration is marked dirty', () => {
       appComponent.insertion.name = 'new name';
       appComponent.declaration.changeDetectorRef.markForCheck();
       fixture.detectChanges(false);
       expect(fixture.nativeElement.textContent)
           .toEqual(
               'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(initial)');
-      expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0);
+      expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1);
     });
 
     it('should update insertion view and template when there is a change to insertion and insertion marked dirty',
@@ -462,8 +452,9 @@ describe('change detection for transplanted views', () => {
       appComponent.insertion.changeDetectorRef.markForCheck();
       fixture.detectChanges(false);
       expect(appComponent.declaration.transplantedViewRefreshCount)
-          .withContext('Should refresh once because backwards references are not rechecked')
-          .toEqual(1);
+          .withContext(
+              'Should refresh twice because insertion executes and then declaration marks transplanted view dirty again')
+          .toEqual(2);
     });
   });
 
@@ -653,7 +644,8 @@ describe('change detection for transplanted views', () => {
       // Detach view, manually call `detectChanges`, and verify the template was refreshed
       component.rootViewContainerRef.detach();
       viewRef.detectChanges();
-      expect(component.templateExecutions).toEqual(1);
+      // This view is a backwards reference so it's refreshed twice
+      expect(component.templateExecutions).toEqual(2);
     });
 
     it('should work when change detecting detached transplanted view already marked for refresh',
@@ -669,7 +661,8 @@ describe('change detection for transplanted views', () => {
            // should not affect parent counters.
            viewRef.detectChanges();
          }).not.toThrow();
-         expect(component.templateExecutions).toEqual(1);
+         // This view is a backwards reference so it's refreshed twice
+         expect(component.templateExecutions).toEqual(2);
        });
 
     it('should work when re-inserting a previously detached transplanted view marked for refresh',
@@ -689,7 +682,10 @@ describe('change detection for transplanted views', () => {
            // the counter, this would fail when attempted to decrement.
            fixture.detectChanges(false);
          }).not.toThrow();
-         expect(component.templateExecutions).toBeGreaterThan(0);
+         // The transplanted view gets refreshed twice because it's actually inserted "backwards"
+         // The view is defined in AppComponent but inserted in its ViewContainerRef (as an
+         // embedded view in AppComponent's host view).
+         expect(component.templateExecutions).toEqual(2);
        });
 
     it('should work when detaching an attached transplanted view with the refresh flag', () => {

packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json

@@ -833,6 +833,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/animations/bundle.golden_symbols.json

@@ -899,6 +899,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json

@@ -668,6 +668,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/defer/bundle.golden_symbols.json

@@ -248,6 +248,9 @@
   {
     "name": "MATH_ML_NAMESPACE"
   },
+  {
+    "name": "MAXIMUM_REFRESH_RERUNS"
+  },
   {
     "name": "MINIMUM_SLOT"
   },
@@ -752,6 +755,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json

@@ -911,6 +911,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json

@@ -881,6 +881,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -527,6 +527,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/hydration/bundle.golden_symbols.json

@@ -725,6 +725,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/router/bundle.golden_symbols.json

@@ -1166,6 +1166,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json

@@ -596,6 +596,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -797,6 +797,9 @@
   {
     "name": "detectChangesInEmbeddedViews"
   },
+  {
+    "name": "detectChangesInView"
+  },
   {
     "name": "detectChangesInViewIfAttached"
   },