fix(ivy): move views that are already attached in insert() (#29047)

Currently if a user accidentally calls ViewContainerRef.insert() with
a view that has already been attached, we do not clean up the references
properly, so we create a view tree with a cycle. This causes an infinite
loop when the view is destroyed.

This PR ensures that we fall back to ViewContainerRef.move() behavior
if we try to insert a view that is already attached. This fixes the
cycle and honors the user intention.

PR Close #29047

packages/core/src/render3/instructions.ts

@@ -47,7 +47,7 @@ import {ANIMATION_PROP_PREFIX, allocateDirectiveIntoContext, createEmptyStylingC
 import {NO_CHANGE} from './tokens';
 import {INTERPOLATION_DELIMITER, renderStringify} from './util/misc_utils';
 import {findComponentView, getLViewParent, getRootContext, getRootView} from './util/view_traversal_utils';
-import {getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getTNode, isComponent, isComponentDef, isContentQueryHost, isRootView, loadInternal, readPatchedLView, unwrapRNode} from './util/view_utils';
+import {getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getTNode, isComponent, isComponentDef, isContentQueryHost, isRootView, loadInternal, readPatchedLView, unwrapRNode, viewAttachedToChangeDetector} from './util/view_utils';
 
 
 
@@ -2531,7 +2531,8 @@ export function componentRefresh<T>(adjustedElementIndex: number): void {
   ngDevMode && assertNodeType(lView[TVIEW].data[adjustedElementIndex] as TNode, TNodeType.Element);
 
   // Only attached CheckAlways components or attached, dirty OnPush components should be checked
-  if (viewAttached(hostView) && hostView[FLAGS] & (LViewFlags.CheckAlways | LViewFlags.Dirty)) {
+  if (viewAttachedToChangeDetector(hostView) &&
+      hostView[FLAGS] & (LViewFlags.CheckAlways | LViewFlags.Dirty)) {
     syncViewWithBlueprint(hostView);
     checkView(hostView, hostView[CONTEXT]);
   }
@@ -2570,11 +2571,6 @@ function syncViewWithBlueprint(componentView: LView) {
   }
 }
 
-/** Returns a boolean for whether the view is attached */
-export function viewAttached(view: LView): boolean {
-  return (view[FLAGS] & LViewFlags.Attached) === LViewFlags.Attached;
-}
-
 /**
  * Instruction to distribute projectable nodes among <ng-content> occurrences in a given template.
  * It takes all the selectors from the entire component's template and decides where

packages/core/src/render3/node_manipulation.ts

@@ -354,6 +354,7 @@ export function detachView(lContainer: LContainer, removeIndex: number): LView {
     viewToDetach[QUERIES] !.removeView();
   }
   viewToDetach[PARENT] = null;
+  viewToDetach[NEXT] = null;
   // Unsets the attached flag
   viewToDetach[FLAGS] &= ~LViewFlags.Attached;
   return viewToDetach;

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

@@ -13,7 +13,7 @@ import {ComponentDef, DirectiveDef} from '../interfaces/definition';
 import {TNode, TNodeFlags} from '../interfaces/node';
 import {RNode} from '../interfaces/renderer';
 import {StylingContext} from '../interfaces/styling';
-import {FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, TData, TVIEW} from '../interfaces/view';
+import {FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, PARENT, TData, TVIEW} from '../interfaces/view';
 
 
 
@@ -182,3 +182,18 @@ export function readPatchedLView(target: any): LView|null {
   }
   return null;
 }
+
+/**
+ * Returns a boolean for whether the view is attached to the change detection tree.
+ *
+ * Note: This determines whether a view should be checked, not whether it's inserted
+ * into a container. For that, you'll want `viewAttachedToContainer` below.
+ */
+export function viewAttachedToChangeDetector(view: LView): boolean {
+  return (view[FLAGS] & LViewFlags.Attached) === LViewFlags.Attached;
+}
+
+/** Returns a boolean for whether the view is attached to a container. */
+export function viewAttachedToContainer(view: LView): boolean {
+  return isLContainer(view[PARENT]);
+}

packages/core/src/render3/view_engine_compatibility.ts

@@ -22,14 +22,14 @@ import {addToViewTree, createEmbeddedViewAndNode, createLContainer, renderEmbedd
 import {ACTIVE_INDEX, LContainer, NATIVE, VIEWS} from './interfaces/container';
 import {TContainerNode, TElementContainerNode, TElementNode, TNode, TNodeType, TViewNode} from './interfaces/node';
 import {RComment, RElement, isProceduralRenderer} from './interfaces/renderer';
-import {CONTEXT, LView, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view';
+import {CONTEXT, FLAGS, LView, LViewFlags, PARENT, QUERIES, RENDERER, TView, T_HOST} from './interfaces/view';
 import {assertNodeOfPossibleTypes} from './node_assert';
 import {addRemoveViewFromContainer, appendChild, detachView, getBeforeNodeForView, insertView, nativeInsertBefore, nativeNextSibling, nativeParentNode, removeView} from './node_manipulation';
 import {getParentInjectorTNode} from './node_util';
 import {getLView, getPreviousOrParentTNode} from './state';
 import {getParentInjectorView, hasParentInjector} from './util/injector_utils';
 import {findComponentView} from './util/view_traversal_utils';
-import {getComponentViewByIndex, getNativeByTNode, isComponent, isLContainer, isRootView} from './util/view_utils';
+import {getComponentViewByIndex, getNativeByTNode, isComponent, isLContainer, isRootView, viewAttachedToChangeDetector, viewAttachedToContainer} from './util/view_utils';
 import {ViewRef} from './view_ref';
 
 
@@ -236,6 +236,12 @@ export function createContainerRef(
         const lView = (viewRef as ViewRef<any>)._lView !;
         const adjustedIdx = this._adjustIndex(index);
 
+        if (viewAttachedToContainer(lView)) {
+          // If view is already attached, fall back to move() so we clean up
+          // references appropriately.
+          return this.move(viewRef, adjustedIdx);
+        }
+
         insertView(lView, this._lContainer, adjustedIdx);
 
         const beforeNode =
@@ -253,8 +259,8 @@ export function createContainerRef(
           throw new Error('Cannot move a destroyed View in a ViewContainer!');
         }
         const index = this.indexOf(viewRef);
-        this.detach(index);
-        this.insert(viewRef, this._adjustIndex(newIndex));
+        if (index !== -1) this.detach(index);
+        this.insert(viewRef, newIndex);
         return viewRef;
       }
 

packages/core/test/acceptance/view_container_ref_spec.ts

@@ -0,0 +1,107 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Component, QueryList, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core';
+import {TestBed} from '@angular/core/testing';
+import {expect} from '@angular/platform-browser/testing/src/matchers';
+import {ivyEnabled, onlyInIvy} from '@angular/private/testing';
+
+describe('ViewContainerRef', () => {
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({declarations: [ViewContainerRefComp, ViewContainerRefApp]});
+  });
+
+  describe('insert', () => {
+    it('should not blow up on destroy when inserting a view that is already attached', () => {
+      const fixture = TestBed.createComponent(ViewContainerRefApp);
+      fixture.detectChanges();
+
+      const template0 = fixture.componentInstance.vcrComp.templates.first;
+      const viewContainerRef = fixture.componentInstance.vcrComp.vcr;
+      const ref0 = viewContainerRef.createEmbeddedView(template0);
+
+      // Insert the view again at the same index
+      viewContainerRef.insert(ref0, 0);
+
+      expect(() => { fixture.destroy(); }).not.toThrow();
+
+      expect(fixture.nativeElement.textContent).toEqual('0');
+    });
+
+    it('should move views if they are already attached', () => {
+      const fixture = TestBed.createComponent(ViewContainerRefApp);
+      fixture.detectChanges();
+
+      const templates = fixture.componentInstance.vcrComp.templates.toArray();
+      const viewContainerRef = fixture.componentInstance.vcrComp.vcr;
+      const ref0 = viewContainerRef.createEmbeddedView(templates[0]);
+      const ref1 = viewContainerRef.createEmbeddedView(templates[1]);
+      const ref2 = viewContainerRef.createEmbeddedView(templates[2]);
+      fixture.detectChanges();
+
+      expect(fixture.nativeElement.textContent).toEqual('012');
+
+      // Insert the view again at a different index
+      viewContainerRef.insert(ref0, 2);
+      fixture.detectChanges();
+
+      // In Ivy, we correctly move the "0" view to index 2. VE
+      // has a bug that duplicates the view, so it's at index 1.
+      if (ivyEnabled) {
+        expect(fixture.nativeElement.textContent).toEqual('120');
+      } else {
+        expect(fixture.nativeElement.textContent).toEqual('102');
+      }
+    });
+  });
+
+  describe('move', () => {
+    onlyInIvy('Ivy will insert detached views in move')
+        .it('should insert detached views in move()', () => {
+          const fixture = TestBed.createComponent(ViewContainerRefApp);
+          fixture.detectChanges();
+
+          const templates = fixture.componentInstance.vcrComp.templates.toArray();
+          const viewContainerRef = fixture.componentInstance.vcrComp.vcr;
+          const ref0 = viewContainerRef.createEmbeddedView(templates[0]);
+          const ref1 = viewContainerRef.createEmbeddedView(templates[1]);
+          const ref2 = viewContainerRef.createEmbeddedView(templates[2]);
+
+          viewContainerRef.detach(0);
+          viewContainerRef.move(ref0, 0);
+
+          expect(fixture.nativeElement.textContent).toEqual('012');
+        });
+  });
+
+});
+
+@Component({
+  selector: 'view-container-ref-comp',
+  template: `
+    <ng-template #ref0>0</ng-template>
+    <ng-template #ref1>1</ng-template>
+    <ng-template #ref2>2</ng-template>
+  `
+})
+class ViewContainerRefComp {
+  @ViewChildren(TemplateRef) templates !: QueryList<TemplateRef<any>>;
+
+  constructor(public vcr: ViewContainerRef) {}
+}
+
+@Component({
+  selector: 'view-container-ref-app',
+  template: `
+    <view-container-ref-comp></view-container-ref-comp>
+  `
+})
+class ViewContainerRefApp {
+  @ViewChild(ViewContainerRefComp) vcrComp !: ViewContainerRefComp;
+}

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

@@ -654,6 +654,6 @@
     "name": "unwrapRNode"
   },
   {
-    "name": "viewAttached"
+    "name": "viewAttachedToChangeDetector"
   }
 ]

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

@@ -474,6 +474,6 @@
     "name": "unwrapRNode"
   },
   {
-    "name": "viewAttached"
+    "name": "viewAttachedToChangeDetector"
   }
 ]

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

@@ -1296,7 +1296,10 @@
     "name": "valueExists"
   },
   {
-    "name": "viewAttached"
+    "name": "viewAttachedToChangeDetector"
+  },
+  {
+    "name": "viewAttachedToContainer"
   },
   {
     "name": "walkTNodeTree"