Assertion failure: !childNIF || mFrames.ContainsFrame(childNIF) || (pifEOC && pifEOC->ContainsFrame(childNIF)) || (oc && oc->ContainsFrame(childNIF)) || (eoc && eoc->ContainsFrame(childNIF)) in [@ nsGridContainerFrame::SanityCheckGridItemsBeforeReflow]
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: tsmith, Assigned: TYLin)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(5 files, 1 obsolete file)
|
||
Comment 1•7 years ago
|
||
|
|
||
Updated•7 years ago
|
|
||
Comment 2•7 years ago
|
||
|
Assignee | |
Comment 3•4 years ago
•
|
||
If we dump "ColumnSet" log via MOZ_LOG=ColumnSet:5 ./mach run, the assertion
happens in the fourth grid container in the second iteration of column balancing.
GridContainer(summary):frag3
content:frag1
Block
OverflowList<
video
>
OverflowContainerList >
content:frag2
<empty>
>
GridContainer(summary):frag4 <-- Assertion happens in SanityCheckChildListsBeforeReflow at the beginning of reflow.
<empty>
The Content doesn't fit since it has overflow list. However the Content has a
next-in-flow in grid frag3's OC list (created in the first iteration of column
balancing). This looks wrong. If it is incomplete, it's next-in-flow should be
in OverflowList; if it is overflow-incomplete, its next-in-flow should be in
ExcessOverflowContainerList. In both cases, it's next-in-flow is expected to be
drained by frag4.
So let's rewind back to take a look at the frame tree at the beginning of
the second column balancing iteration.
===========================================
Frame tree before we call NormalizeChildLists() on frag1. This is the result at
the end of the first iteration of the column balancing.
GridContainer(summary):frag1 <-- At here, before calling NormalizeChildLists()
Block
GridContainer(summary):frag2
Link
GridContainer(summary):frag3
OverflowContainerList<
Content:frag1
Block
>
GridContainer(summary):frag4
OverflowContainerList<
Content:frag2
Video
>
===========================================
After we call NormalizeChildLists() on grid's frag 1, all the children's
first-in-flows are pulled up to frag1's principal child list, and the Content's
frag2 is put in grid frag2's ExcessOverflowContainerList because it is a
overflow container.
GridContainer(summary):frag1 <-- At here, after calling NormalizeChildLists()
Block
Link
Content:frag1
Block
GridContainer(summary):frag2
ExcessOverflowContainerList<
Content:frag2
Video
>
GridContainer(summary):frag3
<empty>
GridContainer(summary):frag4
<empty>
===========================================
After reflowing frag1, Link and Content cannot fit, and is on frag1's overflow
list. So, after calling NormalizeChildLists() for frag2, they're pulled from
frag1.
GridContainer(summary):frag1
Block
GridContainer(summary):frag2 <-- At here, after calling NormalizeChildLists()
Link
Content:frag1
Block
ExcessOverflowContainerList<
Content:frag2
Video
>
GridContainer(summary):frag3
<empty>
GridContainer(summary):frag4
<empty>
Note: Content:frag2 is still in ExcessOverflowContainerList after
DrainExcessOverflowContainersList() because we don't drain the child in self
excess overflow containers list if it's prev-in-flow has the same parent. (In
this case both Content:frag1 and Content:frag2 has the same parent.) So
Content:frag2 won't reflow until frag3 pulls it, which is pretty bad ...
(Edit: See mat's response in comment 5, which makes more sense)
===========================================
After my proposed fix, the frame tree after we call NormalizeChildLists() on
grid's frag 1 should look like this. That is, we remove the
NS_FRAME_IS_OVERFLOW_CONTAINER bit for Content:frag2, and put it in grid frag2's
OverflowList. The rationale is that Content:frag2 should start from scratch and
it doesn't necessary a overflow container. If it needs to be a overflow
container after reflow, PushIncompleteChildren() will add the bit, and move it
to a proper child list.
GridContainer(summary):frag1 <-- At here, after calling NormalizeChildLists()
Block
Link
Content:frag1
GridContainer(summary):frag2
OverflowList< <-- After the fix.
Content:frag2
>
GridContainer(summary):frag3
<empty>
GridContainer(summary):frag4
<empty>
|
|
Assignee | |
Comment 4•4 years ago
|
||
The rationale is that the pulled up child's first
next-in-flow (childNIF) should start from scratch and it doesn't
necessary be a overflow container. If it needs to be a overflow
container after reflow the child, PushIncompleteChildren() will add the
bit to it, and move it to a proper child list.
|
||
Updated•4 years ago
|
|
||
Comment 5•4 years ago
|
||
(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #3)
GridContainer(summary):frag1 Block GridContainer(summary):frag2 <-- At here, after calling NormalizeChildLists() Link Content:frag1 Block ExcessOverflowContainerList< Content:frag2 Video > GridContainer(summary):frag3 <empty> GridContainer(summary):frag4 <empty>Note: Content:frag2 is still in ExcessOverflowContainerList after
DrainExcessOverflowContainersList() because we don't drain the child in self
excess overflow containers list if it's prev-in-flow has the same parent. (In
this case both Content:frag1 and Content:frag2 has the same parent.) So
Content:frag2 won't reflow until frag3 pulls it, which is pretty bad ...
Hmm, I disagree with that description. The frame tree above looks entirely valid to me and it's not bad in any way.
Quite the opposite in fact -- reflowing two fragments for the same child in the same container would be bad, that should never happen. So the "Content:frag2 won't reflow until frag3 pulls it" is normal and intentional.
There are two outcomes of reflowing the child frame Content:frag1 above: either it pulls up all children from its next-in-flow because they all fit, i.e. the reflow is COMPLETE, which results in Content:frag2 becoming empty and thus destroyed by DeleteNextInFlowChild (which in turn will make the EOC list empty and also destroyed); or, some child didn't fit in some way, in which case that child is pushed to some overflow list (which one depending on its reflow status).
|
|
||
Comment 6•4 years ago
•
|
||
I think the root of the problem is this. We have the frame tree at the top when NormalizeChildLists is called, and the frame tree at the bottom after that call. That later results in the assertion b/c 0x7feff987fde8 (green) being the childNIF isn't in one of the expected child lists; rather it's still in the EOC of the container's prev-in-flow. (It's worth noting that this assertion is fairly harmless - when this container reflows its children the reflow of 0x7feff987f520 (blue) should take care of it (as described above).)
|
|
||
Comment 7•4 years ago
|
||
So we actually have some code that were intended to handle this case here:
https://searchfox.org/mozilla-central/rev/6dc530332ceb86c9d196c1afe8ba5c90c26be898/layout/generic/nsContainerFrame.cpp#1801-1822
It only checks the container's own OverflowContainers list though, not its prev-in-flow's ExcessOverflowContainers list. (maybe I assumed DrainExcessOverflowContainersList had already been called at this point?)
So if I make it check both (like in this rather hacky patch) then the assertion disappears... (alternatively, just calling DrainExcessOverflowContainersList upfront in NormalizeChildLists should work too).
I'm not saying this is a better fix or anything, but I would encourage you to take another look.
(To be clear, I haven't really looked at what your suggested fix is doing.)
|
|
Assignee | |
Comment 8•4 years ago
|
||
Re comment 5:
reflowing two fragments for the same child in the same container would be bad, that should never happen.
Yeah, I see this is bad after look again at this problem. I've edited my comment 3 to strike the paragraphs of my wrongly proposed fix.
Re comment 7:
So we actually have some code that were intended to handle this case here:
https://searchfox.org/mozilla-central/rev/6dc530332ceb86c9d196c1afe8ba5c90c26be898/layout/generic/nsContainerFrame.cpp#1801-1822
It only checks the container's own OverflowContainers list though, not its prev-in-flow's ExcessOverflowContainers list. (maybe I assumedDrainExcessOverflowContainersListhad already been called at this point?)
Nope, we didn't called DrainExcessOverflowContainersList at this point, but I think we should.
|
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 9•4 years ago
|
||
|
||
Comment 10•4 years ago
|
||
|
||
Comment 11•4 years ago
|
||
| bugherder | ||
|
|
||
Updated•4 years ago
|
Description
•