1374479 - multicol incremental reflow state alternation

Status: REOPENED

(Core :: Layout: Columns, defect, P3)

Description

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

Attachment: testcase

In my work on bug 1308876, a new test failure appeared recently, which was an assertion failure for the crashtest layout/generic/crashtests/1015844.html .  This assertion failure was also associated with a layout difference in the contents of the test.

I minimized the testcase for the layout difference, started to debug it, and realized that the reason it wasn't present without the patch to bug 1308876 was the resize reflow we do once we determine that a scrollbar isn't needed.  If I avoid this reflow using 'html { overflow: hidden }' then I can see the problem on trunk, without the patch.

Steps to reproduce:
 1. load attached testcase
 2. change the zoom level

Actual results:  With each change in zoom, the testcase alternates between having three versus four purple-ish boxes.

Expected results:  The testcase should be consistent in the number of boxes no matter what the zoom level.

Note that it's possible to observe different layouts at the same zoom level by doing:
  Ctrl+0 (go to default zoom level)
  Ctrl++ (increase zoom)
  Ctrl++ (increase zoom)
  Ctrl+0 (go to default zoom level)
since this sequence has two steps at the default zoom level that have an even number of other states between them.

Comment 1

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

Attachment: a diff of two of the reflows that should have been the same

This is a diff of the GECKO_DISPLAY_REFLOW_RULES_FILE output from the third and the eighth reflows in the sequence:
 (1) load
 (2) zoom in (Ctrl++)
 (3) zoom default (Ctrl+0)
 (4) zoom in
 (5) zoom in
 (6) zoom default
 (7) zoom in
 (8) zoom default

Comment 2

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

At least part of the problem here is fundamental to the way our column code works:  mLastBalanceBSize intentionally persists state across reflows.  That massively increases the potential for bugs like this.

But there's still the question of why we get different results.

Comment 3

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

I think a bunch of the problem comes down to the inner columnset returning a height of 0 most but not all of the time... and only occasionally reporting the correct 100px height.

Comment 4

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

Attachment: testcase showing multicols don't honor height when in paginated context

I suspect the underlying problem causing the oscillation is that multicols don't seem to honor their specified height in a paginated context.  The examples here show that; the inner blue multicol should always be 80px tall (sometimes split), not just when it's not inside the outer yellow one.  I think this is mainly due to this code:
https://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/generic/nsColumnSetFrame.cpp#911-914
... though I'm also not sure if nsColumnSetFrame has the necessary code to consume only the remaining height in a continuation of a multicol with a specified height.

Comment 5

[Neerja Pancholi[:neerja]]

I thought it might be significant that with the fix for Bug 1411765 the assert count mentioned in layout/generic/crashtests/crashtest.list associated with this bug goes down from 543 to 11.

Comment 6

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

The tests in comment 0 and comment 4 work as expected after enabling column-span in bug 1426010.

Comment 7

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

I'd like to leave this open unless we've fixed the underlying problems, even if the initial testcase doesn't still trigger it. Or has something changed both of:

• persisting mLastBalanceBSize across reflows
• not honoring specified height in a paginated context

Comment 8

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

The column container's specified height should've been honored [1] in a paginated context now, but the persisting mLastBalanceBSize across reflow can still be an issue.

[1] https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/layout/generic/nsBlockFrame.cpp#3615-3647