Changeset 249821 in webkit

Timestamp:

Sep 12, 2019 5:01:45 PM (5 years ago)

Author:

Chris Dumez

Message:

Node.replaceChild()'s pre-replacement validations are not done in the right order
​https://bugs.webkit.org/show_bug.cgi?id=201741

Reviewed by Geoffrey Garen.

LayoutTests/imported/w3c:

Rebaseline WPT test now that more checks are passing.

• web-platform-tests/dom/nodes/Node-replaceChild-expected.txt:

Source/WebCore:

Node.replaceChild()'s pre-replacement validations are not done in the right order (spec order):

• ​https://dom.spec.whatwg.org/#concept-node-replace

In particular, we do not do check 3 (If child’s parent is not parent, then throw a
"NotFoundError" DOMException.) at the right time, because we were making this check
*after* checkPreReplacementValidity(), instead of *during*.

No new tests, rebaselined existing test.

• dom/ContainerNode.cpp:

(WebCore::checkAcceptChild):
(WebCore::ContainerNode::ensurePreInsertionValidity):
(WebCore::checkPreReplacementValidity):
(WebCore::ContainerNode::replaceChild):

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Node-replaceChild-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ContainerNode.cpp (modified) (7 diffs)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2019-09-12  Chris Dumez  <cdumez@apple.com>
+
+        Node.replaceChild()'s pre-replacement validations are not done in the right order
+        https://bugs.webkit.org/show_bug.cgi?id=201741
+
+        Reviewed by Geoffrey Garen.
+
+        Rebaseline WPT test now that more checks are passing.
+
+        * web-platform-tests/dom/nodes/Node-replaceChild-expected.txt:
+
 2019-09-12  Chris Dumez  <cdumez@apple.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Node-replaceChild-expected.txt

@@ -2 +2 @@
 PASS Should check the 'parent' type before checking whether 'child' is a child of 'parent' 
 PASS Should check that 'node' is not an ancestor of 'parent' before checking whether 'child' is a child of 'parent' 
-FAIL Should check whether 'child' is a child of 'parent' before checking whether 'node' is of a type that can have a parent. assert_throws: function "function () {
-      insertFunc.call(parent, node, child);
-    }" threw object "HierarchyRequestError: The operation would yield an incorrect node tree." that is not a DOMException NotFoundError: property "code" is equal to 3, expected 8
-FAIL 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. assert_throws: function "function () {
-    insertFunc.call(parent, node, child);
-  }" threw object "HierarchyRequestError: The operation would yield an incorrect node tree." that is not a DOMException NotFoundError: property "code" is equal to 3, expected 8
-FAIL 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. assert_throws: function "function () {
-    insertFunc.call(parent, node, child);
-  }" threw object "HierarchyRequestError: The operation would yield an incorrect node tree." that is not a DOMException NotFoundError: property "code" is equal to 3, expected 8
+PASS Should check whether 'child' is a child of 'parent' before checking whether 'node' is of a type that can have a parent. 
+PASS 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. 
+PASS 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. 
 PASS Passing null to replaceChild should throw a TypeError. 
 PASS If child's parent is not the context node, a NotFoundError exception should be thrown 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-09-12  Chris Dumez  <cdumez@apple.com>
+
+        Node.replaceChild()'s pre-replacement validations are not done in the right order
+        https://bugs.webkit.org/show_bug.cgi?id=201741
+
+        Reviewed by Geoffrey Garen.
+
+        Node.replaceChild()'s pre-replacement validations are not done in the right order (spec order):
+        - https://dom.spec.whatwg.org/#concept-node-replace
+
+        In particular, we do not do check 3 (If child’s parent is not parent, then throw a
+        "NotFoundError" DOMException.) at the right time, because we were making this check
+        *after* checkPreReplacementValidity(), instead of *during*.
+
+        No new tests, rebaselined existing test.
+
+        * dom/ContainerNode.cpp:
+        (WebCore::checkAcceptChild):
+        (WebCore::ContainerNode::ensurePreInsertionValidity):
+        (WebCore::checkPreReplacementValidity):
+        (WebCore::ContainerNode::replaceChild):
+
 2019-09-12  Ryan Haddad  <ryanhaddad@apple.com>
 

trunk/Source/WebCore/dom/ContainerNode.cpp

@@ -312 +312 @@
 }
 
-static inline ExceptionOr<void> checkAcceptChild(ContainerNode& newParent, Node& newChild, const Node* refChild, Document::AcceptChildOperation operation)
+enum class ShouldValidateChildParent { No, Yes };
+static inline ExceptionOr<void> checkAcceptChild(ContainerNode& newParent, Node& newChild, const Node* refChild, Document::AcceptChildOperation operation, ShouldValidateChildParent shouldValidateChildParent)
 {
     if (containsIncludingHostElements(newChild, newParent))
@@ -321 +322 @@
         ASSERT(!newParent.isDocumentTypeNode());
         ASSERT(isChildTypeAllowed(newParent, newChild));
-        if (operation == Document::AcceptChildOperation::InsertOrAdd && refChild && refChild->parentNode() != &newParent)
+        if (shouldValidateChildParent == ShouldValidateChildParent::Yes && refChild && refChild->parentNode() != &newParent)
             return Exception { NotFoundError };
         return { };
@@ -331 +332 @@
         return Exception { HierarchyRequestError };
 
-    if (operation == Document::AcceptChildOperation::InsertOrAdd && refChild && refChild->parentNode() != &newParent)
+    if (shouldValidateChildParent == ShouldValidateChildParent::Yes && refChild && refChild->parentNode() != &newParent)
         return Exception { NotFoundError };
 
@@ -355 +356 @@
 ExceptionOr<void> ContainerNode::ensurePreInsertionValidity(Node& newChild, Node* refChild)
 {
-    return checkAcceptChild(*this, newChild, refChild, Document::AcceptChildOperation::InsertOrAdd);
+    return checkAcceptChild(*this, newChild, refChild, Document::AcceptChildOperation::InsertOrAdd, ShouldValidateChildParent::Yes);
 }
 
 // https://dom.spec.whatwg.org/#concept-node-replace
-static inline ExceptionOr<void> checkPreReplacementValidity(ContainerNode& newParent, Node& newChild, Node& oldChild)
-{
-    return checkAcceptChild(newParent, newChild, &oldChild, Document::AcceptChildOperation::Replace);
+static inline ExceptionOr<void> checkPreReplacementValidity(ContainerNode& newParent, Node& newChild, Node& oldChild, ShouldValidateChildParent shouldValidateChildParent)
+{
+    return checkAcceptChild(newParent, newChild, &oldChild, Document::AcceptChildOperation::Replace, shouldValidateChildParent);
 }
 
@@ -490 +491 @@
 
     // Make sure replacing the old child with the new is ok
-    auto validityResult = checkPreReplacementValidity(*this, newChild, oldChild);
+    auto validityResult = checkPreReplacementValidity(*this, newChild, oldChild, ShouldValidateChildParent::Yes);
     if (validityResult.hasException())
         return validityResult.releaseException();
-
-    // NotFoundError: Raised if oldChild is not a child of this node.
-    if (oldChild.parentNode() != this)
-        return Exception { NotFoundError };
 
     RefPtr<Node> refChild = oldChild.nextSibling();
@@ -512 +509 @@
     // Do this one more time because collectChildrenAndRemoveFromOldParent() fires a MutationEvent.
     for (auto& child : targets) {
-        validityResult = checkPreReplacementValidity(*this, child, oldChild);
+        validityResult = checkPreReplacementValidity(*this, child, oldChild, ShouldValidateChildParent::No);
         if (validityResult.hasException())
             return validityResult.releaseException();
@@ -530 +527 @@
         // Does this one more time because removeChild() fires a MutationEvent.
         for (auto& child : targets) {
-            validityResult = checkPreReplacementValidity(*this, child, oldChild);
+            validityResult = checkPreReplacementValidity(*this, child, oldChild, ShouldValidateChildParent::No);
             if (validityResult.hasException())
                 return validityResult.releaseException();