- Revision
- 121275
- Author
- jchaffr...@webkit.org
- Date
- 2012-06-26 12:16:08 -0700 (Tue, 26 Jun 2012)
Log Message
Crash in FixedTableLayout::layout
https://bugs.webkit.org/show_bug.cgi?id=88676
Reviewed by Abhishek Arya.
Source/WebCore:
Tests: fast/table/auto-table-layout-colgroup-removal-crash.html
fast/table/fixed-table-layout/colgroup-removal-crash.html
fast/table/fixed-table-layout/prepend-in-fixed-table.html
The issue comes from RenderTable not properly dirtying its preferred logical
widths. As the table layout codes (both fixed and auto), recomputes their internal
structures at computePreferredLogicalWidth, the internal structure doesn't match
the table sizing and we crash.
This fix adds a work-around in FixedTableLayout::layout (which matches AutoTableLayout).
The long-term fix would be to properly fix the logic but this is a lot safer, especially
since our logic is really not bullet-proof at the moment.
* rendering/FixedTableLayout.cpp:
(WebCore::FixedTableLayout::layout):
Added an internal structure recomputation, if we have drifted from our table's structure.
Also we need to update nEffCols if we call calcWidthArray.
* rendering/AutoTableLayout.cpp:
(WebCore::AutoTableLayout::layout):
Added a comment matching FixedTableLayout. The nEffCols is unneeded but kept for consistency
with FixedTableLayout.
LayoutTests:
* fast/table/auto-table-layout-colgroup-removal-crash-expected.txt: Added.
* fast/table/auto-table-layout-colgroup-removal-crash.html: Added.
* fast/table/fixed-table-layout/colgroup-removal-crash-expected.txt: Added.
* fast/table/fixed-table-layout/colgroup-removal-crash.html: Added.
2 cases where we remove a colgroup after having done layout.
* fast/table/fixed-table-layout/prepend-in-fixed-table-expected.txt: Added.
* fast/table/fixed-table-layout/prepend-in-fixed-table.html: Added.
This is a copy of fast/table/prepend-in-anonymous-table.html modified to work with
fixed table layout. This covers a bug found during testing that is fixed as part of
this broader change.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (121274 => 121275)
--- trunk/LayoutTests/ChangeLog 2012-06-26 18:58:44 UTC (rev 121274)
+++ trunk/LayoutTests/ChangeLog 2012-06-26 19:16:08 UTC (rev 121275)
@@ -1,3 +1,22 @@
+2012-06-26 Julien Chaffraix <jchaffr...@webkit.org>
+
+ Crash in FixedTableLayout::layout
+ https://bugs.webkit.org/show_bug.cgi?id=88676
+
+ Reviewed by Abhishek Arya.
+
+ * fast/table/auto-table-layout-colgroup-removal-crash-expected.txt: Added.
+ * fast/table/auto-table-layout-colgroup-removal-crash.html: Added.
+ * fast/table/fixed-table-layout/colgroup-removal-crash-expected.txt: Added.
+ * fast/table/fixed-table-layout/colgroup-removal-crash.html: Added.
+ 2 cases where we remove a colgroup after having done layout.
+
+ * fast/table/fixed-table-layout/prepend-in-fixed-table-expected.txt: Added.
+ * fast/table/fixed-table-layout/prepend-in-fixed-table.html: Added.
+ This is a copy of fast/table/prepend-in-anonymous-table.html modified to work with
+ fixed table layout. This covers a bug found during testing that is fixed as part of
+ this broader change.
+
2012-06-26 Alpha Lam <hc...@chromium.org>
[chromium] Mark a layout test as fail
Added: trunk/LayoutTests/fast/table/auto-table-layout-colgroup-removal-crash-expected.txt (0 => 121275)
--- trunk/LayoutTests/fast/table/auto-table-layout-colgroup-removal-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/table/auto-table-layout-colgroup-removal-crash-expected.txt 2012-06-26 19:16:08 UTC (rev 121275)
@@ -0,0 +1,5 @@
+Bug 88676: Crash in FixedTableLayout::layout
+
+PASSED, the test didn't crash.
+
+
Added: trunk/LayoutTests/fast/table/auto-table-layout-colgroup-removal-crash.html (0 => 121275)
--- trunk/LayoutTests/fast/table/auto-table-layout-colgroup-removal-crash.html (rev 0)
+++ trunk/LayoutTests/fast/table/auto-table-layout-colgroup-removal-crash.html 2012-06-26 19:16:08 UTC (rev 121275)
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<p>Bug <a href="" Crash in FixedTableLayout::layout</p>
+<p id="console">FAILED, the test didn't run.</p>
+<table id="table">
+ <tbody>
+ <tr></tr>
+ </tbody>
+ <colgroup id="colGroup"></colgroup>
+</table>
+<script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+
+ document.body.offsetTop;
+ table.removeChild(colGroup);
+ document.getElementById("console").innerHTML = "PASSED, the test didn't crash.";
+</script>
Added: trunk/LayoutTests/fast/table/fixed-table-layout/colgroup-removal-crash-expected.txt (0 => 121275)
--- trunk/LayoutTests/fast/table/fixed-table-layout/colgroup-removal-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/table/fixed-table-layout/colgroup-removal-crash-expected.txt 2012-06-26 19:16:08 UTC (rev 121275)
@@ -0,0 +1,5 @@
+Bug 88676: Crash in FixedTableLayout::layout
+
+PASSED, the test didn't crash.
+
+
Added: trunk/LayoutTests/fast/table/fixed-table-layout/colgroup-removal-crash.html (0 => 121275)
--- trunk/LayoutTests/fast/table/fixed-table-layout/colgroup-removal-crash.html (rev 0)
+++ trunk/LayoutTests/fast/table/fixed-table-layout/colgroup-removal-crash.html 2012-06-26 19:16:08 UTC (rev 121275)
@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<p>Bug <a href="" Crash in FixedTableLayout::layout</p>
+<p id="console">FAILED, the test didn't run.</p>
+<table id="table" width="1" style="table-layout:fixed;">
+ <tbody>
+ <tr></tr>
+ </tbody>
+ <colgroup id="colGroup"></colgroup>
+</table>
+<script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+
+ document.body.offsetTop;
+ table.removeChild(colGroup);
+ document.getElementById("console").innerHTML = "PASSED, the test didn't crash.";
+</script>
Added: trunk/LayoutTests/fast/table/fixed-table-layout/prepend-in-fixed-table-expected.txt (0 => 121275)
--- trunk/LayoutTests/fast/table/fixed-table-layout/prepend-in-fixed-table-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/table/fixed-table-layout/prepend-in-fixed-table-expected.txt 2012-06-26 19:16:08 UTC (rev 121275)
@@ -0,0 +1,40 @@
+Bug 88676: Crash in FixedTableLayout::layout.
+
+PASSED, the test didn't crash.
+
+Prepending block to table-cell:
+Prepending inline to table-cell:
+Prepending table-cell to table-cell:
+Prepending table-row to table-cell:
+Prepending table-row-group to table-cell:
+Prepending table-column-group to table-cell:
+Prepending table-caption to table-cell:
+Prepending block to table-row:
+Prepending inline to table-row:
+Prepending table-cell to table-row:
+Prepending table-row to table-row:
+Prepending table-row-group to table-row:
+Prepending table-column-group to table-row:
+Prepending table-caption to table-row:
+Prepending block to table-row-group:
+Prepending inline to table-row-group:
+Prepending table-cell to table-row-group:
+Prepending table-row to table-row-group:
+Prepending table-row-group to table-row-group:
+Prepending table-column-group to table-row-group:
+Prepending table-caption to table-row-group:
+Prepending block to table-column-group:
+Prepending inline to table-column-group:
+Prepending table-cell to table-column-group:
+Prepending table-row to table-column-group:
+Prepending table-row-group to table-column-group:
+Prepending table-column-group to table-column-group:
+Prepending table-caption to table-column-group:
+Prepending block to table-caption:
+Prepending inline to table-caption:
+Prepending table-cell to table-caption:
+Prepending table-row to table-caption:
+Prepending table-row-group to table-caption:
+Prepending table-column-group to table-caption:
+Prepending table-caption to table-caption:
+
Added: trunk/LayoutTests/fast/table/fixed-table-layout/prepend-in-fixed-table.html (0 => 121275)
--- trunk/LayoutTests/fast/table/fixed-table-layout/prepend-in-fixed-table.html (rev 0)
+++ trunk/LayoutTests/fast/table/fixed-table-layout/prepend-in-fixed-table.html 2012-06-26 19:16:08 UTC (rev 121275)
@@ -0,0 +1,53 @@
+<!DOCTYPE html>
+<html>
+<body>
+ <p>Bug <a href="" Crash in FixedTableLayout::layout.</p>
+ <p id="console">FAILED, the test didn't run</p>
+ <script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+
+ var tableParts = [
+ "table-cell",
+ "table-row",
+ "table-row-group",
+ "table-column-group",
+ "table-caption"
+ ];
+
+ var childTypes = [
+ "block",
+ "inline",
+ "table-cell",
+ "table-row",
+ "table-row-group",
+ "table-column-group",
+ "table-caption"
+ ];
+
+ for (var i = 0; i < tableParts.length; ++i) {
+ for (var j = 0; j < childTypes.length; ++j) {
+ document.body.appendChild(document.createElement("pre")).appendChild(document.createTextNode("Prepending " + childTypes[j] + " to " + tableParts[i] + ":"));
+ var container = document.createElement("table");
+ container.style.tableLayout = "fixed";
+ container.width = "1";
+ var tablePart = document.createElement("div");
+ tablePart.style.display = tableParts[i];
+ container.appendChild(tablePart);
+ document.body.appendChild(container);
+ document.body.offsetTop;
+ var newChild = document.createElement("div");
+ newChild.style.display = childTypes[j];
+ container.insertBefore(newChild, tablePart);
+ document.body.offsetTop;
+ // The above should have the same render tree as you get when
+ // you do it all at once, like this:
+ document.body.appendChild(container.cloneNode(true));
+ document.body.offsetTop;
+ }
+ }
+
+ document.getElementById("console").innerHTML = "PASSED, the test didn't crash.";
+ </script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (121274 => 121275)
--- trunk/Source/WebCore/ChangeLog 2012-06-26 18:58:44 UTC (rev 121274)
+++ trunk/Source/WebCore/ChangeLog 2012-06-26 19:16:08 UTC (rev 121275)
@@ -1,3 +1,33 @@
+2012-06-26 Julien Chaffraix <jchaffr...@webkit.org>
+
+ Crash in FixedTableLayout::layout
+ https://bugs.webkit.org/show_bug.cgi?id=88676
+
+ Reviewed by Abhishek Arya.
+
+ Tests: fast/table/auto-table-layout-colgroup-removal-crash.html
+ fast/table/fixed-table-layout/colgroup-removal-crash.html
+ fast/table/fixed-table-layout/prepend-in-fixed-table.html
+
+ The issue comes from RenderTable not properly dirtying its preferred logical
+ widths. As the table layout codes (both fixed and auto), recomputes their internal
+ structures at computePreferredLogicalWidth, the internal structure doesn't match
+ the table sizing and we crash.
+
+ This fix adds a work-around in FixedTableLayout::layout (which matches AutoTableLayout).
+ The long-term fix would be to properly fix the logic but this is a lot safer, especially
+ since our logic is really not bullet-proof at the moment.
+
+ * rendering/FixedTableLayout.cpp:
+ (WebCore::FixedTableLayout::layout):
+ Added an internal structure recomputation, if we have drifted from our table's structure.
+ Also we need to update nEffCols if we call calcWidthArray.
+
+ * rendering/AutoTableLayout.cpp:
+ (WebCore::AutoTableLayout::layout):
+ Added a comment matching FixedTableLayout. The nEffCols is unneeded but kept for consistency
+ with FixedTableLayout.
+
2012-06-26 Ian Vollick <voll...@chromium.org>
[chromium] Layer chromium should need a redraw after getting its first non-empty bounds.
Modified: trunk/Source/WebCore/rendering/AutoTableLayout.cpp (121274 => 121275)
--- trunk/Source/WebCore/rendering/AutoTableLayout.cpp 2012-06-26 18:58:44 UTC (rev 121274)
+++ trunk/Source/WebCore/rendering/AutoTableLayout.cpp 2012-06-26 19:16:08 UTC (rev 121275)
@@ -498,8 +498,11 @@
int available = tableLogicalWidth;
size_t nEffCols = m_table->numEffCols();
+ // FIXME: It is possible to be called without having properly updated our internal representation.
+ // This means that our preferred logical widths were not recomputed as expected.
if (nEffCols != m_layoutStruct.size()) {
fullRecalc();
+ // FIXME: Table layout shouldn't modify our table structure (but does due to columns and colum-groups).
nEffCols = m_table->numEffCols();
}
Modified: trunk/Source/WebCore/rendering/FixedTableLayout.cpp (121274 => 121275)
--- trunk/Source/WebCore/rendering/FixedTableLayout.cpp 2012-06-26 18:58:44 UTC (rev 121274)
+++ trunk/Source/WebCore/rendering/FixedTableLayout.cpp 2012-06-26 19:16:08 UTC (rev 121275)
@@ -207,6 +207,15 @@
{
int tableLogicalWidth = m_table->logicalWidth() - m_table->bordersPaddingAndSpacingInRowDirection();
unsigned nEffCols = m_table->numEffCols();
+
+ // FIXME: It is possible to be called without having properly updated our internal representation.
+ // This means that our preferred logical widths were not recomputed as expected.
+ if (nEffCols != m_width.size()) {
+ calcWidthArray(tableLogicalWidth);
+ // FIXME: Table layout shouldn't modify our table structure (but does due to columns and colum-groups).
+ nEffCols = m_table->numEffCols();
+ }
+
Vector<int> calcWidth(nEffCols, 0);
unsigned numAuto = 0;
