Closed Bug 322678 Opened 19 years ago Closed 19 years ago

[FIX]Crash [@ nsIFrame::GetParent] with evil testcase position:relative/absolute/display:table-column, etc


(Core :: Layout, defect, P1)






(Reporter: martijn.martijn, Assigned: bzbarsky)



(4 keywords, Whiteboard: [rft-dl])

Crash Data


(5 files, 1 obsolete file)

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.
Attached file testcase
Attached file backtrace
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,
    at c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp:1959
Assignee: nobody → mats.palmgren
OS: Windows XP → All
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:


I'll incorporate that into the upcoming patch for bug 310638 if you
agree this is the correct fix.
> 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.
Depends on: 310638
(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(),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.
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Attachment #209934 - Flags: superreview?(bzbarsky)
Attachment #209934 - Flags: review?(bzbarsky)
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.  :(
Blocks: 325024
Blocks: 325218
No longer depends on: 310638
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-
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.
Attached file Frame dump
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


(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.
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.  ;)
Attached patch My proposed fixSplinter Review
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)
I filed bug 325543 on the wrong parent thing.
Blocks: 310638
(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 on attachment 210447 [details] [diff] [review]
My proposed fix

I agree with your conclusions in comment 10, BTW.
Attachment #210447 - Flags: review?(mats.palmgren) → review+
Attachment #209934 - Attachment is obsolete: true
Assignee: mats.palmgren → bzbarsky
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+
Fix checked in on trunk.
Closed: 19 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Resolution: --- → FIXED
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?
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 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+
*** 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.; previous revision: 1.1110.
Whiteboard: [nvn-dl]
Whiteboard: [nvn-dl] → [rft-dl]
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20060301 Firefox/, no crash with testcase.
Crash Signature: [@ nsIFrame::GetParent]
You need to log in before you can comment on or make changes to this bug.