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

VERIFIED FIXED in mozilla1.9alpha1



14 years ago
8 years ago


(Reporter: martijn.martijn, Assigned: bzbarsky)


(5 keywords)

Dependency tree / graph
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [rft-dl], crash signature)


(7 attachments, 1 obsolete attachment)



14 years ago
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).

Comment 1

14 years ago
Posted file testcase

Comment 2

14 years ago
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

Comment 3

14 years ago
This is a backtrace from the crash I once got in a debug build.


14 years ago
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?

Comment 5

14 years ago
Yes. (the crash is very hard to reproduce)

Comment 6

14 years ago
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.

Comment 8

14 years ago
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.

Comment 9

14 years ago
Minimized version of "testcase2 (crashes on hovering)". Hopefully useful and not spam...

Comment 10

14 years ago
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)


14 years ago
Posted 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...


13 years ago
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.
Last Resolved: 13 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?


13 years ago
Resolution: FIXED → ---


13 years ago
Assignee: nobody → bzbarsky


13 years ago
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
Verified FIXED using:

Testcase 1: https://bugzilla.mozilla.org/attachment.cgi?id=203779
Testcase 2: https://bugzilla.mozilla.org/attachment.cgi?id=203844
Testcase 3: https://bugzilla.mozilla.org/attachment.cgi?id=203917

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)


13 years ago
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]

Comment 28

13 years ago
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.


13 years ago
Depends on: 337124
Crash Signature: [@ nsFrame::GetFirstLeaf]
You need to log in before you can comment on or make changes to this bug.