Closed Bug 847208 Opened 7 years ago Closed 7 years ago
"ASSERTION: not in child list" and crash with floating first-letter
244 bytes, text/html
12.32 KB, text/plain
8.27 KB, text/html
6.75 KB, patch
|Details | Diff | Splinter Review|
3.38 KB, patch
|Details | Diff | Splinter Review|
###!!! 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
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
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*) ]
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
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.