diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 5780bbc929..dce6ae1d66 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -374,13 +374,14 @@ export function getLViewChild(lView: LView): LView|LContainer|null { * @param view The view to be destroyed. */ export function destroyLView(view: LView) { - const renderer = view[RENDERER]; - if (isProceduralRenderer(renderer) && renderer.destroyNode) { - walkTNodeTree(view, WalkTNodeTreeAction.Destroy, renderer, null); + if (!(view[FLAGS] & LViewFlags.Destroyed)) { + const renderer = view[RENDERER]; + if (isProceduralRenderer(renderer) && renderer.destroyNode) { + walkTNodeTree(view, WalkTNodeTreeAction.Destroy, renderer, null); + } + + destroyViewTree(view); } - destroyViewTree(view); - // Sets the destroyed flag - view[FLAGS] |= LViewFlags.Destroyed; } /** @@ -418,6 +419,14 @@ export function getParentState(state: LView | LContainer, rootView: LView): LVie function cleanUpView(viewOrContainer: LView | LContainer): void { if ((viewOrContainer as LView).length >= HEADER_OFFSET) { const view = viewOrContainer as LView; + + // Mark the LView as destroyed *before* executing the onDestroy hooks. An onDestroy hook + // runs arbitrary user code, which could include its own `viewRef.destroy()` (or similar). If + // We don't flag the view as destroyed before the hooks, this could lead to an infinite loop. + // This also aligns with the ViewEngine behavior. It also means that the onDestroy hook is + // really more of an "afterDestroy" hook if you think about it. + view[FLAGS] |= LViewFlags.Destroyed; + executeOnDestroys(view); removeListeners(view); const hostTNode = view[HOST_NODE]; diff --git a/packages/core/test/render3/render_util.ts b/packages/core/test/render3/render_util.ts index 6480ef0c1f..444b9dc371 100644 --- a/packages/core/test/render3/render_util.ts +++ b/packages/core/test/render3/render_util.ts @@ -180,7 +180,12 @@ export class ComponentFixture extends BaseFixture { } destroy(): void { - this.containerElement.removeChild(this.hostElement); + // Skip removing the DOM element if it has already been removed (the view has already + // been destroyed). + if (this.hostElement.parentNode === this.containerElement) { + this.containerElement.removeChild(this.hostElement); + } + destroyLView(getRootView(this.component)); } } diff --git a/packages/core/test/render3/view_container_ref_spec.ts b/packages/core/test/render3/view_container_ref_spec.ts index 00de54645e..67c57b98fe 100644 --- a/packages/core/test/render3/view_container_ref_spec.ts +++ b/packages/core/test/render3/view_container_ref_spec.ts @@ -6,22 +6,23 @@ * 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, createInjector, defineInjector, ɵAPP_ROOT as APP_ROOT, ɵNgModuleDef as NgModuleDef} from '../../src/core'; +import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, ElementRef, EmbeddedViewRef, NgModuleRef, Pipe, PipeTransform, QueryList, RendererFactory2, TemplateRef, ViewContainerRef, ViewRef, createInjector, defineInjector, ɵAPP_ROOT as APP_ROOT, ɵNgModuleDef as NgModuleDef} from '../../src/core'; import {ViewEncapsulation} from '../../src/metadata'; -import {AttributeMarker, NO_CHANGE, NgOnChangesFeature, defineComponent, defineDirective, definePipe, injectComponentFactoryResolver, loadViewQuery, queryRefresh, viewQuery} from '../../src/render3/index'; -import {allocHostVars, bind, container, containerRefreshEnd, containerRefreshStart, directiveInject, element, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation3, nextContext, projection, projectionDef, reference, template, text, textBinding, elementHostAttrs} from '../../src/render3/instructions'; +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'; import {RenderFlags} from '../../src/render3/interfaces/definition'; import {RElement} from '../../src/render3/interfaces/renderer'; -import {templateRefExtractor} from '../../src/render3/view_engine_compatibility_prebound'; 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 {templateRefExtractor} from '../../src/render3/view_engine_compatibility_prebound'; import {NgForOf} from '../../test/render3/common_with_def'; import {getRendererFactory2} from './imported_renderer2'; -import {ComponentFixture, TemplateFixture, createComponent, getDirectiveOnNode} from './render_util'; +import {ComponentFixture, createComponent, getDirectiveOnNode, TemplateFixture,} from './render_util'; const Component: typeof _Component = function(...args: any[]): any { // In test we use @Component for documentation only so it's safe to mock out the implementation. @@ -2087,4 +2088,56 @@ describe('ViewContainerRef', () => { }); }); + + describe('view destruction', () => { + class CompWithListenerThatDestroysItself { + constructor(private viewRef: ViewRef) {} + + onClick() {} + + ngOnDestroy() { this.viewRef.destroy(); } + + static ngComponentDef = defineComponent({ + type: CompWithListenerThatDestroysItself, + selectors: [['comp-with-listener-and-on-destroy']], + consts: 2, + vars: 0, + /** */ + template: function CompTemplate(rf: RenderFlags, ctx: any) { + if (rf & RenderFlags.Create) { + elementStart(0, 'button'); + { + listener('click', function() { return ctx.onClick(); }); + text(1, 'Click me'); + } + elementEnd(); + } + }, + // We want the ViewRef, so we rely on the knowledge that `ViewRef` is actually given + // when injecting `ChangeDetectorRef`. + factory: + () => new CompWithListenerThatDestroysItself(directiveInject(ChangeDetectorRef as any)), + }); + } + + + it('should not error when destroying a view with listeners twice', () => { + const CompWithChildListener = createComponent('test-app', (rf: RenderFlags, ctx: any) => { + if (rf & RenderFlags.Create) { + element(0, 'comp-with-listener-and-on-destroy'); + } + }, 1, 0, [CompWithListenerThatDestroysItself]); + + const fixture = new ComponentFixture(CompWithChildListener); + fixture.update(); + + // Destroying the parent view will also destroy all of its children views and call their + // onDestroy hooks. Here, our child view attempts to destroy itself *again* in its onDestroy. + // This test exists to verify that no errors are thrown when doing this. We want the test + // component to destroy its own view in onDestroy because the destroy hooks happen as a + // *part of* view destruction. We also ensure that the test component has at least one + // listener so that it runs the event listener cleanup code path. + fixture.destroy(); + }); + }); });