Skip to content

Commit

Permalink
fix(core): Fix possible infinite loop with markForCheck by partiall…
Browse files Browse the repository at this point in the history
…y reverting #54074 (#54329)

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 #54329
  • Loading branch information
atscott authored and thePunderWoman committed Feb 8, 2024
1 parent adfc3f0 commit 898a532
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 5 deletions.
5 changes: 4 additions & 1 deletion packages/core/src/application/application_ref.ts
Expand Up @@ -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); */
}
3 changes: 2 additions & 1 deletion packages/core/test/acceptance/after_render_hook_spec.ts
Expand Up @@ -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', () => {
Expand Down
Expand Up @@ -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';

Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 898a532

Please sign in to comment.