REGRESSION (r162334): RenderTableCol::styleDidChange uses out-of-date table information

dbates@webkit.org · dbates@webkit.org · commit f04ec0f78f12 · 2014-03-18T20:00:47.000Z
https://bugs.webkit.org/show_bug.cgi?id=129561 Reviewed by Antti Koivisto. Source/WebCore: Test: fast/table/update-col-width-and-remove-table-cell-crash.html Fixes an issue where a table column or table column group may query an out- of-date model of its associated table as part of its process to propagate style changes to affected table cells. * rendering/RenderTableCol.cpp: (WebCore::RenderTableCol::styleDidChange): Ensure that all sections in the table are up-to-date before querying for a table cell. * rendering/RenderTableSection.cpp: (WebCore::RenderTableSection::recalcCells): Update comment to read well. In particular, remove the reference to RenderTableSection::fillRowsWithDefaultStartingAtPosition() as this function was removed in <http://trac.webkit.org/changeset/99919>. (WebCore::RenderTableSection::setNeedsCellRecalc): Clear the grid preemptively to to ensure that accessors cannot access stale data. We'll build the grid again in RenderTableSection::recalcCells(). (WebCore::RenderTableSection::numColumns): Add ASSERT(!m_needsCellRecalc) to assert that the grid cells are up-to-date. That is, we don't need to calculate them again. * rendering/RenderTableSection.h: Add ASSERT(!m_needsCellRecalc) or call recalcCellsIfNeeded() before accessing the grid to ensure that it's up-to-date. LayoutTests: Add a test to ensure that a table column propagates a style change to applicable table cells. * fast/table/update-col-width-and-remove-table-cell-crash-expected.txt: Added. * fast/table/update-col-width-and-remove-table-cell-crash.html: Added. git-svn-id: http://svn.webkit.org/repository/webkit/trunk@165837 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2014-03-18  Daniel Bates  <dabates@apple.com>
+
+        REGRESSION (r162334): RenderTableCol::styleDidChange uses out-of-date table information
+        https://bugs.webkit.org/show_bug.cgi?id=129561
+
+        Reviewed by Antti Koivisto.
+
+        Add a test to ensure that a table column propagates a style change to applicable
+        table cells.
+
+        * fast/table/update-col-width-and-remove-table-cell-crash-expected.txt: Added.
+        * fast/table/update-col-width-and-remove-table-cell-crash.html: Added.
+
 2014-03-18  Daniel Bates  <dabates@apple.com>
 
         REGRESSION (r163560): ASSERTION FAILED: childrenInline() in WebCore::RenderSVGText::layout
diff --git a/LayoutTests/fast/table/update-col-width-and-remove-table-cell-crash-expected.txt b/LayoutTests/fast/table/update-col-width-and-remove-table-cell-crash-expected.txt
@@ -0,0 +1 @@
+This test PASSED if it doesn't cause a crash, especially when run with Guard Malloc or MallocScribble enabled.
diff --git a/LayoutTests/fast/table/update-col-width-and-remove-table-cell-crash.html b/LayoutTests/fast/table/update-col-width-and-remove-table-cell-crash.html
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function runTest() {
+    document.getElementById("column1").width = "90";
+    var firstRow = document.getElementById("firstRow");
+    firstRow.removeChild(firstRow.firstElementChild);
+    document.getElementById("row").offsetWidth;
+}
+</script>
+<style>
+.column2 {
+    width: 10px;
+}
+</style>
+</head>
+<body onload="runTest()">
+<table>
+    <colgroup>
+        <col id="column1" class="column1">
+        <col class="column2">
+    </colgroup>
+    <tbody>
+        <tr id="firstRow">
+            <td colspan="4"></td>
+        </tr>
+        <tr id="row">
+            <td colspan="4"></td>
+        </tr>
+    </tbody>
+</table>
+<p>This test PASSED if it doesn't cause a crash, especially when run with Guard Malloc or MallocScribble enabled.</p>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2014-03-18  Daniel Bates  <dabates@apple.com>
+
+        REGRESSION (r162334): RenderTableCol::styleDidChange uses out-of-date table information
+        https://bugs.webkit.org/show_bug.cgi?id=129561
+
+        Reviewed by Antti Koivisto.
+
+        Test: fast/table/update-col-width-and-remove-table-cell-crash.html
+
+        Fixes an issue where a table column or table column group may query an out-
+        of-date model of its associated table as part of its process to propagate
+        style changes to affected table cells.
+
+        * rendering/RenderTableCol.cpp:
+        (WebCore::RenderTableCol::styleDidChange): Ensure that all sections in the table
+        are up-to-date before querying for a table cell.
+        * rendering/RenderTableSection.cpp:
+        (WebCore::RenderTableSection::recalcCells): Update comment to read well. In
+        particular, remove the reference to RenderTableSection::fillRowsWithDefaultStartingAtPosition()
+        as this function was removed in <http://trac.webkit.org/changeset/99919>.
+        (WebCore::RenderTableSection::setNeedsCellRecalc): Clear the grid preemptively to
+        to ensure that accessors cannot access stale data. We'll build the grid again
+        in RenderTableSection::recalcCells().
+        (WebCore::RenderTableSection::numColumns): Add ASSERT(!m_needsCellRecalc) to assert
+        that the grid cells are up-to-date. That is, we don't need to calculate them again.
+        * rendering/RenderTableSection.h: Add ASSERT(!m_needsCellRecalc) or call recalcCellsIfNeeded()
+        before accessing the grid to ensure that it's up-to-date.
+
 2014-03-18  Daniel Bates  <dabates@apple.com>
 
         REGRESSION (r163560): ASSERTION FAILED: childrenInline() in WebCore::RenderSVGText::layout
diff --git a/Source/WebCore/rendering/RenderTableCol.cpp b/Source/WebCore/rendering/RenderTableCol.cpp
@@ -55,6 +55,7 @@ void RenderTableCol::styleDidChange(StyleDifference diff, const RenderStyle* old
         if (table && !table->selfNeedsLayout() && !table->normalChildNeedsLayout() && oldStyle && oldStyle->border() != style().border())
             table->invalidateCollapsedBorders();
         else if (oldStyle->width() != style().width()) {
+            table->recalcSectionsIfNeeded();
             for (auto& section : childrenOfType<RenderTableSection>(*table)) {
                 unsigned nEffCols = table->numEffCols();
                 for (unsigned j = 0; j < nEffCols; j++) {
diff --git a/Source/WebCore/rendering/RenderTableSection.cpp b/Source/WebCore/rendering/RenderTableSection.cpp
@@ -1361,9 +1361,9 @@ void RenderTableSection::imageChanged(WrappedImagePtr, const IntRect*)
 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.
+    // We reset the flag here to ensure that addCell() works. This is safe to do because we clear the grid
+    // and update its dimensions to be consistent with the table's column representation before we rebuild
+    // the grid using addCell().
     m_needsCellRecalc = false;
 
     m_cCol = 0;
@@ -1403,12 +1403,17 @@ void RenderTableSection::rowLogicalHeightChanged(unsigned rowIndex)
 void RenderTableSection::setNeedsCellRecalc()
 {
     m_needsCellRecalc = true;
+
+    // Clear the grid now to ensure that we don't hold onto any stale pointers (e.g. a cell renderer that is being removed).
+    m_grid.clear();
+
     if (RenderTable* t = table())
         t->setNeedsSectionRecalc();
 }
 
 unsigned RenderTableSection::numColumns() const
 {
+    ASSERT(!m_needsCellRecalc);
     unsigned result = 0;
     
     for (unsigned r = 0; r < m_grid.size(); ++r) {
diff --git a/Source/WebCore/rendering/RenderTableSection.h b/Source/WebCore/rendering/RenderTableSection.h
@@ -142,15 +142,30 @@ class RenderTableSection final : public RenderBox {
     const RenderTableCell* firstRowCellAdjoiningTableStart() const;
     const RenderTableCell* firstRowCellAdjoiningTableEnd() const;
 
-    CellStruct& cellAt(unsigned row,  unsigned col) { return m_grid[row].row[col]; }
-    const CellStruct& cellAt(unsigned row, unsigned col) const { return m_grid[row].row[col]; }
+    CellStruct& cellAt(unsigned row,  unsigned col)
+    {
+        recalcCellsIfNeeded();
+        return m_grid[row].row[col];
+    }
+
+    const CellStruct& cellAt(unsigned row, unsigned col) const
+    {
+        ASSERT(!m_needsCellRecalc);
+        return m_grid[row].row[col];
+    }
+
     RenderTableCell* primaryCellAt(unsigned row, unsigned col)
     {
+        recalcCellsIfNeeded();
         CellStruct& c = m_grid[row].row[col];
         return c.primaryCell();
     }
 
-    RenderTableRow* rowRendererAt(unsigned row) const { return m_grid[row].rowRenderer; }
+    RenderTableRow* rowRendererAt(unsigned row) const
+    {
+        ASSERT(!m_needsCellRecalc);
+        return m_grid[row].rowRenderer;
+    }
 
     void appendColumn(unsigned pos);
     void splitColumn(unsigned pos, unsigned first);
@@ -194,7 +209,12 @@ class RenderTableSection final : public RenderBox {
     return styleForCellFlow->isLeftToRightDirection() ? outerBorderEnd() : outerBorderStart();
     }
 
-    unsigned numRows() const { return m_grid.size(); }
+    unsigned numRows() const
+    {
+        ASSERT(!m_needsCellRecalc);
+        return m_grid.size();
+    }
+
     unsigned numColumns() const;
     void recalcCells();
     void recalcCellsIfNeeded()
@@ -206,7 +226,11 @@ class RenderTableSection final : public RenderBox {
     bool needsCellRecalc() const { return m_needsCellRecalc; }
     void setNeedsCellRecalc();
 
-    LayoutUnit rowBaseline(unsigned row) { return m_grid[row].baseline; }
+    LayoutUnit rowBaseline(unsigned row)
+    {
+        recalcCellsIfNeeded();
+        return m_grid[row].baseline;
+    }
 
     void rowLogicalHeightChanged(unsigned rowIndex);
 
@@ -263,7 +287,12 @@ class RenderTableSection final : public RenderBox {
     bool hasOverflowingCell() const { return m_overflowingCells.size() || m_forceSlowPaintPathWithOverflowingCell; }
     void computeOverflowFromCells(unsigned totalRows, unsigned nEffCols);
 
-    CellSpan fullTableRowSpan() const { return CellSpan(0, m_grid.size()); }
+    CellSpan fullTableRowSpan() const
+    {
+        ASSERT(!m_needsCellRecalc);
+        return CellSpan(0, m_grid.size());
+    }
+
     CellSpan fullTableColumnSpan() const { return CellSpan(0, table()->columns().size()); }
 
     // Flip the rect so it aligns with the coordinates used by the rowPos and columnPos vectors.

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+This test PASSED if it doesn't cause a crash, especially when run with Guard Malloc or MallocScribble enabled.`