fix(ivy): remove DOM nodes from their real parent vs saved parent (angular#28455)

Currently, DOM node removal called `removeChild` on the saved parent
node when destroying a component. However, this will fail if the
component has been manually moved in the DOM. This change makes the
removal always use the node's real `parentNode` and ignore the provided
`parent`.

PR Close angular#28455

Author: jelbourn

packages/core/src/render3/i18n.ts

@@ -19,7 +19,7 @@ import {RComment, RElement} from './interfaces/renderer';
 import {SanitizerFn} from './interfaces/sanitization';
 import {StylingContext} from './interfaces/styling';
 import {BINDING_INDEX, HEADER_OFFSET, HOST_NODE, LView, RENDERER, TVIEW, TView} from './interfaces/view';
-import {appendChild, createTextNode, removeChild} from './node_manipulation';
+import {appendChild, createTextNode, removeNode as removeRNode} from './node_manipulation';
 import {getIsParent, getLView, getPreviousOrParentTNode, setIsParent, setPreviousOrParentTNode} from './state';
 import {NO_CHANGE} from './tokens';
 import {addAllToArray, getNativeByIndex, getNativeByTNode, getTNode, isLContainer, renderStringify} from './util';
@@ -830,7 +830,7 @@ function removeNode(index: number, viewData: LView) {
   const removedPhTNode = getTNode(index, viewData);
   const removedPhRNode = getNativeByIndex(index, viewData);
   if (removedPhRNode) {
-    removeChild(removedPhTNode, removedPhRNode, viewData);
+    removeRNode(removedPhTNode, removedPhRNode, viewData);
   }
 
   removedPhTNode.detached = true;
@@ -840,7 +840,7 @@ function removeNode(index: number, viewData: LView) {
   if (isLContainer(slotValue)) {
     const lContainer = slotValue as LContainer;
     if (removedPhTNode.type !== TNodeType.Container) {
-      removeChild(removedPhTNode, lContainer[NATIVE], viewData);
+      removeRNode(removedPhTNode, lContainer[NATIVE], viewData);
     }
   }
 }

packages/core/src/render3/node_manipulation.ts

@@ -181,7 +181,7 @@ function executeNodeAction(
   if (action === WalkTNodeTreeAction.Insert) {
     nativeInsertBefore(renderer, parent !, node, beforeNode || null);
   } else if (action === WalkTNodeTreeAction.Detach) {
-    nativeRemoveChild(renderer, parent !, node, isComponent(tNode));
+    nativeRemoveChild(renderer, node, isComponent(tNode));
   } else if (action === WalkTNodeTreeAction.Destroy) {
     ngDevMode && ngDevMode.rendererDestroyNode++;
     (renderer as ProceduralRenderer3).destroyNode !(node);
@@ -593,13 +593,19 @@ function nativeAppendOrInsertBefore(
   }
 }
 
-/**
- * Removes a native child node from a given native parent node.
- */
+/** Removes a node from the DOM. */
 export function nativeRemoveChild(
-    renderer: Renderer3, parent: RElement, child: RNode, isHostElement?: boolean): void {
-  isProceduralRenderer(renderer) ? renderer.removeChild(parent as RElement, child, isHostElement) :
-                                   parent.removeChild(child);
+    renderer: Renderer3, child: RNode, isHostElement?: boolean): void {
+  if (isProceduralRenderer(renderer)) {
+    const renderParent = renderer.parentNode(child);
+    if (renderParent) {
+      renderer.removeChild(renderParent, child, isHostElement);
+    }
+  } else {
+    // We intentionally don't use the given parent node since it may no longer
+    // match the state of the DOM (if the child node has been manually moved).
+    child.parentNode && child.parentNode.removeChild(child);
+  }
 }
 
 /**
@@ -692,14 +698,9 @@ export function getBeforeNodeForView(index: number, views: LView[], containerNat
  * @param childTNode The TNode of the child to remove
  * @param childEl The child that should be removed
  * @param currentView The current LView
- * @returns Whether or not the child was removed
  */
-export function removeChild(childTNode: TNode, childEl: RNode, currentView: LView): void {
-  const parentNative = getRenderParent(childTNode, currentView);
-  // We only remove the element if it already has a render parent.
-  if (parentNative) {
-    nativeRemoveChild(currentView[RENDERER], parentNative, childEl);
-  }
+export function removeNode(childTNode: TNode, childEl: RNode, currentView: LView): void {
+  nativeRemoveChild(currentView[RENDERER], childEl);
 }
 
 /**

packages/core/test/linker/view_injector_integration_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Attribute, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, DebugElement, Directive, ElementRef, Host, Inject, InjectionToken, Injector, Input, NgModule, Optional, Pipe, PipeTransform, Provider, Self, SkipSelf, TemplateRef, Type, ViewContainerRef} from '@angular/core';
+import {Attribute, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, DebugElement, Directive, ElementRef, EmbeddedViewRef, Host, Inject, InjectionToken, Injector, Input, NgModule, Optional, Pipe, PipeTransform, Provider, Self, SkipSelf, TemplateRef, Type, ViewContainerRef} from '@angular/core';
 import {ComponentFixture, TestBed, fakeAsync} from '@angular/core/testing';
 import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
@@ -1021,6 +1021,58 @@ class TestComp {
         expect(impurePipe4).not.toBe(impurePipe1);
       });
     });
+
+    describe('view destruction', () => {
+      @Component({selector: 'some-component', template: ''})
+      class SomeComponent {
+      }
+
+      @NgModule({
+        declarations: [SomeComponent],
+        exports: [SomeComponent],
+        entryComponents: [SomeComponent]
+      })
+      class SomeModule {
+      }
+
+      @Component({selector: 'listener-and-on-destroy', template: ''})
+      class ComponentThatLoadsAnotherComponentThenMovesIt {
+        constructor(
+            private viewContainerRef: ViewContainerRef,
+            private componentFactoryResolver: ComponentFactoryResolver) {}
+
+
+        ngOnInit() {
+          // Dynamically load some component.
+          const componentFactory =
+              this.componentFactoryResolver.resolveComponentFactory(SomeComponent);
+          const componentRef =
+              this.viewContainerRef.createComponent(componentFactory, this.viewContainerRef.length);
+
+          // Manually move the loaded component to some arbitrary DOM node.
+          const componentRootNode =
+              (componentRef.hostView as EmbeddedViewRef<any>).rootNodes[0] as HTMLElement;
+          document.createElement('div').appendChild(componentRootNode);
+
+          // Destroy the component we just moved to ensure that it does not error during
+          // destruction.
+          componentRef.destroy();
+        }
+      }
+
+      it('should not error when destroying a component that has been moved in the DOM', () => {
+        TestBed.configureTestingModule({
+          imports: [SomeModule],
+          declarations: [ComponentThatLoadsAnotherComponentThenMovesIt],
+        });
+        const fixture =
+            createComponentFixture(`<listener-and-on-destroy></listener-and-on-destroy>`);
+        fixture.detectChanges();
+
+        // This test will fail if the ngOnInit of ComponentThatLoadsAnotherComponentThenMovesIt
+        // throws an error.
+      });
+    });
   });
 })();
 

packages/core/test/render3/view_container_ref_spec.ts

@@ -6,9 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ElementRef, EmbeddedViewRef, NgModuleRef, Pipe, PipeTransform, QueryList, RendererFactory2, TemplateRef, ViewContainerRef, ViewRef, defineInjector, ɵAPP_ROOT as APP_ROOT, ɵNgModuleDef as NgModuleDef} from '../../src/core';
+import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ComponentRef, defineInjector, ElementRef, EmbeddedViewRef, NgModuleRef, Pipe, PipeTransform, QueryList, RendererFactory2, TemplateRef, ViewContainerRef, ViewRef, ɵAPP_ROOT as APP_ROOT, ɵNgModuleDef as NgModuleDef,} from '../../src/core';
+import {createInjector} from '../../src/di/r3_injector';
 import {ViewEncapsulation} from '../../src/metadata';
-
 import {AttributeMarker, defineComponent, defineDirective, definePipe, injectComponentFactoryResolver, listener, loadViewQuery, NgOnChangesFeature, queryRefresh, viewQuery,} from '../../src/render3/index';
 
 import {allocHostVars, bind, container, containerRefreshEnd, containerRefreshStart, directiveInject, element, elementEnd, elementHostAttrs, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation3, nextContext, projection, projectionDef, reference, template, text, textBinding,} from '../../src/render3/instructions';
@@ -18,7 +18,6 @@ import {NgModuleFactory} from '../../src/render3/ng_module_ref';
 import {pipe, pipeBind1} from '../../src/render3/pipe';
 import {getLView} from '../../src/render3/state';
 import {getNativeByIndex} from '../../src/render3/util';
-import {createInjector} from '../../src/di/r3_injector';
 import {templateRefExtractor} from '../../src/render3/view_engine_compatibility_prebound';
 import {NgForOf} from '../../test/render3/common_with_def';
 

packages/platform-browser/test/dom/dom_renderer_spec.ts

@@ -87,6 +87,17 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
       });
     });
 
+    describe('removeChild', () => {
+      it('should not error when removing a child with a different parent than given', () => {
+        const savedParent = document.createElement('div');
+        const realParent = document.createElement('div');
+        const child = document.createElement('div');
+
+        realParent.appendChild(child);
+        renderer.removeChild(savedParent, child);
+      });
+    });
+
     if (browserDetection.supportsDeprecatedShadowDomV0) {
       it('should allow to style components with emulated encapsulation and no encapsulation inside of components with shadow DOM',
          () => {