fix(ivy): prevent errors from views being destroyed twice (angular#28413)

Previously, attempting to destroy a view with listeners more than once
throws an error during event listener cleanup. This happens because
`cleanup` field on the `TView` has already been cleared out by the time
the second destruction runs.

The `destroyed` flag on LView was previously being set in the `destroyLView` function,
but this flag was never _checked_ anywhere in the codebase. This commit
moves _setting_ this flag to the `cleanupView` function, just before
destroy hooks are called. This is necessary because the destroy hooks
can contain arbitrary user code, such as (surprise!) attempting to
destroy the view (again). We also add a check to `destroyLView` to skip
already-destroyed views. This prevents the cleanup code path from running twice.

PR Close angular#28413

Author: jelbourn

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];

packages/core/test/render3/render_util.ts

@@ -180,7 +180,12 @@ export class ComponentFixture<T> 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));
   }
 }

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,
+        /** <button (click)="onClick()"> Click me </button> */
+        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();
+    });
+  });
 });