Bug 1500956 part 2. Change our EnsurePreInsertionValidity implementation to follow the spec more closely. r=smaug
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 23 Oct 2018 00:32:00 +0200
changeset 490725 5c6566236a81d93dd1f41aec87b2dc56fbf5c8c9
parent 490724 b5d831c8d69c567921c24afd111e22bd42223e29
child 490726 bbb8bac97270807cf396a7d0c33a777e6a57d4d0
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewerssmaug
bugs1500956
milestone65.0a1
Bug 1500956 part 2. Change our EnsurePreInsertionValidity implementation to follow the spec more closely. r=smaug This is basically putting in https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity step 3 and moving step 4 to the right place in the order of steps. This matters, because step 3 throws a different exception from all the other steps.
dom/base/nsINode.cpp
dom/base/nsINode.h
--- a/dom/base/nsINode.cpp
+++ b/dom/base/nsINode.cpp
@@ -2044,47 +2044,67 @@ nsINode::RemoveChildNode(nsIContent* aKi
   aKid->UnbindFromTree();
 }
 
 // When replacing, aRefChild is the content being replaced; when
 // inserting it's the content before which we're inserting.  In the
 // latter case it may be null.
 //
 // If aRv is a failure after this call, the insertion should not happen.
+//
+// This implements the parts of
+// https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity and
+// the checks in https://dom.spec.whatwg.org/#concept-node-replace that
+// depend on the child nodes or come after steps that depend on the child nodes
+// (steps 2-6 in both cases).
 static
-void EnsureAllowedAsChild(nsIContent* aNewChild, nsINode* aParent,
+void EnsureAllowedAsChild(nsINode* aNewChild, nsINode* aParent,
                           bool aIsReplace, nsINode* aRefChild,
                           ErrorResult& aRv)
 {
   MOZ_ASSERT(aNewChild, "Must have new child");
   MOZ_ASSERT_IF(aIsReplace, aRefChild);
   MOZ_ASSERT(aParent);
   MOZ_ASSERT(aParent->IsDocument() ||
              aParent->IsDocumentFragment() ||
              aParent->IsElement(),
              "Nodes that are not documents, document fragments or elements "
              "can't be parents!");
 
+  // Step 2.
   // A common case is that aNewChild has no kids, in which case
   // aParent can't be a descendant of aNewChild unless they're
   // actually equal to each other.  Fast-path that case, since aParent
   // could be pretty deep in the DOM tree.
   if (aNewChild == aParent ||
       ((aNewChild->GetFirstChild() ||
         // HTML template elements and ShadowRoot hosts need
         // to be checked to ensure that they are not inserted into
         // the hosted content.
         aNewChild->NodeInfo()->NameAtom() == nsGkAtoms::_template ||
-        aNewChild->GetShadowRoot()) &&
+        (aNewChild->IsElement() && aNewChild->AsElement()->GetShadowRoot())) &&
        nsContentUtils::ContentIsHostIncludingDescendantOf(aParent,
                                                           aNewChild))) {
     aRv.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
     return;
   }
 
+  // Step 3.
+  if (aRefChild && aRefChild->GetParentNode() != aParent) {
+    aRv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);
+    return;
+  }
+
+  // Step 4.
+  if (!aNewChild->IsContent()) {
+    aRv.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
+    return;
+  }
+
+  // Steps 5 and 6 combined.
   // The allowed child nodes differ for documents and elements
   switch (aNewChild->NodeType()) {
   case nsINode::COMMENT_NODE :
   case nsINode::PROCESSING_INSTRUCTION_NODE :
     // OK in both cases
     return;
   case nsINode::TEXT_NODE :
   case nsINode::CDATA_SECTION_NODE :
@@ -2221,67 +2241,70 @@ void EnsureAllowedAsChild(nsIContent* aN
      * aNewChild is of invalid type.
      */
     break;
   }
 
   aRv.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
 }
 
+// Implements https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity
 void
 nsINode::EnsurePreInsertionValidity(nsINode& aNewChild, nsINode* aRefChild,
                                     ErrorResult& aError)
 {
-  EnsurePreInsertionValidity1(aNewChild, aRefChild, aError);
+  EnsurePreInsertionValidity1(aError);
   if (aError.Failed()) {
     return;
   }
   EnsurePreInsertionValidity2(false, aNewChild, aRefChild, aError);
 }
 
+// Implements the parts of
+// https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity and
+// the checks in https://dom.spec.whatwg.org/#concept-node-replace that can be
+// evaluated before ever looking at the child nodes (step 1 in both cases).
 void
-nsINode::EnsurePreInsertionValidity1(nsINode& aNewChild, nsINode* aRefChild,
-                                     ErrorResult& aError)
+nsINode::EnsurePreInsertionValidity1(ErrorResult& aError)
 {
-  if ((!IsDocument() && !IsDocumentFragment() && !IsElement()) ||
-      !aNewChild.IsContent()) {
+  if (!IsDocument() && !IsDocumentFragment() && !IsElement()) {
     aError.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
     return;
   }
 }
 
 void
 nsINode::EnsurePreInsertionValidity2(bool aReplace, nsINode& aNewChild,
                                      nsINode* aRefChild, ErrorResult& aError)
 {
-  nsIContent* newContent = aNewChild.AsContent();
-  if (newContent->IsRootOfAnonymousSubtree()) {
+  if (aNewChild.IsContent() &&
+      aNewChild.AsContent()->IsRootOfAnonymousSubtree()) {
     // This is anonymous content.  Don't allow its insertion
     // anywhere, since it might have UnbindFromTree calls coming
     // its way.
     aError.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
     return;
   }
 
   // Make sure that the inserted node is allowed as a child of its new parent.
-  EnsureAllowedAsChild(newContent, this, aReplace, aRefChild, aError);
+  EnsureAllowedAsChild(&aNewChild, this, aReplace, aRefChild, aError);
 }
 
 nsINode*
 nsINode::ReplaceOrInsertBefore(bool aReplace, nsINode* aNewChild,
                                nsINode* aRefChild, ErrorResult& aError)
 {
   // 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);
 
-  EnsurePreInsertionValidity1(*aNewChild, aRefChild, aError);
+  EnsurePreInsertionValidity1(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
@@ -2313,21 +2336,16 @@ 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();
     }
-    // Verify that our aRefChild is still sensible
-    if (aRefChild && aRefChild->GetParentNode() != this) {
-      aError.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);
-      return nullptr;
-    }
   }
 
   EnsurePreInsertionValidity2(aReplace, *aNewChild, aRefChild, aError);
   if (aError.Failed()) {
     return nullptr;
   }
 
   // Record the node to insert before, if any
@@ -2371,42 +2389,31 @@ nsINode::ReplaceOrInsertBefore(bool aRep
         mb.SetNextSibling(next);
       }
     }
 
     // We expect one mutation (the removal) to have happened.
     if (guard.Mutated(1)) {
       // XBL destructors, yuck.
 
-      // Verify that nodeToInsertBefore, if non-null, is still our child.  If
-      // it's not, there's no way we can do this insert sanely; just bail out.
-      if (nodeToInsertBefore && nodeToInsertBefore->GetParent() != this) {
-        aError.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
-        return nullptr;
-      }
-
       // Verify that newContent has no parent.
       if (newContent->GetParentNode()) {
         aError.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
         return nullptr;
       }
 
       // And verify that newContent is still allowed as our child.
       if (aNewChild == aRefChild) {
         // We've already removed aRefChild.  So even if we were doing a replace,
         // now we're doing a simple insert before nodeToInsertBefore.
         EnsureAllowedAsChild(newContent, this, false, nodeToInsertBefore, aError);
         if (aError.Failed()) {
           return nullptr;
         }
       } else {
-        if (aRefChild && aRefChild->GetParent() != this) {
-          aError.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
-          return nullptr;
-        }
         EnsureAllowedAsChild(newContent, this, aReplace, aRefChild, aError);
         if (aError.Failed()) {
           return nullptr;
         }
 
         // And recompute nodeToInsertBefore, just in case.
         if (aReplace) {
           nodeToInsertBefore = aRefChild->GetNextSibling();
--- a/dom/base/nsINode.h
+++ b/dom/base/nsINode.h
@@ -1965,18 +1965,17 @@ protected:
   }
 
 #ifdef DEBUG
   // Note: virtual so that IsInNativeAnonymousSubtree can be called accross
   // module boundaries.
   virtual void CheckNotNativeAnonymous() const;
 #endif
 
-  void EnsurePreInsertionValidity1(nsINode& aNewChild, nsINode* aRefChild,
-                                   mozilla::ErrorResult& aError);
+  void EnsurePreInsertionValidity1(mozilla::ErrorResult& aError);
   void EnsurePreInsertionValidity2(bool aReplace, nsINode& aNewChild,
                                    nsINode* aRefChild,
                                    mozilla::ErrorResult& aError);
   nsINode* ReplaceOrInsertBefore(bool aReplace, nsINode* aNewChild,
                                  nsINode* aRefChild,
                                  mozilla::ErrorResult& aError);
 
   /**