appendChild should throw when inserting an ancestor of a template into its content...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Mar 2019 20:26:18 +0000 (20:26 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Mar 2019 20:26:18 +0000 (20:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195984

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaselined the test that is not fully passing.

* web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content-hierarcy-expected.txt:

Source/WebCore:

The WPT test caught a bug that appendChild and other DOM insertion functions were incorrectly assuming that
any node that's in a HTML template element has the current document's template document as its owner.
The assumption is wrong when the template element's content DocumentFragment is adopted to another document.

Fixed the bug by always checking the ancestor host elements in checkAcceptChild. Also

Test: fast/dom/insert-template-parent-into-adopted-content.html

* dom/ContainerNode.cpp:
(WebCore::isInTemplateContent): Deleted. This code is simply wrong.
(WebCore::containsConsideringHostElements): Deleted. Call sites are updated to use containsIncludingHostElements.
(WebCore::containsIncludingHostElements): Moved from Node.cpp and optimized this code a bit. It's more efficient
to get the parent node and check for ShadowRoot and DocumentFragment only when the parent is null than to check
for those two node types before getting the parent node.
(WebCore::checkAcceptChild): Merged two code paths to call containsIncludingHostElements. The early return for
a pseudo element is there only to prevent tree corruption in release build even in the presence of a major bug
so it shouldn't be an spec compliance issue.
* dom/Node.cpp:
(WebCore::Node::containsIncludingHostElements const): Deleted.
* dom/Node.h:

LayoutTests:

Added a regression test.

* fast/dom/insert-template-parent-into-adopted-content-expected.txt: Added.
* fast/dom/insert-template-parent-into-adopted-content.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@243233 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/dom/insert-template-parent-into-adopted-content-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/insert-template-parent-into-adopted-content.html [new file with mode: 0644]
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content-hierarcy-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/dom/ContainerNode.cpp
Source/WebCore/dom/Node.cpp
Source/WebCore/dom/Node.h

index 4b4ffdd..82bc2d6 100644 (file)
@@ -1,3 +1,15 @@
+2019-03-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        appendChild should throw when inserting an ancestor of a template into its content adopted to another document
+        https://bugs.webkit.org/show_bug.cgi?id=195984
+
+        Reviewed by Darin Adler.
+
+        Added a regression test.
+
+        * fast/dom/insert-template-parent-into-adopted-content-expected.txt: Added.
+        * fast/dom/insert-template-parent-into-adopted-content.html: Added.
+
 2019-03-20  Simon Fraser  <simon.fraser@apple.com>
 
         Unreviewed test gardening. Fix the results for absolute-in-async-overflow-scroll.html.
diff --git a/LayoutTests/fast/dom/insert-template-parent-into-adopted-content-expected.txt b/LayoutTests/fast/dom/insert-template-parent-into-adopted-content-expected.txt
new file mode 100644 (file)
index 0000000..6b5752e
--- /dev/null
@@ -0,0 +1,13 @@
+This tests inserting the parent of a template element into its content document fragment
+after adopting the document fragment to another document. WebKit should throw HierarchyRequestError
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS template.content.appendChild(templateParent) threw exception HierarchyRequestError: The operation would yield an incorrect node tree..
+PASS template.content.insertBefore(templateParent, template.content.firstChild) threw exception HierarchyRequestError: The operation would yield an incorrect node tree..
+PASS template.content.replaceChild(templateParent, template.content.firstChild) threw exception HierarchyRequestError: The operation would yield an incorrect node tree..
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/insert-template-parent-into-adopted-content.html b/LayoutTests/fast/dom/insert-template-parent-into-adopted-content.html
new file mode 100644 (file)
index 0000000..2f1e274
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div id="templateParent"><template id="template"><span></span></template></div>
+<script src="../../resources/js-test.js"></script>
+<script>
+
+description('This tests inserting the parent of a template element into its content document fragment<br>'
+    + 'after adopting the document fragment to another document. WebKit should throw HierarchyRequestError');
+
+const newDocument = document.implementation.createHTMLDocument();
+newDocument.adoptNode(template.content);
+shouldThrowErrorName('template.content.appendChild(templateParent)', 'HierarchyRequestError');
+shouldThrowErrorName('template.content.insertBefore(templateParent, template.content.firstChild)', 'HierarchyRequestError');
+shouldThrowErrorName('template.content.replaceChild(templateParent, template.content.firstChild)', 'HierarchyRequestError');
+
+</script>
+</body>
+</html>
index 0ad7015..87392a0 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        appendChild should throw when inserting an ancestor of a template into its content adopted to another document
+        https://bugs.webkit.org/show_bug.cgi?id=195984
+
+        Reviewed by Darin Adler.
+
+        Rebaselined the test that is not fully passing.
+
+        * web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content-hierarcy-expected.txt:
+
 2019-03-20  Oriol Brufau  <obrufau@igalia.com>
 
         [css-grid] Always consider baseline shim for the minimum contribution
index c985120..ed92cda 100644 (file)
@@ -1,6 +1,4 @@
 
 PASS Template content should throw when its ancestor is being appended. 
-FAIL Template content should throw exception when its ancestor in a different document but connected via host is being append. assert_throws: Template content should throw if any of ancestor is being appended. function "() => {
-    tmpl.content.appendChild(parent);
-  }" did not throw
+PASS Template content should throw exception when its ancestor in a different document but connected via host is being append. 
 
index 07a48a7..1841b9b 100644 (file)
@@ -1,3 +1,31 @@
+2019-03-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        appendChild should throw when inserting an ancestor of a template into its content adopted to another document
+        https://bugs.webkit.org/show_bug.cgi?id=195984
+
+        Reviewed by Darin Adler.
+
+        The WPT test caught a bug that appendChild and other DOM insertion functions were incorrectly assuming that
+        any node that's in a HTML template element has the current document's template document as its owner.
+        The assumption is wrong when the template element's content DocumentFragment is adopted to another document.
+
+        Fixed the bug by always checking the ancestor host elements in checkAcceptChild. Also
+
+        Test: fast/dom/insert-template-parent-into-adopted-content.html
+
+        * dom/ContainerNode.cpp:
+        (WebCore::isInTemplateContent): Deleted. This code is simply wrong.
+        (WebCore::containsConsideringHostElements): Deleted. Call sites are updated to use containsIncludingHostElements.
+        (WebCore::containsIncludingHostElements): Moved from Node.cpp and optimized this code a bit. It's more efficient
+        to get the parent node and check for ShadowRoot and DocumentFragment only when the parent is null than to check
+        for those two node types before getting the parent node.
+        (WebCore::checkAcceptChild): Merged two code paths to call containsIncludingHostElements. The early return for
+        a pseudo element is there only to prevent tree corruption in release build even in the presence of a major bug
+        so it shouldn't be an spec compliance issue.
+        * dom/Node.cpp:
+        (WebCore::Node::containsIncludingHostElements const): Deleted.
+        * dom/Node.h:
+
 2019-03-20  Timothy Hatcher  <timothy@apple.com>
 
         Unreviewed followup to r243169 to fix test failures.
index 6297b7c..b85af13 100644 (file)
@@ -292,27 +292,34 @@ static inline bool isChildTypeAllowed(ContainerNode& newParent, Node& child)
     return true;
 }
 
-static inline bool isInTemplateContent(const Node* node)
-{
-    Document& document = node->document();
-    return &document == document.templateDocument();
-}
+static bool containsIncludingHostElements(const Node& possibleAncestor, const Node& node)
+{
+    const Node* currentNode = &node;
+    do {
+        if (currentNode == &possibleAncestor)
+            return true;
+        const ContainerNode* parent = currentNode->parentNode();
+        if (!parent) {
+            if (is<ShadowRoot>(currentNode))
+                parent = downcast<ShadowRoot>(currentNode)->host();
+            else if (is<DocumentFragment>(*currentNode) && downcast<DocumentFragment>(*currentNode).isTemplateContent())
+                parent = static_cast<const TemplateContentDocumentFragment*>(currentNode)->host();
+        }
+        currentNode = parent;
+    } while (currentNode);
 
-static inline bool containsConsideringHostElements(const Node& newChild, const Node& newParent)
-{
-    return (newParent.isInShadowTree() || isInTemplateContent(&newParent))
-        ? newChild.containsIncludingHostElements(&newParent)
-        : newChild.contains(&newParent);
+    return false;
 }
 
 static inline ExceptionOr<void> checkAcceptChild(ContainerNode& newParent, Node& newChild, const Node* refChild, Document::AcceptChildOperation operation)
 {
+    if (containsIncludingHostElements(newChild, newParent))
+        return Exception { HierarchyRequestError };
+
     // Use common case fast path if possible.
     if ((newChild.isElementNode() || newChild.isTextNode()) && newParent.isElementNode()) {
         ASSERT(!newParent.isDocumentTypeNode());
         ASSERT(isChildTypeAllowed(newParent, newChild));
-        if (containsConsideringHostElements(newChild, newParent))
-            return Exception { HierarchyRequestError };
         if (operation == Document::AcceptChildOperation::InsertOrAdd && refChild && refChild->parentNode() != &newParent)
             return Exception { NotFoundError };
         return { };
@@ -323,9 +330,6 @@ static inline ExceptionOr<void> checkAcceptChild(ContainerNode& newParent, Node&
     if (newChild.isPseudoElement())
         return Exception { HierarchyRequestError };
 
-    if (containsConsideringHostElements(newChild, newParent))
-        return Exception { HierarchyRequestError };
-
     if (operation == Document::AcceptChildOperation::InsertOrAdd && refChild && refChild->parentNode() != &newParent)
         return Exception { NotFoundError };
 
@@ -342,7 +346,7 @@ static inline ExceptionOr<void> checkAcceptChildGuaranteedNodeTypes(ContainerNod
 {
     ASSERT(!newParent.isDocumentTypeNode());
     ASSERT(isChildTypeAllowed(newParent, newChild));
-    if (containsConsideringHostElements(newChild, newParent))
+    if (containsIncludingHostElements(newChild, newParent))
         return Exception { HierarchyRequestError };
     return { };
 }
index e9ff1b3..42a43d9 100644 (file)
@@ -1029,19 +1029,6 @@ bool Node::containsIncludingShadowDOM(const Node* node) const
     return false;
 }
 
-bool Node::containsIncludingHostElements(const Node* node) const
-{
-    while (node) {
-        if (node == this)
-            return true;
-        if (is<DocumentFragment>(*node) && downcast<DocumentFragment>(*node).isTemplateContent())
-            node = static_cast<const TemplateContentDocumentFragment*>(node)->host();
-        else
-            node = node->parentOrShadowHostNode();
-    }
-    return false;
-}
-
 Node* Node::pseudoAwarePreviousSibling() const
 {
     Element* parentOrHost = is<PseudoElement>(*this) ? downcast<PseudoElement>(*this).hostElement() : parentElement();
index 3aa597e..28f0abc 100644 (file)
@@ -392,7 +392,6 @@ public:
     bool isDescendantOrShadowDescendantOf(const Node*) const;
     WEBCORE_EXPORT bool contains(const Node*) const;
     bool containsIncludingShadowDOM(const Node*) const;
-    bool containsIncludingHostElements(const Node*) const;
 
     // Number of DOM 16-bit units contained in node. Note that rendered text length can be different - e.g. because of
     // css-transform:capitalize breaking up precomposed characters and ligatures.