Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Node-replaceChild-expected.txt (249820 => 249821)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Node-replaceChild-expected.txt 2019-09-12 23:59:32 UTC (rev 249820)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/dom/nodes/Node-replaceChild-expected.txt 2019-09-13 00:01:45 UTC (rev 249821)
@@ -1,15 +1,9 @@
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
PASS If the context node is not a node that can contain children, a HierarchyRequestError exception should be thrown
Modified: trunk/Source/WebCore/ChangeLog (249820 => 249821)
--- trunk/Source/WebCore/ChangeLog 2019-09-12 23:59:32 UTC (rev 249820)
+++ trunk/Source/WebCore/ChangeLog 2019-09-13 00:01:45 UTC (rev 249821)
@@ -1,3 +1,25 @@
+2019-09-12 Chris Dumez <cdu...@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 <ryanhad...@apple.com>
Unreviewed, rolling out r249801.
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (249820 => 249821)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2019-09-12 23:59:32 UTC (rev 249820)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2019-09-13 00:01:45 UTC (rev 249821)
@@ -311,7 +311,8 @@
return false;
}
-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))
return Exception { HierarchyRequestError };
@@ -320,7 +321,7 @@
if ((newChild.isElementNode() || newChild.isTextNode()) && newParent.isElementNode()) {
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 { };
}
@@ -330,7 +331,7 @@
if (newChild.isPseudoElement())
return Exception { HierarchyRequestError };
- if (operation == Document::AcceptChildOperation::InsertOrAdd && refChild && refChild->parentNode() != &newParent)
+ if (shouldValidateChildParent == ShouldValidateChildParent::Yes && refChild && refChild->parentNode() != &newParent)
return Exception { NotFoundError };
if (is<Document>(newParent)) {
@@ -354,13 +355,13 @@
// https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity
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)
+static inline ExceptionOr<void> checkPreReplacementValidity(ContainerNode& newParent, Node& newChild, Node& oldChild, ShouldValidateChildParent shouldValidateChildParent)
{
- return checkAcceptChild(newParent, newChild, &oldChild, Document::AcceptChildOperation::Replace);
+ return checkAcceptChild(newParent, newChild, &oldChild, Document::AcceptChildOperation::Replace, shouldValidateChildParent);
}
ExceptionOr<void> ContainerNode::insertBefore(Node& newChild, Node* refChild)
@@ -489,14 +490,10 @@
Ref<ContainerNode> protectedThis(*this);
// 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();
if (refChild.get() == &newChild)
refChild = refChild->nextSibling();
@@ -511,7 +508,7 @@
// 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();
}
@@ -529,7 +526,7 @@
// 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();
}
