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

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: 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,
    aPadding=0x0)
    at c:/mozilla/mozilla/layout/generic/nsHTMLReflowState.cpp:1959
etc...

Comment 3

[Mats Palmgren (inactive)]

Attachment: Frame dump of first testcase

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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

[Mats Palmgren (inactive)]

(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

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 7

[Boris Zbarsky [: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.  :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: My proposed fix

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I filed bug 325543 on the wrong parent thing.

Comment 14

[Mats Palmgren (inactive)]

(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

[Mats Palmgren (inactive)]

Comment on attachment 210447 [details] [diff] [review]
My proposed fix

I agree with your conclusions in comment 10, BTW.
r=mats

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fix checked in on trunk.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210447 [details] [diff] [review]
My proposed fix

I think this is worth taking on branches.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 210447 [details] [diff] [review]
My proposed fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 19

[Boris Zbarsky [:bzbarsky]]

*** 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

Comment 20

[Jay Patel [:jay]]

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.