1501046 - Checkerboarding severity graph has strange peaks due to sanity test window

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

On Windows first startup, we create a gfx sanity test window that's 160x234 in size and use that to check the user's hardware for various things. However the APZ code treats this window as checkerboarding because it has no displayport.

With WebRender enabled, the sanity test window sticks around for longer, and so the checkerboard that's reported is larger/more severe. But really we don't care so much about this checkerboard because it's on the sanity test window and not really user-visible. So we should find a way to stop reporting this.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

After some investigation and guidance by mattwoodrow, it looks like what happens is that the resize at [1] triggers a sync paint. During this paint, the root metrics have a scrollId assigned but no displayport, and so APZ counts it as checkerboarding. On the next paint the displayport and scrollbars and nsDisplayRemote all appear and all is well again. So it seems to be a fairly unique situation where the empty window is painted before anything useful is put into it.

[1] https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/toolkit/components/gfx/SanityTest.js#355

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I tried to detect this scenario and apply a displayport on the root content at [1] but it turns out we don't even go down that path because the enclosing `else if` condition is false, as the presShell has a root scrollframe. This seems odd to me, since I don't see why the root XUL window should have a root scrollframe. (This also means the original patch I had proposed at [2] wouldn't have fallen into this trap, and I suspect that what :tnikkel said in bug 1161040 comment 6 is not actually true).

[1] https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/layout/base/nsLayoutUtils.cpp#3712
[2] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1161040&attachment=8600926

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Here's the easy patch that Matt doesn't like: https://github.com/staktrace/gecko-dev/commit/5d986154662b64ab4f882ddc5912d2ace5312cda

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I'm having a lot of trouble figuring out a non-hacky solution. I might land this patch in the meantime so we get cleaner data sooner, and continue working on a proper fix.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Yeah my latest attempt also didn't work right (in that it had unexpected side-effects). Let's get something landed to mitigate this while I continue investigation.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1501046 - Skip reporting checkerboarding to telemetry if the sanity test is running. r?botond

This is not really the best fix (a better one would be to ensure that the
displayport is set on the root element of the gfx sanity window), but should
do as a stopgap solution.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Here's another fix that's slightly less hacky but that seems to do what I want: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c2c9c4ceeb7f566e9efe36d9427e27f3042a3aa3

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/798b3777f30f
Skip reporting checkerboarding to telemetry if the sanity test is running. r=botond

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/798b3777f30f

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1501046 - Set a displayport on the root document's root scrollframe if we paint it before ChromeProcessController sets the displayport. r?botond,tnikkel

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> (This also means the original patch I had
> proposed at [2] wouldn't have fallen into this trap, and I suspect that what
> :tnikkel said in bug 1161040 comment 6 is not actually true).

Ah, oops, looks like I fell into a simple logic error. The if condition is

  if (ignoreViewportScroll && IsRootContentDocument)

which means we take the else branch when ignoreViewportScroll is true and IsRootContentDocument is false.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> After some investigation and guidance by mattwoodrow, it looks like what
> happens is that the resize at [1] triggers a sync paint. During this paint,
> the root metrics have a scrollId assigned but no displayport, and so APZ
> counts it as checkerboarding.

Who assigns the scrollid?

If the patch in https://phabricator.services.mozilla.com/D9829 works then why doesn't the call to MaybeCreateDisplayPortInFirstScrollFrameEncountered just a few lines up create a display port on the same element? AFAICK it should.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tnikkel) from comment #12)
> Who assigns the scrollid?

It seems to be coming from the ensureMetricsForRootId codepath [1] which is triggered from [2].

> If the patch in https://phabricator.services.mozilla.com/D9829 works then
> why doesn't the call to MaybeCreateDisplayPortInFirstScrollFrameEncountered
> just a few lines up create a display port on the same element? AFAICK it
> should.

It goes into the MaybeCreateDisplayPort call at [3] which then fails because WantAsyncScroll returns false [4].

[1] https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/base/nsLayoutUtils.cpp#9492,9511
[2] https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/painting/nsDisplayList.cpp#2666
[3] https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/base/nsLayoutUtils.cpp#3374
[4] https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/base/nsLayoutUtils.cpp#3314

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I'll land the new patch with r=botond since it should be better than the first one, but I'd still like to hear from Timothy in case there's a better way to do this. I've filed bug 1503237 as a follow-up to explore getting rid of the init code from ChromeProcessController and always doing it in nsLayoutUtils::PaintFrame per Botond's suggestion in phabricator.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/755f66b7a73a
Back out cset 798b3777f30f as we have a better fix now. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/052cd2cda89a
Set a displayport on the root document's root scrollframe if we paint it before ChromeProcessController sets the displayport. r=botond

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/755f66b7a73a
https://hg.mozilla.org/mozilla-central/rev/052cd2cda89a