286137 - Crash [@ nsIFrame::Invalidate] with this evil testcase, using display:table-caption and then reloading page

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Martijn Wargers (dead)]

Bernd, I don't want to spoil your private roadmap, but here is another crasher ;)
This looks rather similar to bug 280009.
Follow the steps in the testcase to trigger the crash.

Talkback ID: TB4346075X

nsIFrame::Invalidate 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/generic/nsFrame.cpp,
line 2526]
nsImageFrame::FrameChanged 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/generic/nsImageFrame.cpp,
line 727]
nsImageListener::FrameChanged 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/generic/nsImageFrame.cpp,
line 2149]
imgRequest::FrameChanged 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/modules/libpr0n/src/imgRequest.cpp,
line 396]
nsTimerImpl::Fire 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/xpcom/threads/nsTimerImpl.cpp,
line 396]
nsAppStartup::Run 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 145]
main 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 60]
kernel32.dll + 0x1eb69 (0x77e5eb69)

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: Testcase

Oops! Previous testcase was the wrong one.

Comment 3

[Bernd]

Martijn, if you and Robert would not feed new bugs the task would be trivial,
while it is initialy easier with the testcases plugging the holes should make it
more and more difficult to create them.

Comment 4

[Bernd]

As outlined in other animated gif crashers we loose the image and don't tear it
down when unloading the page. So it would be good to have a second testcase that
causes with minimal js if possible also without :hover the drop of a static
image or even better some text instead of the image. Thats the first step that I
usually do when going to find the cause.

Comment 5

[Martijn Wargers (dead)]

Attachment: Testcase2

Ok, following the directions in comment 4, I come to this.
This testcase looks rather similar to (the testcase in) bug 286143.
So this and that bug have perhaps similar causes.

Comment 6

[Bernd]

Attachment: testcase3

broken frame hierachy after load.

	      line 032987A0: count=1 state=block,clean,prevm
		TableOuter(table)(1)@0328FA34 next=03298730
		  Table(table)(1)@0328FB98 {0,0,48,0} [state
###!!! ASSERTION: bad parent frame pointer: 'kid->GetParent(
		    Block(tbody)(0)@0329833C {0,0,0,0} [stat
		      line 03233208: count=1 state=block,dir
			TableOuter(tbody)(0)@03232E14 {0,0,0
			  Table(tbody)(0)@03232EBC {0,0,0,0}
			    TableRowGroup(tbody)(0)@0328FD58
			      TableRow(tr)(0)@032989F4 {0,0,
				TableCell(td)(0)@032982DC {0
				  Block(td)(0)@03233040 {0,0
				    line 032984C8: count=1 s
The following should happen here:
As the row-group becomes a caption it should live under the outer 
TableOuter(table) and go onto the secondary childlist.	The row can not be a
direct child of the caption is needs to be wrapped, so there should be a outer
, inner table frame and a rowgroup around it.

Comment 7

[Bernd]

Attachment: patch

The patch does several things:
First it cleans up ::AppendFrames there should never be a reference to a named
child list in this function.
Second it fixes the invalid in ContentAppended of that function so that the
there removed vodoo is not necessary any longer
Third it copies the caption list logic from ContentAppended to ContentInserted
This logic involves preventing two captions on a outer table frame, the outer
table frame beeing the parent, and that the captions go on a named childlist
And we prevent content to be inserted below a column, thats not part of this
bug but we do the same thing in ContentAppended.
Btw it fixes the blocking bug too.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

r+sr=bzbarsky; file a followup bug on making GetCaptionAdjustedParent and
AdjustCaptionParentFrame share some code?

Comment 9

[Bernd]

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

I did run the regression tests on the patch. The risk seems medium to me, at
least the code that it replaces is simply wrong.

Comment 10

[Asa Dotzler [:asa]]

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

a=asa

Comment 11

[Bernd]

fix checked in, I filed the followup bug 289936 as requested

Comment 12

[Martijn Wargers (dead)]

Well, the first testcase still crashes for me with the 2005-04-12 trunk build.

Comment 13

[Bernd]

Attachment: patch that fixes the crash (for me at least)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180979 [details] [diff] [review]
patch that fixes the crash (for me at least)

So.. why is the first hunk needed?  Because we're skipping construction of
needed pseudo-frames with the current code?

Comment 15

[Bernd]

Let me remphasize the old code comment above
       // aParentFrame may be an inner table frame rather than an outer frame 
       // In this case we need to get the outer frame.
In *this* and only in *this* case we need to do this. What the code without the
patch does, is it looks up the grandparent if it is a outer table frame then it
*assumes* that the parent is a inner table frame and fixes up the parent frame.

So lets assume now that you end here with two nested divs the outer has
display="table-caption" and the inner also and we look from the perspective of
the inner div.

The proposed parent frame is caption frame, corresponding to the outer div,
which sits on the named childlist of the outer table frame (a wrapping pseudo).
The grandparent in this case is the outer table frame. With the existing code we
will fix up the parent frame to be the outer table frame. As a direct child of
the outer table frame we don't need a outer table pseudo around the caption and
do not create one. As this code is not sync with the reminding code we will end
with a caption as a direct child of caption at the assert in the second hunk. If
I had added the assert in the first place finding this would be a nobrainer :-(.
Then the caption tries to go on the named childlist of the caption, which it
does not have and the caption frame corresponding to the inner div is lost as
the corresponding frame manager call fails.

Easy isn't it?? At least not for me on the first round of debugging (8-10h).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180979 [details] [diff] [review]
patch that fixes the crash (for me at least)

Right.	Makes sense to me.

Comment 17

[Bernd]

Comment on attachment 180979 [details] [diff] [review]
patch that fixes the crash (for me at least)

asking for approval the risk assessment is identical to comment 9, only this
time it fixes a reminder that will help with the testcase.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 180979 [details] [diff] [review]
patch that fixes the crash (for me at least)

a=asa

Comment 19

[Bernd]

fix checked in

Comment 20

[Martijn Wargers (dead)]

Ok, seems fixed now. Great job!