perf(core): Avoid traversal when markForCheck is called on a view b…

…eing refreshed

This commit updates the internal `markViewDirty` logic to avoid marking
views dirty up to the root when we encounter a view that is already
being refreshed. This is wasteful because we clear the dirty bit at the
end of refreshing a view anyways so it really has no affect on that
view.

Sometimes we can reach a view and refresh it when the parents have been
skipped. For example, this happens with views that have updated signals.
Ancestors are only traversed and not refreshed. Marking dirty all the
way up to the root in this case can result in those ancestor views being
marked with the dirty flag and it not being cleared.

This only became problematic when we started to support backwards
referenced transplanted views and changed signal change detection to
not mark ancestors for check. In addition to that, the loop in change
detection support to remove `ExpressionChanged...` errors for these cases
can potentially cause infinite loops in rare cases when these views are
refreshed at a time when parents have only been traversed and
`markForCheck` is called. Prior to the loop, if this `markForCheck` was
combined with an actual change to binding values, this would cause
`ExpressionChangedAfterItWasCheckedError`.

For regular cases where we are refreshing a view, update a binding, and call
`markForCheck`, this is either `ExpressionChanged...` in views with
default change detection or no error at all (but without any state
update) in views that are `OnPush` since the `Dirty` flag is cleared at
the end of `refreshView` and `checkNoChanges` uses the same `Dirty` bit for
traversal (angular#45612).

The only case where this might cause different and unexpected behavior
is if change detection starts in the middle of a view tree via
`ChangeDetectorRef.detectChanges` _and_ a state update happens where the
state is read in a view above the root of the traversal along with `markForCheck`.
This might be the case for updating host bindings during change
detection. That said, we have never really intented to support updating
state during change detection, especially in a location above the
current spot in the current view tree. This violates unidirectional data
flow, a principle we have always intented to enforce outside of using
signals for state.

packages/core/src/application/application_ref.ts

@@ -702,8 +702,5 @@ export function whenStable(applicationRef: ApplicationRef): Promise<void> {
 
 
 function shouldRecheckView(view: LView): boolean {
-  return requiresRefreshOrTraversal(view);
-  // TODO(atscott): We need to support rechecking views marked dirty again in afterRender hooks
-  // in order to support the transition to zoneless. b/308152025
-  /* || !!(view[FLAGS] & LViewFlags.Dirty); */
+  return requiresRefreshOrTraversal(view) || !!(view[FLAGS] & LViewFlags.Dirty); 
 }

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

@@ -134,6 +134,7 @@ export function refreshView<T>(
   !isInCheckNoChangesPass && lView[ENVIRONMENT].inlineEffectRunner?.flush();
 
 
+  lView[FLAGS] |= LViewFlags.ExecutingRefresh;
   // Start component reactive context
   // - We might already be in a reactive context if this is an embedded view of the host.
   // - We might be descending into a view that needs a consumer.
@@ -277,6 +278,7 @@ export function refreshView<T>(
       maybeReturnReactiveLViewConsumer(currentConsumer);
     }
     leaveView();
+    lView[FLAGS] &= ~LViewFlags.ExecutingRefresh;
   }
 }
 

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

@@ -19,19 +19,23 @@ import {getLViewParent} from '../util/view_utils';
  * an additional time to get the root view and schedule a tick on it.
  *
  * @param lView The starting LView to mark dirty
- * @returns the root LView
  */
-export function markViewDirty(lView: LView): LView|null {
+export function markViewDirty(lView: LView) {
   lView[ENVIRONMENT].changeDetectionScheduler?.notify();
   while (lView) {
+    // If we're already refreshing the view, we should not mark it or ancestors dirty.
+    // This is `ExpressionHasChangedAfterItWasCheckedError` if any bindings have actually changed
+    // and completely unnecessary to refresh views again if there weren't any updated bindings.
+    if (lView[FLAGS] & LViewFlags.ExecutingRefresh) {
+      return;
+    }
     lView[FLAGS] |= LViewFlags.Dirty;
     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) {
-      return lView;
+      return;
     }
     // continue otherwise
     lView = parent!;
   }
-  return null;
 }

packages/core/src/render3/interfaces/view.ts

@@ -442,10 +442,15 @@ export const enum LViewFlags {
    */
   HasChildViewsToRefresh = 1 << 13,
 
+  /**
+   * Flag used to indicate that we are current running `refreshView` on the LView.
+   */
+  ExecutingRefresh = 1 << 14,
+
   /**
    * This is the count of the bits the 1 was shifted above (base 10)
    */
-  IndexWithinInitPhaseShift = 14,
+  IndexWithinInitPhaseShift = 15,
 
   /**
    * Index of the current init phase on last 21 bits

packages/core/test/acceptance/after_render_hook_spec.ts

@@ -946,8 +946,7 @@ describe('after render hooks', () => {
       const appRef = TestBed.inject(ApplicationRef);
       appRef.attachView(fixture.componentRef.hostView);
       appRef.tick();
-      // TODO(atscott): We need to support this for zoneless to succeed
-      expect(fixture.nativeElement.innerText).toBe('0');
+      expect(fixture.nativeElement.innerText).toBe('1');
     });
 
     it('throws error when causing infinite updates', () => {