REGRESSION (r190430): WTFCrashWithSecurityImplication in:void SVGRootInlineBox::layou...
authorsaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Feb 2016 23:59:25 +0000 (23:59 +0000)
committersaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 Feb 2016 23:59:25 +0000 (23:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154185

Reviewed by Ryosuke Niwa.
Source/WebCore:

This is a regression caused by adding support for HTMLSlotElement. The
crash happens when adding an HTMLSlotElement to anther element which should
not have it as a child like SVGTextElement for example. In this case, we
were creating a RenderText which should not be happen inside an SVG document.
The RenderText::createTextBox() was creating InlineTextBox for the slot's
text and attach it to the SVGRootInlineBox. In layoutCharactersInTextBoxes(),
the assumption is the inline box is either SVGInlineTextBox or SVGInlineFlowBox.
But since we have an InlineTextBox instead, the crash happens when casting
the InlineTextBox to SVGInlineFlowBox.

The fix is for createRenderTreeForSlotAssignees() to not create a renderer
when the parent element should not have a renderer for the this element.
This is the same thing we do for createRenderer() which handles the non
HTMLSlotElement case and which is called also from createRenderTreeRecursively().

Test: fast/shadow-dom/text-slot-child-crash.svg

* style/StyleTreeResolver.cpp:
(WebCore::Style::moveToFlowThreadIfNeeded):
(WebCore::Style::TreeResolver::createRenderer): Delete the check for
shouldCreateRenderer() and handling the case when resolvedStyle is null
since these are handled by the caller createRenderTreeRecursively().

(WebCore::Style::TreeResolver::createRenderTreeForSlotAssignees):
Assert shouldCreateRenderer() is true for this element.

(WebCore::Style::TreeResolver::createRenderTreeRecursively): Don't create
the renderer if shouldCreateRenderer() returns false. Also handle the case
when resolvedStyle is null and pass the new style to createRenderer().

* style/StyleTreeResolver.h:

LayoutTests:

Ensure that adding an HTMLSlotElement with text to an SVGTextElement will
not create a renderer and we won't crash.

* fast/shadow-dom/text-slot-child-crash-expected.txt: Added.
* fast/shadow-dom/text-slot-child-crash.svg: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/shadow-dom/text-slot-child-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/shadow-dom/text-slot-child-crash.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/style/StyleTreeResolver.cpp
Source/WebCore/style/StyleTreeResolver.h

index 97b8a40..5116809 100644 (file)
@@ -1,3 +1,16 @@
+2016-02-16  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION (r190430): WTFCrashWithSecurityImplication in:void SVGRootInlineBox::layoutCharactersInTextBoxes()
+        https://bugs.webkit.org/show_bug.cgi?id=154185
+
+        Reviewed by Ryosuke Niwa.
+        
+        Ensure that adding an HTMLSlotElement with text to an SVGTextElement will
+        not create a renderer and we won't crash.
+
+        * fast/shadow-dom/text-slot-child-crash-expected.txt: Added.
+        * fast/shadow-dom/text-slot-child-crash.svg: Added.
+
 2016-02-16  Simon Fraser  <simon.fraser@apple.com>
 
         Add tests for iframe and overflow scrollability after navigating back
diff --git a/LayoutTests/fast/shadow-dom/text-slot-child-crash-expected.txt b/LayoutTests/fast/shadow-dom/text-slot-child-crash-expected.txt
new file mode 100644 (file)
index 0000000..7ef22e9
--- /dev/null
@@ -0,0 +1 @@
+PASS
diff --git a/LayoutTests/fast/shadow-dom/text-slot-child-crash.svg b/LayoutTests/fast/shadow-dom/text-slot-child-crash.svg
new file mode 100644 (file)
index 0000000..e5bada4
--- /dev/null
@@ -0,0 +1,13 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+    <text id="text">
+        <tspan>PASS</tspan>
+    </text>
+    <script>
+        if (window.testRunner)
+            testRunner.dumpAsText();
+        var slotElement = document.createElementNS("http://www.w3.org/1999/xhtml", "slot");
+        slotElement.innerHTML = "Some text";
+        var parent = document.getElementById("text");
+        parent.appendChild(slotElement);
+    </script>
+</svg>
index 18ca70f..4bdd14b 100644 (file)
@@ -1,3 +1,42 @@
+2016-02-16  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION (r190430): WTFCrashWithSecurityImplication in:void SVGRootInlineBox::layoutCharactersInTextBoxes()
+        https://bugs.webkit.org/show_bug.cgi?id=154185
+
+        Reviewed by Ryosuke Niwa.
+
+        This is a regression caused by adding support for HTMLSlotElement. The
+        crash happens when adding an HTMLSlotElement to anther element which should
+        not have it as a child like SVGTextElement for example. In this case, we
+        were creating a RenderText which should not be happen inside an SVG document.
+        The RenderText::createTextBox() was creating InlineTextBox for the slot's
+        text and attach it to the SVGRootInlineBox. In layoutCharactersInTextBoxes(),
+        the assumption is the inline box is either SVGInlineTextBox or SVGInlineFlowBox.
+        But since we have an InlineTextBox instead, the crash happens when casting
+        the InlineTextBox to SVGInlineFlowBox.
+
+        The fix is for createRenderTreeForSlotAssignees() to not create a renderer
+        when the parent element should not have a renderer for the this element.
+        This is the same thing we do for createRenderer() which handles the non
+        HTMLSlotElement case and which is called also from createRenderTreeRecursively().
+        
+        Test: fast/shadow-dom/text-slot-child-crash.svg
+
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::moveToFlowThreadIfNeeded):
+        (WebCore::Style::TreeResolver::createRenderer): Delete the check for
+        shouldCreateRenderer() and handling the case when resolvedStyle is null
+        since these are handled by the caller createRenderTreeRecursively().
+        
+        (WebCore::Style::TreeResolver::createRenderTreeForSlotAssignees):
+        Assert shouldCreateRenderer() is true for this element.
+        
+        (WebCore::Style::TreeResolver::createRenderTreeRecursively): Don't create
+        the renderer if shouldCreateRenderer() returns false. Also handle the case
+        when resolvedStyle is null and pass the new style to createRenderer().
+        
+        * style/StyleTreeResolver.h:
+
 2016-02-16  Simon Fraser  <simon.fraser@apple.com>
 
         Every RenderLayer should not have to remove itself from the scrollableArea set
index edb3b21..ab7e8db 100644 (file)
@@ -188,24 +188,17 @@ static RenderNamedFlowThread* moveToFlowThreadIfNeeded(Element& element, const R
 }
 #endif
 
-void TreeResolver::createRenderer(Element& element, RenderStyle& inheritedStyle, RenderTreePosition& renderTreePosition, RefPtr<RenderStyle>&& resolvedStyle)
+void TreeResolver::createRenderer(Element& element, RenderTreePosition& renderTreePosition, RefPtr<RenderStyle>&& resolvedStyle)
 {
-    ASSERT(!element.renderer());
-
-    RefPtr<RenderStyle> style = resolvedStyle;
-
-    if (!shouldCreateRenderer(element, renderTreePosition.parent()))
-        return;
-
-    if (!style)
-        style = styleForElement(element, inheritedStyle);
+    ASSERT(shouldCreateRenderer(element, renderTreePosition.parent()));
+    ASSERT(resolvedStyle);
 
     RenderNamedFlowThread* parentFlowRenderer = 0;
 #if ENABLE(CSS_REGIONS)
-    parentFlowRenderer = moveToFlowThreadIfNeeded(element, *style);
+    parentFlowRenderer = moveToFlowThreadIfNeeded(element, *resolvedStyle);
 #endif
 
-    if (!element.rendererIsNeeded(*style))
+    if (!element.rendererIsNeeded(*resolvedStyle))
         return;
 
     renderTreePosition.computeNextSibling(element);
@@ -214,7 +207,7 @@ void TreeResolver::createRenderer(Element& element, RenderStyle& inheritedStyle,
         ? RenderTreePosition(*parentFlowRenderer, parentFlowRenderer->nextRendererForElement(element))
         : renderTreePosition;
 
-    RenderElement* newRenderer = element.createElementRenderer(style.releaseNonNull(), insertionPosition).leakPtr();
+    RenderElement* newRenderer = element.createElementRenderer(resolvedStyle.releaseNonNull(), insertionPosition).leakPtr();
     if (!newRenderer)
         return;
     if (!insertionPosition.canInsert(*newRenderer)) {
@@ -479,6 +472,8 @@ void TreeResolver::createRenderTreeForBeforeOrAfterPseudoElement(Element& curren
 #if ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)
 void TreeResolver::createRenderTreeForSlotAssignees(HTMLSlotElement& slot, RenderStyle& inheritedStyle, RenderTreePosition& renderTreePosition)
 {
+    ASSERT(shouldCreateRenderer(slot, renderTreePosition.parent()));
+
     if (auto* assignedNodes = slot.assignedNodes()) {
         pushEnclosingScope();
         for (auto* child : *assignedNodes) {
@@ -500,12 +495,21 @@ void TreeResolver::createRenderTreeForSlotAssignees(HTMLSlotElement& slot, Rende
 
 void TreeResolver::createRenderTreeRecursively(Element& current, RenderStyle& inheritedStyle, RenderTreePosition& renderTreePosition, RefPtr<RenderStyle>&& resolvedStyle)
 {
+    ASSERT(!current.renderer());
+
     PostResolutionCallbackDisabler callbackDisabler(m_document);
     WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
 
+    bool shouldCallCreateRenderer = shouldCreateRenderer(current, renderTreePosition.parent());
+
+    RefPtr<RenderStyle> style = resolvedStyle;
+    if (!style)
+        style = styleForElement(current, inheritedStyle);
+
 #if ENABLE(SHADOW_DOM) || ENABLE(DETAILS_ELEMENT)
     if (is<HTMLSlotElement>(current)) {
-        createRenderTreeForSlotAssignees(downcast<HTMLSlotElement>(current), inheritedStyle, renderTreePosition);
+        if (shouldCallCreateRenderer && current.rendererIsNeeded(*style))
+            createRenderTreeForSlotAssignees(downcast<HTMLSlotElement>(current), inheritedStyle, renderTreePosition);
         return;
     }
 #endif
@@ -513,7 +517,8 @@ void TreeResolver::createRenderTreeRecursively(Element& current, RenderStyle& in
     if (current.hasCustomStyleResolveCallbacks())
         current.willAttachRenderers();
 
-    createRenderer(current, inheritedStyle, renderTreePosition, WTFMove(resolvedStyle));
+    if (shouldCallCreateRenderer)
+        createRenderer(current, renderTreePosition, style.releaseNonNull());
 
     if (auto* renderer = current.renderer()) {
         SelectorFilterPusher selectorFilterPusher(scope().selectorFilter, current, SelectorFilterPusher::NoPush);
index 3fc7884..5aa560f 100644 (file)
@@ -67,7 +67,7 @@ private:
     void resolveBeforeOrAfterPseudoElement(Element&, Change, PseudoId, RenderTreePosition&);
 
     void createRenderTreeRecursively(Element&, RenderStyle&, RenderTreePosition&, RefPtr<RenderStyle>&& resolvedStyle);
-    void createRenderer(Element&, RenderStyle& inheritedStyle, RenderTreePosition&, RefPtr<RenderStyle>&& resolvedStyle);
+    void createRenderer(Element&, RenderTreePosition&, RefPtr<RenderStyle>&& resolvedStyle);
     void createRenderTreeForBeforeOrAfterPseudoElement(Element&, PseudoId, RenderTreePosition&);
     void createRenderTreeForChildren(ContainerNode&, RenderStyle&, RenderTreePosition&);
     void createRenderTreeForShadowRoot(ShadowRoot&);