Track parent view of cleaned up render nodes

This commit fixes a bug where views created inside an `{{#each}}` were
not getting removed appropriately from their parent’s `childViews`
array when they were destroyed.

Previously, we used a boolean flag to track whether we had removed the
destroyed view from its parent. The first time we saw a view while
walking the tree of destroyed render nodes, we should remove it from
its parent view’s `childViews` and flip the flag to true.

As we walked the tree, we assumed that any subsequent views were
descendants of the root destroyed view, and therefore could be left in
place while we waited for the GC to do its magic.

However, the case we missed was when there were two sibling views being
destroyed as part of the same root render node being cleaned up.

For example, imagine this view graph:

       A
      / \
     B  /\
       C  D <— both created via an {{#each}}

If the sibling views were inside a non-view-associated render node,
and that render node was cleaned up (e.g., an {{#if}} that became
falsy), the tree of nodes would be walked. Once it reached the render
node associated with the C view, it would remove C from A’s
`childViews` and toggle the flag to true, indicating that we had seen
the view and removed it.

However, the tree walker would then get to the render node
representing the D view. Because the flag was now set to true, it
would erroneously assume D was a child of C and leave it in A’s
`childViews`.

In this commit, we change the flag to a property that tracks the
nearest view to the render node being cleaned up. As we traverse the
render node tree, if we find a node with a view, we remove that view
from it’s parent view’s `childViews` array if and only if its parent
view is the same as the render node’s nearest view. This is a more
correct version of the heuristic for determining when to remove
destroyed views from their parents’ than our previous attempt.

packages/ember-htmlbars/lib/env.js

@@ -20,6 +20,7 @@ import getCellOrValue from 'ember-htmlbars/hooks/get-cell-or-value';
 import cleanupRenderNode from 'ember-htmlbars/hooks/cleanup-render-node';
 import destroyRenderNode from 'ember-htmlbars/hooks/destroy-render-node';
 import didRenderNode from 'ember-htmlbars/hooks/did-render-node';
+import willCleanupTree from 'ember-htmlbars/hooks/will-cleanup-tree';
 import didCleanupTree from 'ember-htmlbars/hooks/did-cleanup-tree';
 import classify from 'ember-htmlbars/hooks/classify';
 import component from 'ember-htmlbars/hooks/component';
@@ -52,6 +53,7 @@ merge(emberHooks, {
   concat: concat,
   cleanupRenderNode: cleanupRenderNode,
   destroyRenderNode: destroyRenderNode,
+  willCleanupTree: willCleanupTree,
   didCleanupTree: didCleanupTree,
   didRenderNode: didRenderNode,
   classify: classify,

packages/ember-htmlbars/lib/hooks/did-cleanup-tree.js

@@ -1,6 +1,5 @@
 export default function didCleanupTree(env) {
-  var view;
-  if (view = env.view) {
-    view.ownerView.isDestroyingSubtree = false;
-  }
+  // Once we have finsihed cleaning up the render node and sub-nodes, reset
+  // state tracking which view those render nodes belonged to.
+  env.view.ownerView._destroyingSubtreeForView = null;
 }

packages/ember-htmlbars/lib/hooks/will-cleanup-tree.js

@@ -0,0 +1,32 @@
+export default function willCleanupTree(env) {
+  let view = env.view;
+
+  // When we go to clean up the render node and all of its children, we may
+  // encounter views/components associated with those nodes along the way. In
+  // those cases, we need to make sure we need to sever the link between the
+  // existing view hierarchy and those views.
+  //
+  // However, we do *not* need to remove the child views of child views, since
+  // severing the connection to their parent effectively severs them from the
+  // view graph.
+  //
+  // For example, imagine the following view graph:
+  //
+  //    A
+  //   / \
+  //  B  C
+  //    / \
+  //   D  E
+  //
+  // If we are cleaning up the node for view C, we need to remove that view
+  // from A's child views. However, we do not need to remove D and E from C's
+  // child views, since removing C transitively removes D and E as well.
+  //
+  // To accomplish this, we track the nearest view to this render node on the
+  // owner view, the root-most view in the graph (A in the example above). If
+  // we detect a view that is a direct child of that view, we remove it from
+  // the `childViews` array. Other parent/child view relationships are
+  // untouched.  This view is then cleared once cleanup is complete in
+  // `didCleanupTree`.
+  view.ownerView._destroyingSubtreeForView = view;
+}

packages/ember-htmlbars/lib/morphs/morph.js

@@ -31,12 +31,13 @@ proto.addDestruction = function(toDestroy) {
 };
 
 proto.cleanup = function() {
-  var view;
+  let view = this.emberView;
 
-  if (view = this.emberView) {
-    if (!view.ownerView.isDestroyingSubtree) {
-      view.ownerView.isDestroyingSubtree = true;
-      if (view.parentView) { view.parentView.removeChild(view); }
+  if (view) {
+    let parentView = view.parentView;
+
+    if (parentView && view.ownerView._destroyingSubtreeForView === parentView) {
+      parentView.removeChild(view);
     }
   }
 

packages/ember-htmlbars/lib/node-managers/view-node-manager.js

@@ -166,6 +166,10 @@ export function createOrUpdateComponent(component, options, createOptions, rende
   let defaultController = View.proto().controller;
   let hasSuppliedController = 'controller' in attrs || 'controller' in props;
 
+  if (!props.ownerView && options.parentView) {
+    props.ownerView = options.parentView.ownerView;
+  }
+
   props.attrs = snapshot;
   if (component.create) {
     let proto = component.proto();

packages/ember-views/lib/mixins/view_child_views_support.js

@@ -28,7 +28,7 @@ var ViewChildViewsSupport = Mixin.create({
     // setup child views. be sure to clone the child views array first
     // 2.0TODO: Remove Ember.A() here
     this.childViews = Ember.A(this.childViews.slice());
-    this.ownerView = this;
+    this.ownerView = this.ownerView || this;
   },
 
   appendChild(view) {

packages/ember-views/lib/views/core_view.js

@@ -54,7 +54,7 @@ var CoreView = EmberObject.extend(Evented, ActionHandler, {
       this.renderer = renderer;
     }
 
-    this.isDestroyingSubtree = false;
+    this._destroyingSubtreeForView = null;
     this._dispatching = null;
   },
 
@@ -106,20 +106,19 @@ var CoreView = EmberObject.extend(Evented, ActionHandler, {
   },
 
   destroy() {
-    var parent = this.parentView;
-
     if (!this._super(...arguments)) { return; }
 
     this.currentState.cleanup(this);
 
-    if (!this.ownerView.isDestroyingSubtree) {
-      this.ownerView.isDestroyingSubtree = true;
-      if (parent) { parent.removeChild(this); }
-      if (this._renderNode) {
-        Ember.assert('BUG: Render node exists without concomitant env.', this.ownerView.env);
-        internal.clearMorph(this._renderNode, this.ownerView.env, true);
-      }
-      this.ownerView.isDestroyingSubtree = false;
+    // If the destroyingSubtreeForView property is not set but we have an
+    // associated render node, it means this view is being destroyed from user
+    // code and not via a change in the templating layer (like an {{if}}
+    // becoming falsy, for example).  In this case, it is our responsibility to
+    // make sure that any render nodes created as part of the rendering process
+    // are cleaned up.
+    if (!this.ownerView._destroyingSubtreeForView && this._renderNode) {
+      Ember.assert('BUG: Render node exists without concomitant env.', this.ownerView.env);
+      internal.clearMorph(this._renderNode, this.ownerView.env, true);
     }
 
     return this;

packages/ember-views/tests/views/view/child_views_test.js

@@ -1,4 +1,5 @@
 import run from 'ember-metal/run_loop';
+import Ember from 'ember-metal/core';
 import EmberView from 'ember-views/views/view';
 import Component from 'ember-views/views/component';
 import { compile } from 'ember-template-compiler';