Changeset 243233 in webkit

Timestamp:

Mar 20, 2019, 1:26:18 PM (7 years ago)

Author:

rniwa@webkit.org

Message:

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.

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/dom/insert-template-parent-into-adopted-content-expected.txt (added)
• LayoutTests/fast/dom/insert-template-parent-into-adopted-content.html (added)
• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content-hierarcy-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ContainerNode.cpp (modified) (3 diffs)
• Source/WebCore/dom/Node.cpp (modified) (1 diff)
• Source/WebCore/dom/Node.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/html/semantics/scripting-1/the-template-element/template-element/template-content-hierarcy-expected.txt

@@ -1 +1 @@
 
 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. 
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/ContainerNode.cpp

@@ -293 +293 @@
 }
 
-static inline bool isInTemplateContent(const Node* node)
-{
-    Document& document = node->document();
-    return &document == document.templateDocument();
-}
-
-static inline bool containsConsideringHostElements(const Node& newChild, const Node& newParent)
-{
-    return (newParent.isInShadowTree() || isInTemplateContent(&newParent))
-        ? newChild.containsIncludingHostElements(&newParent)
-        : newChild.contains(&newParent);
+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);
+
+    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 };
@@ -322 +329 @@
     ASSERT(!newChild.isPseudoElement());
     if (newChild.isPseudoElement())
-        return Exception { HierarchyRequestError };
-
-    if (containsConsideringHostElements(newChild, newParent))
         return Exception { HierarchyRequestError };
 
@@ -343 +347 @@
     ASSERT(!newParent.isDocumentTypeNode());
     ASSERT(isChildTypeAllowed(newParent, newChild));
-    if (containsConsideringHostElements(newChild, newParent))
+    if (containsIncludingHostElements(newChild, newParent))
         return Exception { HierarchyRequestError };
     return { };

trunk/Source/WebCore/dom/Node.cpp

@@ -1030 +1030 @@
 }
 
-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
 {

trunk/Source/WebCore/dom/Node.h

@@ -393 +393 @@
     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