Closed Bug 339246 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Layout: Tables, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: mats, Assigned: bernd_mozilla)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical?] post-1.8 regression? retest)

Crash Data

Attachments

(3 files, 4 obsolete files)

Attached file stack
Attached file reduced testcase
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.
OS: Linux → All
Hardware: PC → All
Keywords: testcase
 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)
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.
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Attached patch revised patch (obsolete) — Splinter Review
this will reset the whole cellmap not only the group one, by this we are allowed to reset the colinfo
Attachment #225203 - Attachment is obsolete: true
Attached patch next rev. (obsolete) — Splinter Review
Attachment #225471 - Attachment is obsolete: true
Blocks: 341227
The newest patch touches a different file than the previous patch.  Does the newest patch replace the previous patch or complement it?
Attached patch next. rev (obsolete) — Splinter Review
that was the wrong patch
Attachment #225587 - Attachment is obsolete: true
Attached patch rev6Splinter Review
Attachment #225604 - Attachment is obsolete: true
Attachment #226961 - Flags: superreview?(bzbarsky)
Attachment #226961 - Flags: review?(bzbarsky)
Blocks: 341814
Attachment #226961 - Flags: superreview?(bzbarsky)
Attachment #226961 - Flags: superreview+
Attachment #226961 - Flags: review?(bzbarsky)
Attachment #226961 - Flags: review+
fix checked in
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 343588
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?
Whiteboard: [sg:critical?] post-1.8 regression? retest
Probably don't need on branches, but nominating until we get a definitive answer.
Flags: blocking1.8.0.6?
Flags: blocking1.8.0.6?
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.
Blocks: 346980
fixed on branches by the cellmap branch patch
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)
Group: security
Flags: in-testsuite?
crash test landed
http://hg.mozilla.org/mozilla-central/rev/2fae448c1311
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsIFrame::GetStyleData]
You need to log in before you can comment on or make changes to this bug.