Checkerboarding severity graph has strange peaks due to sanity test window

RESOLVED FIXED in Firefox 65

Status

()

P2
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks: 1 bug)

64 Branch
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments)

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.
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
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
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.
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.
Keywords: leave-open
Created attachment 9019733 [details]
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.
Assignee: nobody → kats

Comment 8

4 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/798b3777f30f
Skip reporting checkerboarding to telemetry if the sanity test is running. r=botond
Blocks: 1386669
Priority: -- → P2
Created attachment 9020130 [details]
Bug 1501046 - Set a displayport on the root document's root scrollframe if we paint it before ChromeProcessController sets the displayport. r?botond,tnikkel
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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
See Also: → bug 1503237
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

4 months ago
Pushed by kgupta@mozilla.com:
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
Keywords: leave-open

Comment 16

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/755f66b7a73a
https://hg.mozilla.org/mozilla-central/rev/052cd2cda89a
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.