331679 - Crash involving ::-moz-table-row-group, overflow, position, and opacity [@ nsIView::GetOffsetTo]

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

[sg:critical] because before I reduced the testcase, I got crashes with random addresses on top.  The only reduced testcases I managed to make were for nondeterministic null dereferences, so I will retest with the original file once this is fixed.

Stack signatures (functions on top of the stack) included:

[@ nsIView::GetOffsetTo] 
[@ nsCSSFrameConstructor::BeginBuildingScrollFrame] -- with random at top 
[@ nsCSSFrameConstructor::ContentInserted] 
[@ nsHTMLContainerFrame::CreateViewForFrame] 
[@ IncrementalReflow::AddCommand] 
[@ nsHTMLReflowState::InitConstraints]

Comment 1

[Jesse Ruderman]

Attachment: reduced testcase for crash [@ nsIView::GetOffsetTo]

Usually crashes after about 10 reloads.

Comment 2

[Bernd]

Attachment: reduced tectase that triggers assertion

###!!! ASSERTION: unexpected second call to SetInitialChildList: 'Not Reached',
file d:/moz_src/mozilla/layout/generic/nsContainerFrame.cpp, line 108

This happens on the scroll frame around the rowgroup which is a abs. containing block.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So what's GetAbsoluteContainingBlock returning here, and why?

Comment 4

[Bernd]

Attachment: testcase without abs.pos. which triggers the assert

rowgroup pseudos are the parent frames at pseudoFrames.mRowGroup.mFrame. If we build however a scrollframe for the rowgroup, we have the scrollframe there and then we put the row on the childlist of the.... scrollframe allready occupied by the rowgroupframe. (The typical case of: NOBODY expects the Spanish Inquisition!)

Comment 5

[Bernd]

Attachment: patch

This code is wrong since it has been written, the typical effect is that we loose the rowframe and all its children. Then its only a question what you stuffed inside this row to determine where we crash, abs. pos with opacity, seems nice, the abs.pos animated gif should work too. I guess we need to get this, once it has baked, back to branches.

Comment 6

[Bernd]

fix checked in, open for some stress tests by Jesse 

Comment 7

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 8

[Bernd]

fix checked in into branches

Comment 9

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US;
rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5, no crash with testcase.

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to comment #0)
> [sg:critical] because before I reduced the testcase, I got crashes with random
> addresses on top.  The only reduced testcases I managed to make were for
> nondeterministic null dereferences, so I will retest with the original file
> once this is fixed.

Jesse: Do you still have the original testcase, and if so did this really fix it?

asac: I don't think anyone tested this on the 1.7 branch.

Comment 11

[Jesse Ruderman]

I think I tested the original testcase (and various intermediate testcases) shortly after this was fixed and didn't hit any more crashes.  I think the more recent fix for bug 331883 affects how Gecko thinks about this testcase.

Comment 12

[Alexander Sack]

Attachment: 1.0.x patch

Comment 13

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=216197
ff2b2 no crash windows, linux, macppc

https://bugzilla.mozilla.org/attachment.cgi?id=216414&action=view
ff2b2 windows, linux, macppc no crash; windows, linux no assert

https://bugzilla.mozilla.org/attachment.cgi?id=219177
ff2b2 windows, linux, macppc; windows, linux no assert

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.8

Comment 15

[Jesse Ruderman]

Crashtests checked in.

Comment 16

[Jesse Ruderman]

The crashtests trigger CSS errors because bug 331883 has been fixed -- web pages cannot reference these internal pseudo-elements at all.