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

RESOLVED FIXED

Status

()

Core
Layout: Tables
--
critical
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: mats, Assigned: Bernd)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] post-1.8 regression? retest, crash signature)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Comment 1

11 years ago
Created attachment 223343 [details]
stack

Comment 2

11 years ago
Created attachment 223417 [details]
reduced testcase

Comment 3

11 years ago
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

Updated

11 years ago
Keywords: testcase
(Assignee)

Comment 4

11 years ago
 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)
(Assignee)

Comment 5

11 years ago
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.
(Assignee)

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
Created attachment 225203 [details] [diff] [review]
patch
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 9

11 years ago
Created attachment 225471 [details] [diff] [review]
revised patch

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
(Assignee)

Comment 10

11 years ago
Created attachment 225587 [details] [diff] [review]
next rev.
Attachment #225471 - Attachment is obsolete: true

Updated

11 years ago
Blocks: 341227

Comment 11

11 years ago
The newest patch touches a different file than the previous patch.  Does the newest patch replace the previous patch or complement it?
(Assignee)

Comment 12

11 years ago
Created attachment 225604 [details] [diff] [review]
next. rev 

that was the wrong patch
Attachment #225587 - Attachment is obsolete: true
(Assignee)

Comment 13

11 years ago
Created attachment 226961 [details] [diff] [review]
rev6
Attachment #225604 - Attachment is obsolete: true
Attachment #226961 - Flags: superreview?(bzbarsky)
Attachment #226961 - Flags: review?(bzbarsky)
(Assignee)

Updated

11 years ago
Blocks: 341814
Attachment #226961 - Flags: superreview?(bzbarsky)
Attachment #226961 - Flags: superreview+
Attachment #226961 - Flags: review?(bzbarsky)
Attachment #226961 - Flags: review+
(Assignee)

Comment 14

11 years ago
fix checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
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.
(Assignee)

Updated

11 years ago
Blocks: 346980
(Assignee)

Comment 18

11 years ago
fixed on branches by the cellmap branch patch
Keywords: fixed1.8.0.7, fixed1.8.1

Comment 19

11 years ago
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)
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
Group: security
Flags: in-testsuite?

Comment 20

8 years ago
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.