Crash due to column span under button element
authorkenrb@chromium.org <kenrb@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Nov 2012 04:55:23 +0000 (04:55 +0000)
committerkenrb@chromium.org <kenrb@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Nov 2012 04:55:23 +0000 (04:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=101402

Reviewed by Abhishek Arya.

Source/WebCore:

When there is a column-spanning child of a RenderButton
splitBlocks() must split the RenderButton as well as its
only permitted direct child, the anonymous block referenced
by m_inner. A crash was occurring because splitBlocks()
calls addChildIgnoringAnonymousColumnBlocks() to add the
cloned m_inner to the cloned RenderButton, which meant the
m_inner for the cloned RenderButton was not being set even
though a child was being added. This violates state
assumptions in the RenderButton code.

This patch prevents any descendants of RenderButton from
spanning columns. Also, it adds a precautionary check in
RenderButton::removeChild() to mitigate problems if similar
state problems are found in future.

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::containingColumnsBlock):
* rendering/RenderButton.cpp:
(WebCore::RenderButton::removeChild):

LayoutTests:

Test creates crashing condition for bug 101402.

* fast/block/colspan-under-button-crash.html: Added.
* fast/block/colspan-under-button-crash-expected.txt: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/block/colspan-under-button-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/block/colspan-under-button-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderButton.cpp

index 7a3cb1a..342aefb 100644 (file)
@@ -1,3 +1,15 @@
+2012-11-06  Ken Buchanan  <kenrb@chromium.org>
+
+        Crash due to column span under button element
+        https://bugs.webkit.org/show_bug.cgi?id=101402
+
+        Reviewed by Abhishek Arya.
+
+        Test creates crashing condition for bug 101402.
+
+        * fast/block/colspan-under-button-crash.html: Added.
+        * fast/block/colspan-under-button-crash-expected.txt: Added.
+
 2012-11-06  Jaehun Lim  <ljaehun.lim@samsung.com>
 
         [EFL][WK2] fast/dom/shadow/shadowdom-for-object-only-shadow.html fails
diff --git a/LayoutTests/fast/block/colspan-under-button-crash-expected.txt b/LayoutTests/fast/block/colspan-under-button-crash-expected.txt
new file mode 100644 (file)
index 0000000..72416a1
--- /dev/null
@@ -0,0 +1 @@
+PASS if no crash or assert in debug
diff --git a/LayoutTests/fast/block/colspan-under-button-crash.html b/LayoutTests/fast/block/colspan-under-button-crash.html
new file mode 100644 (file)
index 0000000..c135168
--- /dev/null
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<style>
+.c0 { display: inherit; }
+.c10 { display: table-column-group; -webkit-column-span: all; }
+.c11[class$="c11"] { vertical-align: -10; -webkit-column-width: 1px; }
+.c17 { overflow: hidden; position: fixed; }
+.c17::after { position: inherit; content: no-close-quote;</style>
+<script>
+window.onload = function() {
+    pElem = document.createElement('p');
+    pElem.setAttribute('class', 'c11');
+
+    citeElem = document.createElement('cite');
+    citeElem.setAttribute('class', 'c10');
+
+    buttonElem = document.createElement('button');
+    buttonElem.setAttribute('class', 'c0');
+
+    document.documentElement.appendChild(pElem);
+    pElem.appendChild(buttonElem);
+    buttonElem.appendChild(citeElem);
+
+    document.documentElement.offsetHeight;
+    buttonElem.setAttribute('class', 'c17');
+    document.documentElement.offsetHeight;
+    document.documentElement.removeChild(pElem);
+
+    document.body.innerHTML = "PASS if no crash or assert in debug";
+    if (window.testRunner)
+        testRunner.dumpAsText();
+}
+</script>
+</html>
index 19257e7..1986d47 100644 (file)
@@ -1,3 +1,30 @@
+2012-11-06  Ken Buchanan  <kenrb@chromium.org>
+
+        Crash due to column span under button element
+        https://bugs.webkit.org/show_bug.cgi?id=101402
+
+        Reviewed by Abhishek Arya.
+
+        When there is a column-spanning child of a RenderButton
+        splitBlocks() must split the RenderButton as well as its
+        only permitted direct child, the anonymous block referenced
+        by m_inner. A crash was occurring because splitBlocks()
+        calls addChildIgnoringAnonymousColumnBlocks() to add the
+        cloned m_inner to the cloned RenderButton, which meant the
+        m_inner for the cloned RenderButton was not being set even
+        though a child was being added. This violates state
+        assumptions in the RenderButton code.
+
+        This patch prevents any descendants of RenderButton from
+        spanning columns. Also, it adds a precautionary check in
+        RenderButton::removeChild() to mitigate problems if similar
+        state problems are found in future.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::containingColumnsBlock):
+        * rendering/RenderButton.cpp:
+        (WebCore::RenderButton::removeChild):
+
 2012-11-06  Shinya Kawanaka  <shinyak@chromium.org>
 
         [Shadow] Pseudo custom-elements should start with 'x-'.
index 7175c28..4708985 100644 (file)
@@ -533,7 +533,10 @@ RenderBlock* RenderBlock::containingColumnsBlock(bool allowAnonymousColumnBlock)
         // FIXME: Table manages its own table parts, most of which are RenderBoxes.
         // Multi-column code cannot handle splitting the flow in table. Disabling it
         // to prevent crashes.
-        if (curr->isTable())
+        // Similarly, RenderButton maintains an anonymous block child and overrides
+        // addChild() to prevent itself from having additional direct children. This
+        // causes problems for split flows.
+        if (curr->isTable() || curr->isRenderButton())
             return 0;
         
         RenderBlock* currBlock = toRenderBlock(curr);
index 7103e6e..69cef29 100644 (file)
@@ -60,7 +60,11 @@ void RenderButton::addChild(RenderObject* newChild, RenderObject* beforeChild)
 
 void RenderButton::removeChild(RenderObject* oldChild)
 {
-    if (oldChild == m_inner || !m_inner) {
+    // m_inner should be the only child, but checking for direct children who
+    // are not m_inner prevents security problems when that assumption is
+    // violated.
+    if (oldChild == m_inner || !m_inner || oldChild->parent() == this) {
+        ASSERT(oldChild == m_inner || !m_inner);
         RenderDeprecatedFlexibleBox::removeChild(oldChild);
         m_inner = 0;
     } else