Crash in render tree after dynamically mutating the slot value
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Feb 2017 02:19:04 +0000 (02:19 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Feb 2017 02:19:04 +0000 (02:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167502

Patch by Ryosuke Niwa <rniwa@webkit.org> on 2017-02-09
Reviewed by Antti Koivisto.

Source/WebCore:

The crash was caused by attributeChanged not destructing the render tree after an assigned element had been
removed from its slot. Since the style resolver can no longer find this element in the flat tree, we need to
delete its render object as if the element had been removed from the DOM tree.

Tests: fast/html/details-summary-slot.html
       fast/shadow-dom/shadow-slot-attribute-change-crash.html

* dom/Element.cpp:
(WebCore::Element::attributeChanged):
* html/HTMLSummaryElement.cpp:
(WebCore::SummarySlotElement): Added. Always use the default slot regardless of the slot attribute's value.
(WebCore::HTMLSummaryElement::create): Use SummarySlotElement

LayoutTests:

Added regression tests for the crash, and one for assigning non-empty slot value to a child
of a summary element. The slot attribute should always be ignored since the fact summary
element has its own shadow tree is an implementation detail that should never be exposed.

* fast/html/details-summary-slot-expected.html: Added.
* fast/html/details-summary-slot.html: Added.
* fast/shadow-dom/shadow-slot-attribute-change-crash-expected.txt: Added.
* fast/shadow-dom/shadow-slot-attribute-change-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/html/details-summary-slot-expected.html [new file with mode: 0644]
LayoutTests/fast/html/details-summary-slot.html [new file with mode: 0644]
LayoutTests/fast/shadow-dom/shadow-slot-attribute-change-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/shadow-dom/shadow-slot-attribute-change-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/ShadowRoot.h
Source/WebCore/dom/SlotAssignment.h
Source/WebCore/html/HTMLSummaryElement.cpp

index 5f442e9..d9a8a00 100644 (file)
@@ -1,3 +1,19 @@
+2017-02-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash in render tree after dynamically mutating the slot value
+        https://bugs.webkit.org/show_bug.cgi?id=167502
+
+        Reviewed by Antti Koivisto.
+
+        Added regression tests for the crash, and one for assigning non-empty slot value to a child
+        of a summary element. The slot attribute should always be ignored since the fact summary
+        element has its own shadow tree is an implementation detail that should never be exposed.
+
+        * fast/html/details-summary-slot-expected.html: Added.
+        * fast/html/details-summary-slot.html: Added.
+        * fast/shadow-dom/shadow-slot-attribute-change-crash-expected.txt: Added.
+        * fast/shadow-dom/shadow-slot-attribute-change-crash.html: Added.
+
 2017-02-09  Antti Koivisto  <antti@apple.com>
 
         Details element doesn't work correctly when mutating content between closing and opening
diff --git a/LayoutTests/fast/html/details-summary-slot-expected.html b/LayoutTests/fast/html/details-summary-slot-expected.html
new file mode 100644 (file)
index 0000000..68a4c04
--- /dev/null
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests sets slot on a child of summary element inside a details element. It should have no effect.</p>
+<details><summary><div>PASS</div></summary></details>
+</body>
+</html>
diff --git a/LayoutTests/fast/html/details-summary-slot.html b/LayoutTests/fast/html/details-summary-slot.html
new file mode 100644 (file)
index 0000000..edfee2b
--- /dev/null
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests sets slot on a child of summary element inside a details element. It should have no effect.</p>
+<details><summary><div>PASS</div></summary></details>
+<script>
+document.querySelector('div').slot = 'foo';
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/shadow-dom/shadow-slot-attribute-change-crash-expected.txt b/LayoutTests/fast/shadow-dom/shadow-slot-attribute-change-crash-expected.txt
new file mode 100644 (file)
index 0000000..6477f6c
--- /dev/null
@@ -0,0 +1,3 @@
+This tests dynamically mutating the slot value. WebKit should not crash.
+
+PASS - WebKit did not crash
diff --git a/LayoutTests/fast/shadow-dom/shadow-slot-attribute-change-crash.html b/LayoutTests/fast/shadow-dom/shadow-slot-attribute-change-crash.html
new file mode 100644 (file)
index 0000000..67579ea
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests dynamically mutating the slot value. WebKit should not crash.</p>
+<outer-host style="display: block; -webkit-column-count: 2;"><inner-host style="display: block;"><div id="mutateThis">?<section style="column-span: all;"></section></div></inner-host></outer-host>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+let outerHost = document.querySelector('outer-host');
+outerHost.attachShadow({mode: 'closed'}).innerHTML = '<slot></slot>';
+
+let innerHost = document.querySelector('inner-host').attachShadow({mode: 'closed'});
+innerHost.innerHTML = '<slot></slot>';
+
+outerHost.getBoundingClientRect();
+mutateThis.slot = "f";
+outerHost.getBoundingClientRect();
+mutateThis.innerText = "";
+
+document.write('PASS - WebKit did not crash');
+
+</script>
+</body>
+</html>
index b4f1a53..a1dbaee 100644 (file)
@@ -1,3 +1,23 @@
+2017-02-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Crash in render tree after dynamically mutating the slot value
+        https://bugs.webkit.org/show_bug.cgi?id=167502
+
+        Reviewed by Antti Koivisto.
+
+        The crash was caused by attributeChanged not destructing the render tree after an assigned element had been
+        removed from its slot. Since the style resolver can no longer find this element in the flat tree, we need to
+        delete its render object as if the element had been removed from the DOM tree.
+
+        Tests: fast/html/details-summary-slot.html
+               fast/shadow-dom/shadow-slot-attribute-change-crash.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::attributeChanged):
+        * html/HTMLSummaryElement.cpp:
+        (WebCore::SummarySlotElement): Added. Always use the default slot regardless of the slot attribute's value.
+        (WebCore::HTMLSummaryElement::create): Use SummarySlotElement
+
 2017-02-09  Antti Koivisto  <antti@apple.com>
 
         Details element doesn't work correctly when mutating content between closing and opening
index 1b12645..9c3a726 100644 (file)
@@ -1326,7 +1326,7 @@ void Element::attributeChanged(const QualifiedName& name, const AtomicString& ol
         else if (name == HTMLNames::slotAttr) {
             if (auto* parent = parentElement()) {
                 if (auto* shadowRoot = parent->shadowRoot())
-                    shadowRoot->hostChildElementDidChangeSlotAttribute(oldValue, newValue);
+                    shadowRoot->hostChildElementDidChangeSlotAttribute(*this, oldValue, newValue);
             }
         }
     }
index e34ec01..86532e1 100644 (file)
@@ -77,7 +77,7 @@ public:
     void didRemoveAllChildrenOfShadowHost();
     void didChangeDefaultSlot();
     void hostChildElementDidChange(const Element&);
-    void hostChildElementDidChangeSlotAttribute(const AtomicString& oldValue, const AtomicString& newValue);
+    void hostChildElementDidChangeSlotAttribute(Element&, const AtomicString& oldValue, const AtomicString& newValue);
 
     const Vector<Node*>* assignedNodesForSlot(const HTMLSlotElement&);
 
index e693bb2..e5ce9a7 100644 (file)
@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include "RenderTreeUpdater.h"
 #include "ShadowRoot.h"
 #include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
@@ -113,12 +114,13 @@ inline void ShadowRoot::hostChildElementDidChange(const Element& childElement)
         m_slotAssignment->hostChildElementDidChange(childElement, *this);
 }
 
-inline void ShadowRoot::hostChildElementDidChangeSlotAttribute(const AtomicString& oldValue, const AtomicString& newValue)
+inline void ShadowRoot::hostChildElementDidChangeSlotAttribute(Element& element, const AtomicString& oldValue, const AtomicString& newValue)
 {
-    if (m_slotAssignment) {
-        m_slotAssignment->didChangeSlot(oldValue, *this);
-        m_slotAssignment->didChangeSlot(newValue, *this);
-    }
+    if (!m_slotAssignment)
+        return;
+    m_slotAssignment->didChangeSlot(oldValue, *this);
+    m_slotAssignment->didChangeSlot(newValue, *this);
+    RenderTreeUpdater::tearDownRenderers(element);
 }
 
 } // namespace WebCore
index f35f962..9910bd1 100644 (file)
 #include "PlatformMouseEvent.h"
 #include "RenderBlockFlow.h"
 #include "ShadowRoot.h"
+#include "SlotAssignment.h"
 
 namespace WebCore {
 
 using namespace HTMLNames;
 
+class SummarySlotElement final : public SlotAssignment {
+private:
+    void hostChildElementDidChange(const Element&, ShadowRoot& shadowRoot) override
+    {
+        didChangeSlot(SlotAssignment::defaultSlotName(), shadowRoot);
+    }
+
+    const AtomicString& slotNameForHostChild(const Node&) const override { return SlotAssignment::defaultSlotName(); }
+};
+
 Ref<HTMLSummaryElement> HTMLSummaryElement::create(const QualifiedName& tagName, Document& document)
 {
     Ref<HTMLSummaryElement> summary = adoptRef(*new HTMLSummaryElement(tagName, document));
-    summary->addShadowRoot(ShadowRoot::create(document, ShadowRootMode::UserAgent));
+    summary->addShadowRoot(ShadowRoot::create(document, std::make_unique<SummarySlotElement>()));
     return summary;
 }