Merge pull request #11667 from emberjs/stefanpenner-memory-leak

[BUGFIX release] Fix Glimmer memory leak

packages/ember-htmlbars/lib/hooks/bind-self.js

@@ -17,7 +17,6 @@ export default function bindSelf(env, scope, _self) {
   }
 
   if (self && self.isView) {
-    scope.view = self;
     newStream(scope.locals, 'view', self, null);
     newStream(scope.locals, 'controller', scope.locals.view.getKey('controller'));
     newStream(scope, 'self', scope.locals.view.getKey('context'), null, true);

packages/ember-htmlbars/lib/hooks/create-fresh-scope.js

@@ -1,9 +1,56 @@
+/*
+  Ember's implementation of HTMLBars creates an enriched scope.
+
+  * self: same as HTMLBars, this field represents the dynamic lookup
+    of root keys that are not special keywords or block arguments.
+  * blocks: same as HTMLBars, a bundle of named blocks the layout
+    can yield to.
+  * component: indicates that the scope is the layout of a component,
+    which is used to trigger lifecycle hooks for the component when
+    one of the streams in its layout fires.
+  * attrs: a map of key-value attributes sent in by the invoker of
+    a template, and available in the component's layout.
+  * locals: a map of locals, produced by block params (`as |a b|`)
+  * localPresent: a map of available locals to avoid expensive
+    `hasOwnProperty` checks.
+
+  The `self` field has two special meanings:
+
+  * If `self` is a view (`isView`), the actual HTMLBars `self` becomes
+    the view's `context`. This is legacy semantics; components always
+    use the component itself as the `this`.
+  * If `self` is a view, two special locals are created: `view` and
+    `controller`. These locals are legacy semantics.
+  * If self has a `hasBoundController` property, it is coming from
+    a legacy form of #with or #each
+    (`{{#with something controller=someController}}`). This has
+    the special effect of giving the child scope the supplied
+    `controller` keyword, with an unrelated `self`. This is
+    legacy functionality, as both the `view` and `controller`
+    keywords have been deprecated.
+
+  **IMPORTANT**: There are two places in Ember where the ambient
+  controller is looked up. Both of those places use the presence
+  of `scope.locals.view` to indicate that the controller lookup
+  should be dynamic off of the ambient view. If `scope.locals.view`
+  does not exist, the code assumes that it is inside of a top-level
+  template (without a view) and uses the `self` itself as the
+  controller. This means that if you remove `scope.locals.view`
+  (perhaps because we are finally ready to shed the view keyword),
+  there may be unexpected consequences on controller semantics.
+  If this happens to you, I hope you find this comment. - YK & TD
+
+  In practice, this means that with the exceptions of top-level
+  view-less templates and the legacy `controller=foo` semantics,
+  the controller hierarchy is managed dynamically by looking at
+  the current view's `controller`.
+*/
+
 export default function createFreshScope() {
   return {
     self: null,
     blocks: {},
     component: null,
-    view: null,
     attrs: null,
     locals: {},
     localPresent: {}

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/link-render-node.js

@@ -24,7 +24,7 @@ export default function linkRenderNode(renderNode, env, scope, path, params, has
       case 'if': params[0] = shouldDisplay(params[0]); break;
       case 'each': params[0] = eachParam(params[0]); break;
       default:
-        helper = findHelper(path, scope.view, env);
+        helper = findHelper(path, env.view, env);
 
         if (helper && helper.isHandlebarsCompat && params[0]) {
           params[0] = processHandlebarsCompatDepKeys(params[0], helper._dependentKeys);

packages/ember-htmlbars/lib/hooks/update-self.js

@@ -20,7 +20,6 @@ export default function updateSelf(env, scope, _self) {
   Ember.assert('BUG: scope.attrs and self.isView should not both be true', !(scope.attrs && self.isView));
 
   if (self && self.isView) {
-    scope.view = self;
     updateScope(scope.locals, 'view', self, null);
     updateScope(scope, 'self', get(self, 'context'), null, true);
     return;

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

@@ -1,10 +1,32 @@
-export default function willCleanupTree(env, morph, destroySelf) {
-  var view = morph.emberView;
-  if (destroySelf && view && view.parentView) {
-    view.parentView.removeChild(view);
-  }
+export default function willCleanupTree(env) {
+  let view = env.view;
 
-  if (view = env.view) {
-    view.ownerView.isDestroyingSubtree = true;
-  }
+  // 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/keywords/collection.js

@@ -13,7 +13,7 @@ export default {
     var read = env.hooks.getValue;
 
     return assign({}, state, {
-      parentView: read(scope.locals.view),
+      parentView: env.view,
       viewClassOrInstance: getView(read(params[0]), env.container)
     });
   },

packages/ember-htmlbars/lib/keywords/view.js

@@ -18,11 +18,11 @@ export default {
 
     // if parentView exists, use its controller (the default
     // behavior), otherwise use `scope.self` as the controller
-    var controller = scope.view ? null : read(scope.self);
+    var controller = scope.locals.view ? null : read(scope.self);
 
     return {
       manager: state.manager,
-      parentView: scope.view,
+      parentView: env.view,
       controller,
       targetObject,
       viewClassOrInstance

packages/ember-htmlbars/lib/keywords/with.js

@@ -1,6 +1,7 @@
 import Ember from 'ember-metal/core';
 import { get } from 'ember-metal/property_get';
 import { internal } from 'htmlbars-runtime';
+import { read } from 'ember-metal/streams/utils';
 
 export default {
   setupState(state, env, scope, params, hash) {
@@ -10,7 +11,13 @@ export default {
       if (!state.controller) {
         var context = params[0];
         var controllerFactory = env.container.lookupFactory('controller:' + controller);
-        var parentController = scope.view ? get(scope.view, 'context') : null;
+        var parentController = null;
+
+        if (scope.locals.controller) {
+          parentController = read(scope.locals.controller);
+        } else if (scope.locals.view) {
+          parentController = get(read(scope.locals.view), 'context');
+        }
 
         var controllerInstance = controllerFactory.create({
           model: env.hooks.getValue(context),

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-routing-htmlbars/lib/keywords/render.js

@@ -24,7 +24,7 @@ export default {
     );
 
     return {
-      parentView: scope.view,
+      parentView: env.view,
       manager: prevState.manager,
       controller: prevState.controller,
       childOutletState: childOutletState(name, env)

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,5 +1,7 @@
 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';
 
 var parentView, childView;
@@ -68,3 +70,85 @@ QUnit.test('should not duplicate childViews when rerendering', function() {
     outerView.destroy();
   });
 });
+
+QUnit.test('should remove childViews inside {{if}} on destroy', function() {
+  var outerView = EmberView.extend({
+    component: 'my-thing',
+    value: false,
+    container: {
+      lookup() {
+        return {
+          componentFor() {
+            return Component.extend();
+          },
+
+          layoutFor() {
+            return null;
+          }
+        };
+      }
+    },
+    template: compile(`
+      {{#if view.value}}
+        {{component view.component value=view.value}}
+      {{/if}}
+    `)
+  }).create();
+
+  run(outerView, 'append');
+  run(outerView, 'set', 'value', true);
+
+  equal(outerView.get('childViews.length'), 1);
+
+  run(outerView, 'set', 'value', false);
+
+  equal(outerView.get('childViews.length'), 0, 'expected no views to be leaked');
+});
+
+QUnit.test('should remove childViews inside {{each}} on destroy', function() {
+  var outerView = EmberView.extend({
+    component: 'my-thing',
+    init() {
+      this._super(...arguments);
+      this.value = false;
+    },
+    container: {
+      lookup() {
+        return {
+          componentFor() {
+            return Component.extend();
+          },
+
+          layoutFor() {
+            return null;
+          }
+        };
+      }
+    },
+    template: compile(`
+      {{#if view.value}}
+        {{#each view.data as |item|}}
+          {{component view.component value=item.value}}
+        {{/each}}
+      {{/if}}
+    `)
+  }).create();
+
+  run(outerView, 'append');
+
+  equal(outerView.get('childViews.length'), 0);
+
+  run(outerView, 'set', 'data', Ember.A([
+    { id: 1, value: new Date() },
+    { id: 2, value: new Date() }
+  ]));
+
+  equal(outerView.get('childViews.length'), 0);
+
+  run(outerView, 'set', 'value', true);
+  equal(outerView.get('childViews.length'), 2);
+
+  run(outerView, 'set', 'value', false);
+
+  equal(outerView.get('childViews.length'), 0, 'expected no views to be leaked');
+});