339129 - Crash in [@ TableBackgroundData::IsVisible]

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

A few of the assertions leading up to the crash, the stack and some data.

Comment 6

[Jesse Ruderman]

Attachment: reduction for comment 5

An [sg:critical] crash involving rowspan="0".  Nightlies and debug builds alike dereference random memory from various functions.  Common crashes are within nsCSSRendering::PaintBackgroundWithSC, either in nsNativeThemeMac::ThemeSupportsWidget or nsCachedStyleData::GetStyleData.

Comment 7

[Bernd]

Mats and Jesse, big thanks for the help, that is now something one can work on.

Comment 8

[Bernd]

Attachment: patch

Comment 9

[Bernd]

Comment on attachment 240411 [details] [diff] [review]
patch

This partly answers your question in bug 351942

bz:

In fact, where do we handle zero rowspans with the new code?  Or is that
already handled by nsCellMap::AppendCell?  But what if after that the number of
rows in the rowgroup increases?

Comment 10

[Boris Zbarsky [:bzbarsky]]

So what does this patch actually do?  Why does it fix the problem?

Should there be some documentation about how mRowCount and mRows.Count() differ?

Comment 11

[Bernd]

>Should there be some documentation about how mRowCount and mRows.Count()
differ?
Yes, been there, done that: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsCellMap.h&rev=3.56&mark=486-490#480

There are two separate problems:

1.) We currently use the number of rows in the cellmap at the moment when we create the rowspan 0. This is a problem if we build up the cellmap the number of rows is typically smaller than the number of rows in the content. Only once we have processed the last content row the number of cellmap rows will reach the content row number. The rowspan should however cover down to the last content row.

This is the part: https://bugzilla.mozilla.org/attachment.cgi?id=240411&action=diff#mozilla/layout/tables/nsCellMap.cpp_sec1

2.) There is a another problem in the RebuildConsideringCells routine where we nuke the cellmap rows and create new entries. This routine does  not alter the number of content rows. So all that voodoo is just a plain hack, that has been biting me before and hit again during debugging so I removed it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So for the first hunk, shouldn't we use the PR_MAX of mRows.Count() and mRowCount?  If not, why not?

Comment 13

[Bernd]

We would violate the spec.

"The value zero ("0") means that the cell spans all rows from the current row to the last row of the table section (THEAD, TBODY, or TFOOT) in which the cell is defined" 

This does not state that we should have the rowspan down to the last possible virtual cell position. Imagine the following its a case where both signficantly differ:

<tr><td rowspan="0"></td></tr>
<tr><td rowspan="5"></td></tr>

With current patch the effective rowspan will be 2 with your proposal it will be 6.

Both sections are related. I believe you can rely on mRowCount, a better name would be mContentRowCount (probably I shouldn't file the next ~50kb within this bug) as long as RebuildConsideringCells does not hork its value.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> With current patch the effective rowspan will be 2 with your proposal it will
> be 6.

Right... but isn't "6" where the row group ends?

Comment 15

[Bernd]

not exactly, row 2 to 6 are virtual rows where the rowspan exceeds the rowgroup

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 240411 [details] [diff] [review]
patch

Mmmmm....  I'm not sure I buy that, but if that's what we do now in general I guess we shouldn't change it in this patch...

r+sr=bzbarsky

Comment 17

[Bernd]

fixed on trunk

Comment 18

[Daniel Veditz [:dveditz]]

The reduction testcase WFM in FF1.5.0.5, 1.5.0.8, and 1.5.0.10pre as well as FF2 and 2.0.0.1

Was this a trunk-only crash? I thought the cell-map code was pretty old and buggy on all branches before Bernd started fixing things, and I thought those fixes first went into 1.5.0.7

Comment 19

[Bernd]

there have been a couple of checkins http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/tables/nsCellMap.cpp&rev=MOZILLA_1_8_0_BRANCH&mark=3.92.12.1.2.5
I am not sure about the timeline. The bug might have been hidden on branch by other problems that got changed on trunk