339246 - Crash [@ nsIFrame::GetStyleData] called by nsTableRowFrame::CollapseRowIfNecessary

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

Comment 2

[Jesse Ruderman]

Attachment: reduced testcase

Comment 3

[Jesse Ruderman]

Before the crash, I see:

###!!! ASSERTION: cell map has too many cols: 'numColsInCache == numColsInMap', file /Users/admin/trunk/mozilla/layout/tables/nsTableFrame.cpp, line 1055

The crash looks like a null deref:

0x0734cee4 in nsIFrame::GetStyleData (this=0x0, aSID=eStyleStruct_Visibility) at /Users/admin/trunk/mozilla/layout/base/../generic/nsIFrame.h:590

I did all my testing on Mac, so -> All/All.

Comment 4

[Bernd]

 row 0 : C0,0  C0,1  C
 row 1 : C1,0  R     R C
 C0,0=033B31E4(0)  C0,1=033B3454(1)
 C1,0=033B35D4(0)
 ***** END GROUP CELL MAP DUMP *****
**** END TABLE CELL MAP DUMP *****
***END TABLE DUMP***
ppended cell=033B3454 row=0
**** START TABLE CELL MAP DUMP ***** 03355200
ols array orig/span-> 033552000=3/0 1=1/2 2=0/2  cols in cache 3
 ***** START GROUP CELL MAP DUMP ***** 033B40E0
 tbody mapRowCount=2 tableRowCount=1
 row 0 : C0,0  C     C
 row 1 : R     R C   R C
 C0,0=033B3454(0)

 ***** END GROUP CELL MAP DUMP *****
**** END TABLE CELL MAP DUMP *****
ppended cell=033B35D4 row=1
**** START TABLE CELL MAP DUMP ***** 03355200
ols array orig/span-> 033552000=3/0 1=1/2 2=0/2 3=1/0  cols in cache 3
 ***** START GROUP CELL MAP DUMP ***** 033B40E0
 tbody mapRowCount=2 tableRowCount=2
 row 0 : C0,0  C     C
 row 1 : R     R C   R C   C1,3
 C0,0=033B3454(0)
 C1,3=033B35D4(3)
 ***** END GROUP CELL MAP DUMP *****
**** END TABLE CELL MAP DUMP *****
fter RemoveCell
**** START TABLE CELL MAP DUMP ***** 03355200
ols array orig/span-> 033552000=3/0 1=1/2 2=0/2 3=1/0  cols in cache 3
 ***** START GROUP CELL MAP DUMP ***** 033B40E0
 tbody mapRowCount=2 tableRowCount=2
 row 0 : C0,0  C     C
 row 1 : R     R C   R C   C1,3 <<<<<< the cell is in the fourth column while it should be in the third at least ideally even the second
 C0,0=033B3454(0)
 C1,3=033B35D4(3)

Comment 5

[Bernd]

http://lxr.mozilla.org/seamonkey/source/layout/tables/nsCellMap.cpp#1989 is basically a MemMove operation where the overlap between dest and src becomes especially critical if zero span can indefinetely expand towards the end of the src.

Comment 6

[Bernd]

The core issue is that nsCellMap::RebuildConsideringCells the most harsh form of cellmap adjustment, does not reset the column info. So when we rebuild the cellmap we use the old column information  like the number of columns and since we add new data to the number of cells originating in a given column after one call to RebuildConsideringCells is always wrong. The attached patch will fix this, however my stomach tells me that this code is wrong as http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsCellMap.cpp&rev=3.102&mark=1911-1913&#1893
 The reason to believe this is that the column info is shared between the rowgroup cellmaps and sits at the whole tablecellmap. So my guess is that one needs to rebuild the table cellmap rather than one group map.

Comment 7

[Bernd]

Attachment: patch

Comment 9

[Bernd]

Attachment: revised patch

this will reset the whole cellmap not only the group one, by this we are allowed to reset the colinfo

Comment 10

[Bernd]

Attachment: next rev.

Comment 11

[Jesse Ruderman]

The newest patch touches a different file than the previous patch.  Does the newest patch replace the previous patch or complement it?

Comment 12

[Bernd]

Attachment: next. rev

that was the wrong patch

Comment 13

[Bernd]

Attachment: rev6

Comment 14

[Bernd]

fix checked in

Comment 15

[Daniel Veditz [:dveditz]]

This reduced testcase does not crash Bon Echo or ff1.5.0.4 (winXP). Is it a regression? If so from what? If we land the other stir-table fixes will we need this one on the branch anyway?

This testcase looks like a null-deref as Jesse noted, but the description in comment 6 makes it sound potentially worse. "sg:critical?" then?

Comment 16

[Daniel Veditz [:dveditz]]

Probably don't need on branches, but nominating until we get a definitive answer.

Comment 17

[Martijn Wargers (dead)]

As Bernd said in bug 341814, comment 6, it fixes the crash in bug 341814, which can be seen on branch. The testcase in bug 341814 crashes branch builds.

Comment 18

[Bernd]

fixed on branches by the cellmap branch patch

Comment 19

[Jay Patel [:jay]]

v.fixed on both branches, no crash with testcase from bug 341814 (did crash 1.5.0.6):
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060829 Firefox/1.5.0.7 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060830 BonEcho/2.0b2

NOTE: Jesse's reduced testcase in this bug did not crash any of my branch builds (1506, 1507, or bonecho)

Comment 20

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/2fae448c1311