1343948 - ASSERTION: conflicting overflow containers lists: '!(overflowContainers && GetPrevInFlow() && static_cast<nsContainerFrame*>(GetPrevInFlow()) ->GetPropTableFrames(ExcessOverflowContainersProperty()))'

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Mats Palmgren (inactive)]

Follow-up from bug 1343606.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

This is one of the root cause behind the recent flex item fragmentation regressions like bug 1640028.

The reason is that we unconditionally merge overflow incomplete children to a flex / grid ExcessOverflowContainersProperty list in PushIncompleteChildren [1]. We expect the excess list to be drained by the next-in-flow. However, if the flex /grid container already has a next-in-flow that is an overflow container having an existing OverflowContainersProperty list, the assertion [2] triggers when we drain the excess overflow container list when reflowing the next-in-flow.

There's an invariant described in [3], saying that only one overflow containers list can exist, either a frame's own OverflowContainersProperty or its prev-in-flow's ExcessOverflowContainersProperty, not both. I think we should fix PushIncompleteChildren to respect that.

[1] https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/layout/generic/nsContainerFrame.cpp#1597
[2] https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/layout/generic/nsContainerFrame.cpp#1967-1970
[3] https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/layout/generic/nsContainerFrame.h#317-319

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1343948 - Merge overflow container children to next-in-flow's OverflowContainersProperty() if the property already exists.

This is to prevent the assertion at the beginning of
DrainExcessOverflowContainersList().

The invariant is described in the comment revised in this patch. That
is, "only one overflow containers list exists for a given frame: either
its own OverflowContainersProperty or its prev-in-flow's
ExcessOverflowContainersProperty, not both."

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7925ab748617
Merge overflow container children to next-in-flow's OverflowContainersProperty() if the property already exists. r=mats

Comment 4

[Razvan Maries]

Backed out for perma failures on 1343606.html.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Creftests%2Ctest-android-em-7.0-x86_64%2Fdebug-geckoview-crashtest-e10s%2Cr%28c%29&revision=7925ab74861760cb8e1e8d00a846fa25f3d2418d&selectedTaskRun=FRvOa27LRr6hGPAdlfjfLQ-0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304641793&repo=autoland&lineNumber=9252

Backout: https://hg.mozilla.org/integration/autoland/rev/9c1ad8bba1a069e3ff102c91986d4fab44e8cd76

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

By looking at the logcat-emulator-5554.log (in the "Jobs Details" panel) on Android, my patch only reduce the assertion count by 1. The other three assertions are ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0'.

06-02 00:34:17.791  2814  2829 I Gecko   : REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/base/crashtests/1343606.html | 1844 / 3809 (48%)
06-02 00:34:17.791  2814  2829 I Gecko   : 
06-02 00:34:17.791  2814  2829 I Gecko   : {"action":"log","time":1591054457791,"thread":null,"pid":null,"source":"reftest","level":"DEBUG","message":"START http://10.0.2.2:8854/tests/layout/base/crashtests/1343606.html"}
06-02 00:34:17.870  2845  2862 I Gecko   : [Child 2845, Main Thread] ###!!! ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0', file /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp, line 1947
06-02 00:34:17.886  2814  2829 I Gecko   : 
06-02 00:34:17.886  2814  2829 I Gecko   : {"action":"log","time":1591054457886,"thread":null,"pid":null,"source":"reftest","level":"DEBUG","message":"[CONTENT] OnDocumentLoad triggering AfterOnLoadScripts"}
06-02 00:34:17.887  2845  2862 I Gecko   : [Child 2845, Main Thread] ###!!! ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0', file /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp, line 1947
06-02 00:34:17.894  2845  2862 I Gecko   : [Child 2845, Main Thread] ###!!! ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0', file /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp, line 1947

I'll adjust the expectation for Android and reland my patch.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adac15659f36
Merge overflow container children to next-in-flow's OverflowContainersProperty() if the property already exists. r=mats

Comment 7

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/adac15659f36