Diff
Modified: trunk/LayoutTests/ChangeLog (99743 => 99744)
--- trunk/LayoutTests/ChangeLog 2011-11-09 19:45:08 UTC (rev 99743)
+++ trunk/LayoutTests/ChangeLog 2011-11-09 19:57:16 UTC (rev 99744)
@@ -1,3 +1,20 @@
+2011-11-09 Julien Chaffraix <[email protected]>
+
+ Crash in RenderTableSection::splitColumn
+ https://bugs.webkit.org/show_bug.cgi?id=70171
+
+ Reviewed by David Hyatt.
+
+ Added a couple of tests where different sections get their
+ m_needsCellRecalc set independently.
+
+ * fast/table/crash-splitColumn-2-expected.txt: Added.
+ * fast/table/crash-splitColumn-2.html: Added.
+ * fast/table/crash-splitColumn-3-expected.txt: Added.
+ * fast/table/crash-splitColumn-3.html: Added.
+ * fast/table/crash-splitColumn-expected.txt: Added.
+ * fast/table/crash-splitColumn.html: Added.
+
2011-11-09 Arko Saha <[email protected]>
Microdata: fast/dom/MicroData/itemid-attribute-test.html assertion failure in Element::getURLAttribute().
Added: trunk/LayoutTests/fast/table/crash-splitColumn-2-expected.txt (0 => 99744)
--- trunk/LayoutTests/fast/table/crash-splitColumn-2-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/table/crash-splitColumn-2-expected.txt 2011-11-09 19:57:16 UTC (rev 99744)
@@ -0,0 +1,6 @@
+Bug 70171: Crash in RenderTableSection::splitColumn
+
+This test PASSES if it does not CRASH or ASSERT.
+
+
+
Property changes on: trunk/LayoutTests/fast/table/crash-splitColumn-2-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/table/crash-splitColumn-2.html (0 => 99744)
--- trunk/LayoutTests/fast/table/crash-splitColumn-2.html (rev 0)
+++ trunk/LayoutTests/fast/table/crash-splitColumn-2.html 2011-11-09 19:57:16 UTC (rev 99744)
@@ -0,0 +1,64 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+.c7 { display: table-row-group; }
+.c7:nth-last-of-type(-n+6) { float: none; }
+.c21:nth-child(2n) { position: static; float: left; }
+.c26 { border-style: ridge; content: counter(section);</style>
+<script>
+if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+}
+
+function crash()
+{
+ var img = document.createElement('img');
+ img.appendChild(select);
+ if (window.layoutTestController)
+ layoutTestController.notifyDone();
+}
+
+function insertNodes() {
+ document.documentElement.appendChild(document.createElement('a'));
+ document.documentElement.appendChild(document.createElement('dfn'));
+ document.documentElement.appendChild(document.createElement('keygen'));
+ var iframe = document.createElement('iframe');
+ iframe.setAttribute('src', 'dne.html');
+ document.documentElement.appendChild(iframe);
+ document.documentElement.appendChild(document.createElement('rp'));
+ document.documentElement.appendChild(document.createElement('ul'));
+ document.documentElement.appendChild(document.createElement('option'));
+ document.documentElement.appendChild(document.createElement('label'));
+ document.documentElement.appendChild(document.createElement('table'));
+ document.documentElement.appendChild(document.createElement('mark'));
+ document.documentElement.appendChild(document.createElement('bdo'));
+ document.documentElement.appendChild(document.createElement('colgroup'));
+ document.documentElement.appendChild(document.createElement('strong'));
+
+ select = document.createElement('select');
+ document.documentElement.appendChild(select);
+
+ var sup = document.createElement('sup');
+ sup.setAttribute('class', 'c7');
+ document.documentElement.appendChild(sup);
+ var td = document.createElement('td');
+ td.setAttribute('class', 'c21');
+ document.documentElement.appendChild(td);
+
+ var th = document.createElement('th');
+ th.setAttribute('colspan', '2');
+ th.setAttribute('class', 'c26');
+ sup.appendChild(th);
+
+ setTimeout(crash, 0);
+}
+window.addEventListener("load", insertNodes, false);
+</script>
+</head>
+<body>
+<p> Bug <a href="" Crash in RenderTableSection::splitColumn</p>
+<p> This test PASSES if it does not CRASH or ASSERT.</p>
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/table/crash-splitColumn-2.html
___________________________________________________________________
Added: svn:executable
Added: svn:eol-style
Added: trunk/LayoutTests/fast/table/crash-splitColumn-3-expected.txt (0 => 99744)
--- trunk/LayoutTests/fast/table/crash-splitColumn-3-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/table/crash-splitColumn-3-expected.txt 2011-11-09 19:57:16 UTC (rev 99744)
@@ -0,0 +1,5 @@
+Bug 70171: Crash in RenderTableSection::splitColumn
+
+This test PASSES if it does not CRASH or ASSERT.
+
+
Property changes on: trunk/LayoutTests/fast/table/crash-splitColumn-3-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/table/crash-splitColumn-3.html (0 => 99744)
--- trunk/LayoutTests/fast/table/crash-splitColumn-3.html (rev 0)
+++ trunk/LayoutTests/fast/table/crash-splitColumn-3.html 2011-11-09 19:57:16 UTC (rev 99744)
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.layoutTestController)
+ layoutTestController.dumpAsText();
+
+function crash()
+{
+ var firstCell = document.getElementById("firstCell");
+ firstCell.parentNode.removeChild(firstCell);
+}
+
+window.addEventListener("load", crash, false);
+</script>
+</head>
+<body>
+<p> Bug <a href="" Crash in RenderTableSection::splitColumn</p>
+<p> This test PASSES if it does not CRASH or ASSERT.</p>
+<table><tr><td id="firstCell">foobar</td><td colspan="2"></td><td></td></tr></table>
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/table/crash-splitColumn-3.html
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/table/crash-splitColumn-expected.txt (0 => 99744)
--- trunk/LayoutTests/fast/table/crash-splitColumn-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/table/crash-splitColumn-expected.txt 2011-11-09 19:57:16 UTC (rev 99744)
@@ -0,0 +1,5 @@
+Bug 70171: Crash in RenderTableSection::splitColumn
+
+This test PASSES if it does not CRASH or ASSERT.
+
+
Property changes on: trunk/LayoutTests/fast/table/crash-splitColumn-expected.txt
___________________________________________________________________
Added: svn:eol-style
Added: trunk/LayoutTests/fast/table/crash-splitColumn.html (0 => 99744)
--- trunk/LayoutTests/fast/table/crash-splitColumn.html (rev 0)
+++ trunk/LayoutTests/fast/table/crash-splitColumn.html 2011-11-09 19:57:16 UTC (rev 99744)
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+.lastTableHeaderGroup:last-of-type { display: table-header-group; }</style>
+</style>
+<script>
+if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+}
+
+function crash()
+{
+ rubyTag.appendChild(lastTableHead);
+ if (window.layoutTestController)
+ layoutTestController.notifyDone();
+}
+
+function insertNodes() {
+ var tableHead = document.createElement('th');
+ tableHead.setAttribute('colspan', '5');
+ tableHead.setAttribute('class', 'lastTableHeaderGroup');
+ document.documentElement.appendChild(tableHead);
+ tableHead.appendChild(document.createElement('p'));
+ lastTableHead = document.createElement('th');
+ document.documentElement.appendChild(lastTableHead);
+ rubyTag = document.createElement('rt');
+ setTimeout(crash, 0);
+}
+window.addEventListener("load", insertNodes, false);
+</script>
+</head>
+<body>
+<p> Bug <a href="" Crash in RenderTableSection::splitColumn</p>
+<p> This test PASSES if it does not CRASH or ASSERT.</p>
+</body>
+</html>
Property changes on: trunk/LayoutTests/fast/table/crash-splitColumn.html
___________________________________________________________________
Added: svn:executable
Added: svn:eol-style
Modified: trunk/Source/WebCore/ChangeLog (99743 => 99744)
--- trunk/Source/WebCore/ChangeLog 2011-11-09 19:45:08 UTC (rev 99743)
+++ trunk/Source/WebCore/ChangeLog 2011-11-09 19:57:16 UTC (rev 99744)
@@ -1,3 +1,40 @@
+2011-11-09 Julien Chaffraix <[email protected]>
+
+ Crash in RenderTableSection::splitColumn
+ https://bugs.webkit.org/show_bug.cgi?id=70171
+
+ Reviewed by David Hyatt.
+
+ Tests: fast/table/crash-splitColumn-2.html
+ fast/table/crash-splitColumn-3.html
+ fast/table/crash-splitColumn.html
+
+ The old code would not take into account the fact that each RenderTableSection
+ can set its m_needsCellRecalc flag independently of the rest.
+
+ This means that you cannot assume that you can always split or append columns to
+ all the sections. Our approach is to skip sections needing cell recalc in several
+ parts of the code as they will be properly reset to the table's representations
+ during a cell recalc.
+
+ * rendering/RenderTable.cpp:
+ (WebCore::RenderTable::splitColumn):
+ (WebCore::RenderTable::appendColumn):
+ Skip sections needing cell recalc as they will be properly updated later.
+
+ * rendering/RenderTableSection.cpp:
+ (WebCore::RenderTableSection::addCell):
+ Ignore a section needing cell recalc as addCell will be called after sync'ing
+ the internal column representation in recalcCells.
+
+ (WebCore::RenderTableSection::recalcCells):
+ Clear the flag at the beginning of the function to activate the previous functions.
+ Added a comment as to why this is fine.
+
+ (WebCore::RenderTableSection::appendColumn):
+ Added an ASSERT. If we need cell recalc, we should NEVER update m_grid outside
+ of recalcCells().
+
2011-11-09 Arko Saha <[email protected]>
Microdata: fast/dom/MicroData/itemid-attribute-test.html assertion failure in Element::getURLAttribute().
Modified: trunk/Source/WebCore/rendering/RenderTable.cpp (99743 => 99744)
--- trunk/Source/WebCore/rendering/RenderTable.cpp 2011-11-09 19:45:08 UTC (rev 99743)
+++ trunk/Source/WebCore/rendering/RenderTable.cpp 2011-11-09 19:57:16 UTC (rev 99744)
@@ -645,10 +645,17 @@
memmove(m_columns.data() + position + 1, m_columns.data() + position, (oldSize - position) * sizeof(ColumnStruct));
m_columns[position + 1].span = oldSpan - firstSpan;
- // change width of all rows.
+ // Propagate the change in our columns representation to the sections that don't need
+ // cell recalc. If they do, they will be synced up directly with m_columns later.
for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
- if (child->isTableSection())
- toRenderTableSection(child)->splitColumn(position, firstSpan);
+ if (!child->isTableSection())
+ continue;
+
+ RenderTableSection* section = toRenderTableSection(child);
+ if (section->needsCellRecalc())
+ continue;
+
+ section->splitColumn(position, firstSpan);
}
m_columnPos.grow(numEffCols() + 1);
@@ -657,16 +664,22 @@
void RenderTable::appendColumn(int span)
{
- // easy case.
- int pos = m_columns.size();
- int newSize = pos + 1;
+ unsigned pos = m_columns.size();
+ unsigned newSize = pos + 1;
m_columns.grow(newSize);
m_columns[pos].span = span;
- // change width of all rows.
+ // Propagate the change in our columns representation to the sections that don't need
+ // cell recalc. If they do, they will be synced up directly with m_columns later.
for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
- if (child->isTableSection())
- toRenderTableSection(child)->appendColumn(pos);
+ if (!child->isTableSection())
+ continue;
+
+ RenderTableSection* section = toRenderTableSection(child);
+ if (section->needsCellRecalc())
+ continue;
+
+ section->appendColumn(pos);
}
m_columnPos.grow(numEffCols() + 1);
Modified: trunk/Source/WebCore/rendering/RenderTableSection.cpp (99743 => 99744)
--- trunk/Source/WebCore/rendering/RenderTableSection.cpp 2011-11-09 19:45:08 UTC (rev 99743)
+++ trunk/Source/WebCore/rendering/RenderTableSection.cpp 2011-11-09 19:57:16 UTC (rev 99744)
@@ -191,6 +191,12 @@
void RenderTableSection::addCell(RenderTableCell* cell, RenderTableRow* row)
{
+ // We don't insert the cell if we need cell recalc as our internal columns' representation
+ // will have drifted from the table's representation. Also recalcCells will call addCell
+ // at a later time after sync'ing our columns' with the table's.
+ if (needsCellRecalc())
+ return;
+
int rSpan = cell->rowSpan();
int cSpan = cell->colSpan();
Vector<RenderTable::ColumnStruct>& columns = table()->columns();
@@ -1122,6 +1128,12 @@
void RenderTableSection::recalcCells()
{
+ ASSERT(m_needsCellRecalc);
+ // We reset the flag here to ensure that |addCell| works. This is safe to do as
+ // fillRowsWithDefaultStartingAtPosition makes sure we match the table's columns
+ // representation.
+ m_needsCellRecalc = false;
+
m_cCol = 0;
m_cRow = 0;
fillRowsWithDefaultStartingAtPosition(0);
@@ -1154,7 +1166,6 @@
gridSize = max(gridSize, m_cRow);
m_grid.shrink(gridSize);
- m_needsCellRecalc = false;
setNeedsLayout(true);
}
@@ -1202,6 +1213,8 @@
void RenderTableSection::appendColumn(int pos)
{
+ ASSERT(!m_needsCellRecalc);
+
for (unsigned row = 0; row < m_grid.size(); ++row)
m_grid[row].row.resize(pos + 1);
}
