From b1d03fe70b2e5714e6319b7034db9b477fec26e8 Mon Sep 17 00:00:00 2001
From: Pawel Kozlowski <pkozlowski.opensource@gmail.com>
Date: Thu, 19 Apr 2018 22:59:58 +0200
Subject: [PATCH] fix(ivy): properly destroy view trees where root is an
 embedded view without children (#23482)

The bug fixed here steams from the fact that we are traversing too far up
in the views tree hierarchy in the destroyViewTree function.

The logic in destroyViewTree is off if we start removal at an embedded view
without any child views. For such a case we should just clean up (cleanUpView)
this one view without paying attention to next / parent views.

PR Close #23482
---
 .../core/src/render3/node_manipulation.ts     | 15 +++-
 .../test/render3/common_integration_spec.ts   | 78 ++++++++++++++++++-
 2 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
index 3fcfd2a99a..d6ee59f71a 100644
--- a/packages/core/src/render3/node_manipulation.ts
+++ b/packages/core/src/render3/node_manipulation.ts
@@ -209,7 +209,7 @@ export function addRemoveViewFromContainer(
 }
 
 /**
- * Traverses the tree of component views and containers to remove listeners and
+ * Traverses down and up the tree of views and containers to remove listeners and
  * call onDestroy callbacks.
  *
  * Notes:
@@ -222,7 +222,11 @@ export function addRemoveViewFromContainer(
  *  @param rootView The view to destroy
  */
 export function destroyViewTree(rootView: LView): void {
-  let viewOrContainer: LViewOrLContainer|null = rootView;
+  // A view to cleanup doesn't have children so we should not try to descend down the view tree.
+  if (!rootView.child) {
+    return cleanUpView(rootView);
+  }
+  let viewOrContainer: LViewOrLContainer|null = rootView.child;
 
   while (viewOrContainer) {
     let next: LViewOrLContainer|null = null;
@@ -232,13 +236,16 @@ export function destroyViewTree(rootView: LView): void {
     } else if (viewOrContainer.child) {
       next = viewOrContainer.child;
     } else if (viewOrContainer.next) {
+      // Only move to the side and clean if operating below rootView -
+      // otherwise we would start cleaning up sibling views of the rootView.
       cleanUpView(viewOrContainer as LView);
       next = viewOrContainer.next;
     }
 
     if (next == null) {
-      // If the viewOrContainer is the rootView, then the cleanup is done twice.
-      // Without this check, ngOnDestroy would be called twice for a directive on an element.
+      // If the viewOrContainer is the rootView and next is null it means that we are dealing
+      // with a root view that doesn't have children. We didn't descend into child views
+      // so no need to go back up the views tree.
       while (viewOrContainer && !viewOrContainer !.next && viewOrContainer !== rootView) {
         cleanUpView(viewOrContainer as LView);
         viewOrContainer = getParentState(viewOrContainer, rootView);
diff --git a/packages/core/test/render3/common_integration_spec.ts b/packages/core/test/render3/common_integration_spec.ts
index d09acb934a..c78872fa1d 100644
--- a/packages/core/test/render3/common_integration_spec.ts
+++ b/packages/core/test/render3/common_integration_spec.ts
@@ -9,8 +9,9 @@
 import {NgForOfContext} from '@angular/common';
 
 import {defineComponent} from '../../src/render3/index';
-import {bind, container, elementEnd, elementProperty, elementStart, interpolation3, text, textBinding, tick} from '../../src/render3/instructions';
+import {bind, container, elementEnd, elementProperty, elementStart, interpolation1, interpolation3, listener, text, textBinding, tick} from '../../src/render3/instructions';
 import {RenderFlags} from '../../src/render3/interfaces/definition';
+
 import {NgForOf, NgIf} from './common_with_def';
 import {ComponentFixture} from './render_util';
 
@@ -123,6 +124,81 @@ describe('@angular/common integration', () => {
           .toEqual('<ul><li>0 of 3: first</li><li>1 of 3: middle</li><li>2 of 3: second</li></ul>');
     });
 
+
+    it('should retain parent view listeners when the NgFor destroy views', () => {
+
+      class MyApp {
+        private _data: number[] = [1, 2, 3];
+        items: number[] = [];
+
+        toggle() {
+          if (this.items.length) {
+            this.items = [];
+          } else {
+            this.items = this._data;
+          }
+        }
+
+        static ngComponentDef = defineComponent({
+          type: MyApp,
+          factory: () => new MyApp(),
+          selectors: [['my-app']],
+          // <button (click)="toggle()">Toggle List</button>
+          // <ul>
+          //   <li *ngFor="let item of items">{{index}}</li>
+          // </ul>
+          template: (rf: RenderFlags, myApp: MyApp) => {
+            if (rf & RenderFlags.Create) {
+              elementStart(0, 'button');
+              {
+                listener('click', function() { return myApp.toggle(); });
+                text(1, 'Toggle List');
+              }
+              elementEnd();
+              elementStart(2, 'ul');
+              { container(3, liTemplate, undefined, ['ngForOf', '']); }
+              elementEnd();
+            }
+            if (rf & RenderFlags.Update) {
+              elementProperty(3, 'ngForOf', bind(myApp.items));
+            }
+
+            function liTemplate(rf1: RenderFlags, row: NgForOfContext<string>) {
+              if (rf1 & RenderFlags.Create) {
+                elementStart(0, 'li');
+                { text(1); }
+                elementEnd();
+              }
+              if (rf1 & RenderFlags.Update) {
+                textBinding(1, interpolation1('', row.$implicit, ''));
+              }
+            }
+          },
+          directives: () => [NgForOf]
+        });
+      }
+
+      const fixture = new ComponentFixture(MyApp);
+      const button = fixture.hostElement.querySelector('button') !;
+
+      expect(fixture.html).toEqual('<button>Toggle List</button><ul></ul>');
+
+      // this will fill the list
+      fixture.component.toggle();
+      fixture.update();
+      expect(fixture.html)
+          .toEqual('<button>Toggle List</button><ul><li>1</li><li>2</li><li>3</li></ul>');
+
+      button.click();
+      fixture.update();
+
+      expect(fixture.html).toEqual('<button>Toggle List</button><ul></ul>');
+
+      button.click();
+      fixture.update();
+      expect(fixture.html)
+          .toEqual('<button>Toggle List</button><ul><li>1</li><li>2</li><li>3</li></ul>');
+    });
   });
 
   describe('ngIf', () => {