didMoveToNewDocument doesn't get called on an Attr inside a shadow tree
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Jun 2017 03:46:47 +0000 (03:46 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Jun 2017 03:46:47 +0000 (03:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173133

Reviewed by Antti Koivisto.

Source/WebCore:

The bug was caused by moveShadowTreeToNewDocument never calling didMoveToNewDocument on Attr nodes.
Fixed the bug by using the same traversal code as moveTreeToNewScope in moveShadowTreeToNewDocument
by extracting the traversal code as a templatized traverseSubtreeToUpdateTreeScope.

Also removed the code to increment the DOM tree version in moveTreeToNewScope. This code was there
to invalidate the HTML collection caches which used to clear the cache whenever the DOM tree version
changed before r122621 (five years ago! by me). Since we now eagerly invalidate each node list and
HTML collection's caches via NodeListsNodeData::adoptTreeScope and NodeListsNodeData::adoptDocument.

Test: fast/dom/adopt-attr-with-shadow-tree.html

* dom/Node.cpp:
(WebCore::moveNodeToNewDocument): Assert that the node had been adopted to a new document.
(WebCore::traverseSubtreeToUpdateTreeScope): Extracted from moveTreeToNewScope.
(WebCore::moveShadowTreeToNewDocument): Use traverseSubtreeToUpdateTreeScope to adopt each node
to the new document.
(WebCore::Node::moveTreeToNewScope): See above.
* testing/Internals.cpp:
(WebCore::Internals::referencingNodeCount): Added. Used in the newly added regression test.
* testing/Internals.h:
* testing/Internals.idl:

LayoutTests:

Added a regression test for moving a shadow tree with an Attr node across a document.
The test hits an assertion in a debug build and fails in a release build without the fix.

* fast/dom/adopt-attr-with-shadow-tree-expected.txt: Added.
* fast/dom/adopt-attr-with-shadow-tree.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/adopt-attr-with-shadow-tree-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/adopt-attr-with-shadow-tree.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Node.cpp
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index d972084..0699be7 100644 (file)
@@ -1,3 +1,16 @@
+2017-06-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        didMoveToNewDocument doesn't get called on an Attr inside a shadow tree
+        https://bugs.webkit.org/show_bug.cgi?id=173133
+
+        Reviewed by Antti Koivisto.
+
+        Added a regression test for moving a shadow tree with an Attr node across a document.
+        The test hits an assertion in a debug build and fails in a release build without the fix.
+
+        * fast/dom/adopt-attr-with-shadow-tree-expected.txt: Added.
+        * fast/dom/adopt-attr-with-shadow-tree.html: Added.
+
 2017-06-11  Keith Miller  <keith_miller@apple.com>
 
         TypedArray constructor with string shouldn't throw
diff --git a/LayoutTests/fast/dom/adopt-attr-with-shadow-tree-expected.txt b/LayoutTests/fast/dom/adopt-attr-with-shadow-tree-expected.txt
new file mode 100644 (file)
index 0000000..8ec5e6b
--- /dev/null
@@ -0,0 +1,10 @@
+This tests adopting a shadow tree with an Attr node.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS referenceCountInDestination is referenceCountInSource
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/adopt-attr-with-shadow-tree.html b/LayoutTests/fast/dom/adopt-attr-with-shadow-tree.html
new file mode 100644 (file)
index 0000000..671c39b
--- /dev/null
@@ -0,0 +1,50 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<script>
+
+description('This tests adopting a shadow tree with an Attr node.');
+
+var referenceCountInSource;
+var referenceCountInDestination;
+function runTest() {
+    let startCount = internals.referencingNodeCount(document);
+
+    const outerHost = document.createElement('shadow-host');
+    document.body.appendChild(outerHost);
+    const outerRoot = outerHost.attachShadow({mode: 'closed'});
+    outerRoot.innerHTML = '<span title="foo"></span>';
+
+    const innerHost = outerRoot.firstChild;
+    const innerRoot = innerHost.attachShadow({mode: 'closed'});
+    innerRoot.innerHTML = '<div lang="en"></div>';
+
+    const outerAttr = innerHost.attributes[0];
+    const outerAttrNodeList = outerAttr.childNodes;
+    const innerAttr = innerRoot.firstChild.attributes[0];
+    const innerAttrNodeList = innerAttr.childNodes;
+
+    referenceCountInSource = internals.referencingNodeCount(document) - startCount;
+
+    const iframe = document.createElement('iframe');
+    document.body.appendChild(iframe);
+
+    startCount = internals.referencingNodeCount(iframe.contentDocument);
+    iframe.contentDocument.body.appendChild(outerHost);
+    referenceCountInDestination = internals.referencingNodeCount(iframe.contentDocument) - startCount;
+
+    iframe.remove();
+}
+
+if (!window.GCController || !window.internals)
+    testFailed('This test requires testRunner, internals, and GCController objects.');
+else {
+    runTest();
+    GCController.collect();
+    shouldBe('referenceCountInDestination', 'referenceCountInSource');
+}
+
+</script>
+</body>
+</html>
index cfc66df..e753e66 100644 (file)
@@ -1,3 +1,32 @@
+2017-06-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        didMoveToNewDocument doesn't get called on an Attr inside a shadow tree
+        https://bugs.webkit.org/show_bug.cgi?id=173133
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by moveShadowTreeToNewDocument never calling didMoveToNewDocument on Attr nodes.
+        Fixed the bug by using the same traversal code as moveTreeToNewScope in moveShadowTreeToNewDocument
+        by extracting the traversal code as a templatized traverseSubtreeToUpdateTreeScope.
+
+        Also removed the code to increment the DOM tree version in moveTreeToNewScope. This code was there
+        to invalidate the HTML collection caches which used to clear the cache whenever the DOM tree version
+        changed before r122621 (five years ago! by me). Since we now eagerly invalidate each node list and
+        HTML collection's caches via NodeListsNodeData::adoptTreeScope and NodeListsNodeData::adoptDocument.
+
+        Test: fast/dom/adopt-attr-with-shadow-tree.html
+
+        * dom/Node.cpp:
+        (WebCore::moveNodeToNewDocument): Assert that the node had been adopted to a new document.
+        (WebCore::traverseSubtreeToUpdateTreeScope): Extracted from moveTreeToNewScope.
+        (WebCore::moveShadowTreeToNewDocument): Use traverseSubtreeToUpdateTreeScope to adopt each node
+        to the new document.
+        (WebCore::Node::moveTreeToNewScope): See above.
+        * testing/Internals.cpp:
+        (WebCore::Internals::referencingNodeCount): Added. Used in the newly added regression test.
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2017-06-11  Dan Bernstein  <mitz@apple.com>
 
         [Mac] Unaligned pointers in static CMBufferCallbacks structs defined in WebCoreDecompressionSession.mm
index 74db8b7..2d69b29 100644 (file)
@@ -1975,45 +1975,14 @@ static ALWAYS_INLINE void moveNodeToNewDocument(Node& node, Document& oldDocumen
     ASSERT(!node.isConnected() || &oldDocument != &newDocument);
     DidMoveToNewDocumentAssertionScope scope(node, oldDocument, newDocument);
     node.didMoveToNewDocument(oldDocument, newDocument);
+    ASSERT_WITH_SECURITY_IMPLICATION(&node.document() == &newDocument);
 }
 
-static void moveShadowTreeToNewDocument(ShadowRoot& shadowRoot, Document& oldDocument, Document& newDocument)
+template <typename MoveNodeFunction, typename MoveShadowRootFunction>
+static void traverseSubtreeToUpdateTreeScope(Node& root, MoveNodeFunction moveNode, MoveShadowRootFunction moveShadowRoot)
 {
-    for (Node* node = &shadowRoot; node; node = NodeTraversal::next(*node, &shadowRoot)) {
-        moveNodeToNewDocument(*node, oldDocument, newDocument);
-        if (auto* shadow = node->shadowRoot())
-            moveShadowTreeToNewDocument(*shadow, oldDocument, newDocument);
-    }
-}
-
-void Node::moveTreeToNewScope(Node& root, TreeScope& oldScope, TreeScope& newScope)
-{
-    ASSERT(&oldScope != &newScope);
-    ASSERT_WITH_SECURITY_IMPLICATION(&root.treeScope() == &oldScope);
-
-    // If an element is moved from a document and then eventually back again the collection cache for
-    // that element may contain stale data as changes made to it will have updated the DOMTreeVersion
-    // of the document it was moved to. By increasing the DOMTreeVersion of the donating document here
-    // we ensure that the collection cache will be invalidated as needed when the element is moved back.
-    Document& oldDocument = oldScope.documentScope();
-    Document& newDocument = newScope.documentScope();
-    bool shouldUpdateDocumentScope = &oldDocument != &newDocument;
-    if (shouldUpdateDocumentScope) {
-        oldDocument.incrementReferencingNodeCount();
-        oldDocument.incDOMTreeVersion();
-    }
-
     for (Node* node = &root; node; node = NodeTraversal::next(*node, &root)) {
-        ASSERT(!node->isTreeScope());
-        ASSERT(&node->treeScope() == &oldScope);
-        node->setTreeScope(newScope);
-
-        if (shouldUpdateDocumentScope)
-            moveNodeToNewDocument(*node, oldDocument, newDocument);
-        else if (node->hasRareData()) {
-            if (auto* nodeLists = node->rareData()->nodeLists())
-                nodeLists->adoptTreeScope();
-        }
+        moveNode(*node);
 
         if (!is<Element>(*node))
             continue;
@@ -2021,19 +1990,58 @@ void Node::moveTreeToNewScope(Node& root, TreeScope& oldScope, TreeScope& newSco
 
         if (element.hasSyntheticAttrChildNodes()) {
             for (auto& attr : element.attrNodeList())
-                moveTreeToNewScope(*attr, oldScope, newScope);
+                moveNode(*attr);
         }
 
-        if (auto* shadow = element.shadowRoot()) {
-            ASSERT_WITH_SECURITY_IMPLICATION(&shadow->document() == &oldDocument);
-            shadow->setParentTreeScope(newScope);
-            if (shouldUpdateDocumentScope)
-                moveShadowTreeToNewDocument(*shadow, oldDocument, newDocument);
-        }
+        if (auto* shadow = element.shadowRoot())
+            moveShadowRoot(*shadow);
     }
+}
 
-    if (shouldUpdateDocumentScope)
+static void moveShadowTreeToNewDocument(ShadowRoot& shadowRoot, Document& oldDocument, Document& newDocument)
+{
+    traverseSubtreeToUpdateTreeScope(shadowRoot, [&oldDocument, &newDocument](Node& node) {
+        moveNodeToNewDocument(node, oldDocument, newDocument);
+    }, [&oldDocument, &newDocument](ShadowRoot& innerShadowRoot) {
+        ASSERT_WITH_SECURITY_IMPLICATION(&innerShadowRoot.document() == &oldDocument);
+        moveShadowTreeToNewDocument(innerShadowRoot, oldDocument, newDocument);
+    });
+}
+
+void Node::moveTreeToNewScope(Node& root, TreeScope& oldScope, TreeScope& newScope)
+{
+    ASSERT(&oldScope != &newScope);
+    ASSERT_WITH_SECURITY_IMPLICATION(&root.treeScope() == &oldScope);
+
+    Document& oldDocument = oldScope.documentScope();
+    Document& newDocument = newScope.documentScope();
+    if (&oldDocument != &newDocument) {
+        oldDocument.incrementReferencingNodeCount();
+        traverseSubtreeToUpdateTreeScope(root, [&](Node& node) {
+            ASSERT(!node.isTreeScope());
+            ASSERT_WITH_SECURITY_IMPLICATION(&node.treeScope() == &oldScope);
+            ASSERT_WITH_SECURITY_IMPLICATION(&node.document() == &oldDocument);
+            node.setTreeScope(newScope);
+            moveNodeToNewDocument(node, oldDocument, newDocument);
+        }, [&](ShadowRoot& shadowRoot) {
+            ASSERT_WITH_SECURITY_IMPLICATION(&shadowRoot.document() == &oldDocument);
+            shadowRoot.setParentTreeScope(newScope);
+            moveShadowTreeToNewDocument(shadowRoot, oldDocument, newDocument);
+        });
         oldDocument.decrementReferencingNodeCount();
+    } else {
+        traverseSubtreeToUpdateTreeScope(root, [&](Node& node) {
+            ASSERT(!node.isTreeScope());
+            ASSERT_WITH_SECURITY_IMPLICATION(&node.treeScope() == &oldScope);
+            node.setTreeScope(newScope);
+            if (!node.hasRareData())
+                return;
+            if (auto* nodeLists = node.rareData()->nodeLists())
+                nodeLists->adoptTreeScope();
+        }, [&newScope](ShadowRoot& shadowRoot) {
+            shadowRoot.setParentTreeScope(newScope);
+        });
+    }
 }
 
 void Node::didMoveToNewDocument(Document& oldDocument, Document& newDocument)
index 0de12be..a156c74 100644 (file)
@@ -2195,6 +2195,11 @@ unsigned Internals::numberOfLiveDocuments() const
     return Document::allDocuments().size();
 }
 
+unsigned Internals::referencingNodeCount(const Document& document) const
+{
+    return document.referencingNodeCount();
+}
+
 RefPtr<DOMWindow> Internals::openDummyInspectorFrontend(const String& url)
 {
     auto* inspectedPage = contextDocument()->frame()->page();
index a5cb515..07ae99e 100644 (file)
@@ -325,6 +325,7 @@ public:
 
     unsigned numberOfLiveNodes() const;
     unsigned numberOfLiveDocuments() const;
+    unsigned referencingNodeCount(const Document&) const;
 
     RefPtr<DOMWindow> openDummyInspectorFrontend(const String& url);
     void closeDummyInspectorFrontend();
index dc2d119..8f7f40f 100644 (file)
@@ -305,6 +305,7 @@ enum EventThrottlingBehavior {
 
     unsigned long numberOfLiveNodes();
     unsigned long numberOfLiveDocuments();
+    unsigned long referencingNodeCount(Document document);
     DOMWindow? openDummyInspectorFrontend(DOMString url);
     void closeDummyInspectorFrontend();
     [MayThrowException] void setInspectorIsUnderTest(boolean isUnderTest);