1693176 - CSS animations in hidden panels cause the refresh driver to keep ticking

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Markus Stange [:mstange]]

Example profile: https://share.firefox.dev/2NCh1TC

Comment 1

[Markus Stange [:mstange]]

This was encountered by Henrik Skupin. Using the browser toolbox, he was able to find the permission-popup-permission-icon-in-use-blink animation.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Here some steps:

1. Use a Google Meet link and open the page
2. Accept the permission for the microphone
3. The animation of the camera icon in the url bar will start
4. Click the permissions popup
5. Close the page

Now the main process consumes quit a good amount of CPU until the permissions popup gets opened again.

The MBT showed me the animation for permission-popup-permission-icon-in-use-blink — CSS Animation. Clicking the icon to highlight it in the inspector the camera icon was selected: permission-popup-permission-icon camera-icon in-use.

Note that I closed the tab for Google Meet several hours ago.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Looks like frames (nsIFrames) in the permission popup keep alive even if the popup window is closed. Is it intentional?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Wondering whether this is regressed by bug 1596897 or not.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

CCing :pbz.

Comment 6

[Timothy Nikkel (:tnikkel)]

I don't see anything in the menu popup frame code that would destroy frames on hide. Probably so it can be shown again quickly?

Comment 7

[Markus Stange [:mstange]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

Looks like frames (nsIFrames) in the permission popup keep alive even if the popup window is closed. Is it intentional?

I think that's how popup windows are implemented, yes. When a popup is closed, its frames do not get destroyed. It's possible that we could change that, but it seems a bit risky.

And yes, while front-end can do various things to work around this, I think it would be preferable to have a general solution so that we don't tick animations which are in the frame subtree of an nsMenuPopupFrame which returns false from IsOpen().

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1693176 - Throttle animations in hidden nsViews.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Markus Stange [:mstange] from comment #7)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

Looks like frames (nsIFrames) in the permission popup keep alive even if the popup window is closed. Is it intentional?

I think that's how popup windows are implemented, yes. When a popup is closed, its frames do not get destroyed. It's possible that we could change that, but it seems a bit risky.

Thanks for the quick reply.

And yes, while front-end can do various things to work around this, I think it would be preferable to have a general solution so that we don't tick animations which are in the frame subtree of an nsMenuPopupFrame which returns false from IsOpen().

D105377 should fix this issue (The change does check nsView::GetVisibility() state instead of the menu state though). That said, it just throttles animations in hidden popup window, so the refresh driver needs to keep ticking for the animations (for animation events, etc.). So eventually we should de-activate the presshell even if we keep frames in popup.

Comment 10

[Markus Stange [:mstange]]

If the refresh driver keeps ticking, that won't help enough, I think. We want to stop waking up the CPU.

The popup's frame subtree is part of the same presshell as the main browser window, so we can't deactivate it.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Hmm, okay, then we should discard the frames in popup windows. (or front-end needs to stop animations when the popup is closed)

Comment 12

[Markus Stange [:mstange]]

Okay, that does sound like the better solution. In the meantime, I've filed bug 1693201 for the front-end workaround.

Comment 13

[Markus Stange [:mstange]]

Emilio, can you foresee any trouble from destroying frames in closed menupopups?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I think it's worth a try. It was more risky when XBL was around (because frames also implied XBL bindings). But nowadays that shouldn't be a concern.

It'd be good if they used display: none... Should be easy-ish to prototype in menupopup.js by hiding the arrowscrollbox on popuphidden or such? Of course it'd be much better if it was done by Gecko but that requires more tweaking of the popup code.

Comment 15

[Markus Stange [:mstange]]

Ah, good idea.

Hmm, what about add-on popups which contain remote browsers? Do those handle display:none the way we want?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

That is a good question. I think top level XUL <browser> elements don't care about whether they have frames much (in terms of throttling etc). I think we might start the load the first time they get frames, but otherwise is DOM connection what matters I believe.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1693176 - WIP - Destroy frames of contents inside menupopups.

This seems to work, though I've only done some light-weight testing.
Let's see what try says.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I did a quick test and extension browsers seem to behave nicely with this still.

Comment 19

[Markus Stange [:mstange]]

Nice!

For historical context, this would also have helped with bug 1541314.

Furthermore, I just found bug 1413763 (which I'm not sure still reproduces), which has the fade-out opacity animation on the panel itself. The experimental patch does not destroy the panel's menupopup frame, so that case would not be fixed by it.

Comment 20

[Neil Deakin]

Emilio, can you foresee any trouble from destroying frames in closed menupopups?

This will destroy the native widget as well, no? That can have performance implications when quickly rolling over menus/submenus.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

My patch only destroys the popup contents. Destroying the popup frame itself seems potentially trickier indeed, not only because of the widget thing, but also because all the state nsMenuPopupFrame stores.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Here's a try run for the patch above btw, let's see what breaks... https://treeherder.mozilla.org/jobs?repo=try&revision=fb0c5541e76222b22ff499052062278e98812453

Comment 23

[Emilio Cobos Álvarez (:emilio)]

There seem to be some failures with the <select> popup tests on windows and mac, but other than that it seems pretty workable... I can try to dig tomorrow.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Current status is tests work, except some a11y focus mochitests which I'm pretty confused about.

Comment 25

[Markus Stange [:mstange]]

Just a note: the STR in comment 2 will no longer reproduce this because this particular animation has been removed in bug 1687397.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1693176 - Destroy frames of contents inside panels. r=Gijs

Panels don't rely so much on the popup manager code to deal with
menuitems etc, so this is trivial and fixes the issue as reported.

I think it's pretty unlikely to have animated stuff in menulists /
menupopups, so this should get us most of the way.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

So the a11y tests were a real bug about stuff we otherwise don't have a test for of course. I updated the commit message of my original phab revision, but basically doing this for menus/menulists is much harder because lots of the keyboard navigation code for them is implemented in terms of the frame tree.

However for the original case, which is a <panel>, this is trivial and works nicely. I think having animated stuff in a <menupopup> or <menulist> is not likely, so I think we should land the <panel> change without having to rewrite nsMenuPopupFrame / xul menus.

But let me know if that doesn't make sense for some reason :)

Comment 28

[Markus Stange [:mstange]]

That sounds reasonable! Removing "menupopups" from the bug title.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a3f1073b7b5
Destroy frames of contents inside panels. r=Gijs

Comment 30

[Dorel Luca [:dluca]]

Backed out changeset 4a3f1073b7b5 (bug 1693176) for Browser-chrome failures in extensions/test/browser/browser_ext_browserAction_contextMenu.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=330422774&repo=autoland&lineNumber=3506

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=4a3f1073b7b50517bbcf75665bb94ec8f8ddfdf3

Log:
https://hg.mozilla.org/integration/autoland/rev/07a3c0c3642deb2e03450872bdf2fb8cb5331f5a

Comment 31

[Florian Quèze [:florian]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)

These steps to reproduce won't work anymore as bug 1687397 removed this specific animation.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3fab819b622
Destroy frames of contents inside panels. r=Gijs

Comment 34

[Iulian Moraru]

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