1695509 - many empty pages when printing pages with multiple tall floats inside of flex containers (and layout.display-list.improve-fragmentation enabled, the default in Firefox 86+)

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P2)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

[Tracking Requested - why for this release]: Breaks basic feature (printing) on an unknown set of pages (unless one manually identifies the pages to actually print)

This is a regression from bug 1681052 which set the preference layout.display-list.improve-fragmentation to true. It got set back to false for 85 (bug 1689789).

Reproduces with Firefox 86.0, 87.0b3, 88.0a1 20210227 on Windows 8.1 with the built-in PDF export of Firefox and at least a hardware printer.

Steps to reproduce:

1. Load https://www.tagesschau.de/
2. Call the Print command.

Actual result:
Print preview mentions a high number of pages to print (between ~670 and ~5330, depending on the printer/exporter and maybe the page's changing content).

Expected result:
~25 pages as print output.

Comment 1

[Mats Palmgren (inactive)]

At first glance, it seems like the fragmentation fallback mechanism is working as intended. It looks like some frames are reporting a bogus overflow area, which we then reserve pages for. I'll attach a somewhat reduced testcase...

Comment 2

[Mats Palmgren (inactive)]

Attachment: testcase 1 (1695509.zip)

In Print Preview with Portrait/US Letter/Fit to page mode I get 4 pages. It seems it should only be 3. It looks like it's the flex item that is reporting the wrong overflow areas? (There's only one flex container in this document, the <div class="layout-content">, and it only has one flex item.)
I've added a colored border+outline on the flex item so you can see it.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Frame dump

FTR, here's a dump of the final Print Preview frame tree of the above testcase.

Comment 4

[Mats Palmgren (inactive)]

Daniel, could you take a quick look and see if the overflow areas on the flex item are correct? I don't quite understand where they came from... Perhaps something odd going on with the <picture> descendant?

Comment 5

[Mats Palmgren (inactive)]

Attachment: 1695509-debug.patch

Comment 6

[Mats Palmgren (inactive)]

Attachment: Log output from above patch

So, page 3 finds that page 2 has remaining overflow that it needs to paint.
Page 2 has a flex item continuation with scr-overflow=(x=0, y=0, w=44276, h=138880). That scrollable overflow height is more than two pages. The flex item only contains a float child with (x=120, y=0, w=44036, h=60314) and no scrollable overflow. So I don't see where that large overflow came from.

Perhaps we did a measuring reflow of the item that gave it the larger overflow and then we didn't reset the areas before the final reflow?

Page 2 contains the <picture> element, btw. (cue the Twilight Zone theme...)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #6)

Perhaps we did a measuring reflow of the item that gave it the larger overflow and then we didn't reset the areas before the final reflow?

This sounds like a reasonable theory, yeah. I'll take a closer look later on today...

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced)

(reposting testcase 2 with some minor tweaks for clarity)

Comment 10

[Daniel Holbert [:dholbert]]

So, here's whats approximately happening:
(1) While laying out the flex container on the first page, we do a "measuring reflow" for its flex item, with unconstrained available height.
(2) This ends up populating mRect for its descendant frames, including the second float (the one with the descriptive text at the bottom of testcase 2). That float ends up with a mRect whose y-position is quite large.
(3) We end up pushing that float (because it obviously doesn't fit on page 1)

(4) A bit later, when we're reflowing page 2, we get to that page's continuation for the flex item, and we call nsBlockFrame::ReflowPushedFloats for that frame.
(5) This ends up calling ConsiderChildOverflow(aOverflowAreas, f); on each of the floats that we've pulled in, including the one discussed above; and it's still got a (now-bogus) huge y value, which bloats our aOverflowAreas to be huge.
(6) This ends up with that flex item on page 2 thinking that it's got a huge overflow area, when in fact it does not.

So, bottom-line, we probably need to be resetting the position on that pushed float at some point between the measuring reflow & when it ends up being used in the ConsiderChildOverflow call on the next page, because we don't want its bogus position to influence overflow areas in a later continuation.

Comment 11

[Julien Cristau [:jcristau]]

Do we have an idea for how common this is? Should we turn the pref back off for 87, while we wait for a fix?

Comment 12

[Daniel Holbert [:dholbert]]

I suspect this scenario isn't super common; no need to turn the pref off at this point. We can keep an eye out for incoming bug reports and take action if needed (but I think this feature is still a net win, in terms of reducing user pain).

I'm also hoping to have a patch ready for landing on Nightly early next week, too, and it should hopefully be suitable for uplift to 87.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (with multicol, no printing required)

Here's a testcase demonstrating the same underlying issue, using multicol for fragmentation instead of printing.

EXPECTED RESULTS: No content should overflow off the bottom end of the red box.
ACTUAL RESULTS: Quite a bit of blue-outline extends off the bottom of the red box.

With this testcase, I can reproduce this as far back as Nightly 2020-05-13 which is the first build that had support for fragmenting flex items (via bug 1622935 comment 17).

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: screenshot of overflowing blue dashes in testcase 3, in affected Nightly versions (e.g. Nightly 2021-03-08)

Here's a screenshot of the "actual results" on testcase 3. The overflowing blue dashed area (off the bottom of the red box) is showing the overflow area that we are improperly setting up, when carrying the final float (the one with text) forward from one column to the next.

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Daniel, do you have an update on this bug? Thanks

Comment 16

[Daniel Holbert [:dholbert]]

No updates. Still on my to-do list, still hasn't risen to the top, still hoping to get to it soon.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: Bug 1695509: Don't inflate overflow areas with stale positions of pushed floats that we drained but weren't able to place. r?TYLin

Before this patch, there's an edge case where we may drain a pushed float (with
a stale position), and then discover that it won't fit in the current block (so
we push it and leave its position untouched), but we still inadvertently
include its rect in the current block's overflow areas. This means we're
feeding stale/bogus position into the overflow areas, which can make them
unnecessarily huge.

This patch accounts for this by only considering overflow from floats that we
actually successfully placed, in ReflowPushedFloats.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)

(3) We end up pushing that float (because it obviously doesn't fit on page 1)
(4) A bit later, when we're reflowing page 2, we get to that page's continuation for the flex item, and we call nsBlockFrame::ReflowPushedFloats for that frame.
(5) This ends up calling ConsiderChildOverflow(aOverflowAreas, f); on each of the floats that we've pulled in, including the one discussed above; and it's still got a (now-bogus) huge y value, which bloats our aOverflowAreas to be huge.

When revisiting this today, I realized that the real issue is actually just before (5) here.

In ReflowPushedFloats, we call FlowAndPlaceFloat, ostensibly to establish the position for floats that we might never have seen before (and which might have a stale position). But FlowAndPlaceFloat might not actually give the float a new position, e.g. if it can already tell that we're already out of space and it's going to have to push the float. (In the specific case of this bug's "testcase 3", FlowAndPlaceFloat hits https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/layout/generic/BlockReflowInput.cpp#768 , with floatAvailableSpace.mRect.BSize(wm) equal to 0, which makes that function declare // No space, nowhere to put anything. and return false.

In that scenario where FlowAndPlaceFloat declines to update the float's position, we need to avoid incorporating the float's potentially-stale/bogus rect into our overflow-area. (It's not up-to-date, and we're planning on pushing the float to our next-continuation anyway, for our next-continuation to place and incorporate into its own overflow area.)

So, the attached patch just checks whether FlowAndPlaceFloat successfully placed the float, before unioning the float's rect into our overflow areas.

Comment 19

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

Thanks for comment 18 explaining the details. It makes sense to exclude a pushed float's rect that can have stale position from the overflow area.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f62700d85c2
Don't inflate overflow areas with stale positions of pushed floats that we drained but weren't able to place. r=TYLin

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29319 for changes under testing/web-platform/tests

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: Bug 1695509 followup: adjust terminology in code-comment, for consistency. (no review, just minor change to a code-comment)

DONTBUILD

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afd13a3cb91e
followup: adjust terminology in code-comment, for consistency. (no review, just minor change to a code-comment)

Comment 24

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/1f62700d85c2

Comment 25

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 26

[Alexandru Michis [:malexandru]]

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