538267 - "ASSERTION: Creating a circular frame list, this is very bad" with -moz-column, floating first-letter & crash [@ nsFrameList::RemoveFrame(nsIFrame*) ]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: Out of flow frame doesn't have the expected parent: 'outOfFlowFrame->GetParent() == this', file layout/generic/nsBlockFrame.cpp, line 6682

###!!! ASSERTION: Broken frame linkage: 'prevSibling && prevSibling->GetNextSibling() == aFrame', file layout/generic/nsFrameList.cpp, line 132

###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling', file nsIFrame.h, line 901

Crash [@ nsFrameList::RemoveFrame]

This testcase is similar to the one in bug 417902.

Comment 1

[Martijn Wargers (dead)]

Worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 6.1; nl; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6

Comment 2

[Daniel Holbert [:dholbert]]

Crashes for me, using:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100105 Minefield/3.7a1pre
(and I get the same 3 assertions as Jesse when I try a linux debug build)

But, confirmed WFM using 3.5.6:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6

So, regression since then 3.5.6.

Comment 3

[Daniel Holbert [:dholbert]]

WFM in 3.6, too:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2pre) Gecko/20100106 Namoroka/3.6pre

Here's a crash report from my mozilla-central nightly:
http://crash-stats.mozilla.com/report/index/62e6f024-74b0-4fa3-a9ab-abc402100106

Comment 4

[Boris Zbarsky [:bzbarsky]]

That first assert comes when outOfFlowFrame is an nsFirstLetterFrame (naturally) and its parent is our prev-in-flow.  This is happening with this stack:

#3  0x00007ffff609c775 in nsBlockFrame::CollectFloats (this=0x7fffe17a9878, aFrame=0x7fffe17a9168, aList=..., aFromOverflow=0, aCollectSiblings=1)
    at ../../../mozilla/layout/generic/nsBlockFrame.cpp:6681
#4  0x00007ffff609c80e in nsBlockFrame::CollectFloats (this=0x7fffe17a9878, aFrame=0x7fffe17cddd8, aList=..., aFromOverflow=0, aCollectSiblings=1)
    at ../../../mozilla/layout/generic/nsBlockFrame.cpp:6693
#5  0x00007ffff608b7be in nsBlockFrame::ReparentFloats (this=0x7fffe17cd640, aFirstFrame=0x7fffe17cddd8, aOldParent=0x7fffe17a9878, aFromOverflow=0, 
    aReparentSiblings=1) at ../../../mozilla/layout/generic/nsBlockFrame.cpp:1677

where that aOldParent passed to ReparentFloats is the next-in-flow and the |this| in frame 5 is the thing that actually has the placeholder and the first-letter frame on its child lists (at least at the point when that assert actually fires).

Then we call RemoveFrame on the mFloats of the next-in-flow block with the letter frame which gives us that second assert.  Then we hit that "circular frame list" assert due to calling SetNextSibling(nsnull) on a null frame pointer (which asserts this != aNextSibling).

Then we crash due to trying to read mNextSibling on said null frame pointer.

So fundamentally, the issue is calling RemoveFrame on a frame list with a frame that is not in that list and also has no previous sibling...

Comment 5

[Boris Zbarsky [:bzbarsky]]

And the key is that after the RecoverLetterFrames call for the frame removal we end up with this frametree (just the part under the columnset):

  ColumnSet(div)(1)@0x7fffddb06fa8 {0,0,0,1140} [state=00001020] [content=0x7fffe0c9ebf0] [overflow=0,-360,2161,1500] [sc=0x7fffddb06bc0]<
    Block(div)(1)@0x7fffddb06e60 next=0x7fffe57a9058 next-in-flow=0x7fffe57a9058 [content=0x7fffe0c9ebf0] {0,0,1,1140} [state=20101000] [overflow=0,-360,600,1500] sc=0x7fffddb170b0(i=0,b=0) pst=:-moz-column-content<
      Float-list<
        Letter(span)(0)@0x7fffddb17868 {0,0,0,0} [state=00000502] [content=0x7fffe59c22e0] [sc=0x7fffddceba90] pst=:first-letter<
          Text(0)@0x7fffddb17598[0,1,T]  {0,0,0,0} [state=00000402] [content=0x7fffe9fe2c80] sc=0x7fffddb172b8 pst=:-moz-non-element<
            "c"
          >
        >
      >
    >
    Block(div)(1)@0x7fffe57a9058 prev-in-flow=0x7fffddb06e60 [content=0x7fffe0c9ebf0] {961,0,1,1140} [state=20101004] [overflow=0,0,1200,1140] sc=0x7fffddb170b0(i=1,b=0) pst=:-moz-column-content<
      line 0x7fffe57a9020: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8141] {0,0,1200,1140} <
        Inline(span)(0)@0x7fffddb17600 {660,0,540,1140} [state=00001000] [content=0x7fffe59c22e0] [sc=0x7fffddb17468]<
          Placeholder(span)(0)@0x7fffddb17948 {0,0,0,0} [state=00100402] [content=0x7fffe59c22e0] outOfFlowFrame=Letter(span)(0)@0x7fffddb17868
        >
      >
    >
  >

In particular, the float is parented to the wrong block.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'll bet this is a regression from bug 499841.  :(

Timothy, can you take a look?

Comment 7

[Timothy Nikkel (:tnikkel)]

I will look into it.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch

We need to pass down the block continuation as a separate parameter in addition to the parent frame as when the first-letter text is inside a span the parent frame is the span and we don't have a reference to the continuation that should be the parent anymore.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why the change from using GetGeometricParent?  What does GetGeometricParent return in this case?

Note that the return value of GetGeometricParent is what will get the float list in the end, so if it's wrong then _that_ is what we need to address, no?  It looks like you do address that, which makes me wonder why you need to then pass around aBlockContinuation to CreateFloatingLetterFrame; it seems like you could just grab it from the frame constructor state.

I think the arguments for WrapFramesInFirstLetterFrame could use some documenting; I'm starting to get lost in them.

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to comment #9)
> Hmm.  Why the change from using GetGeometricParent?  What does
> GetGeometricParent return in this case?
> 
> Note that the return value of GetGeometricParent is what will get the float
> list in the end, so if it's wrong then _that_ is what we need to address, no? 
> It looks like you do address that, which makes me wonder why you need to then
> pass around aBlockContinuation to CreateFloatingLetterFrame; it seems like you
> could just grab it from the frame constructor state.

GetGeometricParent will just return whatever float containing block we passed to the constructor of the frame constructor state we create in CreateLetterFrame just before calling CreateFloatingLetterFrame. So you are correct that I don't need to change the GetGeometricParent part, and that I don't need to pass aBlockContinuation to CreateFloatingLetterFrame. It is the float containing block of the frame constructor state that needs to be changed.

I'll make these changes.
 
> I think the arguments for WrapFramesInFirstLetterFrame could use some
> documenting; I'm starting to get lost in them.

Good point, I will add some documentation.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Made the changes mentioned in comment 10 and also changed the assertion in CreateLetterFrame.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 420846 [details] [diff] [review]
patch v2

Excellent.  Thanks!

Comment 13

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/853fc7683a46

The bug that caused this (bug 499841) only landed on trunk, so we don't need this anywhere else.

Comment 14

[Timothy Nikkel (:tnikkel)]

Added crashtest
http://hg.mozilla.org/mozilla-central/rev/ad14722d17f0