1847584 - Hidden remote browser with an animation keeps vsync going indefinitely

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[:Gijs (out for now; he/him)]

I was seeing this locally on macOS, causing test failures in shopping tests. I worked around it there (by ensuring we open the shopping browser in separate tabs that get closed so the browser gets removed completely instead of just hidden), but it seems like a platform issue that could potentially have serious power-draining effects.

I'll attach a phab revision that can be used to reproduce (and hopefully be checked in as a test that could catch this kind of issue regressing, assuming we can fix it straightforwardly).

The other thing I just noticed is that browsingContext.isActive stays true. I don't know if that's expected and/or if something else is meant to communicate the hidden state to the child process and/or gpu.

Comment 1

[:Gijs (out for now; he/him)]

Attachment: Bug 1847584 - this test causes vsync to run indefinitely

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Thanks for the test-case! So, I agree this is kind of a bad default, but it's sort of expected right now. Top level browserchilds are assumed active unless the front-end says the opposite. Some bits of relevant code:

• https://searchfox.org/mozilla-central/rev/fb43eb3bdf5b51000bc7dfe3474cbe56ca2ab63c/dom/ipc/BrowserChild.cpp#2781-2782
• https://searchfox.org/mozilla-central/rev/fb43eb3bdf5b51000bc7dfe3474cbe56ca2ab63c/docshell/base/BrowsingContext.cpp#429
• We also have the HiddenUnderEmbedderElement which doesn't do anything special for throttling.

So I think making them not active by default would probably involve an opt-in for the front-end into the current behavior (because tabs want to be unthrottled even in the background). But maybe just checking the HiddenUnderEmbedderElement to compute the PresShell activeness would be better?

I can check tomorrow.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847584 - Misc BrowserChild clean-ups. r=smaug,nika

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847584 - Account for IsUnderHiddenEmbedderElement() for BrowserChild visibility. r=smaug

Test by Gijs. Note that I removed the .isActive assertion, because this doesn't
affect activeness.

We could make that change too, but that's a bigger change, and I think we want
to do this regardless.

Depends on D185642

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03bf43b4ae07
Misc BrowserChild clean-ups. r=smaug

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847584 - Consistently throttle visibility:hidden frames. r=smaug

Before this patch OOP frames were being throttled but in-process ones
weren't.

Depends on D185643

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

So the main issue is that we don't mark the relevant <browser> inactive. Being active has also other side effects than just letting animations to run.
(but sure, we can tweak the hidden-state handling too)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54b34c76db47
Account for IsUnderHiddenEmbedderElement() for BrowserChild visibility. r=smaug

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdfc3c7388cf
Consistently throttle visibility:hidden frames. r=smaug

Comment 10

[Natalia Csoregi [:nataliaCs]]

Backed out for causing vsync failures.

• backout: https://hg.mozilla.org/integration/autoland/rev/dd89bc6e84a4201b52b881eee44ae8c9666f6d20
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=54b34c76db4740af9c65f043f70519a76975da83
• failure log example: https://treeherder.mozilla.org/logviewer?job_id=425391130&repo=autoland&lineNumber=5874

[task 2023-08-09T12:12:42.212Z] 12:12:42     INFO - TEST-PASS | devtools/client/inspector/animation/test/browser_animation_keyframes-graph_keyframe-marker-rtl.js | The main process DevToolsServer has no pending connection when the test ends - 
[task 2023-08-09T12:12:42.214Z] 12:12:42     INFO - Buffered messages finished
[task 2023-08-09T12:12:42.219Z] 12:12:42     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/animation/test/browser_animation_keyframes-graph_keyframe-marker-rtl.js | waiting for vsync to be disabled - timed out after 50 tries. - false == true - {"filename":"chrome://mochikit/content/browser-test.js","name":"ensureVsyncDisabled","sourceId":532,"lineNumber":603,"columnNumber":19,"sourceLine":"","asyncCause":null,"asyncCaller":null,"caller":null,"formattedStack":"ensureVsyncDisabled@chrome://mochikit/content/browser-test.js:603:19\n","nativeSavedFrame":{}}
[task 2023-08-09T12:12:42.221Z] 12:12:42     INFO - Stack trace:
[task 2023-08-09T12:12:42.222Z] 12:12:42     INFO - chrome://mochikit/content/browser-test.js:ensureVsyncDisabled:603
[task 2023-08-09T12:12:42.224Z] 12:12:42     INFO - Not taking screenshot here: see the one that was previously logged
[task 2023-08-09T12:12:42.226Z] 12:12:42     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/animation/test/browser_animation_keyframes-graph_keyframe-marker-rtl.js | vsync remained enabled at the end of the test. Is there an animation still running? Consider talking to the performance team for tips to solve this. - false == true - {"filename":"chrome://mochikit/content/browser-test.js","name":"ensureVsyncDisabled","sourceId":532,"lineNumber":604,"columnNumber":19,"sourceLine":"","asyncCause":null,"asyncCaller":null,"caller":null,"formattedStack":"ensureVsyncDisabled@chrome://mochikit/content/browser-test.js:604:19\n","nativeSavedFrame":{}}
[task 2023-08-09T12:12:42.227Z] 12:12:42     INFO - Stack trace:
[task 2023-08-09T12:12:42.228Z] 12:12:42     INFO - chrome://mochikit/content/browser-test.js:ensureVsyncDisabled:604

Comment 11

[Natalia Csoregi [:nataliaCs]]

Also failing:

• TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_unified_extensions.js | Test timed out -
• TEST-UNEXPECTED-FAIL | browser/base/content/test/keyboard/browser_toolbarKeyNav.js | Test timed out -

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c508433b9b8
Misc BrowserChild clean-ups. r=smaug

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0c508433b9b8

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1847584 - Don't unconditionally notify of layer tree updates from ClearCachedWebRenderResources(). r=#gfx-reviewers

Only do that if needed. Otherwise the extra event confuses the
front-end.

Before the rest of the patches of this bug,
ClearCachedWebRenderResources() happened to be only called when
renderLayers changed, but now it can happen as a result of the browser
being hidden too.

This means that we send extra LayersCleared events, but for the actual
transactions we weren't sending LayersReady afterwards, which caused the
front-end to get in a confused state.

Depends on D185809

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ec8b9db7177
Don't unconditionally notify of layer tree updates from ClearCachedWebRenderResources(). r=gfx-reviewers,nical

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0ec8b9db7177

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a741e0bcdca4
Account for IsUnderHiddenEmbedderElement() for BrowserChild visibility. r=smaug

Comment 18

[Cristian Tuns]

Backed out for causing bc failures in browser_reload_tab.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_reload_tab.js | Test timed out -

• https://treeherder.mozilla.org/logviewer?job_id=426494865&repo=autoland
  TEST-UNEXPECTED-FAIL | browser/base/content/test/menubar/browser_history_recently_closed_tabs.js | Recently closed tabs menu item is disabled - false == true - {"filename":"chrome://mochitests/content/browser/browser/base/content/test/menubar/browser_history_recently_closed_tabs.js","name":"checkMenu","sourceId":677,"lineNumber":41,"columnNumber":10,"sourceLine":"","asyncCause":null,"asyncCaller":{"filename":"chrome://mochitests/content/browser

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08da3ca498f6
Account for IsUnderHiddenEmbedderElement() for BrowserChild visibility. r=smaug

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/08da3ca498f6

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08dfb3655933
Consistently throttle visibility:hidden frames. r=smaug

Comment 22

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/08dfb3655933

Comment 23

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Definitely not upliftable.

Comment 25

[Alex Finder|PTO, back on Nov 25th]

(In reply to Pulsebot from comment #21)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08dfb3655933
Consistently throttle visibility:hidden frames. r=smaug

== Change summary for alert #39479 (as of Tue, 12 Sep 2023 03:21:41 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
2%	reddit LastVisualChange	linux1804-64-shippable-qr	cold fission webrender	5,719.44 -> 5,584.75

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=39479