1748466 - Waiting for vsync to be disabled at the end of tests often waits for 4s for a first contentful paint in about:blank

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Florian Quèze [:florian]]

In bug 1742842 I'm trying to identify tests that leave vsync enabled at the end of the test.

I've noticed that lots of tests seem to wait for more than 3 seconds before vsync is disabled.

toolkit/components/aboutprocesses/tests/browser/browser_aboutprocesses_selection.js is an example of test that reproduces this behavior. The new test being written in bug 1742797 also reproduces.

Here is a profile of browser_aboutprocesses_selection.js https://share.firefox.dev/330TiUu
There is a content process that has "RefreshDriverTick waiting for first contentful paint" markers at 60Hz for a few seconds, and the test finishes when these markers stop.

Reading the code a bit (https://searchfox.org/mozilla-central/rev/996a2cafe472e9934b8cb91db63050f96d8a59cb/layout/base/nsRefreshDriver.cpp#1811-1828), it waits at most for 4s, so that explains the 3+s wait.

Not blocking the work in bug 1742842, I see that the Privileged Content process and the WebExtension process also listen to vsync for a while and generate many "RefreshDriverTick after page load" markers. This also seems wasteful.

Olli, I was told you know about this refresh driver stuff. Any idea about what could be changed to avoid this?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

https://searchfox.org/mozilla-central/rev/996a2cafe472e9934b8cb91db63050f96d8a59cb/layout/base/nsRefreshDriver.cpp#1811-1828 is one part of this, other is possibly
https://searchfox.org/mozilla-central/rev/996a2cafe472e9934b8cb91db63050f96d8a59cb/layout/base/nsRefreshDriver.cpp#1831-1868
It is hard to avoid those. We need to get vsync asap during page load (and page load doesn't mean just until load event fires but also after that).

Though, ShouldKeepTimerRunningWhileWaitingForFirstContentfulPaint() should return false when the page is loaded.
Is that not happening?

In theory Catchup ticks should help
https://searchfox.org/mozilla-central/rev/996a2cafe472e9934b8cb91db63050f96d8a59cb/layout/base/nsRefreshDriver.cpp#1604
but that landed before bug 1708121, so clearly it isn't enough.

Comment 2

[Florian Quèze [:florian]]

(In reply to Olli Pettay [:smaug] from comment #1)

https://searchfox.org/mozilla-central/rev/996a2cafe472e9934b8cb91db63050f96d8a59cb/layout/base/nsRefreshDriver.cpp#1811-1828 is one part of this, other is possibly
https://searchfox.org/mozilla-central/rev/996a2cafe472e9934b8cb91db63050f96d8a59cb/layout/base/nsRefreshDriver.cpp#1831-1868
It is hard to avoid those. We need to get vsync asap during page load (and page load doesn't mean just until load event fires but also after that).

I understand that we need vsync asap for a foreground page loads, but none of the cases I mentioned here involve pages that would be painted at the end of their load.

Though, ShouldKeepTimerRunningWhileWaitingForFirstContentfulPaint() should return false when the page is loaded.
Is that not happening?

I put a printf to know the value of mPresContext->Document()->GetReadyStateEnum() in ShouldKeepTimerRunningWhileWaitingForFirstContentfulPaint, and it is 0 (ie. READYSTATE_UNINITIALIZED). Does this give a hint about how we could fix this?

Comment 3

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] from comment #0)

Not blocking the work in bug 1742842, I see that the Privileged Content process and the WebExtension process also listen to vsync for a while and generate many "RefreshDriverTick after page load" markers. This also seems wasteful.

Bug 1548683 might be related for the Privileged Content process case.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

READYSTATE_UNINITIALIZED hints that we're dealing with the initial about:blank.
Could we add a check there
https://searchfox.org/mozilla-central/rev/618f9970972adc5a21194d39d690ec0865f26024/dom/base/Document.h#1057

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Florian, could you try if IsInitialDocument() returns true. We could skip ShouldKeepTimerRunningWhileWaitingForFirstContentfulPaint() in that case.

Comment 6

[Dzmitry Malyshau [:kvark]]

Florian, it looks like the discussion here is productive. Please ping if you need this prioritized by the graphics team.

Comment 7

[Florian Quèze [:florian]]

Attachment: Bug 1748466 - Make ShouldKeepTimerRunningWhileWaitingForFirstContentfulPaint return early for initial about:blank documents, r=smaug.

Comment 8

[Florian Quèze [:florian]]

(In reply to Olli Pettay [:smaug] from comment #5)

Florian, could you try if IsInitialDocument() returns true.

Yes, it does! After making the change in the patch I sent to phabricator, the time it takes for vsync to be disabled at the end of the browser_aboutprocesses_selection.js test goes down from 3+s to ~500ms (mostly due to bug 1748270).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/faf471bc7099
Make ShouldKeepTimerRunningWhileWaitingForFirstContentfulPaint return early for initial about:blank documents, r=smaug.

Comment 10

[Atila Butkovits]

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