1440607 - promiseLayoutFlushed/requestAnimationFrame with .getAnimations in the identity popup causes leakcheck failures

Status: RESOLVED FIXED

(Firefox :: Site Identity, enhancement, P3)

Description

[Johann Hofmann [:johannh]]

An example try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5264d92695cc66b667490560664bc0b2f599660a&selectedJob=163815401

The following code causes the leaks: https://hg.mozilla.org/try/rev/1f37ca56a73d0623c8ffbdf3b755716c915e972b#l1.96

Note that if you move 'let imgBlink = img.getAnimations()[0];' inside the rAF the leak is stopped, which is what I'm going to do to be able to move forward with bug 1333468. This presumably still causes style flushes so it's not what we want to do permanently.

With all the changes in bug 1434376 my hope is that it fixes the problem somehow, so it's worth re-trying when that bug is fixed.

Comment 1

[Johann Hofmann [:johannh]]

Seems like this is not failing anymore, probably fixed by bug 1434376.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8022cf7b0ad52e8ef006edb03ca2ddebe3014a07

I'll put up a patch.

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking animation.

Review commit: https://reviewboard.mozilla.org/r/223834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223834/

Comment 3

[Johann Hofmann [:johannh]]

Comment on attachment 8954704 [details]
Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223834/diff/1-2/

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8954704 [details]
Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223834/diff/2-3/

Comment 5

[:Paolo Amadini]

Comment on attachment 8954704 [details]
Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking animation.

https://reviewboard.mozilla.org/r/223834/#review229852

::: browser/base/content/browser.js:8072
(Diff revision 3)
>         (aPermission.id == "screen" && aPermission.sharingState &&
>          !aPermission.sharingState.includes("Paused"))) {
>        classes += " in-use";
>  
>        // Synchronize control center and identity block blinking animations.
>        window.promiseDocumentFlushed(() => {}).then(() => {

You probably need just:

window.promiseDocumentFlushed(() => {

That is, style is read inside the callback, and writes are done once the Promise has resolved.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to :Paolo Amadini from comment #5)
> Comment on attachment 8954704 [details]
> Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking
> animation.
> 
> https://reviewboard.mozilla.org/r/223834/#review229852
> 
> ::: browser/base/content/browser.js:8072
> (Diff revision 3)
> >         (aPermission.id == "screen" && aPermission.sharingState &&
> >          !aPermission.sharingState.includes("Paused"))) {
> >        classes += " in-use";
> >  
> >        // Synchronize control center and identity block blinking animations.
> >        window.promiseDocumentFlushed(() => {}).then(() => {
> 
> You probably need just:
> 
> window.promiseDocumentFlushed(() => {
> 
> That is, style is read inside the callback, and writes are done once the
> Promise has resolved.

Hm, I think I need a guaranteed async callback to do .getAnimations() on the img element I just created.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8954704 [details]
Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking animation.

https://reviewboard.mozilla.org/r/223834/#review230306

I agree with comment 5.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8954704 [details]
Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking animation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/223834/diff/3-4/

Comment 9

[Johann Hofmann [:johannh]]

Ok, indeed, thanks, chatting with florian it seems like the callback will always be async since I change the DOM earlier.

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8954704 [details]
Bug 1440607 - Move getAnimations call out of the rAF for the WebRTC blinking animation.

https://reviewboard.mozilla.org/r/223834/#review230666

Looks good to me now, thanks!

::: browser/base/content/browser.js:8081
(Diff revision 4)
> -            // the getAnimations() call outside of rAF causes a leak.
> -            let imgBlink = img.getAnimations()[0];
> +        let imgBlink = img.getAnimations()[0];
> -            if (imgBlink) {
> -              imgBlink.startTime = startTime;
> -            }
> -          });
> +        return [sharingIconBlink, imgBlink];
> +      }).then(([sharingIconBlink, imgBlink]) => {
> +        if (sharingIconBlink && imgBlink) {
> +          imgBlink.startTime = sharingIconBlink.startTime;

Note for future reference: the startTime getter doesn't trigger another style flush here, johannh verified with emilio.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f89d397ad02
Move getAnimations call out of the rAF for the WebRTC blinking animation. r=florian

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3f89d397ad02

Comment 13

[Hiroyuki Ikezoe (:hiro)]

I am curious the leak also happens in the wild, I mean in normal web content pages.  If it does we should fix the leak.

Also this noticed me that we don't need to flush throttled animation for getAnimations() call.  I will file a bug for that.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> I am curious the leak also happens in the wild, I mean in normal web content
> pages.  If it does we should fix the leak.
> 
> Also this noticed me that we don't need to flush throttled animation for
> getAnimations() call.  I will file a bug for that.

Filed bug 1442817.