Merge pull request #14159 from emberjs/fix-child-views-leak

[BUGFIX beta] Fix cleanup
emberjs · Aug 30, 2016 · 2dbde63 · 2dbde63
2 parents dd60011 + 08803e3
commit 2dbde63
Show file tree

Hide file tree

Showing 10 changed files with 223 additions and 49 deletions.
diff --git a/packages/ember-glimmer/tests/integration/components/curly-components-test.js b/packages/ember-glimmer/tests/integration/components/curly-components-test.js
@@ -2470,4 +2470,74 @@ moduleFor('Components test: curly components', class extends RenderingTest {
       attrs: { id: 'foo-bar',  style: styles('color: blue; display: none;') }
     });
   }
+
+  ['@test child triggers revalidate during parent destruction (GH#13846)']() {
+    let select;
+
+    this.registerComponent('x-select', {
+      ComponentClass: Component.extend({
+        tagName: 'select',
+
+        init() {
+          this._super();
+          this.options = emberA([]);
+          this.value = null;
+
+          select = this;
+        },
+
+        updateValue() {
+          var newValue = this.get('options.lastObject.value');
+
+          this.set('value', newValue);
+        },
+
+        registerOption(option) {
+          this.get('options').addObject(option);
+        },
+
+        unregisterOption(option) {
+          this.get('options').removeObject(option);
+
+          this.updateValue();
+        }
+      }),
+
+      template: '{{yield this}}'
+    });
+
+    this.registerComponent('x-option', {
+      ComponentClass: Component.extend({
+        tagName: 'option',
+        attributeBindings: ['selected'],
+
+        didInsertElement() {
+          this._super(...arguments);
+
+          this.get('select').registerOption(this);
+        },
+
+        selected: computed('select.value', function() {
+          return this.get('value') === this.get('select.value');
+        }),
+
+        willDestroyElement() {
+          this._super(...arguments);
+          this.get('select').unregisterOption(this);
+        }
+      })
+    });
+
+    this.render(strip`
+      {{#x-select value=value as |select|}}
+        {{#x-option value="1" select=select}}1{{/x-option}}
+        {{#x-option value="2" select=select}}2{{/x-option}}
+      {{/x-select}}
+    `);
+
+
+    this.teardown();
+
+    this.assert.ok(true, 'no errors during teardown');
+  }
 });
diff --git a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js
@@ -51,17 +51,19 @@ class LifeCycleHooksTest extends RenderingTest {
     };
 
     let assertParentView = (hookName, instance) => {
-      if (!instance.parentView) {
-        this.assert.ok(false, `parentView should be present in ${hookName}`);
+      this.assert.ok(instance.parentView, `parentView should be present in ${hookName}`);
+      if (hookName === 'willDestroyElement') {
+        this.assert.ok(instance.parentView.childViews.indexOf(instance) !== -1, `view is still connected to parentView in ${hookName}`);
+      }
+      if (this.isHTMLBars) {
+        this.assert.ok(instance.ownerView, `ownerView should be present in ${hookName}`);
       }
     };
 
     let assertElement = (hookName, instance) => {
       if (instance.tagName === '') { return; }
 
-      if (!instance.element) {
-        this.assert.ok(false, `element property should be present on ${instance} during ${hookName}`);
-      }
+      this.assert.ok(instance.element && document.body.contains(instance.element), `element property should be present on ${instance} during ${hookName}`);
 
       let inDOM = this.$(`#${instance.elementId}`)[0];
       if (!inDOM) {
@@ -134,6 +136,14 @@ class LifeCycleHooksTest extends RenderingTest {
         pushHook('willDestroyElement');
         assertParentView('willDestroyElement', this);
         assertElement('willDestroyElement', this);
+      },
+
+      didDestroyElement() {
+        pushHook('didDestroyElement');
+      },
+
+      willDestroy() {
+        pushHook('willDestroy');
       }
     });
 
@@ -387,8 +397,26 @@ class LifeCycleHooksTest extends RenderingTest {
         'destroy',
         ['the-top', 'willDestroyElement'],
         ['the-middle', 'willDestroyElement'],
-        ['the-bottom', 'willDestroyElement']
+        ['the-bottom', 'willDestroyElement'],
+        ['the-top', 'didDestroyElement'],
+        ['the-middle', 'didDestroyElement'],
+        ['the-bottom', 'didDestroyElement'],
+        ['the-top', 'willDestroy'],
+        ['the-middle', 'willDestroy'],
+        ['the-bottom', 'willDestroy']
       );
+
+      this.assert.equal(this.components['the-top']._state, 'preRender');
+      this.assert.equal(this.components['the-top'].isDestroying, true);
+      this.assert.equal(this.components['the-top'].isDestroyed, true);
+
+      this.assert.equal(this.components['the-middle']._state, 'preRender');
+      this.assert.equal(this.components['the-middle'].isDestroying, true);
+      this.assert.equal(this.components['the-middle'].isDestroyed, true);
+
+      this.assert.equal(this.components['the-bottom']._state, 'preRender');
+      this.assert.equal(this.components['the-bottom'].isDestroying, true);
+      this.assert.equal(this.components['the-bottom'].isDestroyed, true);
     });
   }
 
@@ -559,20 +587,42 @@ class LifeCycleHooksTest extends RenderingTest {
         'destroy',
         ['the-top', 'willDestroyElement'],
         ['the-middle', 'willDestroyElement'],
-        ['the-bottom', 'willDestroyElement']
+        ['the-bottom', 'willDestroyElement'],
+        ['the-top', 'didDestroyElement'],
+        ['the-middle', 'didDestroyElement'],
+        ['the-bottom', 'didDestroyElement'],
+        ['the-top', 'willDestroy'],
+        ['the-middle', 'willDestroy'],
+        ['the-bottom', 'willDestroy']
       );
+
+      this.assert.equal(this.components['the-top']._state, 'preRender');
+      this.assert.equal(this.components['the-top'].isDestroying, true);
+      this.assert.equal(this.components['the-top'].isDestroyed, true);
+
+      this.assert.equal(this.components['the-middle']._state, 'preRender');
+      this.assert.equal(this.components['the-middle'].isDestroying, true);
+      this.assert.equal(this.components['the-middle'].isDestroyed, true);
+
+      this.assert.equal(this.components['the-bottom']._state, 'preRender');
+      this.assert.equal(this.components['the-bottom'].isDestroying, true);
+      this.assert.equal(this.components['the-bottom'].isDestroyed, true);
     });
   }
 
   ['@test components rendered from `{{each}}` have correct life-cycle hooks to be called']() {
     let { invoke } = this.boundHelpers;
 
+    this.registerComponent('nested-item', { template: strip`
+      {{yield}}
+    ` });
+
     this.registerComponent('an-item', { template: strip`
-      <div>Item: {{count}}</div>
+      {{#nested-item}}Item: {{count}}{{/nested-item}}
     ` });
 
     this.registerComponent('no-items', { template: strip`
-      <div>Nothing to see here</div>
+      {{#nested-item}}<div>Nothing to see here</div>{{/nested-item}}
     ` });
 
     this.render(strip`
@@ -592,12 +642,18 @@ class LifeCycleHooksTest extends RenderingTest {
         ['an-item', 'init'],
         ['an-item', 'didInitAttrs',       { attrs: { count } }],
         ['an-item', 'didReceiveAttrs',    { newAttrs: { count } }],
-        ['an-item', 'willRender']
+        ['an-item', 'willRender'],
+        ['nested-item', 'init'],
+        ['nested-item', 'didInitAttrs',       { attrs: { } }],
+        ['nested-item', 'didReceiveAttrs',    { newAttrs: { } }],
+        ['nested-item', 'willRender']
       ];
     };
 
     let initialAfterRenderHooks = (count) => {
       return [
+        ['nested-item', 'didInsertElement'],
+        ['nested-item', 'didRender'],
         ['an-item', 'didInsertElement'],
         ['an-item', 'didRender']
       ];
@@ -622,34 +678,81 @@ class LifeCycleHooksTest extends RenderingTest {
         ...initialAfterRenderHooks(1)
     );
 
+    this.assert.equal(this.component.childViews.length, 5, 'childViews precond');
     this.runTask(() => set(this.context, 'items', []));
+    this.assert.equal(this.component.childViews.length, 1, 'childViews updated');
 
     this.assertText('Nothing to see here');
 
     this.assertHooks(
       'reset to empty array',
 
       ['an-item', 'willDestroyElement'],
+      ['nested-item', 'willDestroyElement'],
+      ['an-item', 'didDestroyElement'],
+      ['nested-item', 'didDestroyElement'],
       ['an-item', 'willDestroyElement'],
+      ['nested-item', 'willDestroyElement'],
+      ['an-item', 'didDestroyElement'],
+      ['nested-item', 'didDestroyElement'],
       ['an-item', 'willDestroyElement'],
+      ['nested-item', 'willDestroyElement'],
+      ['an-item', 'didDestroyElement'],
+      ['nested-item', 'didDestroyElement'],
       ['an-item', 'willDestroyElement'],
+      ['nested-item', 'willDestroyElement'],
+      ['an-item', 'didDestroyElement'],
+      ['nested-item', 'didDestroyElement'],
       ['an-item', 'willDestroyElement'],
+      ['nested-item', 'willDestroyElement'],
+      ['an-item', 'didDestroyElement'],
+      ['nested-item', 'didDestroyElement'],
 
       ['no-items', 'init'],
       ['no-items', 'didInitAttrs',       { attrs: { } }],
       ['no-items', 'didReceiveAttrs',    { newAttrs: { } }],
       ['no-items', 'willRender'],
 
+      ['nested-item', 'init'],
+      ['nested-item', 'didInitAttrs',       { attrs: { } }],
+      ['nested-item', 'didReceiveAttrs',    { newAttrs: { } }],
+      ['nested-item', 'willRender'],
+
+      ['nested-item', 'didInsertElement'],
+      ['nested-item', 'didRender'],
       ['no-items', 'didInsertElement'],
-      ['no-items', 'didRender']
+      ['no-items', 'didRender'],
+
+      ['an-item', 'willDestroy'],
+      ['nested-item', 'willDestroy'],
+      ['an-item', 'willDestroy'],
+      ['nested-item', 'willDestroy'],
+      ['an-item', 'willDestroy'],
+      ['nested-item', 'willDestroy'],
+      ['an-item', 'willDestroy'],
+      ['nested-item', 'willDestroy'],
+      ['an-item', 'willDestroy'],
+      ['nested-item', 'willDestroy']
     );
 
     this.teardownAssertions.push(() => {
       this.assertHooks(
         'destroy',
-
-        ['no-items', 'willDestroyElement']
+        ['no-items', 'willDestroyElement'],
+        ['nested-item', 'willDestroyElement'],
+        ['no-items', 'didDestroyElement'],
+        ['nested-item', 'didDestroyElement'],
+        ['no-items', 'willDestroy'],
+        ['nested-item', 'willDestroy']
       );
+
+      this.assert.equal(this.components['no-items']._state, 'preRender');
+      this.assert.equal(this.components['no-items'].isDestroying, true);
+      this.assert.equal(this.components['no-items'].isDestroyed, true);
+
+      this.assert.equal(this.components['nested-item']._state, 'preRender');
+      this.assert.equal(this.components['nested-item'].isDestroying, true);
+      this.assert.equal(this.components['nested-item'].isDestroyed, true);
     });
   }
 }

diff --git a/packages/ember-htmlbars/lib/hooks/cleanup-render-node.js b/packages/ember-htmlbars/lib/hooks/cleanup-render-node.js
@@ -4,5 +4,25 @@
 */
 
 export default function cleanupRenderNode(renderNode) {
+  let view = renderNode.emberView;
+  if (view) {
+    view.renderer.willDestroyElement(view);
+    view.ownerView._destroyingSubtreeForView.push(env => {
+      view._transitionTo('destroying'); // unregisters view
+      // prevents rerender and scheduling
+      view._renderNode = null;
+      renderNode.emberView = null;
+
+      view.renderer.didDestroyElement(view);
+
+      if (view.parentView && view.parentView === env.view) {
+        view.parentView.removeChild(view);
+      }
+      view.parentView = null;
+
+      view._transitionTo('preRender');
+    });
+  }
+
   if (renderNode.cleanup) { renderNode.cleanup(); }
 }
diff --git a/packages/ember-htmlbars/lib/hooks/destroy-render-node.js b/packages/ember-htmlbars/lib/hooks/destroy-render-node.js
@@ -5,13 +5,15 @@
 export default function destroyRenderNode(renderNode) {
   let view = renderNode.emberView;
   if (view) {
-    view.renderer.remove(view, true);
+    view.ownerView._destroyingSubtreeForView.push(() => {
+      view.destroy();
+    });
   }
-
   let streamUnsubscribers = renderNode.streamUnsubscribers;
   if (streamUnsubscribers) {
     for (let i = 0; i < streamUnsubscribers.length; i++) {
       streamUnsubscribers[i]();
     }
   }
+  renderNode.streamUnsubscribers = null;
 }
diff --git a/packages/ember-htmlbars/lib/hooks/did-cleanup-tree.js b/packages/ember-htmlbars/lib/hooks/did-cleanup-tree.js
@@ -1,5 +1,9 @@
 export default function didCleanupTree(env) {
   // Once we have finsihed cleaning up the render node and sub-nodes, reset
   // state tracking which view those render nodes belonged to.
+  let queue = env.view.ownerView._destroyingSubtreeForView;
+  for (let i = 0; i < queue.length; i++) {
+    queue[i](env);
+  }
   env.view.ownerView._destroyingSubtreeForView = null;
 }
diff --git a/packages/ember-htmlbars/lib/hooks/will-cleanup-tree.js b/packages/ember-htmlbars/lib/hooks/will-cleanup-tree.js
@@ -1,5 +1,5 @@
 export default function willCleanupTree(env) {
-  let view = env.view;
+  let { view } = env;
 
   // 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
@@ -28,5 +28,5 @@ export default function willCleanupTree(env) {
   // 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;
+  view.ownerView._destroyingSubtreeForView = [];
 }
diff --git a/packages/ember-htmlbars/lib/keywords/element-component.js b/packages/ember-htmlbars/lib/keywords/element-component.js
@@ -19,7 +19,7 @@ export default {
     });
   },
 
-  render(morph, ...rest) {
+  render(morph) {
     let state = morph.getState();
 
     if (state.manager) {
@@ -31,7 +31,7 @@ export default {
     // but the `{{component}}` helper can.
     state.manager = null;
 
-    render(morph, ...rest);
+    render(...arguments);
   },
 
   rerender: render

diff --git a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js
@@ -151,13 +151,10 @@ ComponentNodeManager.prototype.rerender = function ComponentNodeManager_rerender
 };
 
 ComponentNodeManager.prototype.destroy = function ComponentNodeManager_destroy() {
-  let component = this.component;
-
   // Clear component's render node. Normally this gets cleared
   // during view destruction, but in this case we're re-assigning the
   // node to a different view and it will get cleaned up automatically.
-  component._renderNode = null;
-  component.destroy();
+  this.component.destroy();
 };
 
 export function createComponent(_component, props, renderNode, env, attrs = {}) {