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

RESOLVED FIXED in mozilla1.9alpha1



12 years ago
6 years ago


(Reporter: Martijn Wargers (dead), Assigned: bz)


(Blocks: 1 bug, 4 keywords)

crash, fixed1.8.1, testcase, verified1.8.0.2
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [rft-dl], crash signature)


(5 attachments, 1 obsolete attachment)



12 years ago
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

12 years ago
Created attachment 207826 [details]

Comment 2

12 years ago
Created attachment 207828 [details]

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
Created attachment 207868 [details]
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:


I'll incorporate that into the upcoming patch for bug 310638 if you
agree this is the correct fix.

Comment 4

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


12 years ago
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()

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.
Created attachment 209934 [details] [diff] [review]
Patch rev. 1
Attachment #209934 - Flags: superreview?(bzbarsky)
Attachment #209934 - Flags: review?(bzbarsky)

Comment 7

12 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.  :(


12 years ago
Blocks: 325024


12 years ago
Blocks: 325218
No longer depends on: 310638

Comment 8

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

Comment 9

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

Comment 10

12 years ago
Created attachment 210446 [details]
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.

Comment 11

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

Comment 12

12 years ago
Created attachment 210447 [details] [diff] [review]
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.
Attachment #210447 - Flags: superreview?(roc)
Attachment #210447 - Flags: review?(mats.palmgren)

Comment 13

12 years ago
I filed bug 325543 on the wrong parent thing.


12 years ago
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


12 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+

Comment 16

12 years ago
Fix checked in on trunk.
Last Resolved: 12 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Resolution: --- → FIXED

Comment 17

12 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?
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+

Comment 19

12 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.; previous revision: 1.1110.
Keywords: fixed1.8.0.2, fixed1.8.1


12 years ago
Whiteboard: [nvn-dl]


12 years ago
Whiteboard: [nvn-dl] → [rft-dl]

Comment 20

12 years ago
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.
Keywords: fixed1.8.0.2 → verified1.8.0.2
Crash Signature: [@ nsIFrame::GetParent]
You need to log in before you can comment on or make changes to this bug.