Closed Bug 366128 Opened 14 years ago Closed 13 years ago
Crash with float and xul in <select> [@ ns
Frame Manager::Capture Frame State] [@ ns Frame Manager::Capture Frame State]
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.
This is essentially the same underlying cause as bug 322436 - a float as child of a XUL display type frame.
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
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.
In ff22.214.171.124 and ff126.96.36.199 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?
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.
Assignee: nobody → mats.palmgren
is the wall paper patch ready to go in on the trunk?
mats, who would be a good reviewer?
Whiteboard: [sg:critical] → [sg:critical] have patch, needs review
Updated to trunk
Attachment #250703 - Attachment is obsolete: true
(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...
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...
Flags: blocking1.9? → blocking1.9+
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....
mats can you write the patch that Boris described?
roc attached a patch to bug 322436.
roc, this still crashes for me, with ###!!! ASSERTION: not in child list: 'found', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 1774
Followup fix. We should call PushFloatContainingBlock(newFrame) if newFrame is a real block.
Comment on attachment 267536 [details] [diff] [review] fix Looks reasonable.
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.
Should the comment above the modified code be updated too?
Sorry Boris ... this is better. XUL IFRAMEs and other subdocuments should not be pushed as float containing blocks.
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?
Oh, we're just avoiding an assert. Nevermind. ;)
Checked in with comment addressed.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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?
bz or roc would know better than me whether this fix is needed on branch.
Whiteboard: [sg:critical] have patch, needs review → [sg:critical] need answer to comment 22
Whiteboard: [sg:critical] need answer to comment 22 → [sg:critical] need answer from roc or bz to comment 22
I think this should be fixed on branch. A bad frame tree is very risky.
Whiteboard: [sg:critical] need answer from roc or bz to comment 22 → [sg:critical]
Comment on attachment 268168 [details] [diff] [review] updated patch approved for 188.8.131.52 and 184.108.40.206, a=dveditz for release-drivers
Can we push this to 220.127.116.11/18.104.22.168? I'm not comfortable pushing this into the branch in a rush.
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.
No, I guess that's OK. At least I'm not on the hook :-)
verified fixed 22.214.171.124 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:126.96.36.199) Gecko/2007071317 Firefox/188.8.131.52 and on Windows XP - no crash on testcase - adding verified keyword
Verified on MacIntel using Thunderbird version 184.108.40.206 (20070809). Used Thunderbrowse extension to load testcase. No crash.
Comment on attachment 268168 [details] [diff] [review] updated patch clearing approval 220.127.116.11 request, this ended up landing in 18.104.22.168 after all (see above comments)
Attachment #268168 - Flags: approval22.214.171.124?
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsFrameManager::CaptureFrameState] [@ nsFrameManager::CaptureFrameState]
You need to log in before you can comment on or make changes to this bug.