Last Comment Bug 317275 - Evil float:right testcase causes assertions and can crash [@ nsFrame::GetFirstLeaf]
: Evil float:right testcase causes assertions and can crash [@ nsFrame::GetFirs...
Status: VERIFIED FIXED
[rft-dl]
: crash, fixed1.8.1, regression, testcase, verified1.8.0.2
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: P1 critical (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 337124
Blocks: ajax-demolisher 322820
  Show dependency treegraph
 
Reported: 2005-11-21 04:33 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (468 bytes, text/html)
2005-11-21 04:41 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
backtrace from assertions (15.43 KB, text/plain)
2005-11-21 04:43 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
backtrace from crash (5.46 KB, text/plain)
2005-11-21 04:44 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase2 (crashes on hovering) (32.70 KB, text/html)
2005-11-21 14:05 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Minimized version of testcase 2 (209 bytes, text/html)
2005-11-22 04:24 PST, José Jeria
no flags Details
Minimal testcase for the assertion (252 bytes, text/html)
2005-11-22 07:36 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
So perhaps something like this (2.24 KB, patch)
2005-11-22 09:09 PST, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
dbaron: superreview+
dbaron: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
Alt. fix? (635 bytes, patch)
2006-01-05 15:01 PST, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2005-11-21 04:33:56 PST
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 [:mwargers] (not working for Mozilla) 2005-11-21 04:41:51 PST
Created attachment 203779 [details]
testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-11-21 04:43:49 PST
Created attachment 203780 [details]
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 [:mwargers] (not working for Mozilla) 2005-11-21 04:44:55 PST
Created attachment 203781 [details]
backtrace from crash

This is a backtrace from the crash I once got in a debug build.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2005-11-21 13:52:46 PST
I assume this is the minimal testcase that triggers those asserts, right?
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-11-21 14:00:06 PST
Yes. (the crash is very hard to reproduce)
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-11-21 14:05:08 PST
Created attachment 203844 [details]
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 [:bz] (still a bit busy) 2005-11-21 14:07:44 PST
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 [:mwargers] (not working for Mozilla) 2005-11-21 15:20:01 PST
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 2005-11-22 04:24:35 PST
Created attachment 203917 [details]
Minimized version of testcase 2

Minimized version of "testcase2 (crashes on hovering)". Hopefully useful and not spam...
Comment 10 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-11-22 05:26:59 PST
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 [:bz] (still a bit busy) 2005-11-22 07:36:28 PST
Created attachment 203932 [details]
Minimal testcase for the assertion

This doesn't crash, but I bet fixing it will fix the crashes.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2005-11-22 07:40:04 PST
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 [:bz] (still a bit busy) 2005-11-22 07:45:21 PST
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 [:bz] (still a bit busy) 2005-11-22 09:09:13 PST
Created attachment 203944 [details] [diff] [review]
So perhaps something like this
Comment 15 Mats Palmgren (:mats) 2006-01-05 15:01:32 PST
Created attachment 207667 [details] [diff] [review]
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 [:bz] (still a bit busy) 2006-01-05 19:52:22 PST
Mats, won't that approach make it impossible to put the legend in the right place?
Comment 17 Mats Palmgren (:mats) 2006-01-06 10:25:40 PST
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 [:bz] (still a bit busy) 2006-01-06 18:33:28 PST
Or at least make it clear that it would have to deal with legends somehow...
Comment 19 David Baron :dbaron: ⌚️UTC-10 2006-01-18 13:38:06 PST
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 [:bz] (still a bit busy) 2006-01-18 19:45:42 PST
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 [:bz] (still a bit busy) 2006-01-18 21:25:14 PST
Fixed on trunk.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2006-01-18 21:25:56 PST
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] 2006-01-19 14:24:55 PST
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 [:bz] (still a bit busy) 2006-02-08 19:47:03 PST
Fixed on 1.8.1 branch.
Comment 25 Daniel Veditz [:dveditz] 2006-02-22 00:47:43 PST
Comment on attachment 203944 [details] [diff] [review]
So perhaps something like this

approved for 1.8.0 branch, a=dveditz
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2006-02-22 18:51:12 PST
Fixed 1.8.0.2 branch
Comment 27 Dave Liebreich [:davel] 2006-03-01 16:26:56 PST
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Comment 28 Jay Patel [:jay] 2006-03-01 17:01:02 PST
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.

Note You need to log in before you can comment on or make changes to this bug.