Fixes #1740: Don't clean nodes that are removed during the clean proc…

…ess. This fix works because the HTMLCollection returned by getElementsByTagName is live and will reflect nodes added or removed after it's run. It's not okay to add/remove nodes before the one being cleaned, so doing so will throw an exception.
knockout · Dec 12, 2016 · d781ab5 · d781ab5
1 parent ac30244
commit d781ab5
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 5 deletions.
diff --git a/spec/domNodeDisposalBehaviors.js b/spec/domNodeDisposalBehaviors.js
@@ -48,6 +48,44 @@ describe('DOM node disposal', function() {
         expect(didRun).toEqual(false); // Didn't run only because we removed it
     });
 
+    it('Should not clean descendant nodes that are removed by a parent dispose handler', function() {
+        var childNode = document.createElement("DIV");
+        var grandChildNode = document.createElement("DIV");
+        var childSpy = jasmine.createSpy('childSpy')
+            .andCallFake(function() {
+                childNode.removeChild(grandChildNode);
+            });
+        var grandChildSpy = jasmine.createSpy('grandChildSpy');
+
+        testNode.appendChild(childNode);
+        childNode.appendChild(grandChildNode);
+        ko.utils.domNodeDisposal.addDisposeCallback(childNode, childSpy);
+        ko.utils.domNodeDisposal.addDisposeCallback(grandChildNode, grandChildSpy);
+
+        ko.cleanNode(testNode);
+        expect(childSpy).toHaveBeenCalledWith(childNode);
+        expect(grandChildSpy).not.toHaveBeenCalled();
+    });
+
+    it('Should throw an error if a cleaned node is removed in a handler', function() {
+        // Test by removing the node itself
+        var childNode = document.createElement("DIV");
+        testNode.appendChild(childNode);
+        ko.utils.domNodeDisposal.addDisposeCallback(childNode, function() {
+            testNode.removeChild(childNode);
+        });
+        expect(function() { ko.cleanNode(testNode); }).toThrowContaining("cleaned node was removed");
+
+        // Test by removing a previous node
+        var childNode2 = document.createElement("DIV");
+        testNode.appendChild(childNode);
+        testNode.appendChild(childNode2);
+        ko.utils.domNodeDisposal.addDisposeCallback(childNode2, function() {
+            testNode.removeChild(childNode);
+        });
+        expect(function() { ko.cleanNode(testNode); }).toThrowContaining("cleaned node was removed");
+    });
+
     it('Should be able to attach disposal callback to a node that has been cloned', function() {
         // This represents bug https://github.com/SteveSanderson/knockout/issues/324
         // IE < 9 copies expando properties when cloning nodes, so if the node already has some DOM data associated with it,

diff --git a/src/utils.domNodeDisposal.js b/src/utils.domNodeDisposal.js
@@ -69,11 +69,14 @@ ko.utils.domNodeDisposal = new (function () {
 
                 // ... then its descendants, where applicable
                 if (cleanableNodeTypesWithDescendants[node.nodeType]) {
-                    // Clone the descendants list in case it changes during iteration
-                    var descendants = [];
-                    ko.utils.arrayPushAll(descendants, node.getElementsByTagName("*"));
-                    for (var i = 0, j = descendants.length; i < j; i++)
-                        cleanSingleNode(descendants[i]);
+                    var descendants = node.getElementsByTagName("*");
+                    var cleanedNode;
+                    for (var i = 0; i < descendants.length; i++) {
+                        cleanSingleNode(cleanedNode = descendants[i]);
+                        if (descendants[i] !== cleanedNode) {
+                            throw Error("ko.cleanNode: An already cleaned node was removed from the document");
+                        }
+                    }
                 }
             }
             return node;