Inserting or removing slot elements can cause a crash
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 19 Sep 2015 04:18:27 +0000 (04:18 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 19 Sep 2015 04:18:27 +0000 (04:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=149365

Reviewed by Antti Koivisto.

Source/WebCore:

HTMLSlotElement::insertedInto and removedFrom were doing completely non-sensical.

Since insertedInto and removedFrom are called on an element whenever it or its ancestor is inserted into
or removed from a container node, we can't always call addSlotElementByName removeSlotElementByName when
those functions are called. Instead, we need to check whether this slot has been inserted into or removed
from a container node that resides inside a shadow root.

Also reverted r189906 since the change was made upon a bogus assumption I had made.

Test: fast/shadow-dom/slot-removal-crash.html

* dom/Element.cpp:
(WebCore::Element::insertedInto): Added comments.
(WebCore::Element::removedFrom): Ditto.
(WebCore::Element::addShadowRoot): Reverted r189906.
(WebCore::Element::removeShadowRoot): Ditto.

* html/HTMLSlotElement.cpp:
(WebCore::HTMLSlotElement::insertedInto): When the insertion point's tree scope is different from ours,
the insertion happened to our shadow host or its ancestor. There is nothing to be done in that case since
the shadow tree was not modified (in particular, our relationship with our shadow root never changed).
We also don't do anything if we got inserted into a parent which is not inside a shadow tree.

(WebCore::HTMLSlotElement::removedFrom): Since Container::removeBetween sets the tree scope before this
function is getting called, we can't compare this element's treeScope with that of the "insertion" point.
They're always different regardless of whether the insertion point was in the same shadow tree to which
we belong or its shadow host's. However, since a node removed from a shadow tree is put into document's
tree scope before this function is called and InShadowTree flag is unset in Node::removedFrom at the end
of this function, this slot element is definitely being removed from its shadow root when isInShadowTree()
is true and the newly set tree scope is of the document. So call removeSlotElementByName if and only if
that condition holds.

(WebCore::HTMLSlotElement::getDistributedNodes): Explicitly check that we're inside a shadow root.

LayoutTests:

Added regression tests.

* fast/shadow-dom/slot-removal-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/shadow-dom/slot-removal-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/shadow-dom/slot-removal-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/html/HTMLSlotElement.cpp

index 0f4e8c9..92204f0 100644 (file)
@@ -1,3 +1,14 @@
+2015-09-18  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Inserting or removing slot elements can cause a crash
+        https://bugs.webkit.org/show_bug.cgi?id=149365
+
+        Reviewed by Antti Koivisto.
+
+        Added regression tests.
+
+        * fast/shadow-dom/slot-removal-crash.html: Added.
+
 2015-09-18  Antti Koivisto  <antti@apple.com>
 
         Don't create renderers for children of shadow host
diff --git a/LayoutTests/fast/shadow-dom/slot-removal-crash-expected.txt b/LayoutTests/fast/shadow-dom/slot-removal-crash-expected.txt
new file mode 100644 (file)
index 0000000..ffab3ba
--- /dev/null
@@ -0,0 +1,3 @@
+This tests inserting or removing shadow tree with slot elements do not cause a crash.
+
+PASS
diff --git a/LayoutTests/fast/shadow-dom/slot-removal-crash.html b/LayoutTests/fast/shadow-dom/slot-removal-crash.html
new file mode 100644 (file)
index 0000000..67f11dd
--- /dev/null
@@ -0,0 +1,59 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests inserting or removing shadow tree with slot elements do not cause a crash.</p>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function addShadowWithSlot() {
+    var host = document.createElement('div');
+    var shadowRoot = host.attachShadow({mode: 'open'});
+    var slot = document.createElement('slot');
+    shadowRoot.appendChild(slot);
+    document.body.appendChild(host);
+
+    slot.remove();
+    slot.setAttribute('name', 'foo');
+}
+
+function removeShadowWithSlot() {
+    var container = document.createElement('div');
+    document.body.appendChild(container);
+
+    var host = document.createElement('div');
+    container.appendChild(host);
+
+    var shadowRoot = host.attachShadow({mode: 'open'});
+    var slot = document.createElement('slot');
+    shadowRoot.appendChild(slot);
+
+    container.remove();
+    host.remove();
+}
+
+function removeNestedShadowsWithSlot() {
+    var outerHost = document.createElement('div');
+    document.body.appendChild(outerHost);
+    
+    var outerShadow = outerHost.attachShadow({mode: 'open'});
+    var innerHost = document.createElement('div');
+    outerShadow.appendChild(innerHost);
+
+    var innerShadow = innerHost.attachShadow({mode: 'open'});
+    var slot = document.createElement('slot');
+    innerShadow.appendChild(slot);
+
+    outerHost.remove();
+    innerHost.remove();
+}
+
+addShadowWithSlot();
+removeShadowWithSlot();
+removeNestedShadowsWithSlot()
+document.write('PASS');
+
+</script>
+</body>
+</html>
index 23a0622..0a28200 100644 (file)
@@ -1,5 +1,46 @@
 2015-09-18  Ryosuke Niwa  <rniwa@webkit.org>
 
+        Inserting or removing slot elements can cause a crash
+        https://bugs.webkit.org/show_bug.cgi?id=149365
+
+        Reviewed by Antti Koivisto.
+
+        HTMLSlotElement::insertedInto and removedFrom were doing completely non-sensical.
+
+        Since insertedInto and removedFrom are called on an element whenever it or its ancestor is inserted into
+        or removed from a container node, we can't always call addSlotElementByName removeSlotElementByName when
+        those functions are called. Instead, we need to check whether this slot has been inserted into or removed
+        from a container node that resides inside a shadow root.
+
+        Also reverted r189906 since the change was made upon a bogus assumption I had made.
+
+        Test: fast/shadow-dom/slot-removal-crash.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::insertedInto): Added comments.
+        (WebCore::Element::removedFrom): Ditto.
+        (WebCore::Element::addShadowRoot): Reverted r189906.
+        (WebCore::Element::removeShadowRoot): Ditto.
+
+        * html/HTMLSlotElement.cpp:
+        (WebCore::HTMLSlotElement::insertedInto): When the insertion point's tree scope is different from ours,
+        the insertion happened to our shadow host or its ancestor. There is nothing to be done in that case since
+        the shadow tree was not modified (in particular, our relationship with our shadow root never changed).
+        We also don't do anything if we got inserted into a parent which is not inside a shadow tree.
+
+        (WebCore::HTMLSlotElement::removedFrom): Since Container::removeBetween sets the tree scope before this
+        function is getting called, we can't compare this element's treeScope with that of the "insertion" point.
+        They're always different regardless of whether the insertion point was in the same shadow tree to which
+        we belong or its shadow host's. However, since a node removed from a shadow tree is put into document's
+        tree scope before this function is called and InShadowTree flag is unset in Node::removedFrom at the end
+        of this function, this slot element is definitely being removed from its shadow root when isInShadowTree()
+        is true and the newly set tree scope is of the document. So call removeSlotElementByName if and only if
+        that condition holds.
+
+        (WebCore::HTMLSlotElement::getDistributedNodes): Explicitly check that we're inside a shadow root.
+
+2015-09-18  Ryosuke Niwa  <rniwa@webkit.org>
+
         REGRESSION(r150187): updateIdForTreeScope may not be called inside shadow trees
         https://bugs.webkit.org/show_bug.cgi?id=149364
 
index 5f22934..bd3074d 100644 (file)
@@ -1521,6 +1521,9 @@ Node::InsertionNotificationRequest Element::insertedInto(ContainerNode& insertio
     if (!insertionPoint.isInTreeScope())
         return InsertionDone;
 
+    // This function could be called when this element's shadow root's host or its ancestor is inserted.
+    // This element is new to the shadow tree (and its tree scope) only if the parent into which this element
+    // or its ancestor is inserted belongs to the same tree scope as this element's.
     TreeScope* newScope = &insertionPoint.treeScope();
     HTMLDocument* newDocument = !wasInDocument && inDocument() && is<HTMLDocument>(newScope->documentScope()) ? &downcast<HTMLDocument>(newScope->documentScope()) : nullptr;
     if (newScope != &treeScope())
@@ -1566,6 +1569,9 @@ void Element::removedFrom(ContainerNode& insertionPoint)
     if (insertionPoint.isInTreeScope()) {
         TreeScope* oldScope = &insertionPoint.treeScope();
         HTMLDocument* oldDocument = inDocument() && is<HTMLDocument>(oldScope->documentScope()) ? &downcast<HTMLDocument>(oldScope->documentScope()) : nullptr;
+
+        // ContainerNode::removeBetween always sets the removed chid's tree scope to Document's but InTreeScope flag is unset in Node::removedFrom.
+        // So this element has been removed from the old tree scope only if InTreeScope flag is set and this element's tree scope is Document's.
         if (!isInTreeScope() || &treeScope() != &document())
             oldScope = nullptr;
 
@@ -1636,19 +1642,10 @@ void Element::addShadowRoot(Ref<ShadowRoot>&& newShadowRoot)
     shadowRoot.setHost(this);
     shadowRoot.setParentTreeScope(&treeScope());
 
-    auto shadowRootInsertionResult = shadowRoot.insertedInto(*this);
-    ASSERT_UNUSED(shadowRootInsertionResult, shadowRootInsertionResult == InsertionDone);
-    if (auto* firstChild = shadowRoot.firstChild()) {
-        NodeVector postInsertionNotificationTargets;
-        {
-            NoEventDispatchAssertion assertNoEventDispatch;
-            for (auto* child = firstChild; child; child = child->nextSibling())
-                notifyChildNodeInserted(shadowRoot, *child, postInsertionNotificationTargets);
-        }
-
-        for (auto& target : postInsertionNotificationTargets)
-            target->finishedInsertingSubtree();
-    }
+    NodeVector postInsertionNotificationTargets;
+    notifyChildNodeInserted(*this, shadowRoot, postInsertionNotificationTargets);
+    for (auto& target : postInsertionNotificationTargets)
+        target->finishedInsertingSubtree();
 
     resetNeedsNodeRenderingTraversalSlowPath();
 
@@ -1675,13 +1672,7 @@ void Element::removeShadowRoot()
     oldRoot->setHost(nullptr);
     oldRoot->setParentTreeScope(&document());
 
-    {
-        NoEventDispatchAssertion assertNoEventDispatch;
-        for (auto* child = oldRoot->firstChild(); child; child = child->nextSibling())
-            notifyChildNodeRemoved(*oldRoot, *child);
-    }
-
-    oldRoot->removedFrom(*this);
+    notifyChildNodeRemoved(*this, *oldRoot);
 }
 
 RefPtr<ShadowRoot> Element::createShadowRoot(ExceptionCode& ec)
index c40315d..abad7f4 100644 (file)
@@ -49,18 +49,25 @@ HTMLSlotElement::InsertionNotificationRequest HTMLSlotElement::insertedInto(Cont
     auto insertionResult = HTMLElement::insertedInto(insertionPoint);
     ASSERT_UNUSED(insertionResult, insertionResult == InsertionDone);
 
-    if (auto shadowRoot = containingShadowRoot())
-        shadowRoot->addSlotElementByName(fastGetAttribute(nameAttr), *this);
+    // This function could be called when this element's shadow root's host or its ancestor is inserted.
+    // This element is new to the shadow tree (and its tree scope) only if the parent into which this element
+    // or its ancestor is inserted belongs to the same tree scope as this element's.
+    if (insertionPoint.isInShadowTree() && isInShadowTree() && &insertionPoint.treeScope() == &treeScope()) {
+        if (auto shadowRoot = containingShadowRoot())
+            shadowRoot->addSlotElementByName(fastGetAttribute(nameAttr), *this);
+    }
 
     return InsertionDone;
 }
 
 void HTMLSlotElement::removedFrom(ContainerNode& insertionPoint)
 {
-    // Can't call containingShadowRoot() here since this node has already been disconnected from the parent.
-    if (isInShadowTree()) {
-        auto& oldShadowRoot = downcast<ShadowRoot>(insertionPoint.treeScope().rootNode());
-        oldShadowRoot.removeSlotElementByName(fastGetAttribute(nameAttr), *this);
+    // ContainerNode::removeBetween always sets the removed chid's tree scope to Document's but InShadowRoot flag is unset in Node::removedFrom.
+    // So if InShadowRoot flag is set but this element's tree scope is Document's, this element has just been removed from a shadow root.
+    if (insertionPoint.isInShadowTree() && isInShadowTree() && &treeScope() == &document()) {
+        auto* oldShadowRoot = insertionPoint.containingShadowRoot();
+        ASSERT(oldShadowRoot);
+        oldShadowRoot->removeSlotElementByName(fastGetAttribute(nameAttr), *this);
     }
 
     HTMLElement::removedFrom(insertionPoint);
@@ -70,7 +77,7 @@ void HTMLSlotElement::attributeChanged(const QualifiedName& name, const AtomicSt
 {
     HTMLElement::attributeChanged(name, oldValue, newValue, reason);
 
-    if (name == nameAttr) {
+    if (isInShadowTree() && name == nameAttr) {
         if (auto* shadowRoot = containingShadowRoot()) {
             shadowRoot->removeSlotElementByName(oldValue, *this);
             shadowRoot->addSlotElementByName(newValue, *this);