368651 - [FIX]Crash [@ nsCellMapColumnIterator::GetNextFrame] with zero rowspan

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase makes Firefox crash [@ nsCellMapColumnIterator::GetNextFrame].

Comment 1

[Jesse Ruderman]

In a debug build, I see

###!!! ASSERTION: Bogus mOrigCells?: 'mCurMapRow < mCurMapRelevantRowCount', file /Users/admin/trunk/mozilla/layout/tables/nsCellMap.cpp, line 2763

before the crash.

Comment 2

[-fullmetaljacket-]

also crashed on

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a2pre) Gecko/20070129 Minefield/3.0a2pre ID:2007012917 [cairo]

Talkback ID: TB28838680Y
nsTableCellMap::GetEffectiveColSpan  [mozilla\layout\tables\nscellmap.cpp, line 412]
BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths  [mozilla\layout\tables\basictablelayoutstrategy.cpp, line 381]
SearchTable  [mozilla\xpcom\build\pldhash.c, line 421]
BasicTableLayoutStrategy::ComputeIntrinsicWidths  [mozilla\layout\tables\basictablelayoutstrategy.cpp, line 596]
nsTableRowFrame::GetContinuousBCBorderWidth  [mozilla\layout\tables\nstablerowframe.h, line 433]
nsTableFrame::OrderRowGroups  [mozilla\layout\tables\nstableframe.cpp, line 2702]
nsFrame::ComputeSize  [mozilla\layout\generic\nsframe.cpp, line 3141]
nsTableFrame::OrderRowGroups  [mozilla\layout\tables\nstableframe.cpp, line 2667]
nsHTMLReflowState::nsHTMLReflowState  [mozilla\layout\generic\nshtmlreflowstate.cpp, line 137]
nsPopupFrameList::nsPopupFrameList  [mozilla\layout\xul\base\src\nspopupsetframe.cpp, line 86]
nsTableOuterFrame::GetMargin  [mozilla\layout\tables\nstableouterframe.cpp, line 445]
nsTableOuterFrame::OuterReflowChild  [mozilla\layout\tables\nstableouterframe.cpp, line 1123]
nsTableOuterFrame::Reflow  [mozilla\layout\tables\nstableouterframe.cpp, line 1228]

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase with text so you can see layout

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Same but without the JS -- just static page

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: HTML version of crashing testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: HTML version of static testcase

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: HTML quirks version (rowspan=0 not supported in quirks mode)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Pathch

It's not my fault!  It's a cellmap bug, I tell you!

If you grab a build from before bug 368651 landed and load the "Testcase with text so you can see layout" and the "Same but without the JS -- just static page" testcase, they render differently.  They shouldn't.  The reason they do is that CellsSpanInOrOut doesn't detect this case, since there's nothing in the row in question _yet_.  There will be once the row is added, however...

The HTML versions of those testcases show the same problem.

I just realized that we don't honor zero-spans in quirks mode, so ignore the quirks testcase.  ;)

Comment 9

[Bernd]

Comment on attachment 253301 [details] [diff] [review]
Pathch

basing decisions with zero rowspans on numrows
as in 
> if (aStartRowIndex == numRows && numRows > 0) {

is wrong. Zerorowspans expand only to mContentRowCount. see coming patch.

Comment 10

[Bernd]

Attachment: patch based on mContentRowCount

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 254318 [details] [diff] [review]
patch based on mContentRowCount

I like this a _lot_ more than what I did.  Could we document this invariant (zerospans only span up to mContentRowCount) somewhere?

>Index: nsCellMap.cpp
>@@ -1608,16 +1611,22 @@ PRBool nsCellMap::CellsSpanInOrOut(PRInt
>+      if (aStartRowIndex >= mContentRowCount) {
>+        cellData = GetDataAt(mContentRowCount - 1, colX);

I know GetDatAt handles random stuff like -1 being passed in at the moment, but I'd still prefer an mContentRowCount > 0 check here.

r+sr=bzbarsky with that.  And thanks!

Comment 12

[Bernd]

fix checked in