Closed Bug 351326 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: Layout: Tables, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: bernd_mozilla)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 1 obsolete file)

230 bytes, application/xhtml+xml
Details
1.38 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
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
Attached file testcase
Assignee: nobody → bernd_mozilla
Still happens on trunk.
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).
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.
Flags: blocking1.9?
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.
How bad is this?
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.
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.
>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.


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.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Attached patch patch (obsolete) — Splinter Review
The assert is primarily triggered when we have to few rows in the cellmap after the cell is removed.
Attachment #303859 - Flags: superreview?(bzbarsky)
Attachment #303859 - Flags: review?(bzbarsky)
Bernd, can you explain the first three changes in that patch?  What are they actually doing?
they patch bug 416742, which I was reviewing in parallel
Attached patch cleaned up patchSplinter Review
Attachment #303859 - Attachment is obsolete: true
Attachment #304154 - Flags: superreview?(bzbarsky)
Attachment #304154 - Flags: review?(bzbarsky)
Attachment #303859 - Flags: superreview?(bzbarsky)
Attachment #303859 - Flags: review?(bzbarsky)
Comment on attachment 304154 [details] [diff] [review]
cleaned up patch

Looks good.
Attachment #304154 - Flags: superreview?(bzbarsky)
Attachment #304154 - Flags: superreview+
Attachment #304154 - Flags: review?(bzbarsky)
Attachment #304154 - Flags: review+
Attachment #304154 - Flags: approval1.9?
Attachment #304154 - Flags: approval1.9? → approval1.9+
fix checked in
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.