fix(core): Prevent markForCheck during change detection from causin…

…g infinite loops (#54900)

This change updates the approach to the loop in `ApplicationRef.tick`
for allowing state updates in `afterRender` hooks. It is valid to update
state in render hooks and we need to ensure we refresh views that may be
marked for check in these hooks (this can happen simply as a result of
focusing an element). This change ensures that the behavior of `markForCheck`
with respect to this loop does not change while we are actively running
change detection on a view tree.

This approach also has the benefit of preventing a regression for #18917,
where updating state in animation listeners can cause `ExpressionChanged...Error`
This should be allowed - there is nothing wrong with respect to unidirectional
data flow in this case.

There may be other cases in the future where it is valid to update
state. Rather than wrapping the render hooks and the animation flushing
in something which flips a global state flag, the idea here is that
`markForCheck` is safe and valid in all cases whenever change detection
is not actively running.

PR Close #54900

packages/core/src/application/application_ref.ts

@@ -722,7 +722,7 @@ export function detectChangesInViewIfRequired(
 }
 
 function shouldRecheckView(view: LView): boolean {
-  return requiresRefreshOrTraversal(view) || !!(view[FLAGS] & LViewFlags.Dirty);
+  return requiresRefreshOrTraversal(view);
 }
 
 function detectChangesInView(lView: LView, notifyErrorHandler: boolean, isFirstPass: boolean) {

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

@@ -16,7 +16,7 @@ import {CONTAINER_HEADER_OFFSET, LContainer, LContainerFlags, MOVED_VIEWS} from
 import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
 import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, REACTIVE_TEMPLATE_CONSUMER, TVIEW, TView, TViewType} from '../interfaces/view';
 import {getOrBorrowReactiveLViewConsumer, maybeReturnReactiveLViewConsumer, ReactiveLViewConsumer} from '../reactive_lview_consumer';
-import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
+import {enterView, isInCheckNoChangesMode, isRefreshingViews, leaveView, setBindingIndex, setIsInCheckNoChangesMode, setIsRefreshingViews} from '../state';
 import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
 import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, requiresRefreshOrTraversal, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
 
@@ -60,26 +60,33 @@ export function detectChangesInternal(
 }
 
 function detectChangesInViewWhileDirty(lView: LView, mode: ChangeDetectionMode) {
-  detectChangesInView(lView, mode);
-
-  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 (requiresRefreshOrTraversal(lView)) {
-    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.');
+  const lastIsRefreshingViewsValue = isRefreshingViews();
+  try {
+    setIsRefreshingViews(true);
+    detectChangesInView(lView, mode);
+
+    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 (requiresRefreshOrTraversal(lView)) {
+      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);
     }
-    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);
+  } finally {
+    // restore state to what it was before entering this change detection loop
+    setIsRefreshingViews(lastIsRefreshingViewsValue);
   }
 }
 

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

@@ -8,6 +8,7 @@
 
 import {isRootView} from '../interfaces/type_checks';
 import {ENVIRONMENT, FLAGS, LView, LViewFlags} from '../interfaces/view';
+import {isRefreshingViews} from '../state';
 import {getLViewParent} from '../util/view_utils';
 
 /**
@@ -22,9 +23,22 @@ import {getLViewParent} from '../util/view_utils';
  * @returns the root LView
  */
 export function markViewDirty(lView: LView): LView|null {
+  const dirtyBitsToUse = isRefreshingViews() ?
+      // When we are actively refreshing views, we only use the `Dirty` bit to mark a view
+      // for check. This bit is ignored in ChangeDetectionMode.Targeted, which is used to
+      // synchronously rerun change detection on a specific set of views (those which have
+      // the `RefreshView` flag and those with dirty signal consumers). `LViewFlags.Dirty`
+      // does not support re-entrant change detection on its own.
+      LViewFlags.Dirty :
+      // When we are not actively refreshing a view tree, it is absolutely
+      // valid to update state and mark views dirty. We use the `RefreshView` flag in this
+      // case to allow synchronously rerunning change detection. This applies today to
+      // afterRender hooks as well as animation listeners which execute after detecting
+      // changes in a view when the render factory flushes.
+      LViewFlags.RefreshView | LViewFlags.Dirty;
   lView[ENVIRONMENT].changeDetectionScheduler?.notify();
   while (lView) {
-    lView[FLAGS] |= LViewFlags.Dirty;
+    lView[FLAGS] |= dirtyBitsToUse;
     const parent = getLViewParent(lView);
     // Stop traversing up as soon as you find a root view that wasn't attached to any container
     if (isRootView(lView) && !parent) {

packages/core/src/render3/state.ts

@@ -202,6 +202,13 @@ const instructionState: InstructionState = {
  */
 let _isInCheckNoChangesMode = false;
 
+/**
+ * Flag used to indicate that we are in the middle running change detection on a view
+ *
+ * @see detectChangesInViewWhileDirty
+ */
+let _isRefreshingViews = false;
+
 /**
  * Returns true if the instruction state stack is empty.
  *
@@ -399,6 +406,14 @@ export function setIsInCheckNoChangesMode(mode: boolean): void {
   _isInCheckNoChangesMode = mode;
 }
 
+export function isRefreshingViews(): boolean {
+  return _isRefreshingViews;
+}
+
+export function setIsRefreshingViews(mode: boolean): void {
+  _isRefreshingViews = mode;
+}
+
 // top level variables should not be exported for performance reasons (PERF_NOTES.md)
 export function getBindingRoot() {
   const lFrame = instructionState.lFrame;

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

@@ -584,6 +584,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_keyMap"
   },
@@ -1121,6 +1124,9 @@
   {
     "name": "isPromise"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isSubscription"
   },
@@ -1358,6 +1364,9 @@
   {
     "name": "setInputsFromAttrs"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSelectedIndex"
   },

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

@@ -641,6 +641,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_keyMap"
   },
@@ -1190,6 +1193,9 @@
   {
     "name": "isPromise"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isSubscription"
   },
@@ -1430,6 +1436,9 @@
   {
     "name": "setInputsFromAttrs"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSelectedIndex"
   },

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

@@ -470,6 +470,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_keyMap"
   },
@@ -941,6 +944,9 @@
   {
     "name": "isPromise"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isSubscription"
   },
@@ -1139,6 +1145,9 @@
   {
     "name": "setInputsFromAttrs"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSelectedIndex"
   },

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

@@ -533,6 +533,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_keyMap"
   },
@@ -2081,6 +2084,9 @@
   {
     "name": "isPromise"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isSubscription"
   },
@@ -2303,6 +2309,9 @@
   {
     "name": "setInputsFromAttrs"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSelectedIndex"
   },

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

@@ -641,6 +641,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_keyMap"
   },
@@ -1349,6 +1352,9 @@
   {
     "name": "isReadableStreamLike"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isStylingMatch"
   },
@@ -1652,6 +1658,9 @@
   {
     "name": "setInputsFromAttrs"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSelectedIndex"
   },

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

@@ -629,6 +629,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_keyMap"
   },
@@ -1310,6 +1313,9 @@
   {
     "name": "isReadableStreamLike"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isStylingMatch"
   },
@@ -1637,6 +1643,9 @@
   {
     "name": "setInputsFromAttrs"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSelectedIndex"
   },

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

@@ -359,6 +359,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_platformInjector"
   },
@@ -737,6 +740,9 @@
   {
     "name": "isPromise"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isSubscription"
   },
@@ -896,6 +902,9 @@
   {
     "name": "setInjectImplementation"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSelectedIndex"
   },

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

@@ -524,6 +524,9 @@
   {
     "name": "_injectImplementation"
   },
+  {
+    "name": "_isRefreshingViews"
+  },
   {
     "name": "_keyMap"
   },
@@ -1037,6 +1040,9 @@
   {
     "name": "isReadableStreamLike"
   },
+  {
+    "name": "isRefreshingViews"
+  },
   {
     "name": "isRootView"
   },
@@ -1265,6 +1271,9 @@
   {
     "name": "setInjectImplementation"
   },
+  {
+    "name": "setIsRefreshingViews"
+  },
   {
     "name": "setSegmentHead"
   },