Title: [123513] branches/chromium/1180
- Revision
- 123513
- Author
- [email protected]
- Date
- 2012-07-24 13:04:42 -0700 (Tue, 24 Jul 2012)
Log Message
Merge 123062 - Crash in RenderTableSection::addCell.
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.
[email protected]
Review URL: https://chromiumcodereview.appspot.com/10809080
Modified Paths
Added Paths
Diff
Copied: branches/chromium/1180/LayoutTests/fast/table/split-table-no-section-update-crash-expected.txt (from rev 123062, trunk/LayoutTests/fast/table/split-table-no-section-update-crash-expected.txt) (0 => 123513)
--- branches/chromium/1180/LayoutTests/fast/table/split-table-no-section-update-crash-expected.txt (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/table/split-table-no-section-update-crash-expected.txt 2012-07-24 20:04:42 UTC (rev 123513)
@@ -0,0 +1,6 @@
+Test for bug 89496: Crash in RenderTableSection::addCell.
+
+This test passes if it doesn't CRASH or ASSERT.
+
+Lorem Ipsum
+
Copied: branches/chromium/1180/LayoutTests/fast/table/split-table-no-section-update-crash.html (from rev 123062, trunk/LayoutTests/fast/table/split-table-no-section-update-crash.html) (0 => 123513)
--- branches/chromium/1180/LayoutTests/fast/table/split-table-no-section-update-crash.html (rev 0)
+++ branches/chromium/1180/LayoutTests/fast/table/split-table-no-section-update-crash.html 2012-07-24 20:04:42 UTC (rev 123513)
@@ -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="" Crash in RenderTableSection::addCell.</p>
+<p>This test passes if it doesn't CRASH or ASSERT.</p>
+</body>
+</html>
Modified: branches/chromium/1180/Source/WebCore/rendering/RenderBox.cpp (123512 => 123513)
--- branches/chromium/1180/Source/WebCore/rendering/RenderBox.cpp 2012-07-24 19:59:45 UTC (rev 123512)
+++ branches/chromium/1180/Source/WebCore/rendering/RenderBox.cpp 2012-07-24 20:04:42 UTC (rev 123513)
@@ -3951,9 +3951,11 @@
{
// 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();
Modified: branches/chromium/1180/Source/WebCore/rendering/RenderTable.cpp (123512 => 123513)
--- branches/chromium/1180/Source/WebCore/rendering/RenderTable.cpp 2012-07-24 19:59:45 UTC (rev 123512)
+++ branches/chromium/1180/Source/WebCore/rendering/RenderTable.cpp 2012-07-24 20:04:42 UTC (rev 123513)
@@ -770,6 +770,8 @@
void RenderTable::recalcSections() const
{
+ ASSERT(m_needsSectionRecalc);
+
m_head = 0;
m_foot = 0;
m_firstBody = 0;
Modified: branches/chromium/1180/Source/WebCore/rendering/RenderTable.h (123512 => 123513)
--- branches/chromium/1180/Source/WebCore/rendering/RenderTable.h 2012-07-24 19:59:45 UTC (rev 123512)
+++ branches/chromium/1180/Source/WebCore/rendering/RenderTable.h 2012-07-24 20:04:42 UTC (rev 123513)
@@ -135,6 +135,12 @@
unsigned span;
};
+ void forceSectionsRecalc()
+ {
+ setNeedsSectionRecalc();
+ recalcSections();
+ }
+
Vector<ColumnStruct>& columns() { return m_columns; }
Vector<int>& columnPositions() { return m_columnPos; }
RenderTableSection* header() const { return m_head; }
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes
