REGRESSION (r190840): crash inside details element's slotNameFunction
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Mar 2016 01:57:17 +0000 (01:57 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Mar 2016 01:57:17 +0000 (01:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155388

Reviewed by Antti Koivisto.

Source/WebCore:

The bug was caused by HTMLDetailsElement::isActiveSummary calling findAssignedSlot with a summary element
inside the shadow tree of the detials element. Fixed it by existing early when the summary element passed
to isActiveSummary is not a direct child of the details element.

Test: fast/html/details-summary-tabindex-crash.html

* dom/ShadowRoot.cpp:
(WebCore::ShadowRoot::findAssignedSlot): Added an assertion for regression testing.
* dom/SlotAssignment.cpp:
(WebCore::SlotAssignment::findAssignedSlot): Removed the superfluous call to assignSlots added in r190840.
There is no need to update the slot assignments here (entires in m_slots are added or removed by
addSlotElementByName or removeSlotElementByName and assignSlots only updates assignedNodes in each SlotInfo
which is never used in this function or findFirstSlotElement.
* html/HTMLDetailsElement.cpp:
(WebCore::HTMLDetailsElement::isActiveSummary): Fixed the bug.

LayoutTests:

Added a regression test.

* fast/html/details-summary-tabindex-crash-expected.txt: Added.
* fast/html/details-summary-tabindex-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/html/details-summary-tabindex-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/html/details-summary-tabindex-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/ShadowRoot.cpp
Source/WebCore/dom/SlotAssignment.cpp
Source/WebCore/html/HTMLDetailsElement.cpp

index df7d38a..2d9ed7a 100644 (file)
@@ -1,3 +1,15 @@
+2016-03-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION (r190840): crash inside details element's slotNameFunction
+        https://bugs.webkit.org/show_bug.cgi?id=155388
+
+        Reviewed by Antti Koivisto.
+
+        Added a regression test.
+
+        * fast/html/details-summary-tabindex-crash-expected.txt: Added.
+        * fast/html/details-summary-tabindex-crash.html: Added.
+
 2016-03-13  Dean Jackson  <dino@apple.com>
 
         <attachment> should be a runtime-enabled feature
diff --git a/LayoutTests/fast/html/details-summary-tabindex-crash-expected.txt b/LayoutTests/fast/html/details-summary-tabindex-crash-expected.txt
new file mode 100644 (file)
index 0000000..0ba67e2
--- /dev/null
@@ -0,0 +1,4 @@
+This tests calling tabIndex on the summary element inside a details element. WebKit should not hit any assertions.
+
+summary content
+PASS
diff --git a/LayoutTests/fast/html/details-summary-tabindex-crash.html b/LayoutTests/fast/html/details-summary-tabindex-crash.html
new file mode 100644 (file)
index 0000000..c789828
--- /dev/null
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests calling tabIndex on the summary element inside a details element. WebKit should not hit any assertions.</p>
+<details tabindex=0>
+<summary>summary content</summary>
+summary details
+</details>
+<script>
+
+if (window.internals) {
+    testRunner.dumpAsText();
+
+    var detailsShadow = internals.shadowRoot(document.querySelector('details'));
+    detailsShadow.querySelector('summary').tabIndex;
+
+    document.write('PASS');
+} else
+    document.write('FAIL - This test requires window.internals');
+
+
+</script>
+</body>
+</html>
index 8ef6488..71bd401 100644 (file)
@@ -1,3 +1,26 @@
+2016-03-13  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION (r190840): crash inside details element's slotNameFunction
+        https://bugs.webkit.org/show_bug.cgi?id=155388
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by HTMLDetailsElement::isActiveSummary calling findAssignedSlot with a summary element
+        inside the shadow tree of the detials element. Fixed it by existing early when the summary element passed
+        to isActiveSummary is not a direct child of the details element.
+
+        Test: fast/html/details-summary-tabindex-crash.html
+
+        * dom/ShadowRoot.cpp:
+        (WebCore::ShadowRoot::findAssignedSlot): Added an assertion for regression testing.
+        * dom/SlotAssignment.cpp:
+        (WebCore::SlotAssignment::findAssignedSlot): Removed the superfluous call to assignSlots added in r190840.
+        There is no need to update the slot assignments here (entires in m_slots are added or removed by
+        addSlotElementByName or removeSlotElementByName and assignSlots only updates assignedNodes in each SlotInfo
+        which is never used in this function or findFirstSlotElement.
+        * html/HTMLDetailsElement.cpp:
+        (WebCore::HTMLDetailsElement::isActiveSummary): Fixed the bug.
+
 2016-03-13  Antti Koivisto  <antti@apple.com>
 
         ComposedTreeIterator fails to traverse slots if root is shadow host
index 47aac34..c6f7212 100644 (file)
@@ -182,6 +182,7 @@ void ShadowRoot::removeAllEventListeners()
 
 HTMLSlotElement* ShadowRoot::findAssignedSlot(const Node& node)
 {
+    ASSERT(node.parentNode() == host());
     if (!m_slotAssignment)
         return nullptr;
     return m_slotAssignment->findAssignedSlot(node, *this);
index 2c45b53..9171849 100644 (file)
@@ -64,9 +64,6 @@ HTMLSlotElement* SlotAssignment::findAssignedSlot(const Node& node, ShadowRoot&
     if (!is<Text>(node) && !is<Element>(node))
         return nullptr;
 
-    if (!m_slotAssignmentsIsValid)
-        assignSlots(shadowRoot);
-
     auto slotName = m_slotNameFunction(node);
     if (!slotName)
         return nullptr;
index 4ce491e..e8bdf30 100644 (file)
@@ -103,6 +103,9 @@ bool HTMLDetailsElement::isActiveSummary(const HTMLSummaryElement& summary) cons
     if (!m_summarySlot->assignedNodes())
         return &summary == m_defaultSummary;
 
+    if (summary.parentNode() != this)
+        return false;
+
     auto* slot = shadowRoot()->findAssignedSlot(summary);
     if (!slot)
         return false;