847208 - "ASSERTION: not in child list" and crash with floating first-letter

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: not in child list: 'found', file layout/base/nsLayoutUtils.cpp, line 665

Assertion failure: GetOverflowOutOfFlows() && GetOverflowOutOfFlows()->ContainsFrame(aFloat) (aFloat is not our child or on an unexpected frame list), at layout/generic/nsBlockFrame.cpp:4964

Or, in opt, a crash [@ nsFrameList::RemoveFrame].
bp-a490c470-0820-4a51-b31f-b9fce2130303

Comment 1

[Jesse Ruderman]

Attachment: assertion stacks

Comment 2

[Jesse Ruderman]

Bug 847209 has a similar testcase.

Comment 3

[Mats Palmgren (inactive)]

This is an old bug, though the assertion is new.  On FF19 the assertion is:
###!!! ASSERTION: Destroying float without removing from a child list.: 'Error', file layout/generic/nsBlockFrame.cpp, line 4910

Comment 4

[Mats Palmgren (inactive)]

Attachment: frame dump

The float to remove is on PushedFloatsList, but nsBlockFrame::RemoveFloat
only considers mFloats and OverflowOutOfFlowList.

Should be an easy fix...

Comment 5

[Scoobidiver (away)]

On Windows: bp-58af27ef-17a6-4dc7-8815-d22452130304.

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix + test

Make RemoveFloat() check the PushedFloatsList too.

I'm splitting out the float cache update part of it into a separate
RemoveFloatFromFloatCache method...

Comment 7

[Mats Palmgren (inactive)]

Attachment: reuse RemoveFloat()

... so that we can reuse RemoveFloat() in StealFrame() and DoCollectFloats()
which currently doesn't update the float cache (maybe they should?).

Comment 8

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=f831066ae085

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 720666 [details] [diff] [review]
fix + test

I'm going to pass to someone who might know answers to the floatcache questions...

Comment 10

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

(In reply to Mats Palmgren [:mats] from comment #7)
> ... so that we can reuse RemoveFloat() in StealFrame() and DoCollectFloats()
> which currently doesn't update the float cache (maybe they should?).

 - the float cache gets cleared anyway whenever we reflow a line
   (DoReflowInlineFrames)
 - we clear the float cache whenever we push a line
   (nsBlockFrame::PushLines), so lines on the overflow lines list
   never have a float cache

 - a float in the overflow out-of-flows has a placeholder in the
   overflow lines, which don't have float caches (see above)
 - a float in the pushed floats list isn't associated with a line;
   pushed floats are managed directly by the block, not through lines.

 - only first continuations of floats end up in a float cache, the rest
   are pushed floats (see above)
 - the only places that call StealFrame on a frame that's not a
   next-continuation (which, in hindsight, might not have been the best
   idea) are:
   - nsBlockReflowState::AddFloat, which isn't a problem because it's
     stealing a pushed float, and pushed floats aren't in a float cache
   - nsBlockReflowState::PushFloatPastBreak, which is more interesting,
     but when we call it, FlowAndPlaceFloat returns false, which causes
     the floats to not be in the float cache (or be removed if they
     already were)

 - CollectFloats is used when we want to move the floats associated with
   a line, so we're already moving the line.  So I think it's safe.  On
   top of that:
   - when it's called from PushLines, we're going to clear the float
     cache anyway (see above)
   - when it's called from ReparentFloats, we're about to reflow the
     line

So I don't think any of these callers *need* to update the float cache.
That said, it still might not be a bad idea.

Comment 11

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

Comment on attachment 720666 [details] [diff] [review]
fix + test

 - splitting RemoveFloatFromFloatCache should have been a separate
   patch; this would have been substantially easier to review that way
   (and I might well have finished the review when I started looking at
   it last week).  (It's also more related to patch 2 than to this patch.)

 - The XXXmats comment in RemoveFloat is good.  If for some reason you
   land the first patch without the second, you should augment the
   XXXmats comment in nsBlockFrame::StealFrame with the same info.

r=dbaron with that, though I still need to figure out whether I think we want patch 2.  If we don't, you should remove the splitting from this patch.

Comment 12

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

Comment on attachment 720668 [details] [diff] [review]
reuse RemoveFloat()

Seems safer to have StealFrame check the overflow out-of-flows list, plus simpler code, so r=dbaron

Comment 13

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/253df4cdfdb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/82bfa9035c6a

Comment 14

[Ed Morley [:emorley]]

Push backed out for:
08:11:45     INFO -  Assertion failure: list && list->ContainsFrame(aChild) (aChild is not our child or on a frame list not supported by StealFrame), at ../../../layout/generic/nsContainerFrame.cpp:1249
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=253df4cdfdb9

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bbc1547d32
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b7db07693ac5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59611a3afcb3

Comment 15

[Mats Palmgren (inactive)]

Nothing wrong with the patches here, I just forgot that the crashtest also depended
on the fix for bug 847209.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6355247c934
https://hg.mozilla.org/integration/mozilla-inbound/rev/3761f1b7df28

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3761f1b7df28
https://hg.mozilla.org/mozilla-central/rev/a6355247c934