Closed Bug 286137 Opened 20 years ago Closed 19 years ago

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

Categories

(Core :: Layout: Tables, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: bernd_mozilla)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(5 files, 1 obsolete file)

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)
Attached file Testcase (obsolete) —
Attached file Testcase
Oops! Previous testcase was the wrong one.
Attachment #177419 - Attachment is obsolete: true
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.
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.
Attached file 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.
Depends on: 286143
Attached file 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.
Attached patch patchSplinter Review
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.
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Attachment #180151 - Flags: superreview?(bzbarsky)
Attachment #180151 - Flags: review?(bzbarsky)
Comment on attachment 180151 [details] [diff] [review]
patch

r+sr=bzbarsky; file a followup bug on making GetCaptionAdjustedParent and
AdjustCaptionParentFrame share some code?
Attachment #180151 - Flags: superreview?(bzbarsky)
Attachment #180151 - Flags: superreview+
Attachment #180151 - Flags: review?(bzbarsky)
Attachment #180151 - Flags: review+
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.
Attachment #180151 - Flags: approval1.8b2?
Comment on attachment 180151 [details] [diff] [review]
patch

a=asa
Attachment #180151 - Flags: approval1.8b2? → approval1.8b2+
fix checked in, I filed the followup bug 289936 as requested
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Well, the first testcase still crashes for me with the 2005-04-12 trunk build.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #180979 - Flags: superreview?(bzbarsky)
Attachment #180979 - Flags: review?(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?
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 on attachment 180979 [details] [diff] [review]
patch that fixes the crash (for me at least)

Right.	Makes sense to me.
Attachment #180979 - Flags: superreview?(bzbarsky)
Attachment #180979 - Flags: superreview+
Attachment #180979 - Flags: review?(bzbarsky)
Attachment #180979 - Flags: review+
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.
Attachment #180979 - Flags: approval1.8b2?
Comment on attachment 180979 [details] [diff] [review]
patch that fixes the crash (for me at least)

a=asa
Attachment #180979 - Flags: approval1.8b2? → approval1.8b2+
fix checked in
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Ok, seems fixed now. Great job!
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsIFrame::Invalidate]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: