Bug 71246 – REGRESSION(98738): RenderTableSection::recalcCells does not properly shrink the RowStruct grid

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

The refactoring introduced a subtle and unintended change in behavior which makes us crash (NULL dereference).

The problem is in:

RenderTableSection::recalcCells()

    m_cCol = 0;
    m_cRow = 0;
    fillRowsWithDefaultStartingAtPosition(0);

    for (RenderObject* row = firstChild(); row; row = row->nextSibling()) {
        if (row->isTableRow()) {
            unsigned insertionRow = m_cRow;
            m_cRow++;
            m_cCol = 0;
            if (!ensureRows(m_cRow))
                break;

            RenderTableRow* tableRow = toRenderTableRow(row);
            m_grid[insertionRow].rowRenderer = tableRow;
            setRowLogicalHeightToRowStyleLogicalHeightIfNotRelative(m_grid[insertionRow]);

            for (RenderObject* cell = row->firstChild(); cell; cell = cell->nextSibling()) {
                if (cell->isTableCell())
                    addCell(toRenderTableCell(cell), tableRow);
            }
        }
    }

    m_grid.shrinkToFit();

The last statement is bogus. It does not guarantee that our size is shrunk properly if our row count decreased.

Patch forthcoming.

[Julien Chaffraix]

There seems to be several call sites affected. Renaming the bug.

[Julien Chaffraix]

>     m_grid.shrinkToFit();
> 
> The last statement is bogus. It does not guarantee that our size is shrunk properly if our row count decreased.

It amend what I said. This statement is fine and my reasoning was completely overlooking the influence of rowSpan (that can force m_grid to be more that just the row count).

The logic introduced in r98738 does not handle empty section well and will be changed to do so.

[Julien Chaffraix]

Created attachment 113119 [details]
Proposed change 1: Keep track of how many cells we really need and trim the buffer appropriately.

[Julien Chaffraix]

Created attachment 113190 [details]
Proposed change 2: With a better ChangeLog and description.

[Darin Adler]

Comment on attachment 113190 [details]
Proposed change 2: With a better ChangeLog and description.

View in context: https://bugs.webkit.org/attachment.cgi?id=113190&action=review

> Source/WebCore/rendering/RenderTableSection.cpp:1152
> +    m_grid.shrink(gridSize);

What guarantees this is a shrink? Is there ever a chance that gridSize could be larger than m_grid.size() at this point?

[Julien Chaffraix]

Comment on attachment 113190 [details]
Proposed change 2: With a better ChangeLog and description.

View in context: https://bugs.webkit.org/attachment.cgi?id=113190&action=review

>> Source/WebCore/rendering/RenderTableSection.cpp:1152
>> +    m_grid.shrink(gridSize);
> 
> What guarantees this is a shrink? Is there ever a chance that gridSize could be larger than m_grid.size() at this point?

It is guaranteed to be a shrink. Detailed explanation:

* If recalcCells ends up increasing or not changing the grid size, |gridSize| will be of the same size as m_grid.size() as we use the same formula to compute |gridSize| as what is given to |ensureRows| (which grows the size during the |addCell| calls). In this case, m_grid.shrink(gridSize) is a no-op.
* If recalcCells ends up shrinking grid size, m_grid will still have the older (bigger) size but |gridSize| will have the new (smaller) value.

[Julien Chaffraix]

Committed r98980: <http://trac.webkit.org/changeset/98980>