Last Comment Bug 366128 - Crash with float and xul in <select> [@ nsFrameManager::CaptureFrameState] [@ nsFrameManager::CaptureFrameState]
: Crash with float and xul in <select> [@ nsFrameManager::CaptureFrameState] [@...
Status: RESOLVED FIXED
[sg:critical]
: crash, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 322436
Blocks: 325861 framedest 384344 384477
  Show dependency treegraph
 
Reported: 2007-01-06 02:16 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
roc: blocking1.9+
jaymoz: blocking1.8.1.2-
dveditz: wanted1.8.1.x+
jaymoz: blocking1.8.0.10-
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes on load or reload) (599 bytes, application/xhtml+xml)
2007-01-06 02:16 PST, Jesse Ruderman
no flags Details
wallpaper (6.68 KB, patch)
2007-01-06 10:32 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
wallpaper (6.61 KB, patch)
2007-03-24 05:36 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
fix (2.80 KB, patch)
2007-06-06 22:24 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
updated patch (3.11 KB, patch)
2007-06-12 17:48 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.5-
dveditz: approval1.8.0.13+
Details | Diff | Review

Description Jesse Ruderman 2007-01-06 02:16:52 PST
Created attachment 250688 [details]
testcase (crashes on load or reload)

Loading this testcase in a debug build causes an assertion and a scary crash:

###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file /Users/admin/trunk/mozilla/layout/generic/nsBlockReflowState.cpp, line 733

EXC_BAD_ACCESS (0x0001)
KERN_INVALID_ADDRESS (0x0001) at 0xddddddfd

Thread 0 Crashed:
0    nsIFrame::GetNextSibling() const + 20 (nsIFrame.h:738)
1    nsBlockFrame::CheckFloats(nsBlockReflowState&) + 548 (nsBlockFrame.cpp:6287)
2    nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&) + 2376 (nsBlockFrame.cpp:997)
3    nsSelectsAreaFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&) + 248 (nsSelectsAreaFrame.cpp:260)

It also crash opt builds, but with a different signature, and only on reload.  The opt crashes are sometimes [@  nsFrameManager::CaptureFrameState] and sometimes somewhere else.
Comment 1 Mats Palmgren (:mats) 2007-01-06 10:31:08 PST
This is essentially the same underlying cause as bug 322436 - a float as
child of a XUL display type frame.
Comment 2 Mats Palmgren (:mats) 2007-01-06 10:32:31 PST
Created attachment 250703 [details] [diff] [review]
wallpaper

Here's a wallpaper that veto creating a float frame as a child of a XUL
frame during frame construction.
This will of course trigger other assertions, eg:

###!!! ASSERTION: Disagreement about whether it's a block or not: 'fromLine->IsBlock() == fromLine->mFirstChild->GetStyleDisplay()->IsBlockLevel()', file nsBlockFrame.cpp, line 2267
###!!! ASSERTION: How'd we get a floated inline frame? The frame ctor should've dealt with this.: 'aReflowState.mStyleDisplay->mFloats == NS_STYLE_FLOAT_NONE', file nsLineLayout.cpp, line 1037

but I think those are probably less harmful to have than this bug.
The patch fixes the crash and makes the text appear in the second
testcase in bug 322436.
Comment 3 Daniel Veditz [:dveditz] 2007-01-08 08:17:29 PST
In ff1.5.0.9 and ff2.0.0.1 debug I get the "Float frame has wrong parent" assertion, but I don't see a crash in debug or opt builds. Should we take the wallpaper patch anyway just in case, or are we better leaving it alone since there's no crash?
Comment 4 Jay Patel [:jay] 2007-01-08 16:17:27 PST
Not blocking, but assigning to mats, let's see if we can figure out what the underlying problem is on the branches.  It'll be nice to take the patch if we think we need it.
Comment 5 chris hofmann 2007-03-01 13:54:48 PST
is the wall paper patch ready to go in on the trunk?
Comment 6 chris hofmann 2007-03-22 11:59:37 PDT
mats, who would be a good reviewer?
Comment 7 Mats Palmgren (:mats) 2007-03-24 05:36:50 PDT
Created attachment 259507 [details] [diff] [review]
wallpaper

Updated to trunk
Comment 8 Mats Palmgren (:mats) 2007-03-24 06:07:46 PDT
(In reply to comment #6)
> is the wall paper patch ready to go in on the trunk?

Yes, I believe so. 

> mats, who would be a good reviewer?

roc, bzbarsky or dbaron I guess.  I don't know if they have plans to fix
layout of floats in XUL in the 1.9 time frame... if not, then we probably
want something like this patch...
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-25 14:50:57 PDT
I can review it, I guess. But what is this doing to floats in XUL? Treating them as normal in-flow content? I suppose that's probably best...
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-10 21:04:06 PDT
Isn't this equivalent to pushing a null float containing block before processing the kids of frames with these XUL display types, in the cases it covers?  Note that if I have an inline kid of a XUL frame we want to suppress floating of kids of the inline as well, which I don't think this patch does....
Comment 11 Window Snyder 2007-05-31 14:08:19 PDT
mats can you write the patch that Boris described?
Comment 12 Jesse Ruderman 2007-05-31 21:50:03 PDT
roc attached a patch to bug 322436.
Comment 13 Jesse Ruderman 2007-06-06 21:44:53 PDT
roc, this still crashes for me, with

###!!! ASSERTION: not in child list: 'found', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 1774

Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-06-06 22:24:02 PDT
Created attachment 267536 [details] [diff] [review]
fix

Followup fix. We should call PushFloatContainingBlock(newFrame) if newFrame is a real block.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-06 22:35:15 PDT
Comment on attachment 267536 [details] [diff] [review]
fix

Looks reasonable.
Comment 16 Jesse Ruderman 2007-06-06 22:39:02 PDT
Better, but now I get

###!!! ASSERTION: Please push a real float containing block!: '!aNewFloatContainingBlock || aNewFloatContainingBlock->GetContentInsertionFrame()-> IsFloatContainingBlock()', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 1285

every time I open a Firefox browser window.
Comment 17 Jesse Ruderman 2007-06-06 22:39:57 PDT
Should the comment above the modified code be updated too?
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-06-12 17:48:14 PDT
Created attachment 268168 [details] [diff] [review]
updated patch

Sorry Boris ... this is better. XUL IFRAMEs and other subdocuments should not be pushed as float containing blocks.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-12 21:51:19 PDT
Comment on attachment 268168 [details] [diff] [review]
updated patch

I'd reverse the condition of the ?: and the results; having the '!' there just seems to confuse things unnecessarily.

That said, why were we ever constructing any kids under the subdocument?  That shouldn't be happening, given that they're leaves, no?
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-12 21:51:47 PDT
Oh, we're just avoiding an assert.  Nevermind.  ;)
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-06-13 02:57:38 PDT
Checked in with comment addressed.
Comment 22 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-28 15:23:27 PDT
I don't crash on the 1.8 branch but I do get the following assertion
###!!! ASSERTION: Float frame has wrong parent: 'floatFrame->GetParent() == mBlock', file c:/moz/mozilla_1_8_branch/mozilla/layout/generic/nsBlockReflowState.cpp, line 847

Jesse, is this assertion important enough to get fixed on the branch?
Comment 23 Jesse Ruderman 2007-06-28 15:32:00 PDT
bz or roc would know better than me whether this fix is needed on branch.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-02 16:52:07 PDT
I think this should be fixed on branch. A bad frame tree is very risky.
Comment 25 Daniel Veditz [:dveditz] 2007-07-06 14:22:56 PDT
Comment on attachment 268168 [details] [diff] [review]
updated patch

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-11 20:11:56 PDT
Can we push this to 1.8.1.6/1.8.0.14? I'm not comfortable pushing this into the branch in a rush.
Comment 27 Mats Palmgren (:mats) 2007-07-11 20:49:15 PDT
This was actually checked in earlier today to fix bug 384344, which is a merge
of this fix and bug 322436.  If you're not comfortable with it we should
back that out.  Sorry for the confusion, I should have let it be handled
by the respective bugs instead of course... my bad.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-11 22:32:45 PDT
No, I guess that's OK. At least I'm not on the hook :-)
Comment 29 Carsten Book [:Tomcat] 2007-07-17 14:59:18 PDT
verified fixed 1.8.1.5 using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.5) Gecko/2007071317 Firefox/2.0.0.5 and on Windows XP - no crash on testcase - adding verified keyword
Comment 30 juan becerra [:juanb] 2007-08-20 12:08:56 PDT
Verified on MacIntel using Thunderbird version 1.5.0.13 (20070809). Used Thunderbrowse extension to load testcase. No crash. 
Comment 31 Daniel Veditz [:dveditz] 2007-08-23 14:29:11 PDT
Comment on attachment 268168 [details] [diff] [review]
updated patch

clearing approval 1.8.1.7 request, this ended up landing in 1.8.1.5 after all (see above comments)
Comment 32 Jesse Ruderman 2007-12-15 21:43:08 PST
Crashtest checked in.

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