Crash in RenderTableSection::splitColumn
https://bugs.webkit.org/show_bug.cgi?id=70171
Reviewed by David Hyatt.
Source/WebCore:
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().
LayoutTests:
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.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@99744 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 388d09a..2f38dbc5 100755
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,20 @@
+2011-11-09 Julien Chaffraix <jchaffraix@webkit.org>
+
+ 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 <arko@motorola.com>
Microdata: fast/dom/MicroData/itemid-attribute-test.html assertion failure in Element::getURLAttribute().
diff --git a/LayoutTests/fast/table/crash-splitColumn-2-expected.txt b/LayoutTests/fast/table/crash-splitColumn-2-expected.txt
new file mode 100644
index 0000000..50f9174
--- /dev/null
+++ b/LayoutTests/fast/table/crash-splitColumn-2-expected.txt
@@ -0,0 +1,6 @@
+Bug 70171: Crash in RenderTableSection::splitColumn
+
+This test PASSES if it does not CRASH or ASSERT.
+
+
+
diff --git a/LayoutTests/fast/table/crash-splitColumn-2.html b/LayoutTests/fast/table/crash-splitColumn-2.html
new file mode 100755
index 0000000..81f4573
--- /dev/null
+++ b/LayoutTests/fast/table/crash-splitColumn-2.html
@@ -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="https://bugs.webkit.org/show_bug.cgi?id=70171">70171</a>: Crash in RenderTableSection::splitColumn</p>
+<p> This test PASSES if it does not CRASH or ASSERT.</p>
+</body>
+</html>
diff --git a/LayoutTests/fast/table/crash-splitColumn-3-expected.txt b/LayoutTests/fast/table/crash-splitColumn-3-expected.txt
new file mode 100644
index 0000000..3ab782a
--- /dev/null
+++ b/LayoutTests/fast/table/crash-splitColumn-3-expected.txt
@@ -0,0 +1,5 @@
+Bug 70171: Crash in RenderTableSection::splitColumn
+
+This test PASSES if it does not CRASH or ASSERT.
+
+
diff --git a/LayoutTests/fast/table/crash-splitColumn-3.html b/LayoutTests/fast/table/crash-splitColumn-3.html
new file mode 100644
index 0000000..f431b12
--- /dev/null
+++ b/LayoutTests/fast/table/crash-splitColumn-3.html
@@ -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="https://bugs.webkit.org/show_bug.cgi?id=70171">70171</a>: 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>
diff --git a/LayoutTests/fast/table/crash-splitColumn-expected.txt b/LayoutTests/fast/table/crash-splitColumn-expected.txt
new file mode 100644
index 0000000..3ab782a
--- /dev/null
+++ b/LayoutTests/fast/table/crash-splitColumn-expected.txt
@@ -0,0 +1,5 @@
+Bug 70171: Crash in RenderTableSection::splitColumn
+
+This test PASSES if it does not CRASH or ASSERT.
+
+
diff --git a/LayoutTests/fast/table/crash-splitColumn.html b/LayoutTests/fast/table/crash-splitColumn.html
new file mode 100755
index 0000000..43898b5
--- /dev/null
+++ b/LayoutTests/fast/table/crash-splitColumn.html
@@ -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="https://bugs.webkit.org/show_bug.cgi?id=70171">70171</a>: Crash in RenderTableSection::splitColumn</p>
+<p> This test PASSES if it does not CRASH or ASSERT.</p>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 6b68f81..f4b4fdc 100755
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,40 @@
+2011-11-09 Julien Chaffraix <jchaffraix@webkit.org>
+
+ 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 <arko@motorola.com>
Microdata: fast/dom/MicroData/itemid-attribute-test.html assertion failure in Element::getURLAttribute().
diff --git a/Source/WebCore/rendering/RenderTable.cpp b/Source/WebCore/rendering/RenderTable.cpp
index cd845c9..98255fa 100644
--- a/Source/WebCore/rendering/RenderTable.cpp
+++ b/Source/WebCore/rendering/RenderTable.cpp
@@ -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);
diff --git a/Source/WebCore/rendering/RenderTableSection.cpp b/Source/WebCore/rendering/RenderTableSection.cpp
index f7cc428..5921527 100644
--- a/Source/WebCore/rendering/RenderTableSection.cpp
+++ b/Source/WebCore/rendering/RenderTableSection.cpp
@@ -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);
}
