bug(core): Error updating transplanted view count with a null parent view

[Achilles1515]

🐞 bug report

Affected Package

The issue is caused by package @angular/core

Is this a regression?

Yes, the previous version in which this bug was not present was: I don't know. The error is occurring with Ivy and not View Engine.

Description

I am working with dynamic views and decided to add an embeddedViewRef.detectChanges() call in my app which unexpectedly spit out the following error:

core.js:4197 ERROR TypeError: Cannot read property '5' of null at updateTransplantedViewCount (core.js:2425) at refreshView (core.js:7268) at detectChangesInternal (core.js:8506) at ViewRef.detectChanges (core.js:9864)

This error is occurring in the first line of the updateTransplantedViewCount function:

angular/packages/core/src/render3/util/view_utils.ts

Lines 198 to 199 in 90cec40

	export function updateTransplantedViewCount(lContainer: LContainer, amount: 1|- 1) {
	lContainer[TRANSPLANTED_VIEWS_TO_REFRESH] += amount;

due to the LContainer input variable being null.

And updateTransplantedViewCount is being called inside refreshView here:

angular/packages/core/src/render3/instructions/shared.ts

Lines 520 to 527 in 90cec40

	if (lView[FLAGS] & LViewFlags.RefreshTransplantedView) {
	lView[FLAGS] &= ~LViewFlags.RefreshTransplantedView;
	updateTransplantedViewCount(lView[PARENT] as LContainer, -1);
	}
	} finally {
	leaveView();
	}
	}

...so lView[PARENT] here is resolving to null.

The obvious first reaction is "well, let's add a null check to one of these functions"...but I'm not familiar enough with the Ivy internals to know if lView[PARENT] being null alongside LViewFlags.RefreshTransplantedView being set represents some larger architecture issue (i.e. this scenario should be "impossible").

For a little more context, I am calling embeddedViewRef.detectChanges() on a non-destroyed embedded view ref that is not attached to a view container ref (was previously attached, but had been detached).

🔬 Minimal Reproduction

Currently struggling to make a repro. If I can't get it to repro, then I'll upload a snapshot of my code.

🌍 Your Environment

Angular Version:

Angular CLI: 10.0.8
Node: 14.8.0
OS: win32 x64

Angular: 10.0.14
... animations, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router
Ivy Workspace: Yes

Package                            Version
------------------------------------------------------------
@angular-devkit/architect          0.1000.8
@angular-devkit/build-angular      0.1000.8
@angular-devkit/build-ng-packagr   0.1000.8
@angular-devkit/build-optimizer    0.1000.8
@angular-devkit/build-webpack      0.1000.8
@angular-devkit/core               10.0.8
@angular-devkit/schematics         10.0.8
@angular/cli                       10.0.8
@ngtools/webpack                   10.0.8
@schematics/angular                10.0.8
@schematics/update                 0.1000.8
ng-packagr                         10.1.0
rxjs                               6.5.5
typescript                         3.9.7
webpack                            4.43.0

[Achilles1515]

Pinging @atscott as I know you did some work with transplanted views recently.

[atscott]

Ah, I think this could happen if the view was previously attached, then manually detached, then you call detectChanges on it. The detachMovedView has code to handle updating the parent container count on detach

angular/packages/core/src/render3/node_manipulation.ts

Lines 299 to 304 in 90cec40

	// If the view was marked for refresh but then detached before it was checked (where the flag
	// would be cleared and the counter decremented), we need to decrement the view counter here
	// instead.
	if (lView[FLAGS] & LViewFlags.RefreshTransplantedView) {
	updateTransplantedViewCount(insertionLContainer, -1);
	}

but if you still have a handle on the embedded view and call detectChanges, it looks like this could cause the error to happen. I hadn't really considered that use-case. I think it's likely that re-attaching that embedded view would also not be handled correctly because trackMovedView doesn't update the parent counters for a transplanted view that is already dirty.

A reproduction would be really great! I'll need one in order to test that a fix works. I might be able to make one from this information, but that might only be possible if my assumptions about why this broke are correct.

[atscott]

Reproduction of error, though the scenario is totally bonkers so it would be nice to get a more real-world scenario: https://stackblitz.com/edit/dynamic-views-qflhmd?file=src%2Fapp%2Fapp.component.ts
Steps to reproduce:

1. click toggle content to create embedded view
2. click detect changes

What happens in the reproduction is as follows:

• Template ref created at the top level
• Template ref passed down two levels from app component: app component -> middle component -> app container
  What this does is create the "transplanted view" scenario. i.e. when app component is dirty and change detection runs, we need to mark its templates which are inserted elsewhere as needing a check.
• Call detect changes on app component. refreshView runs on app component, which also marks transplanted view trees as needing a check
  • Middle component gets checked as well because it's CheckAlways. As part of the template execution, it detaches the embedded view before the container component is checked
  • Then, after the embedded view gets detached, we manually call detectChanges on it. Because it was removed before its insertion component was checked, the RefreshTransplantedView never got cleared. Calling detectChanges will result in the view attempting to decrement the counters on the parent containers but there aren't any because it was detached.

The most obvious fix to me would be to also clear RefreshTransplantedView flag in addition to reducing transplanted view counter on the parens when the view gets detached.

[Achilles1515]

@atscott
I was able to make a super simple reproduction.
StackBlitz

The entire code is as follows:

export class AppComponent  {
  @ViewChild(TemplateRef, {static: true}) templateRef: TemplateRef<any>;
  viewRef: EmbeddedViewRef<any>;

  constructor(public vref: ViewContainerRef, public cdr: ChangeDetectorRef){
  }

  ngOnInit(){
    this.viewRef = this.templateRef.createEmbeddedView(null);

    console.log('inserting...')
    this.vref.insert(this.viewRef);
  
    this.cdr.detectChanges();

    console.log('detaching...')
    this.vref.detach();

    console.log('detecting changes...')
    this.viewRef.detectChanges();
  }
}

The most obvious fix to me would be to also clear RefreshTransplantedView flag in addition to reducing transplanted view counter on the parens when the view gets detached.

After looking at the source code a bit more, I think this makes sense. And I think it would be a one line fix editing the detachMovedView() function to mimic the RefreshTransplantedView flag resetting being done in refreshView().

function detachMovedView(declarationContainer: LContainer, lView: LView) {
  ngDevMode && assertLContainer(declarationContainer);
  ngDevMode &&
      assertDefined(
          declarationContainer[MOVED_VIEWS],
          'A projected view should belong to a non-empty projected views collection');
  const movedViews = declarationContainer[MOVED_VIEWS]!;
  const declarationViewIndex = movedViews.indexOf(lView);
  const insertionLContainer = lView[PARENT] as LContainer;
  ngDevMode && assertLContainer(insertionLContainer);

  // If the view was marked for refresh but then detached before it was checked (where the flag
  // would be cleared and the counter decremented), we need to decrement the view counter here
  // instead.
  if (lView[FLAGS] & LViewFlags.RefreshTransplantedView) {
    // lView[FLAGS] &= ~LViewFlags.RefreshTransplantedView;   // <-- ADD THIS LINE -->
    updateTransplantedViewCount(insertionLContainer, -1);
  }

  movedViews.splice(declarationViewIndex, 1);
}

[atscott]

After looking at the source code a bit more, I think this makes sense. And I think it would be a one line fix editing the detachMovedView() function to mimic the RefreshTransplantedView flag resetting being done in refreshView().

Yep, I believe that's the right fix as well! Do you want to send a PR that I can review? Your test can be added to packages/core/test/acceptance/change_detection_transplanted_view_spec.ts

[Achilles1515]

I'm a total noob when it comes to testing in Angular, and still am somewhat new to Git, but I'll give it a go.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}