46 bytes, text/x-phabricator-request
|Details | Review|
Bug 1501046 - Set a displayport on the root document's root scrollframe if we paint it before ChromeProcessController sets the displayport. r?botond,tnikkel
46 bytes, text/x-phabricator-request
|Details | Review|
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  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.  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  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  wouldn't have fallen into this trap, and I suspect that what :tnikkel said in bug 1161040 comment 6 is not actually true).  https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/layout/base/nsLayoutUtils.cpp#3712  https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1161040&attachment=8600926
Here's the easy patch that Matt doesn't like: https://github.com/staktrace/gecko-dev/commit/5d986154662b64ab4f882ddc5912d2ace5312cda
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.
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.
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
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/798b3777f30f Skip reporting checkerboarding to telemetry if the sanity test is running. r=botond
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:firstname.lastname@example.org) from comment #2) > (This also means the original patch I had > proposed at  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:email@example.com) from comment #1) > After some investigation and guidance by mattwoodrow, it looks like what > happens is that the resize at  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  which is triggered from . > 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  which then fails because WantAsyncScroll returns false .  https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/base/nsLayoutUtils.cpp#9492,9511  https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/painting/nsDisplayList.cpp#2666  https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/base/nsLayoutUtils.cpp#3374  https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/layout/base/nsLayoutUtils.cpp#3314
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.
Pushed by firstname.lastname@example.org: 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
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.