Bug 552110 - Use of deleted object by NodeIterator using NodeFilter which called detach. r=sicking sr=jst
authorBen Newman <bnewman@mozilla.com>
Fri, 09 Apr 2010 18:36:40 -0700
changeset 40639 e6498322182fa5bb20c9e75edf9104e3b8b17dea
parent 40638 91694d19d7b290025971bce9abcadd68cca038e2
child 40640 f414fcdd192d9521a892fa792f8678489d12362c
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssicking, jst
bugs552110
milestone1.9.3a5pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
Bug 552110 - Use of deleted object by NodeIterator using NodeFilter which called detach. r=sicking sr=jst
content/base/src/nsNodeIterator.cpp
content/base/src/nsNodeIterator.h
content/base/test/test_NodeIterator_basics_filters.xhtml
--- a/content/base/src/nsNodeIterator.cpp
+++ b/content/base/src/nsNodeIterator.cpp
@@ -278,68 +278,59 @@ NS_IMETHODIMP nsNodeIterator::GetExpandE
 {
     *aExpandEntityReferences = mExpandEntityReferences;
     return NS_OK;
 }
 
 /* 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;
 
     *_retval = nsnull;
 
     if (mDetached)
         return NS_ERROR_DOM_INVALID_STATE_ERR;
 
     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;
 }
 
 /* void detach (); */
 NS_IMETHODIMP nsNodeIterator::Detach(void)
 {
     if (!mDetached) {
         mRoot->RemoveMutationObserver(this);
--- a/content/base/src/nsNodeIterator.h
+++ b/content/base/src/nsNodeIterator.h
@@ -72,16 +72,17 @@ public:
 
     NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsNodeIterator, nsIDOMNodeIterator)
 
 private:
     struct NodePointer {
         NodePointer() : mNode(nsnull) {};
         NodePointer(nsINode *aNode, PRBool aBeforeNode);
 
+        typedef PRBool (NodePointer::*MoveToMethodType)(nsINode*);
         PRBool MoveToNext(nsINode *aRoot);
         PRBool MoveToPrevious(nsINode *aRoot);
 
         PRBool MoveForward(nsINode *aRoot, nsINode *aParent, PRInt32 aChildNum);
         void MoveBackward(nsINode *aParent, PRInt32 aChildNum);
 
         void AdjustAfterInsertion(nsINode *aRoot, nsINode *aContainer, PRInt32 aIndexInContainer);
         void AdjustAfterRemoval(nsINode *aRoot, nsINode *aContainer, nsIContent *aChild, PRInt32 aIndexInContainer);
@@ -93,14 +94,18 @@ private:
         // points to the root
         nsINode *mNodeParent;
         PRBool mBeforeNode;
         // mNode's index in mNodeParent. Uninitialized if mNodeParent is null
         // or dangling (per above comment).
         PRInt32 mIndexInParent;
     };
 
+    inline nsresult
+    NextOrPrevNode(NodePointer::MoveToMethodType aMove,
+                   nsIDOMNode **_retval);
+
     PRBool mDetached;
     NodePointer mPointer;
     NodePointer mWorkingPointer;
 };
 
 #endif
--- a/content/base/test/test_NodeIterator_basics_filters.xhtml
+++ b/content/base/test/test_NodeIterator_basics_filters.xhtml
@@ -106,12 +106,77 @@
     found.push(node.nodeType);
   compare_arrays(expect_f, found, 'filtered forward');
 
   // backwards
   found.length = 0;
   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>
 </html>