Don't use rowIndex() if needsCellRecalc().

The row index may be bogus or even unset in that case. Rows may be inserted with an initially unknown row index. This happens when the row isn't inserted at the end of its table section, and also when splitting an anonymous row. This would lead to an assertion failure for row()->rowIndexWasSet() in RenderTableCell::styleDidChange(). rowIndex() is used there in a call to RenderTableSection::rowLogicalHeightChanged(). rowIndex() is also used in a similar way in RenderTableRow::styleDidChange(), albeit without asserting first. styleDidChange() may be called while needsCellRecalc() is set, which may mean that row index hasn't been calculated yet (or is bogus). We could of course just recalculate the cells (which also involves calculating all row indices) in styleDidChange(), but it's not really necessary, since the row index was just needed in order to call rowLogicalHeightChanged(), which just bails anyway if cells need recalc. So the solution is to pass a RenderTableRow instead of the row index to rowLogicalHeightChanged(). This way we don't call rowIndex() when row index is unknown. BUG=397442 Review URL: https://codereview.chromium.org/532513002 git-svn-id: svn://svn.chromium.org/blink/trunk@181637 bbb929c8-8fbe-4397-9dbb-9b2b20218538

Don't use rowIndex() if needsCellRecalc().
The row index may be bogus or even unset in that case. Rows may be inserted with an initially unknown row index. This happens when the row isn't inserted at the end of its table section, and also when splitting an anonymous row. This would lead to an assertion failure for row()->rowIndexWasSet() in RenderTableCell::styleDidChange(). rowIndex() is used there in a call to RenderTableSection::rowLogicalHeightChanged(). rowIndex() is also used in a similar way in RenderTableRow::styleDidChange(), albeit without asserting first. styleDidChange() may be called while needsCellRecalc() is set, which may mean that row index hasn't been calculated yet (or is bogus). We could of course just recalculate the cells (which also involves calculating all row indices) in styleDidChange(), but it's not really necessary, since the row index was just needed in order to call rowLogicalHeightChanged(), which just bails anyway if cells need recalc. So the solution is to pass a RenderTableRow instead of the row index to rowLogicalHeightChanged(). This way we don't call rowIndex() when row index is unknown. BUG=397442 Review URL: https://codereview.chromium.org/532513002 git-svn-id: svn://svn.chromium.org/blink/trunk@181637 bbb929c8-8fbe-4397-9dbb-9b2b20218538
881726f5 · mstensho@opera.com · 3ab8682b · 881726f5 · 881726f5 · 881726f5
Commit 881726f5 authored Sep 09, 2014 by mstensho@opera.com
7 changed files
--- a/third_party/WebKit/LayoutTests/fast/table/split-anonymous-crash-expected.txt
+++ b/third_party/WebKit/LayoutTests/fast/table/split-anonymous-crash-expected.txt
+Setting style on a cell whose row hasn't been laid out yet should not crash.
+
+elm1
+elm2
+nested row group
--- a/third_party/WebKit/LayoutTests/fast/table/split-anonymous-crash.html
+++ b/third_party/WebKit/LayoutTests/fast/table/split-anonymous-crash.html
+<!DOCTYPE html>
+<style>
+    #fl:first-letter { color:red; }
+</style>
+<script>
+    if (window.testRunner)
+        testRunner.dumpAsText();
+    window.onload = function() {
+        document.body.offsetTop;
+        elm2.style.display = 'table-row';
+    }
+</script>
+<p>Setting style on a cell whose row hasn't been laid out yet should not crash.</p>
+<div id="fl" style="display:table-row-group; padding-top:1vw;">
+    <div>elm1</div>
+    <div id="elm2" style="display:none;">elm2</div>
+    <div style="display:table-row-group;">nested row group</div>
+</div>
--- a/third_party/WebKit/Source/core/rendering/RenderTableCell.cpp
+++ b/third_party/WebKit/Source/core/rendering/RenderTableCell.cpp
@@ -385,13 +385,12 @@ LayoutUnit RenderTableCell::cellBaselinePosition() const
 void RenderTableCell::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
    ASSERT(style()->display() == TABLE_CELL);
-    ASSERT(!row() || row()->rowIndexWasSet());

    RenderBlockFlow::styleDidChange(diff, oldStyle);
    setHasBoxDecorationBackground(true);

    if (parent() && section() && oldStyle && style()->height() != oldStyle->height())
-        section()->rowLogicalHeightChanged(rowIndex());
+        section()->rowLogicalHeightChanged(row());

    // Our intrinsic padding pushes us down to align with the baseline of other cells on the row. If our vertical-align
    // has changed then so will the padding needed to align with other cells - clear it so we can recalculate it from scratch.

--- a/third_party/WebKit/Source/core/rendering/RenderTableRow.cpp
+++ b/third_party/WebKit/Source/core/rendering/RenderTableRow.cpp
@@ -76,7 +76,7 @@ void RenderTableRow::styleDidChange(StyleDifference diff, const RenderStyle* old
    propagateStyleToAnonymousChildren();

    if (section() && oldStyle && style()->logicalHeight() != oldStyle->logicalHeight())
-        section()->rowLogicalHeightChanged(rowIndex());
+        section()->rowLogicalHeightChanged(this);

    // If border was changed, notify table.
    if (parent()) {

--- a/third_party/WebKit/Source/core/rendering/RenderTableRow.h
+++ b/third_party/WebKit/Source/core/rendering/RenderTableRow.h
@@ -70,6 +70,7 @@ public:
    unsigned rowIndex() const
    {
        ASSERT(rowIndexWasSet());
+        ASSERT(!section() || !section()->needsCellRecalc()); // index may be bogus if cells need recalc.
        return m_rowIndex;
    }


--- a/third_party/WebKit/Source/core/rendering/RenderTableSection.cpp
+++ b/third_party/WebKit/Source/core/rendering/RenderTableSection.cpp
@@ -1571,11 +1571,12 @@ void RenderTableSection::recalcCells()
 }

 // FIXME: This function could be made O(1) in certain cases (like for the non-most-constrainive cells' case).
-void RenderTableSection::rowLogicalHeightChanged(unsigned rowIndex)
+void RenderTableSection::rowLogicalHeightChanged(RenderTableRow* row)
 {
    if (needsCellRecalc())
        return;

+    unsigned rowIndex = row->rowIndex();
    setRowLogicalHeightToRowStyleLogicalHeight(m_grid[rowIndex]);

    for (RenderTableCell* cell = m_grid[rowIndex].rowRenderer->firstCell(); cell; cell = cell->nextCell())

--- a/third_party/WebKit/Source/core/rendering/RenderTableSection.h
+++ b/third_party/WebKit/Source/core/rendering/RenderTableSection.h
@@ -206,7 +206,7 @@ public:

    LayoutUnit rowBaseline(unsigned row) { return m_grid[row].baseline; }

-    void rowLogicalHeightChanged(unsigned rowIndex);
+    void rowLogicalHeightChanged(RenderTableRow*);

    void removeCachedCollapsedBorders(const RenderTableCell*);
    void setCachedCollapsedBorder(const RenderTableCell*, CollapsedBorderSide, CollapsedBorderValue);