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.
There seems to be several call sites affected. Renaming the bug.
> 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.
Created attachment 113119 [details] Proposed change 1: Keep track of how many cells we really need and trim the buffer appropriately.
Created attachment 113190 [details] Proposed change 2: With a better ChangeLog and description.
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?
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.
Committed r98980: <http://trac.webkit.org/changeset/98980>