1781007 - Checkerboarding on gmail "forward" page with maximized sub-window

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Mayank Bansal]

go to gmail
open a relatively longis email
click on "forward"

• A reply windowpane will appear at the bottom of the page*.
  Click to maximize/"pop-out" the window to a pane
  Click to maximize/"pop-out" it again

Scroll
ER: No checkerboard
AR: Checkerboard

RENDERTRACE 0.0159 rect red 0 0 1161 2428.67 // viewport
RENDERTRACE 16.5699 rect red 0 0 1161 2428.67 // viewport
RENDERTRACE 21.2076 rect brown 0 0 1161 2428.67 // page
RENDERTRACE 21.2077 rect lightgreen 0 1627.33 1161 801.333 // painted displayport (this layertree updated)
RENDERTRACE 33.1733 rect red 0 0 1161 2428.67 // viewport
RENDERTRACE 49.8013 rect red 0 0 1161 2428.67 // viewport
-- checkerboarding starts below --
RENDERTRACE 66.5649 rect red 0 0 1161 2428.67 // viewport
RENDERTRACE 71.6067 rect brown 0 0 1161 2428.67 // page
RENDERTRACE 71.6255 rect lightgreen 0 1075.33 1161 801.333 // painted displayport (this layertree updated)
Checkerboarded for 1 frames (17.064 ms), 198531 peak, 1819 severity.

A profile from the profiler didnt show any activity on any thread, and the same was also corroborated by looking at Task Manager. (Not sharing the profile due to privacy concern)

I am happy to provide any logging etc.

Comment 1

[Mayank Bansal]

Attachment: about:support

Comment 2

[Mayank Bansal]

could also repro from a build from Jan22, so not a recent regression

Comment 3

[Mayank Bansal]

Attachment: bandicam 2022-07-25 10-20-52-230.mp4

Comment 4

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=383986e2f5cb835a55c47bbd6e15d81035ca0784&tochange=d6e8528f0a936df88369c71f4e390e31a4d621a1

-> enabling fission caused this.

With fission prefed on bug 1675547 is the regressor.

Interestingly if I turn off fission but set apz.wr.activate_all_scroll_frames to true then the bug still reproduces.

Testing in a 2021-01-01 build with fission disabled, webrender disabled, but apz.nonwr.activate_all_scroll_frames=true then I cannot reproduce the bug.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1675547

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:botond, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 7

[Botond Ballo [:botond]]

S3 for a temporary checkerboarding issue that's not a recent regression.

Comment 8

[Botond Ballo [:botond]]

I haven't been able to reproduce this so far. Mayank, could you kindly check if you can still see this in a recent nightly?

Comment 9

[Timothy Nikkel (:tnikkel)]

I can still reproduce in latest nightly.

Comment 10

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #9)

I can still reproduce in latest nightly.

Thanks. A few follow-up questions:

• Are you testing using PanGestureInput (touchpad scrolling)?
• Are you maximizing the popped-out compose window, or is the issue otherwise dependent on its height?
• Is there perhaps a particular dev-platform email on which you see this, so that I can try the same one?

Comment 11

[Botond Ballo [:botond]]

Update: I do see the checkerboarding on Windows (previously I was testing on Linux), so I guess there is some sort of platform difference relevant to the issue. (I'm kind of curious what the difference is.)

Comment 12

[Botond Ballo [:botond]]

A couple of additional updates:

• Even on Windows, I only see checkerboarding while the pop-out window is loading, not once it's completely loaded.
• On the other hand, about:checkerboard thinks that checkerboarding happens (albeit often at low severity) on both Linux and Windows even in situations when I don't perceive it.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I did debug the case a bit, but I am confused so much. In the poped-out content there are two scroll containers, an outer scroll container and an inner one, and the inner one is overflow-y: hidden. And the checkerboarding happens on the inner overflow-y: hidden one. Gmail scrolls the inner scroll container in response to outer scrolling? Indeed there are two scroll event listeners on the outer one. I'd hope I am seeing something wrong.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

One thing I realized is, in this Gmail's case the poped-out content is resized so the displayport should be changed, but it isn't reflected to the corresponding APZC since we don't update the displayport in NotifyLayersUpdated. Though with updating the displayport in NotifyLayersUpdated I can still see checkerboarding.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: A simplified case

Once after you clicked "change" button in the contents, you can see checkerboarding there.

A key factor to reproduce this issue is making a sub scroll container be non-scrollable by expanding the container size. Even after becoming non-scrollable, the corresponding APZC keeps alive in APZCTree, and I guess it somehow mis-behaves?

EDIT: Looks like you shouldn't scroll the container before clicking the button to reproduce the issue.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(Note I am focusing on checkerboarding persists for a while you can see it on the recording in comment 3 at the last couple of seconds)

So the checkerboarding is caused by this RestrictToRootDisplayPort call. Due to this call, the displayport is smaller than it should be. The displayport it should be I mean is the one where the inner scroll container is originally expanded.

There are a couple of things I've learned.

The reason why no checkerboarding happens if the scroll container is expanded in the first place is;

• the container is not scrollable by APZ, thus there's no nsGkAtoms::DisplayPort and no nsGkAtoms::DisplayPortMargins
• thus we don't use the RestrictedToRootDisplayPort as *aVisibleRect

The reason why no checkerboarding happens before expanding the container is;

• the container is scrollable by APZ thus MinimalDisplayPort property is immediately removed
• thus we fall into this else branch so that the displayport is big enough

The reason why checkerboarding happens after expanding the container is;

• it's a MinimalDisplayPort case
• we fall into this else if branch so that the displayport is small because of no margins

Given that if the container is expanded in the first place we don't apply RestrictedToRootDisplayPort, I'd like to fix this bug by skipping RestrictedToRootDisplayPort if the container is not scrollable by APZ even if there's valid nsGkAtoms::DisplayPort and nsGkAtoms::DisplayPortMargins.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1781007 - Unify two DisplayPortUtils::HasDisplayPort calls in DecideScrollableLayer. r?tnikkel

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1781007 - Skip calling RestrictToRootDisplayPort in the case where the scroll container has empty displayport margins. r?tnikkel

Depends on D189322

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1781007 - A tiny optimization to avoid redandunt calls to tell whether the given content has the displayport or not. r?tnikkel

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Looks like we try to build display items without the primary frame of a scroll container during running test_bug1369072.html . see https://treeherder.mozilla.org/logviewer?job_id=431108875&repo=try&lineNumber=5510

In the test we actually call DecideScrollableLayer even if the target element is in display:none subtree. That's totally odd. There's definitely something messed up because the scroll frame (HTMLSrollFrame) is there!

Comment 21

[Timothy Nikkel (:tnikkel)]

Woah, that seems very weird. Very curious what is going on.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Okay I did track down the case.

So in the case of root scrollable frame, we do create the frame by this SetUpDocElementContainingBlock(aDocElement) call in nsCSSFrameConstructor::ConstructDocElementFrame and then bail out from ConstructDocElementFrame if display style value is none. And later in the ConstructDocElementFrame we do set the primary frame. This is the reason why the nsIContent has no primary frame but have the HTMLScrollFrame.

I am going to add a bailing out code from WillUseEmtpryDisplayPortMarings if the primary frame is nullptr.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/413c83e8ccda
Skip calling RestrictToRootDisplayPort in the case where the scroll container has empty displayport margins. r=tnikkel

Comment 24

[Cristina Horotan [:chorotan]]

Backed out changeset 413c83e8ccda (Bug 1781007) for causing mochitest failures at test_group_checkerboarding.html.

Backout: https://hg.mozilla.org/integration/autoland/rev/a7702515d4fcfa0f818f50d8e7551159c2229bc5

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=413c83e8ccdad48a5ea3e58243d8ec9bb8c317fd

Failure log: https://treeherder.mozilla.org/logviewer?job_id=431241479&repo=autoland&lineNumber=10112

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)

So in the case of root scrollable frame, we do create the frame by this SetUpDocElementContainingBlock(aDocElement) call in nsCSSFrameConstructor::ConstructDocElementFrame and then bail out from ConstructDocElementFrame if display style value is none. And later in the ConstructDocElementFrame we do set the primary frame. This is the reason why the nsIContent has no primary frame but have the HTMLScrollFrame.

Ah, so it's the case of the root scroll frame, where the primary frame is a little tricky in comparison to non-root scroll frames, I was wondering if maybe that was it.

This comment explains the setup

https://searchfox.org/mozilla-central/rev/29bdf6ff9965a647c6f64d63fed2b5bd094532c7/layout/base/nsCSSFrameConstructor.cpp#2649

The primary frame is the "root element frame" which doesn't get constructed in this display none case. And the nsHTMLScrollFrame is an ancestor, and is the same the content element, but it is not the primary frame.

This is what we have the function nsLayoutUtils::FindScrollableFrameFor for

https://searchfox.org/mozilla-central/rev/29bdf6ff9965a647c6f64d63fed2b5bd094532c7/layout/base/nsLayoutUtils.cpp#659

Although in this case it doesn't really matter because we are just getting the presshell via the frame and don't care about the frame.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

So the test I added also catches normal checkerboarding which is not in the scope of this bug. I pushed a comparison test run that the sub scroll in question isn't expanded. https://treeherder.mozilla.org/jobs?repo=try&revision=cd70dfff8e7886890e496d73db87e0d3289e9d03 , the test actually fails.

I will change the test not to use assertNotCheckerboarded.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58b8f7f43ab7
Skip calling RestrictToRootDisplayPort in the case where the scroll container has empty displayport margins. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e5788bc62ef8
A tiny optimization to avoid redandunt calls to tell whether the given content has the displayport or not. r=tnikkel

Comment 28

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/58b8f7f43ab7
https://hg.mozilla.org/mozilla-central/rev/e5788bc62ef8

Comment 29

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-06-19 Nightly build on macOS 12 using the testcase provided in Comment 15.
Verified as fixed on Firefox 120.0b8 and Nightly 121.0a1 on macOS 12, Windows 10, Ubuntu 22.