Fixes disposed components with ref not being removed from owners

packages/metal-incremental-dom/src/IncrementalDomRenderer.js

@@ -163,13 +163,22 @@ class IncrementalDomRenderer extends ComponentRenderer {
 	 */
 	disposeInternal() {
 		super.disposeInternal();
+
+		var comp = this.component_;
+		var ref = this.config_.ref;
+		var owner = this.getOwner();
+		if (owner && owner.components && owner.components[ref] === comp) {
+			delete owner.components[ref];
+		}
+
 		for (var i = 0; i < this.childComponents_.length; i++) {
 			const child = this.childComponents_[i];
 			if (!child.isDisposed()) {
 				child.element = null;
 				child.dispose();
 			}
 		}
+		this.childComponents_ = null;
 	}
 
 	/**

packages/metal-incremental-dom/src/cleanup/IncrementalDomUnusedComponents.js

@@ -1,33 +1,30 @@
 'use strict';
 
 var comps_ = [];
+var disposing_ = false;
 
 class IncrementalDomUnusedComponents {
 	/**
 	 * Disposes all sub components that were not rerendered since the last
 	 * time this function was scheduled.
 	 */
 	static disposeUnused() {
+		if (disposing_) {
+			return;
+		}
+		disposing_ = true;
+
 		for (var i = 0; i < comps_.length; i++) {
 			var comp = comps_[i];
-			if (!comp.isDisposed()) {
-				var renderer = comp.getRenderer();
-				if (!renderer.getParent()) {
-					// Don't let disposing cause the element to be removed, since it may
-					// be currently being reused by another component.
-					comp.element = null;
-
-					var ref = renderer.config_.ref;
-					var owner = renderer.getOwner();
-					if (owner && !owner.isDisposed() && owner.components[ref] === comp) {
-						owner.disposeSubComponents([ref]);
-					} else {
-						comp.dispose();
-					}
-				}
+			if (!comp.isDisposed() && !comp.getRenderer().getParent()) {
+				// Don't let disposing cause the element to be removed, since it may
+				// be currently being reused by another component.
+				comp.element = null;
+				comp.dispose();
 			}
 		}
 		comps_ = [];
+		disposing_ = false;
 	}
 
 	/**
@@ -37,8 +34,10 @@ class IncrementalDomUnusedComponents {
 	 */
 	static schedule(comps) {
 		for (var i = 0; i < comps.length; i++) {
-			comps[i].getRenderer().parent_ = null;
-			comps_.push(comps[i]);
+			if (!comps[i].isDisposed()) {
+				comps[i].getRenderer().parent_ = null;
+				comps_.push(comps[i]);
+			}
 		}
 	}
 }

packages/metal-incremental-dom/test/IncrementalDomRenderer.js

@@ -2575,6 +2575,50 @@ describe('IncrementalDomRenderer', function() {
 		});
 	});
 
+	describe('Dispose', function() {
+		it('should remove component with ref from owner', function() {
+			class Child extends Component {
+			}
+			Child.RENDERER = IncrementalDomRenderer;
+
+			class TestComponent extends Component {
+				render() {
+					IncrementalDOM.elementVoid(Child, null, null, 'ref', 'child');
+				}
+			}
+			TestComponent.RENDERER = IncrementalDomRenderer;
+
+			component = new TestComponent();
+			var child = component.components.child;
+
+			child.dispose();
+			assert.ok(child.isDisposed());
+			assert.ok(!component.components.child);
+		});
+
+		it('should not remove different component with same ref from owner', function() {
+			class Child extends Component {
+			}
+			Child.RENDERER = IncrementalDomRenderer;
+
+			class TestComponent extends Component {
+				render() {
+					IncrementalDOM.elementVoid(Child, null, null, 'ref', 'child');
+				}
+			}
+			TestComponent.RENDERER = IncrementalDomRenderer;
+
+			component = new TestComponent();
+			var child = component.components.child;
+			component.components.child = new Child();
+
+			child.dispose();
+			assert.ok(child.isDisposed());
+			assert.ok(component.components.child);
+			assert.ok(!component.components.child.isDisposed());
+		});
+	});
+
 	describe('IncrementalDomRenderer.isIncDomNode', function() {
 		it('should check if given data is an incremental dom node', function() {
 			assert.ok(!IncrementalDomRenderer.isIncDomNode({}));

packages/metal-incremental-dom/test/cleanup/IncrementalDomUnusedComponents.js

@@ -50,7 +50,7 @@ describe('IncrementalDomUnusedComponents', function() {
 		assert.ok(comps[1].isDisposed());
 	});
 
-	it('should not dispose scheduled components that have parent again', function() {
+	it('should not dispose scheduled components that have received a new parent', function() {
 		var comps = [comp.components.child1, comp.components.child2];
 		IncrementalDomUnusedComponents.schedule(comps);
 		comps[0].getRenderer().parent_ = comp;
@@ -60,10 +60,10 @@ describe('IncrementalDomUnusedComponents', function() {
 		assert.ok(comps[1].isDisposed());
 	});
 
-	it('should not dispose scheduled components that have already been disposed', function() {
+	it('should not dispose scheduled components that have been disposed before scheduled', function() {
 		var comps = [comp.components.child1, comp.components.child2];
-		IncrementalDomUnusedComponents.schedule(comps);
 		comps[0].dispose();
+		IncrementalDomUnusedComponents.schedule(comps);
 		sinon.spy(comps[0], 'dispose');
 		sinon.spy(comps[1], 'dispose');
 		IncrementalDomUnusedComponents.disposeUnused();
@@ -72,22 +72,31 @@ describe('IncrementalDomUnusedComponents', function() {
 		assert.strictEqual(1, comps[1].dispose.callCount);
 	});
 
-	it('should not dispose different component with same ref as a scheduled component', function() {
+	it('should not dispose scheduled components that have been disposed after scheduled', function() {
 		var comps = [comp.components.child1, comp.components.child2];
 		IncrementalDomUnusedComponents.schedule(comps);
+		comps[0].dispose();
+		sinon.spy(comps[0], 'dispose');
+		sinon.spy(comps[1], 'dispose');
+		IncrementalDomUnusedComponents.disposeUnused();
+
+		assert.strictEqual(0, comps[0].dispose.callCount);
+		assert.strictEqual(1, comps[1].dispose.callCount);
+	});
 
-		comp.addSubComponent('child1', new Component());
-		var newChild1 = comp.components.child1;
-		assert.notStrictEqual(comps[0], newChild1);
+	it('should not throw error when `disposeUnused` is called during another `disposeUnused` call', function() {
+		sinon.stub(comp.components.child1, 'disposed', () => {
+			IncrementalDomUnusedComponents.disposeUnused();
+		});
 
-		IncrementalDomUnusedComponents.disposeUnused();
+		var comps = [comp.components.child1];
+		IncrementalDomUnusedComponents.schedule(comps);
 
+		assert.doesNotThrow(() => IncrementalDomUnusedComponents.disposeUnused());
 		assert.ok(comps[0].isDisposed());
-		assert.ok(comps[1].isDisposed());
-		assert.ok(!newChild1.isDisposed());
 	});
 
-	it('should not throw error when disposing component that has previously disposed owner', function() {
+	it('should not throw error when disposing component that has an owner that was previously disposed', function() {
 		var comps = [comp.components.child2, grandchild];
 		IncrementalDomUnusedComponents.schedule(comps);
 		assert.doesNotThrow(() => IncrementalDomUnusedComponents.disposeUnused());