Closed Bug 847208 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase)

Crash Data

Attachments

(5 files)

###!!! 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
Attached file assertion stacks
Bug 847209 has a similar testcase.
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
Assignee: nobody → matspal
OS: Mac OS X → All
Hardware: x86_64 → All
Attached file frame dump
The float to remove is on PushedFloatsList, but nsBlockFrame::RemoveFloat
only considers mFloats and OverflowOutOfFlowList.

Should be an easy fix...
On Windows: bp-58af27ef-17a6-4dc7-8815-d22452130304.
Crash Signature: [@ nsFrameList::RemoveFrame(nsIFrame*) ] [@ nsIFrame::SetNextSibling(nsIFrame*) ]
Attached patch fix + testSplinter Review
Make RemoveFloat() check the PushedFloatsList too.

I'm splitting out the float cache update part of it into a separate
RemoveFloatFromFloatCache method...
Attachment #720666 - Flags: review?(bzbarsky)
... so that we can reuse RemoveFloat() in StealFrame() and DoCollectFloats()
which currently doesn't update the float cache (maybe they should?).
Attachment #720668 - Flags: review?(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...
Attachment #720666 - Flags: review?(bzbarsky) → review?(dbaron)
Attachment #720668 - Flags: review?(bzbarsky) → review?(dbaron)
(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 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.
Attachment #720666 - Flags: review?(dbaron) → review+
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
Attachment #720668 - Flags: review?(dbaron) → review+
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
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
https://hg.mozilla.org/mozilla-central/rev/3761f1b7df28
https://hg.mozilla.org/mozilla-central/rev/a6355247c934
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.