Crash in RenderTableSection::addCell.
authorjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jul 2012 02:24:30 +0000 (02:24 +0000)
committerjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Jul 2012 02:24:30 +0000 (02:24 +0000)
http://webkit.org/b/89496

Reviewed by Abhishek Arya.

Source/WebCore:

The issue comes from RenderBox::splitAnonymousBoxesAroundChild that would move sections
across tables but didn't force the table to do a synchronous section recalc. This opened
the way for race conditions where we would query the table column structure while it's dirty
(this is not uncommon but as usually the table's column representation is always bigger or
more split than a section's, it's usually harmless).

The fix is to force a synchronous section recalc.

Test: fast/table/split-table-no-section-update-crash.html

* rendering/RenderBox.cpp:
(WebCore::markBoxForRelayoutAfterSplit):
Changed to call forceSectionsRecalc ie force a section recalc.

* rendering/RenderTable.cpp:
(WebCore::RenderTable::recalcSections):
Added missing ASSERT for unneeded calls.

* rendering/RenderTable.h:
(WebCore::RenderTable::forceSectionsRecalc):
Added this helper function.

LayoutTests:

The test is still pretty complex as it involves lots of generated content. It should
be possible to get a smaller test case based on the conditions for the crash. However
this test is a pretty good stress test so I decided against creating a more simple test
case.

* fast/table/split-table-no-section-update-crash-expected.txt: Added.
* fast/table/split-table-no-section-update-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/table/split-table-no-section-update-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/table/split-table-no-section-update-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBox.cpp
Source/WebCore/rendering/RenderTable.cpp
Source/WebCore/rendering/RenderTable.h

index a046442..591a89f 100644 (file)
@@ -1,5 +1,20 @@
 2012-07-18  Julien Chaffraix  <jchaffraix@webkit.org>
 
+        Crash in RenderTableSection::addCell.
+        http://webkit.org/b/89496
+
+        Reviewed by Abhishek Arya.
+
+        The test is still pretty complex as it involves lots of generated content. It should
+        be possible to get a smaller test case based on the conditions for the crash. However
+        this test is a pretty good stress test so I decided against creating a more simple test
+        case.
+
+        * fast/table/split-table-no-section-update-crash-expected.txt: Added.
+        * fast/table/split-table-no-section-update-crash.html: Added.
+
+2012-07-18  Julien Chaffraix  <jchaffraix@webkit.org>
+
         Avoid calling GraphicsContext drawing primitives for 0px borders
         https://bugs.webkit.org/show_bug.cgi?id=90039
 
diff --git a/LayoutTests/fast/table/split-table-no-section-update-crash-expected.txt b/LayoutTests/fast/table/split-table-no-section-update-crash-expected.txt
new file mode 100644 (file)
index 0000000..f53e971
--- /dev/null
@@ -0,0 +1,6 @@
+Test for bug 89496: Crash in RenderTableSection::addCell.
+
+This test passes if it doesn't CRASH or ASSERT.
+
+Lorem Ipsum
+
diff --git a/LayoutTests/fast/table/split-table-no-section-update-crash.html b/LayoutTests/fast/table/split-table-no-section-update-crash.html
new file mode 100644 (file)
index 0000000..2e7511e
--- /dev/null
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+    :after { display: table-cell; content: "Foo"; }
+    :first-child { display: table-cell; }
+    :before { display: table-column; content: "Bar"; }
+</style>
+<script type="text/javascript"> 
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function crash(event) {
+    newContent = document.createTextNode("Lorem Ipsum");
+    var divElement = document.createElement("div");
+    divElement.appendChild(newContent);
+    divElement.appendChild(document.createElement("div"));
+    document.getElementById("target").appendChild(divElement);
+
+    // For some reason, DRT dumps the <style> so remove it here to clean the dump.
+    var style = document.getElementsByTagName("style")[0];
+    style.parentNode.removeChild(style);
+}
+
+window.addEventListener("load", crash, false)
+</script>
+</head>
+<body id="target">
+<p>Test for bug <a href="https://bugs.webkit.org/show_bug.cgi?id=89496">89496</a>: Crash in RenderTableSection::addCell.</p>
+<p>This test passes if it doesn't CRASH or ASSERT.</p>
+</body>
+</html>
index 05faee0..84eb98f 100644 (file)
@@ -1,5 +1,34 @@
 2012-07-18  Julien Chaffraix  <jchaffraix@webkit.org>
 
+        Crash in RenderTableSection::addCell.
+        http://webkit.org/b/89496
+
+        Reviewed by Abhishek Arya.
+
+        The issue comes from RenderBox::splitAnonymousBoxesAroundChild that would move sections
+        across tables but didn't force the table to do a synchronous section recalc. This opened
+        the way for race conditions where we would query the table column structure while it's dirty
+        (this is not uncommon but as usually the table's column representation is always bigger or
+        more split than a section's, it's usually harmless).
+
+        The fix is to force a synchronous section recalc.
+
+        Test: fast/table/split-table-no-section-update-crash.html
+
+        * rendering/RenderBox.cpp:
+        (WebCore::markBoxForRelayoutAfterSplit):
+        Changed to call forceSectionsRecalc ie force a section recalc.
+
+        * rendering/RenderTable.cpp:
+        (WebCore::RenderTable::recalcSections):
+        Added missing ASSERT for unneeded calls.
+
+        * rendering/RenderTable.h:
+        (WebCore::RenderTable::forceSectionsRecalc):
+        Added this helper function.
+
+2012-07-18  Julien Chaffraix  <jchaffraix@webkit.org>
+
         Avoid calling GraphicsContext drawing primitives for 0px borders
         https://bugs.webkit.org/show_bug.cgi?id=90039
 
index f55170f..1a2c361 100644 (file)
@@ -3960,9 +3960,11 @@ static void markBoxForRelayoutAfterSplit(RenderBox* box)
 {
     // FIXME: The table code should handle that automatically. If not,
     // we should fix it and remove the table part checks.
-    if (box->isTable())
-        toRenderTable(box)->setNeedsSectionRecalc();
-    else if (box->isTableSection())
+    if (box->isTable()) {
+        // Because we may have added some sections with already computed column structures, we need to
+        // sync the table structure with them now. This avoids crashes when adding new cells to the table.
+        toRenderTable(box)->forceSectionsRecalc();
+    } else if (box->isTableSection())
         toRenderTableSection(box)->setNeedsCellRecalc();
 
     box->setNeedsLayoutAndPrefWidthsRecalc();
index 3eac692..18bc996 100644 (file)
@@ -768,6 +768,8 @@ RenderTableCol* RenderTable::colElement(unsigned col, bool* startEdge, bool* end
 
 void RenderTable::recalcSections() const
 {
+    ASSERT(m_needsSectionRecalc);
+
     m_head = 0;
     m_foot = 0;
     m_firstBody = 0;
index b6d2506..87418f0 100644 (file)
@@ -135,6 +135,12 @@ public:
         unsigned span;
     };
 
+    void forceSectionsRecalc()
+    {
+        setNeedsSectionRecalc();
+        recalcSections();
+    }
+
     Vector<ColumnStruct>& columns() { return m_columns; }
     Vector<int>& columnPositions() { return m_columnPos; }
     RenderTableSection* header() const { return m_head; }