Last Comment Bug 339246 - Crash [@ nsIFrame::GetStyleData] called by nsTableRowFrame::CollapseRowIfNecessary
: Crash [@ nsIFrame::GetStyleData] called by nsTableRowFrame::CollapseRowIfNece...
Status: RESOLVED FIXED
[sg:critical?] post-1.8 regression? r...
: crash, testcase, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Bernd
:
Mentors:
Depends on: 343588
Blocks: stirtable 341227 341814 346980
  Show dependency treegraph
 
Reported: 2006-05-25 11:38 PDT by Mats Palmgren (:mats)
Modified: 2011-06-13 10:01 PDT (History)
7 users (show)
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (18.89 KB, text/plain)
2006-05-25 11:39 PDT, Mats Palmgren (:mats)
no flags Details
reduced testcase (484 bytes, text/html)
2006-05-26 01:26 PDT, Jesse Ruderman
no flags Details
patch (3.09 KB, patch)
2006-06-11 13:07 PDT, Bernd
no flags Details | Diff | Review
revised patch (17.34 KB, patch)
2006-06-13 14:09 PDT, Bernd
no flags Details | Diff | Review
next rev. (1.61 KB, patch)
2006-06-14 09:22 PDT, Bernd
no flags Details | Diff | Review
next. rev (17.35 KB, patch)
2006-06-14 11:44 PDT, Bernd
no flags Details | Diff | Review
rev6 (17.37 KB, patch)
2006-06-24 23:38 PDT, Bernd
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review

Comment 1 Mats Palmgren (:mats) 2006-05-25 11:39:31 PDT
Created attachment 223343 [details]
stack
Comment 2 Jesse Ruderman 2006-05-26 01:26:39 PDT
Created attachment 223417 [details]
reduced testcase
Comment 3 Jesse Ruderman 2006-05-26 01:30:07 PDT
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 2006-06-08 13:43:59 PDT
 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 2006-06-08 23:13:16 PDT
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 2006-06-11 13:03:28 PDT
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 2006-06-11 13:07:45 PDT
Created attachment 225203 [details] [diff] [review]
patch
Comment 9 Bernd 2006-06-13 14:09:25 PDT
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
Comment 10 Bernd 2006-06-14 09:22:29 PDT
Created attachment 225587 [details] [diff] [review]
next rev.
Comment 11 Jesse Ruderman 2006-06-14 09:51:26 PDT
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 2006-06-14 11:44:34 PDT
Created attachment 225604 [details] [diff] [review]
next. rev 

that was the wrong patch
Comment 13 Bernd 2006-06-24 23:38:23 PDT
Created attachment 226961 [details] [diff] [review]
rev6
Comment 14 Bernd 2006-06-28 23:50:05 PDT
fix checked in
Comment 15 Daniel Veditz [:dveditz] 2006-07-07 14:03:58 PDT
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] 2006-07-18 09:09:01 PDT
Probably don't need on branches, but nominating until we get a definitive answer.
Comment 17 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-08-02 02:51:45 PDT
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 2006-08-29 12:33:24 PDT
fixed on branches by the cellmap branch patch
Comment 19 Jay Patel [:jay] 2006-08-30 17:53:01 PDT
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:] 2009-04-24 11:09:15 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/2fae448c1311

Note You need to log in before you can comment on or make changes to this bug.