Bug 1500956 part 3. Fix our DOM pre-insertion checks to actually match the spec. r=smaug
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 23 Oct 2018 00:32:01 +0200
changeset 490726 bbb8bac97270807cf396a7d0c33a777e6a57d4d0
parent 490725 5c6566236a81d93dd1f41aec87b2dc56fbf5c8c9
child 490727 ff8fb091198ee6f4c86ffd4625089dd9751630a0
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewerssmaug
bugs1500956
milestone65.0a1
Bug 1500956 part 3. Fix our DOM pre-insertion checks to actually match the spec. r=smaug
dom/base/nsINode.cpp
testing/web-platform/meta/dom/nodes/Node-replaceChild.html.ini
testing/web-platform/tests/dom/nodes/Node-insertBefore.html
testing/web-platform/tests/dom/nodes/Node-replaceChild.html
testing/web-platform/tests/dom/nodes/pre-insertion-checks.js
--- a/dom/base/nsINode.cpp
+++ b/dom/base/nsINode.cpp
@@ -2294,39 +2294,37 @@ nsINode::ReplaceOrInsertBefore(bool aRep
 {
   // XXXbz I wish I could assert that nsContentUtils::IsSafeToRunScript() so we
   // could rely on scriptblockers going out of scope to actually run XBL
   // teardown, but various crud adds nodes under scriptblockers (e.g. native
   // anonymous content).  The only good news is those insertions can't trigger
   // the bad XBL cases.
   MOZ_ASSERT_IF(aReplace, aRefChild);
 
+  // Before firing DOMNodeRemoved events, make sure this is actually an insert
+  // we plan to do.
   EnsurePreInsertionValidity1(aError);
   if (aError.Failed()) {
     return nullptr;
   }
 
+  EnsurePreInsertionValidity2(aReplace, *aNewChild, aRefChild, aError);
+  if (aError.Failed()) {
+    return nullptr;
+  }
+
   uint16_t nodeType = aNewChild->NodeType();
 
   // Before we do anything else, fire all DOMNodeRemoved mutation events
   // We do this up front as to avoid having to deal with script running
   // at random places further down.
   // Scope firing mutation events so that we don't carry any state that
   // might be stale
   {
-    // This check happens again further down (though then using
-    // ComputeIndexOf).
-    // We're only checking this here to avoid firing mutation events when
-    // none should be fired.
-    // It's ok that we do the check twice in the case when firing mutation
-    // events as we need to recheck after running script anyway.
-    if (aRefChild && aRefChild->GetParentNode() != this) {
-      aError.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);
-      return nullptr;
-    }
+    nsMutationGuard guard;
 
     // If we're replacing, fire for node-to-be-replaced.
     // If aRefChild == aNewChild then we'll fire for it in check below
     if (aReplace && aRefChild != aNewChild) {
       nsContentUtils::MaybeFireNodeRemoved(aRefChild, this);
     }
 
     // If the new node already has a parent, fire for removing from old
@@ -2336,21 +2334,25 @@ nsINode::ReplaceOrInsertBefore(bool aRep
       nsContentUtils::MaybeFireNodeRemoved(aNewChild, oldParent);
     }
 
     // If we're inserting a fragment, fire for all the children of the
     // fragment
     if (nodeType == DOCUMENT_FRAGMENT_NODE) {
       static_cast<FragmentOrElement*>(aNewChild)->FireNodeRemovedForChildren();
     }
-  }
-
-  EnsurePreInsertionValidity2(aReplace, *aNewChild, aRefChild, aError);
-  if (aError.Failed()) {
-    return nullptr;
+
+    if (guard.Mutated(0)) {
+      // Re-check the parts of our pre-insertion validity that might depend on
+      // the tree shape.
+      EnsurePreInsertionValidity2(aReplace, *aNewChild, aRefChild, aError);
+      if (aError.Failed()) {
+        return nullptr;
+      }
+    }
   }
 
   // Record the node to insert before, if any
   nsIContent* nodeToInsertBefore;
   if (aReplace) {
     nodeToInsertBefore = aRefChild->GetNextSibling();
   } else {
     // Since aRefChild is our child, it must be an nsIContent object.
deleted file mode 100644
--- a/testing/web-platform/meta/dom/nodes/Node-replaceChild.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[Node-replaceChild.html]
-  [If node is an inclusive ancestor of the context node, a HierarchyRequestError should be thrown.]
-    expected: FAIL
-
--- a/testing/web-platform/tests/dom/nodes/Node-insertBefore.html
+++ b/testing/web-platform/tests/dom/nodes/Node-insertBefore.html
@@ -1,13 +1,19 @@
 <!DOCTYPE html>
 <title>Node.insertBefore</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <div id="log"></div>
+<!-- First test shared pre-insertion checks that work similarly for replaceChild
+     and insertBefore -->
+<script>
+  var insertFunc = Node.prototype.insertBefore;
+</script>
+<script src="pre-insertion-checks.js"></script>
 <script>
 function testLeafNode(nodeName, createNodeFunction) {
   test(function() {
     var node = createNodeFunction();
     assert_throws(new TypeError(), function() { node.insertBefore(null, null) })
   }, "Calling insertBefore with a non-Node first argument on a leaf node " + nodeName + " must throw TypeError.")
   test(function() {
     var node = createNodeFunction();
--- a/testing/web-platform/tests/dom/nodes/Node-replaceChild.html
+++ b/testing/web-platform/tests/dom/nodes/Node-replaceChild.html
@@ -1,15 +1,21 @@
 <!DOCTYPE html>
 <meta charset=utf-8>
 <title>Node.replaceChild</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <body><a><b></b><c></c></a>
 <div id="log"></div>
+<!-- First test shared pre-insertion checks that work similarly for replaceChild
+     and insertBefore -->
+<script>
+  var insertFunc = Node.prototype.replaceChild;
+</script>
+<script src="pre-insertion-checks.js"></script>
 <script>
 // IDL.
 test(function() {
   var a = document.createElement("div");
   assert_throws(new TypeError(), function() {
     a.replaceChild(null, null);
   });
 
@@ -17,50 +23,47 @@ test(function() {
   assert_throws(new TypeError(), function() {
     a.replaceChild(b, null);
   });
   assert_throws(new TypeError(), function() {
     a.replaceChild(null, b);
   });
 }, "Passing null to replaceChild should throw a TypeError.")
 
-// Step 1.
+// Step 3.
 test(function() {
   var a = document.createElement("div");
   var b = document.createElement("div");
   var c = document.createElement("div");
   assert_throws("NotFoundError", function() {
     a.replaceChild(b, c);
   });
 
   var d = document.createElement("div");
   d.appendChild(b);
   assert_throws("NotFoundError", function() {
     a.replaceChild(b, c);
   });
   assert_throws("NotFoundError", function() {
     a.replaceChild(b, a);
   });
-}, "If child's parent is not the context node, a NotFoundError exception should be thrown")
+}, "If child's parent is not the context node, a NotFoundError exception should be thrown");
+
+// Step 1.
 test(function() {
-  var nodes = [
-    document.implementation.createDocumentType("html", "", ""),
-    document.createTextNode("text"),
-    document.implementation.createDocument(null, "foo", null).createProcessingInstruction("foo", "bar"),
-    document.createComment("comment")
-  ];
+  var nodes = getNonParentNodes();
 
   var a = document.createElement("div");
   var b = document.createElement("div");
   nodes.forEach(function(node) {
     assert_throws("HierarchyRequestError", function() {
       node.replaceChild(a, b);
     });
   });
-}, "If the context node is not a node that can contain children, a NotFoundError exception should be thrown")
+}, "If the context node is not a node that can contain children, a HierarchyRequestError exception should be thrown")
 
 // Step 2.
 test(function() {
   var a = document.createElement("div");
   var b = document.createElement("div");
 
   assert_throws("HierarchyRequestError", function() {
     a.replaceChild(a, a);
@@ -73,30 +76,30 @@ test(function() {
 
   var c = document.createElement("div");
   c.appendChild(a);
   assert_throws("HierarchyRequestError", function() {
     a.replaceChild(c, b);
   });
 }, "If node is an inclusive ancestor of the context node, a HierarchyRequestError should be thrown.")
 
-// Step 3.1.
+// Steps 4/5.
 test(function() {
   var doc = document.implementation.createHTMLDocument("title");
   var doc2 = document.implementation.createHTMLDocument("title2");
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(doc2, doc.documentElement);
   });
 
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(doc.createTextNode("text"), doc.documentElement);
   });
 }, "If the context node is a document, inserting a document or text node should throw a HierarchyRequestError.")
 
-// Step 3.2.1.
+// Step 6.1.
 test(function() {
   var doc = document.implementation.createHTMLDocument("title");
 
   var df = doc.createDocumentFragment();
   df.appendChild(doc.createElement("a"));
   df.appendChild(doc.createElement("b"));
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(df, doc.documentElement);
@@ -122,17 +125,17 @@ test(function() {
   var df = doc.createDocumentFragment();
   df.appendChild(doc.createElement("a"));
   df.appendChild(doc.createElement("b"));
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(df, doc.doctype);
   });
 }, "If the context node is a document (without element children), inserting a DocumentFragment that contains multiple elements should throw a HierarchyRequestError.")
 
-// Step 3.2.2.
+// Step 6.1.
 test(function() {
   // The context node has an element child that is not /child/.
   var doc = document.implementation.createHTMLDocument("title");
   var comment = doc.appendChild(doc.createComment("foo"));
   assert_array_equals(doc.childNodes, [doc.doctype, doc.documentElement, comment]);
 
   var df = doc.createDocumentFragment();
   df.appendChild(doc.createElement("a"));
@@ -152,17 +155,17 @@ test(function() {
 
   var df = doc.createDocumentFragment();
   df.appendChild(doc.createElement("a"));
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(df, comment);
   });
 }, "If the context node is a document, inserting a DocumentFragment with an element before the doctype should throw a HierarchyRequestError.")
 
-// Step 3.3.
+// Step 6.2.
 test(function() {
   var doc = document.implementation.createHTMLDocument("title");
   var comment = doc.appendChild(doc.createComment("foo"));
   assert_array_equals(doc.childNodes, [doc.doctype, doc.documentElement, comment]);
 
   var a = doc.createElement("a");
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(a, comment);
@@ -178,17 +181,17 @@ test(function() {
   assert_array_equals(doc.childNodes, [comment, doc.doctype]);
 
   var a = doc.createElement("a");
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(a, comment);
   });
 }, "If the context node is a document, inserting an element before the doctype should throw a HierarchyRequestError.")
 
-// Step 3.4.
+// Step 6.3.
 test(function() {
   var doc = document.implementation.createHTMLDocument("title");
   var comment = doc.insertBefore(doc.createComment("foo"), doc.firstChild);
   assert_array_equals(doc.childNodes, [comment, doc.doctype, doc.documentElement]);
 
   var doctype = document.implementation.createDocumentType("html", "", "");
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(doctype, comment);
@@ -204,17 +207,17 @@ test(function() {
   assert_array_equals(doc.childNodes, [doc.documentElement, comment]);
 
   var doctype = document.implementation.createDocumentType("html", "", "");
   assert_throws("HierarchyRequestError", function() {
     doc.replaceChild(doctype, comment);
   });
 }, "If the context node is a document, inserting a doctype after the document element should throw a HierarchyRequestError.")
 
-// Step 4.
+// Steps 4/5.
 test(function() {
   var df = document.createDocumentFragment();
   var a = df.appendChild(document.createElement("a"));
 
   var doc = document.implementation.createHTMLDocument("title");
   assert_throws("HierarchyRequestError", function() {
     df.replaceChild(doc, a);
   });
@@ -337,9 +340,10 @@ test(function() {
   var child = document.createElement("div");
   parent.appendChild(child);
   var df = document.createDocumentFragment();
   var fragChild = df.appendChild(document.createElement("div"));
   fragChild.setAttribute("id", TEST_ID);
   parent.replaceChild(df, child);
   assert_equals(document.getElementById(TEST_ID), fragChild, "should not be null");
 }, "Replacing an element with a DocumentFragment should allow a child of the DocumentFragment to be found by Id.")
+
 </script>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/dom/nodes/pre-insertion-checks.js
@@ -0,0 +1,108 @@
+function getNonParentNodes() {
+  return [
+    document.implementation.createDocumentType("html", "", ""),
+    document.createTextNode("text"),
+    document.implementation.createDocument(null, "foo", null).createProcessingInstruction("foo", "bar"),
+    document.createComment("comment"),
+    document.implementation.createDocument(null, "foo", null).createCDATASection("data"),
+  ];
+}
+
+function getNonInsertableNodes() {
+  return [
+    document.implementation.createHTMLDocument("title")
+  ];
+}
+
+function getNonDocumentParentNodes() {
+  return [
+    document.createElement("div"),
+    document.createDocumentFragment(),
+  ];
+}
+
+// Test that the steps happen in the right order, to the extent that it's
+// observable.   The variable names "parent", "child", and "node" match the
+// corresponding variables in the replaceChild algorithm in these tests.
+
+// Step 1 happens before step 3.
+test(function() {
+  var illegalParents = getNonParentNodes();
+  var child = document.createElement("div");
+  var node = document.createElement("div");
+  illegalParents.forEach(function (parent) {
+    assert_throws("HierarchyRequestError", function() {
+      insertFunc.call(parent, node, child);
+    });
+  });
+}, "Should check the 'parent' type before checking whether 'child' is a child of 'parent'");
+
+// Step 2 happens before step 3.
+test(function() {
+  var parent = document.createElement("div");
+  var child = document.createElement("div");
+  var node = document.createElement("div");
+
+  node.appendChild(parent);
+  assert_throws("HierarchyRequestError", function() {
+    insertFunc.call(parent, node, child);
+  });
+}, "Should check that 'node' is not an ancestor of 'parent' before checking whether 'child' is a child of 'parent'");
+
+// Step 3 happens before step 4.
+test(function() {
+  var parent = document.createElement("div");
+  var child = document.createElement("div");
+
+  var illegalChildren = getNonInsertableNodes();
+  illegalChildren.forEach(function (node) {
+    assert_throws("NotFoundError", function() {
+      insertFunc.call(parent, node, child);
+    });
+  });
+}, "Should check whether 'child' is a child of 'parent' before checking whether 'node' is of a type that can have a parent.");
+
+
+// Step 3 happens before step 5.
+test(function() {
+  var child = document.createElement("div");
+
+  var node = document.createTextNode("");
+  var parent = document.implementation.createDocument(null, "foo", null);
+  assert_throws("NotFoundError", function() {
+    insertFunc.call(parent, node, child);
+  });
+
+  node = document.implementation.createDocumentType("html", "", "");
+  getNonDocumentParentNodes().forEach(function (parent) {
+    assert_throws("NotFoundError", function() {
+      insertFunc.call(parent, node, child);
+    });
+  });
+}, "Should check whether 'child' is a child of 'parent' before checking whether 'node' is of a type that can have a parent of the type that 'parent' is.");
+
+// Step 3 happens before step 6.
+test(function() {
+  var child = document.createElement("div");
+  var parent = document.implementation.createDocument(null, null, null);
+
+  var node = document.createDocumentFragment();
+  node.appendChild(document.createElement("div"));
+  node.appendChild(document.createElement("div"));
+  assert_throws("NotFoundError", function() {
+    insertFunc.call(parent, node, child);
+  });
+
+  node = document.createElement("div");
+  parent.appendChild(document.createElement("div"));
+  assert_throws("NotFoundError", function() {
+    insertFunc.call(parent, node, child);
+  });
+
+  parent.firstChild.remove();
+  parent.appendChild(document.implementation.createDocumentType("html", "", ""));
+  node = document.implementation.createDocumentType("html", "", "")
+  assert_throws("NotFoundError", function() {
+    insertFunc.call(parent, node, child);
+  });
+}, "Should check whether 'child' is a child of 'parent' before checking whether 'node' can be inserted into the document given the kids the document has right now.");