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

This commit runs change detection in a loop while there are still dirty
views to be refreshed in the tree. At the moment, this only applies to
transplanted views but will also apply to views with changed signals.

fixes angular#49801

PR Close angular#51854

Author: atscott

goldens/public-api/core/errors.md

@@ -59,6 +59,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

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

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

@@ -884,6 +884,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

@@ -242,6 +242,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"
   },