351326 - "ASSERTION: Appending cells far beyond the last row" with rowspan=0

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Loading this testcase in a debug build causes:

###!!! ASSERTION: Appending cells far beyond the last row: 'numOrigRows == aRowIndex', file /Users/admin/trunk/mozilla/layout/tables/nsCellMap.cpp, line 2181

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Still happens on trunk.

Comment 3

[Boris Zbarsky [:bzbarsky]]

numOrigRows == 0
aRowIndex == 1

(gdb) p cellMap->mContentRowCount
$46 = 2
(gdb) p cellMap->mRows.Length()
$47 = 0

It looks like the RemoveCell() call in nsTableFrame::AttributeChangedFor makes mRows.Length() 0...

The issue is that in nsCellMap::RemoveCell we decide that the frame being removed is a rowspan (because the GetRowSpan() call there uses the new rowspan value in the content) and hence rebuild the cellmap.  Which blows away all the rows and adds in the the cells one by one... except there are no cells to add, so we end up with an empty mRows.

So as I see it, the first bug is that GetRowSpan() in RemoveCell is really the wrong thing to check : in this case we do NOT want to treat the cellframe as having a rowspan when removing from the cellmap, but DO want to treat it that way when readding. That's probably pretty bad.  The second issue is whether we should allow appending past the last row since it can happen in this case (e.g. if the rowspan stayed 0 throughout and it was the colspan that got toggled from 0 to something else; in this case we'd rebuild the cellmap, correctly, and get the same assertion).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that AppendCell does (correctly) grow the row array in this case, so perhaps the assertion just needs to be removed...

But the GetRowSpan() issue bothers me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Perhaps we should introduce methods to ask a cell whether it's a zerospan, ones that use framestate bits (of which cellframe has a bunch free)?  Then RemoveCell() could use these methods.

Comment 6

[David Baron :dbaron:]

How bad is this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

That's a good question.  The problems I see would arise when going from rowspan="0" to no rowspan set.  It looks to me like we'll fail to rebuild the cellmap, after which I suspect we'll crash.  If we're lucky, it'll be a null deref...

Bernd would know better whether this is what will actually happen.

Comment 8

[Bernd]

I am not sure how bad this is in this case, but the rule of thumb is that cellmap errors can be turned into real crash bugs. However given the banging that we got from the fuzz testing the probability is now lower than before.

Comment 9

[Bernd]

>bz: So as I see it, the first bug is that GetRowSpan() in RemoveCell is really the wrong thing to check.

This is not entirely true. GetRowSpan called for the cellmap returns the status of the cellmap. GetRowSpan called on the cellframe however is different. It works as you describe it.

PRInt32 rowSpan = GetRowSpan(aRowIndex, startColIndex, PR_FALSE); <<< the cellmap value
  // record whether removing the cells is going to cause complications due 
  // to existing row spans, col spans or table sizing. 
  PRBool spansCauseRebuild = CellsSpanInOrOut(aRowIndex,
                                              aRowIndex + rowSpan - 1,
                                              startColIndex, numCols - 1);
  // XXX if the cell has a col span to the end of the map, and the end has no originating 
  // cells, we need to assume that this the only such cell, and rebuild so that there are 
  // no extraneous cols at the end. The same is true for removing rows.
  if (!aCellFrame->GetRowSpan() || !aCellFrame->GetColSpan()) <<< the cell value
    spansCauseRebuild = PR_TRUE;

we issue a bogus rebuild in this case but that is just a small performance loss not something critical.

To be clear:  GetRowSpan() in RemoveCell is really the right thing to check.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I was talking about this part:

  if (!aCellFrame->GetRowSpan() || !aCellFrame->GetColSpan())
    spansCauseRebuild = PR_TRUE;

If the rowspan goes from zero to nonzero, we're not going to rebuild due to this clause.  Are you saying that in this case the rebuild will be caused by the CellsSpanInOrOut() call?

That seems reasonable.  If so, it's worth documenting that and removing this assertion.

Comment 11

[Bernd]

Attachment: patch

The assert is primarily triggered when we have to few rows in the cellmap after the cell is removed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Bernd, can you explain the first three changes in that patch?  What are they actually doing?

Comment 13

[Bernd]

they patch bug 416742, which I was reviewing in parallel

Comment 14

[Bernd]

Attachment: cleaned up patch

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 304154 [details] [diff] [review]
cleaned up patch

Looks good.

Comment 16

[Bernd]

fix checked in