505482 - Possible for frame construction to occur while frames are on overflow lists

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:
1) Follow the steps in bug 478527
2) Hit Cmd-U (on Mac).

At that point I typically get:

###!!! ASSERTION: aPrevFrame must be the last continuation in its chain!: '!aPrevFrame || (!aPrevFrame->GetNextContinuation() || IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame->GetNextContinuation())) && !IS_TRUE_OVERFLOW_CONTAINER(aPrevFrame)', file /Users/bzbarsky/mozilla/reflow/mozilla/layout/base/nsFrameManager.cpp, line 696

Stack is:

#5  0x18bc0f11 in nsFrameManager::InsertFrames (this=0x118401c, aParentFrame=0x16ccf14, aListName=0x0, aPrevFrame=0x16cced0, aFrameList=0x16cd380) at /Users/bzbarsky/mozilla/reflow/mozilla/layout/base/nsFrameManager.cpp:693
#6  0x18b766b9 in nsCSSFrameConstructor::AppendFrames (this=0x1e91c750, aState=@0xbfffba5c, aParentFrame=0x16ccf14, aFrameList=@0xbfffbadc, aPrevSibling=0x16cced0) at /Users/bzbarsky/mozilla/reflow/mozilla/layout/base/nsCSSFrameConstructor.cpp:5816
#7  0x18b83e10 in nsCSSFrameConstructor::ContentAppended (this=0x1e91c750, aContainer=0x1e935260, aNewIndexInContainer=1) at /Users/bzbarsky/mozilla/reflow/mozilla/layout/base/nsCSSFrameConstructor.cpp:6478

Relevant part of the frame tree is:

        Canvas(html)(-1)@0x169468c [view=0x1e935bf0] {0,0,72660,53220} [state=00003000] [content=0x1e916ed0] [sc=0x16c714c] pst=:-moz-scrolled-canvas<
          Block(html)(-1)@0x16c7534 {0,0,60,3264} [state=00d01010] [overflow=0,0,960,3264] sc=0x16c72bc(i=0,b=1)<
            line 0x16cd3bc: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4009] bm=480 {480,480,0,2304} ca={480,480,480,2304} <
              Block(body)(1)@0x16c775c {480,480,0,2304} [state=00101000] [overflow=0,0,480,2304] sc=0x16c7450(i=0,b=1)<
                line 0x16ccf78: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4009] {0,0,0,2304} ca={0,0,480,2304} <
                  Block(div)(1)@0x16ccc90 {0,0,0,2304} [state=00100400] [overflow=0,0,480,2304] sc=0x16cce00(i=3,b=0)<
                    line 0x16c77b8: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4101] {0,0,480,1152} <
                      Text(0)@0x16cce50[0,1,T]  next=0x16ccf14 {0,96,480,960} [state=d0600000] [content=0x1e935200] sc=0x16ccd60 pst=:-moz-non-element<
                        "1"
                      >
                    >
                    line 0x16cd1fc: count=1 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x4121] {0,1152,480,1152} <
                      Inline(span)(1)@0x16ccf14 next=0x16cce94 next-continuation=0x16cce94 {0,1248,480,960} [state=00000400] [content=0x1e935260] [sc=0x16c6bf8]<
                        Text(0)@0x16cced0[0,1,F]  next-continuation=0x16cd28c {0,0,480,960} [state=d1600000] [content=0x1e936030] sc=0x16c770c pst=:-moz-non-element<
                          "2"
                        >
                      >
                      Overflow-list<
                        Text(0)@0x16cd28c[1,2,T]  prev-continuation=0x16cced0 {0,0,0,0} [state=00000406] [content=0x1e936030] sc=0x16c770c pst=:-moz-non-element<
                          "34"
                        >
                      >
                    >
                    line 0x16cd310: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4001] {0,2304,0,0} <
                      Inline(span)(1)@0x16cce94 prev-continuation=0x16ccf14 {0,2304,0,0} [state=00000406] [content=0x1e935260] [sc=0x16c6bf8]<>
                    >
                  >
                >
              >
            >

So the problem is that we have a reflow that we interrupt after reflowing "234" textframe and inline containing it, at which point the inline has the continuing text frame on its overflow list.  Then we go back out to the event loop and end up in frame construction, which tries to find the right place in the frame chain to put the new frame and fails.

This is a regression from bug 487449, at the very least.  But I think even before that point we screwed this up... (by calling nsFrameManager::AppendFrames, which does not assert).

This might be the cause of the crashes in bug 478527.

Now the thing is... it's pretty easy to get the correct prev sibling; just have to get the last continuation of the frame.  But the frame list name is going to be all wrong if this prev sibling is on the overflow list.

I'm not quite sure what to do about this near-term short of making sure that ireflow pulls everything off of all overflow lists or something.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Adding more of our overflow experts.  ;)

Note that in the frame constructor I can also pretty easily tell, at least for this append case, whether the right list should be the overflow list, so we could hack containerframe to deal with this exact situation, perhaps.  But for insert, not append, cases, the situation is much more complicated, unfortunately, because even the primary frame for the prevsibling might be on the overflow list.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should we just flush layout before doing frame construction if there's more incremental reflow pending?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  You mean if the last reflow was interrupted?  That might be the simplest solution; should be rare enough to not affect performance...

Comment 4

[Boris Zbarsky [:bzbarsky]]

What worries me is that I think we'd have to do that any time we process restyles (since the reflow can run script, in general, and we don't want to do that _during_ restyle processing if we suddenly detect a reframe hint).

And if we do that, we might effectively never really have interruptible reflows... or rather immediately follow them up with non-interruptible ones.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

I'm not sure how happy I am with this, but it's worth a shot....  It does fix the bug.  I disabled interruption in columns because that way I don't have to deal with reparenting floats in the inline frame code.

That said, we could perhaps do this better if instead of doing this in ReflowDirtyLines we did it in ReflowLine or even ReflowInlineFrames and used aState.mPrevChild and checked for its next-in-flow.  That would guarantee that we're doing the right thing better than what I do now, I think, and we _might_ know what the relevant blocks are at that point.  Would it make sense to move the code there (no matter what we do with columns)?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you'll need to recursively invoke PullOverflowsFromPrevInFlow on the first frame you pull over.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  Is the loop over first children in blockframe not enough for that?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I missed that. Sounds good

Comment 9

[Boris Zbarsky [:bzbarsky]]

Just to bring this up to date, that loop is wrong; need to get the first child before doing the pull.

Comment 10

[Boris Zbarsky [:bzbarsky]]

My suggestion in comment 5 last paragraph doesn't work, because at that point we don't know yet whether we plan to interrupt after the line we're reflowing at that point.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, and comment 9 is on crack now that I reread the nsInlineFrame code.  Basic code flow is that nsInlineFrame reflows its kids one by one.  If a kid comes back incomplete, it creates a next-in-flow for the kid, then pushes the next-in-flow as the first overflow frame.  If no kid comes back incomplete before it runs out of space, there's no problem, of course.

So if a kid _did_ come back as incomplete that means the kid is what (potentially) has overflows, so the child frame that needs to pull overflows is the first one on the overflow list.

So the patch as posted is correct, and the review request is for real now...  I wish I could figure out a way to not duplicate the code in inlineframe and lineframe, and I might think of a way to undo the duplication while getting this right for columnsets, but for now let's do this.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/54adfdd9e5a7

Comment 13

[Daniel Veditz [:dveditz]]

This is due to ireflow and therefore not applicable to the older branches, right?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Comment 15

[Boris Zbarsky [:bzbarsky]]

This landed before the 1.9.2 branchpoint.