Closed
Bug 322678
Opened 19 years ago
Closed 18 years ago
[FIX]Crash [@ nsIFrame::GetParent] with evil testcase position:relative/absolute/display:table-column, etc
Categories
(Core :: Layout, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: martijn.martijn, Assigned: bzbarsky)
References
Details
(4 keywords, Whiteboard: [rft-dl])
Crash Data
Attachments
(5 files, 1 obsolete file)
754 bytes,
text/html
|
Details | |
25.12 KB,
text/plain
|
Details | |
13.75 KB,
text/html
|
Details | |
11.20 KB,
text/plain
|
Details | |
2.90 KB,
patch
|
MatsPalmgren_bugz
:
review+
roc
:
superreview+
roc
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2+
|
Details | Diff | Splinter Review |
See upcoming testcase, when clicking on the button Mozilla crashes. This also crashes my debug build, which has current fixes for bug 322348 and bug 310638 included.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Backtrace from the assertion I get in my debug build: ###!!! ASSERTION: no placeholder frame: 'nsnull != placeholderFrame', file c:/mo zilla/mozilla/layout/generic/nsHTMLReflowState.cpp, line 1039 Break: at file c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp, line 103 and the crash: #0 0x055ed5f2 in nsIFrame::GetParent() const (this=0x0) at c:/mozilla/mozilla/layout/generic/nsIFrame.h:652 #1 0x04f62387 in GetNearestContainingBlock(nsIFrame*, nsMargin&) ( aFrame=0x0, aContentArea=@0x22c088) at c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp:660 #2 0x04f63041 in nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext*, nsH TMLReflowState const*, int, int) (this=0x22c258, aPresContext=0x1001a5c0, cbrs=0x22c488, containingBlockWidth=15120, containingBlockHeight=0) at c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp:1044 #3 0x04f64d22 in nsHTMLReflowState::InitConstraints(nsPresContext*, int, int, n sMargin*, nsMargin*) (this=0x22c258, aPresContext=0x1001a5c0, aContainingBlockWidth=15120, aContainingBlockHeight=0, aBorder=0x0, aPadding=0x0) at c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp:1959 etc...
Updated•19 years ago
|
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Comment 3•19 years ago
|
||
Boris, you were right that we need to keep the recursion on special siblings in bug 310638, however that is not the cause of the crash though. What we have here is simply a placeholder/out-of-flow self-contained within the special sibling. But it seems we never actually remove the special sibling from the tree (DoDeletingFrameSubtree causes the assertion since it now nulls out the out-of-flow pointer). I've searched the code base but I can't find anyone that actually removes and destroys the special sibling. This seems like a pretty serious frame leak if this is the case. Adding this block in DeletingFrameSubtree() fixes the crash: aFrameManager->RemoveFrame(specialSibling->GetParent(), GetChildListNameFor(specialSibling), specialSibling); I'll incorporate that into the upcoming patch for bug 310638 if you agree this is the correct fix.
Assignee | ||
Comment 4•19 years ago
|
||
> But it seems we never actually remove the special sibling from the tree
Yeah, that seems pretty wrong. Where do we handle removing next-in-flows? We should handle removing specil siblings in the same place.
Just removing them in DeletingFrameSubtree seems wrong to me.
Comment 5•19 years ago
|
||
(In reply to comment #4) > Yeah, that seems pretty wrong. Where do we handle removing next-in-flows? In nsBlockFrame::DoRemoveFrame(), which recurses through RemoveBlockChild() http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.750&root=/cvsroot&mark=5581,5590,5601,5625,5773,5809#5580 It looks complicated to remove special siblings there since the special sibling chain is "owned" by the first-in-flow. I think nsBlockFrame::RemoveFrame() is the right place.
Comment 6•19 years ago
|
||
Attachment #209934 -
Flags: superreview?(bzbarsky)
Attachment #209934 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•19 years ago
|
||
Mats, I'm pretty behind on reviews... Might want to check with dbaron whether he might be able to do this one. If not, I'll do it, but probably not for another week or two. :(
Assignee | ||
Updated•19 years ago
|
Assignee | ||
Comment 8•19 years ago
|
||
Comment on attachment 209934 [details] [diff] [review] Patch rev. 1 Given the frame tree in bug 325218 (where we have an {ib} split that's not inside a block), this doesn't look right. But I thought about this a little more, and I'm really confused now. Why is this even an issue? Who's calling DeletingFrameSubtree on part of an {ib} split? Note that ContentRemoved treats this case specially and blows away the whole containing block when this happens...
Attachment #209934 -
Flags: superreview?(bzbarsky)
Attachment #209934 -
Flags: superreview-
Attachment #209934 -
Flags: review?(bzbarsky)
Attachment #209934 -
Flags: review-
Assignee | ||
Comment 9•19 years ago
|
||
OK. So the real issue, as I see it, is that when we get to the code in question in DeletingFrameSubtree our frame model has in it: Inline(span)(3)@0x86d4e70 next=0x8962a08 {5012,238,0,0} [state=00048010] [content=0x896ca20] [sc=0x86d4e20]< Text(0)@0x86d4ef8[0,3,T] {0,0,0,0} [state=51000020] sc=0x86d4ea8 pst=:-moz-non-element< "\n " > > > Block(span)(3)@0x8962a08 next=0x8962b3c {0,336,13958,14} [state=00048010] sc=0x8962a5c(i=1,b=2) pst=:-moz-anonymous-block< Except when I'm in nsCSSFrameConstructor::ReframeContainingBlock the containingBlock I find is "(class nsIFrame *) 0x8889d3c" and has the same conent node as the two above. And does NOT have the special bit set. Digging now to see why we've got this bizarre block floating about.
Assignee | ||
Comment 10•19 years ago
|
||
Bernd, you'll love this one. So will sicking. :( So this is the frame dump we're dealing with. This is at the point when we're trying to reframe due to a restyle. The sequence of events is: 1) We're restyling the span with id="two". We need to recreate frames for it: #6 0xb70aa14f in nsCSSFrameConstructor::RestyleElement (this=0x8856668, aContent=0x8824bb0, aPrimaryFrame=0x8822f6c, aMinHint=0) 2) When we're in RecreateFramesForContent our frame is a block. And its parent is special. (gdb) frame 5 #5 0xb70ac565 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x8856668, aContent=0x8824bb0) at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:11676 11676 if (MaybeRecreateContainerForIBSplitterFrame(frame, &rv) || (gdb) p/x frame->GetStateBits() $9 = 0x2000 (gdb) p/x frame->GetParent()->GetStateBits() p f$10 = 0x48000 (gdb) p frame->GetParent() $11 = (nsIFrame *) 0x86e3f88 3) The parent is the block part of the {ib} split of the span with id="one", which has inline display by this point. 4) For some reason the parent of the frames for the <span id="one"> is a table pseudo-frame. Why, I do not know. I can only assume that somewhere in the jungle that is table frame construction and then finding of the correct parent in ContentInserted we ended up sticking it inside this table cell... 5) We call ReframeContainingBlock() on the <span id="one">. This looks up the nearest non-special block, which is the anon pseudo block inside the anon pseudo cell, etc (the one whose nsIFrame* is 0x8822d28): (gdb) frame 3 #3 0xb70af561 in nsCSSFrameConstructor::ReframeContainingBlock (this=0x8856668, aFrame=0x86e3f88) at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:13327 13327 return ReinsertContent(parentContainer, blockContent); (gdb) p containingBlock $12 = (class nsIFrame *) 0x8822d28 But: (gdb) p containingBlock->GetContent() $13 = (class nsIContent *) 0x8824ac0 which is our outermost span. And its primary frame is a special inline, of course. So conclusions: A) We don't handle at all well attempts to remove a special frame from the tree. B) We try to deal with this by never doing that and reframing containing blocks instead. C) The code in DeletingFrameSubtree that tries to "deal" with it should probably be an assertion instead, unless we change the approach in (B). D) In this case, GetIBContainingBlockFor() is just giving the wrong answer, hence the crash.
Assignee | ||
Comment 11•19 years ago
|
||
Oh, but in addition to it giving the wrong answer we're also sticking things in the wrong places, of course (eg into random table pseudo-frame parents). We may just wish to file a separate bug on that... The testcase in bug 325218 shows this very well (or at least the testcase I can write based on it will). Just not sure whether we have existing bugs on that. ;)
Assignee | ||
Comment 12•19 years ago
|
||
With this patch, the testcases in this bug, bug 325218, and bug 325024 don't crash and don't even assert. Best of all, only the first hunk is _really_ needed to fix the bugs and that hunk should be branch-safe, I think.
Attachment #210447 -
Flags: superreview?(roc)
Attachment #210447 -
Flags: review?(mats.palmgren)
Assignee | ||
Comment 13•19 years ago
|
||
I filed bug 325543 on the wrong parent thing.
Comment 14•19 years ago
|
||
(In reply to comment #8) > Given the frame tree in bug 325218 (where we have an {ib} split that's not > inside a block), this doesn't look right. Right, doing it nsFrameManager::RemoveFrame would have worked better... Your patch avoids the whole issue of destroying special siblings though and it seems to be the original intention of the code...
Comment 15•19 years ago
|
||
Comment on attachment 210447 [details] [diff] [review] My proposed fix I agree with your conclusions in comment 10, BTW. r=mats
Attachment #210447 -
Flags: review?(mats.palmgren) → review+
Updated•19 years ago
|
Attachment #209934 -
Attachment is obsolete: true
Updated•19 years ago
|
Assignee: mats.palmgren → bzbarsky
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Summary: Crash [@ nsIFrame::GetParent] with evil testcase position:relative/absolute/display:table-column, etc → [FIX]Crash [@ nsIFrame::GetParent] with evil testcase position:relative/absolute/display:table-column, etc
Target Milestone: --- → mozilla1.9alpha
Attachment #210447 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 16•18 years ago
|
||
Fix checked in on trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Resolution: --- → FIXED
Assignee | ||
Comment 17•18 years ago
|
||
Comment on attachment 210447 [details] [diff] [review] My proposed fix I think this is worth taking on branches.
Attachment #210447 -
Flags: branch-1.8.1?(roc)
Attachment #210447 -
Flags: approval1.8.0.2?
Updated•18 years ago
|
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Attachment #210447 -
Flags: approval-branch-1.8.1?(roc) → approval-branch-1.8.1+
Comment 18•18 years ago
|
||
Comment on attachment 210447 [details] [diff] [review] My proposed fix approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210447 -
Flags: approval1.8.0.2? → approval1.8.0.2+
Assignee | ||
Comment 19•18 years ago
|
||
*** Committing to MOZILLA_1_8_BRANCH... new revision: 1.1110.6.19; previous revision: 1.1110.6.18 *** Committing layout/base/nsCSSFrameConstructor.cpp on MOZILLA_1_8_0_BRANCH... new revision: 1.1110.6.12.2.5; previous revision: 1.1110.6.12.2.4
Keywords: fixed1.8.0.2,
fixed1.8.1
Updated•18 years ago
|
Whiteboard: [nvn-dl]
Updated•18 years ago
|
Whiteboard: [nvn-dl] → [rft-dl]
Comment 20•18 years ago
|
||
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 testcase.
Keywords: fixed1.8.0.2 → verified1.8.0.2
Updated•13 years ago
|
Crash Signature: [@ nsIFrame::GetParent]
You need to log in
before you can comment on or make changes to this bug.
Description
•