Bug 552110 - Use of deleted object by NodeIterator using NodeFilter which called detach. r=sicking sr=jst

content/base/src/nsNodeIterator.cpp

@@ -283,34 +283,18 @@ NS_IMETHODIMP nsNodeIterator::GetExpandEntityReferences(PRBool *aExpandEntityRef
 /* nsIDOMNode nextNode ()  raises (DOMException); */
 NS_IMETHODIMP nsNodeIterator::NextNode(nsIDOMNode **_retval)
 {
-    nsresult rv;
-    PRInt16 filtered;
-
-    *_retval = nsnull;
-
-    if (mDetached)
-        return NS_ERROR_DOM_INVALID_STATE_ERR;
-
-    mWorkingPointer = mPointer;
-
-    while (mWorkingPointer.MoveToNext(mRoot)) {
-        nsCOMPtr<nsINode> testNode = mWorkingPointer.mNode;
-        rv = TestNode(testNode, &filtered);
-        NS_ENSURE_SUCCESS(rv, rv);
-
-        if (filtered == nsIDOMNodeFilter::FILTER_ACCEPT) {
-            mPointer = mWorkingPointer;
-            mWorkingPointer.Clear();
-            return CallQueryInterface(testNode, _retval);
-        }
-    }
-
-    mWorkingPointer.Clear();
-    return NS_OK;
+    return NextOrPrevNode(&NodePointer::MoveToNext, _retval);
 }
 
 /* nsIDOMNode previousNode ()  raises (DOMException); */
 NS_IMETHODIMP nsNodeIterator::PreviousNode(nsIDOMNode **_retval)
+{
+    return NextOrPrevNode(&NodePointer::MoveToPrevious, _retval);
+}
+
+nsresult
+nsNodeIterator::NextOrPrevNode(NodePointer::MoveToMethodType aMove,
+                               nsIDOMNode **_retval)
 {
     nsresult rv;
     PRInt16 filtered;
@@ -322,19 +306,26 @@ NS_IMETHODIMP nsNodeIterator::PreviousNode(nsIDOMNode **_retval)
 
     mWorkingPointer = mPointer;
 
-    while (mWorkingPointer.MoveToPrevious(mRoot)) {
+    struct AutoClear {
+        NodePointer* mPtr;
+        AutoClear(NodePointer* ptr) : mPtr(ptr) {}
+       ~AutoClear() { mPtr->Clear(); }
+    } ac(&mWorkingPointer);
+
+    while ((mWorkingPointer.*aMove)(mRoot)) {
         nsCOMPtr<nsINode> testNode = mWorkingPointer.mNode;
         rv = TestNode(testNode, &filtered);
         NS_ENSURE_SUCCESS(rv, rv);
 
+        if (mDetached)
+            return NS_ERROR_DOM_INVALID_STATE_ERR;
+
         if (filtered == nsIDOMNodeFilter::FILTER_ACCEPT) {
             mPointer = mWorkingPointer;
-            mWorkingPointer.Clear();
             return CallQueryInterface(testNode, _retval);
         }
     }
 
-    mWorkingPointer.Clear();
     return NS_OK;
 }
 

content/base/src/nsNodeIterator.h

@@ -77,6 +77,7 @@ private:
         NodePointer() : mNode(nsnull) {};
         NodePointer(nsINode *aNode, PRBool aBeforeNode);
 
+        typedef PRBool (NodePointer::*MoveToMethodType)(nsINode*);
         PRBool MoveToNext(nsINode *aRoot);
         PRBool MoveToPrevious(nsINode *aRoot);
 
@@ -98,6 +99,10 @@ private:
         PRInt32 mIndexInParent;
     };
 
+    inline nsresult
+    NextOrPrevNode(NodePointer::MoveToMethodType aMove,
+                   nsIDOMNode **_retval);
+
     PRBool mDetached;
     NodePointer mPointer;
     NodePointer mWorkingPointer;

content/base/test/test_NodeIterator_basics_filters.xhtml

@@ -111,6 +111,71 @@
   while (node = iterator.previousNode())
     found.unshift(node.nodeType);
   compare_arrays(expect_f, found, 'filtered backward');
+
+  function checkBadFilter(method, n) {
+    var iterator =
+      document.createNodeIterator(document, NodeFilter.SHOW_ALL,
+                                  function() {
+                                    if (n < 0)
+                                      iterator.detach();
+                                    return NodeFilter.FILTER_ACCEPT;
+                                  }, false);
+    while (--n >= 0)
+      iterator.nextNode();
+    try {
+      iterator[method]();
+      ok(false, "Able to call " + method + " on a detached NodeIterator");
+    } catch (x) { ok(true, x) }
+  }
+  checkBadFilter("nextNode", 2);
+  checkBadFilter("previousNode", 3);
+
+  (function() {
+    // Implementing the scenario outlined in
+    // http://bugzilla.mozilla.org/show_bug.cgi?id=552110#c4
+
+    var iterator = (function(filter) {
+      var grandparent = document.createElement("div"),
+          parent = document.createElement("span");
+
+      grandparent.appendChild(parent);
+      parent.appendChild(document.createElement("img"));
+      parent.appendChild(document.createElement("p"));
+
+      return document.createNodeIterator(grandparent,
+                                         NodeFilter.SHOW_ALL,
+                                         filter,
+                                         false);
+    })(function filter(n) {
+      if (n.nodeName != "img")
+        return NodeFilter.FILTER_ACCEPT;
+
+      iterator.detach();
+
+      n.parentNode.parentNode.removeChild(n.parentNode);
+      // Drop any node references passed into this function.
+      for (var i = 0; i < arguments.length; ++i)
+        arguments[i] = null;
+      ok(!n, "arguments[0] = null should have nulled out n");
+
+      // Try to trigger GC.
+      var xhr = new XMLHttpRequest();
+      xhr.open("GET", location.href, false);
+      xhr.send();
+
+      return NodeFilter.FILTER_SKIP;
+    });
+
+    is(iterator.nextNode().nodeName, "div",
+       "iterator.nextNode() returned the wrong node");
+    is(iterator.nextNode().nodeName, "span",
+       "iterator.nextNode() returned the wrong node");
+    try {
+      var p = iterator.nextNode();
+      ok(false, "iterator.nextNode() should have thrown, but instead it returned <" + p.nodeName + ">");
+    } catch (x) { ok(true, x) }
+  })();
+        
 ]]></script>
 </pre>
 </body>