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

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

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

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: backtrace from assertions

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
82

Comment 3

[Martijn Wargers (dead)]

Attachment: backtrace from crash

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

I assume this is the minimal testcase that triggers those asserts, right?

Comment 5

[Martijn Wargers (dead)]

Yes. (the crash is very hard to reproduce)

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase2 (crashes on hovering)

I tried to minimise the testcase from this file.
This crashes reliably for me just by hovering over the document.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

[Martijn Wargers (dead)]

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

[José Jeria]

Attachment: Minimized version of testcase 2

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

Comment 10

[Martijn Wargers (dead)]

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:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-21+13&maxdate=2005-08-22+08&cvsroot=%2Fcvsroot
I also crash with a 2005-09-01 branch build.
So maybe a regression from bug 286491 or bug 295767?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal testcase for the assertion

This doesn't crash, but I bet fixing it will fix the crashes.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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...

Comment 13

[Boris Zbarsky [:bzbarsky]]

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...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: So perhaps something like this

Comment 15

[Mats Palmgren (inactive)]

Attachment: Alt. fix?

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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Mats, won't that approach make it impossible to put the legend in the right place?

Comment 17

[Mats Palmgren (inactive)]

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:
http://webtools.mozilla.org/bonsai/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1171&root=/cvsroot&mark=8166#8163
because it shouldn't be implemented...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Or at least make it clear that it would have to deal with legends somehow...

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Stephen Donner [:stephend] Not actively reading bugmail]

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)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.1 branch.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 203944 [details] [diff] [review]
So perhaps something like this

approved for 1.8.0 branch, a=dveditz

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fixed 1.8.0.2 branch

Comment 27

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)

Comment 28

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060301 Firefox/1.5.0.1, no crash with any of the testcases.