Closed Bug 317275 Opened 19 years ago Closed 19 years ago

Evil float:right testcase causes assertions and can crash [@ nsFrame::GetFirstLeaf]


(Core :: Layout, defect, P1)






(Reporter: martijn.martijn, Assigned: bzbarsky)



(5 keywords, Whiteboard: [rft-dl])

Crash Data


(7 files, 1 obsolete file)

See upcoming testcase.
It doesn't crash easily, when following the directions in the testcase.
But the assertions I get in a debug build with the testcase, I get 100% of the time.

It doesn't crash Mozilla1.7, I'm quite certain of that. Also, in Mozilla1.7, the green block doesn't seem 'out of place' (which you can see with the dom inspector).
Attached file testcase
This is a backtrace from the assertions

The first assertion, I get while loading the page:
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlo
ck', file c:/mozilla/mozilla/layout/generic/nsBlockReflowState.cpp, line 838
Break: at file c:/mozilla/mozilla/layout/generic/nsBlockReflowState.cpp, line 83

The second I get when hovering over the green bordered box:
###!!! ASSERTION: not in child list: 'nsFrameList(aChildFrame->GetParent()->GetF
irstChild(listName)) .ContainsFrame(aChildFrame)', file c:/mozilla/mozilla/layou
t/base/nsCSSFrameConstructor.cpp, line 1882
Break: at file c:/mozilla/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 18
Attached file backtrace from crash
This is a backtrace from the crash I once got in a debug build.
Summary: Evil float:right testcase causes assertions and can crash → Evil float:right testcase causes assertions and can crash [@ nsFrame::GetFirstLeaf]
I assume this is the minimal testcase that triggers those asserts, right?
Yes. (the crash is very hard to reproduce)
I tried to minimise the testcase from this file.
This crashes reliably for me just by hovering over the document.
OK.  I'll have to dig into the frame construction here to see why we're screwing up the float containing block...  Might not happen till early next week, though.
Maybe useful to know:
Regression range between 2005-01-25 and 2005-01-27 from correct display to not showing content at all.
Regression range between 2005-03-17 and 2005-03-18 from not showing content at all to showing the green block 'out of place' (when looking with domi). That seems influenced by bug 283385.
Minimized version of "testcase2 (crashes on hovering)". Hopefully useful and not spam...
Thanks for that testcase, José!
With that testcase I don't crash in 2005-08-21 trunk build, but I do crash with 2005-08-22 trunk build:
I also crash with a 2005-09-01 branch build.
So maybe a regression from bug 286491 or bug 295767?
This doesn't crash, but I bet fixing it will fix the crashes.
So the issue is that when we're looking for the right "parent frame" in ContentAppended we adjust from the fieldset to the fielset area frame, except if the content is a legend...  or rather except if the _first_ content is a legend, for the case here (where ContentAppended appends both the legend and the float at once).

So for this case we end up with the fieldset frame as the parent, and that's not a float containing block, so the containing block we find (via GetFloatContainingBlock) for the float is the body.  And then we end up getting this assert...
So the real question is how we fix ContentAppended here, basically.  Almost seems like we should make it do a bunch of ContentInserted calls for this case.  Perhaps we can hack GetInsertionPoint to claim "multiple" here?  That's basically what's going on...
Attachment #203944 - Flags: superreview?(dbaron)
Attachment #203944 - Flags: review?(dbaron)
Attached patch Alt. fix? (obsolete) — Splinter Review
Implementing nsFieldSetFrame::GetContentInsertionFrame() seems to fix the
crash also.  This is just a WIP though, there could be some adjustments
needed in nsCSSFrameConstructor with this change...
What do you think, is it worth exploring this alternative?
OS: Windows XP → All
Mats, won't that approach make it impossible to put the legend in the right place?
Comment on attachment 207667 [details] [diff] [review]
Alt. fix?

Right, don't know what I was thinking...
Maybe we should remove the "yet" from the following comment:
because it shouldn't be implemented...
Attachment #207667 - Attachment is obsolete: true
Or at least make it clear that it would have to deal with legends somehow...
Blocks: 322820
Comment on attachment 203944 [details] [diff] [review]
So perhaps something like this

r+sr=dbaron.  Sorry for the delay.

I'm not entirely comfortable with this code, though.  I filed bug 323926 (feel free to fix!) while looking at GetInsertionPoint.  I also wonder whether this really belongs in GetInsertionPoint or in the one caller of GetInsertionPoint that cares about aMultiple, which is ContentAppended, and which does an additional test for childCount>0 (which has a long comment that doesn't make any sense to me) that's equivalent to having multiple insertion points.
Attachment #203944 - Flags: superreview?(dbaron)
Attachment #203944 - Flags: superreview+
Attachment #203944 - Flags: review?(dbaron)
Attachment #203944 - Flags: review+
I coudn't convince myself of the correctness of putting the check outside this function (since at that point it's non-obvious that we can get to the "right" content node)...

I do think we shoud try to make fieldsets non-magical somehow in terms of their frame structure and just remove this code.  :(
Fixed on trunk.
Closed: 19 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Comment on attachment 203944 [details] [diff] [review]
So perhaps something like this

I think we should take this on the branches... This should be pretty safe, and is along the lines of the sort of thing  we've been trying to take.

Certainly we should take it on the 1.8.1 branch once it's had enough bake time.
Attachment #203944 - Flags: approval1.8.1?
Attachment #203944 - Flags: approval1.8.0.2?
Resolution: FIXED → ---
Assignee: nobody → bzbarsky
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Verified FIXED using:

Testcase 1:
Testcase 2:
Testcase 3:

on both Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060119 Firefox/1.6a1 and SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060119 Mozilla/1.0

(both trunk, of course)
Attachment #203944 - Flags: approval1.8.1? → branch-1.8.1?(dbaron)
Attachment #203944 - Flags: branch-1.8.1?(dbaron) → branch-1.8.1+
Fixed on 1.8.1 branch.
Keywords: fixed1.8.1
Flags: blocking1.8.0.2+
Comment on attachment 203944 [details] [diff] [review]
So perhaps something like this

approved for 1.8.0 branch, a=dveditz
Attachment #203944 - Flags: approval1.8.0.2? → approval1.8.0.2+
Fixed branch
Keywords: fixed1.8.0.2
Marking [rft-dl] (ready for testing in Firefox release candidates)
Whiteboard: [rft-dl]
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20060301 Firefox/, no crash with any of the testcases.
Depends on: 337124
Crash Signature: [@ nsFrame::GetFirstLeaf]
You need to log in before you can comment on or make changes to this bug.