fix(core): Fix possible infinite loop with markForCheck by partially reverting angular#54074 (angular#54329)

atscott · thePunderWoman · commit 898a532aef41 · 2024-02-08T16:45:20.000Z
In some situations, calling `markForCheck` can result in an infinite loop in seemingly valid scenarios. When a transplanted view is inserted before its declaration, it gets refreshed in the retry loop of `detectChanges`. At this point, the `Dirty` flag has been cleared from all parents. Calling `markForCheck` marks the insertion tree up to the root `Dirty`. If the declaration is checked again as a result (i.e. because it has default change detection) and is reachable because its parent was marked `Dirty`, this can cause an infinite loop. The declaration is refreshed again, so the insertion is marked for refresh (again). We enter an infinite loop if the insertion tree always calls `markForCheck` for some reason (i.e. `{{createReplayObservable() | async}}`). While the case above does fall into an infinite loop, it also truly is a problem in the application. While it's not an infinite synchronous loop, the declaration and insertion are infinitely dirty and will be refreshed on every change detection round. Usually `markForCheck` does not have this problem because the `Dirty` flag is not cleared until the very end of change detection. However, if the view did not already have the `Dirty` flag set, it is never cleared because we never entered view refresh. One solution to this problem could be to clear the `Dirty` flag even after skipping view refresh but traversing to children. PR Close angular#54329
diff --git a/packages/core/src/application/application_ref.ts b/packages/core/src/application/application_ref.ts
@@ -702,5 +702,8 @@ export function whenStable(applicationRef: ApplicationRef): Promise<void> {
 
 
 function shouldRecheckView(view: LView): boolean {
-  return requiresRefreshOrTraversal(view) || !!(view[FLAGS] & LViewFlags.Dirty);
+  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); */
 }
diff --git a/packages/core/test/acceptance/after_render_hook_spec.ts b/packages/core/test/acceptance/after_render_hook_spec.ts
@@ -946,7 +946,8 @@ describe('after render hooks', () => {
       const appRef = TestBed.inject(ApplicationRef);
       appRef.attachView(fixture.componentRef.hostView);
       appRef.tick();
-      expect(fixture.nativeElement.innerText).toBe('1');
+      // TODO(atscott): We need to support this for zoneless to succeed
+      expect(fixture.nativeElement.innerText).toBe('0');
     });
 
     it('throws error when causing infinite updates', () => {
diff --git a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts
@@ -6,9 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {CommonModule} from '@angular/common';
-import {ChangeDetectionStrategy, ChangeDetectorRef, Component, computed, Directive, DoCheck, inject, Input, signal, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core';
-import {AfterViewChecked, EmbeddedViewRef} from '@angular/core/src/core';
+import {CommonModule, NgTemplateOutlet} from '@angular/common';
+import {AfterViewChecked, ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, computed, createComponent, Directive, DoCheck, EmbeddedViewRef, EnvironmentInjector, ErrorHandler, inject, Input, signal, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core';
 import {ComponentFixture, TestBed} from '@angular/core/testing';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
 
@@ -1013,6 +1012,63 @@ describe('change detection for transplanted views', () => {
       expect(fixture.nativeElement.innerText).toEqual('new');
     });
   });
+
+  it('can call markForCheck inside insertion tree when inserted as backwards reference', () => {
+    @Component({selector: 'markForCheck', template: '', standalone: true})
+    class MarkForCheck {
+      cdr = inject(ChangeDetectorRef);
+      ngDoCheck() {
+        this.cdr.markForCheck();
+      }
+    }
+
+    @Component({
+      selector: 'insertion',
+      imports: [NgTemplateOutlet],
+      standalone: true,
+      template: ` <ng-container [ngTemplateOutlet]="template"> </ng-container>`,
+    })
+    class Insertion {
+      @Input() template!: TemplateRef<{}>;
+      constructor(readonly changeDetectorRef: ChangeDetectorRef) {}
+    }
+
+    @Component({
+      imports: [MarkForCheck, Insertion],
+      template: `<ng-template #myTmpl> <markForCheck/> </ng-template>`,
+      standalone: true,
+      selector: 'declaration'
+    })
+    class Declaration {
+      @ViewChild('myTmpl', {static: true}) template!: TemplateRef<{}>;
+    }
+    @Component({
+      standalone: true,
+      imports: [Declaration, Insertion],
+      template: '<insertion [template]="declaration.template"/><declaration #declaration/>'
+    })
+    class App {
+    }
+
+    TestBed.configureTestingModule({
+      providers: [{
+        provide: ErrorHandler, useClass: class extends ErrorHandler {
+          override handleError(e: any) {
+            throw e;
+          }
+        }
+      }]
+    });
+
+    const app = createComponent(App, {environmentInjector: TestBed.inject(EnvironmentInjector)});
+    const appRef = TestBed.inject(ApplicationRef);
+    appRef.attachView(app.hostView);
+    // ApplicationRef has a loop to continue refreshing dirty views. If done incorrectly, refreshing
+    // the backwards reference transplanted view can cause an infinite loop because it goes and
+    // marks the root view dirty, which then starts the process all over again by checking the
+    // declaration.
+    expect(() => appRef.tick()).not.toThrow();
+  });
 });
 
 function trim(text: string|null): string {

Original file line number	Diff line number	Diff line change
`@@ -702,5 +702,8 @@ export function whenStable(applicationRef: ApplicationRef): Promise<void> {`
`702`	`702`
`703`	`703`
`704`	`704`	`function shouldRecheckView(view: LView): boolean {`
`705`		`- return requiresRefreshOrTraversal(view) \|\| !!(view[FLAGS] & LViewFlags.Dirty);`
	`705`	`+ return requiresRefreshOrTraversal(view);`
	`706`	`+ // TODO(atscott): We need to support rechecking views marked dirty again in afterRender hooks`
	`707`	`+ // in order to support the transition to zoneless. b/308152025`
	`708`	`+ /* \|\| !!(view[FLAGS] & LViewFlags.Dirty); */`
`706`	`709`	`}`