Closed Bug 1440607 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Site Identity and Permission Panels, enhancement, P3)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

Details

Attachments

(1 file)

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.
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.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
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.
(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 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.
Attachment #8954704 - Flags: review?(florian)
Ok, indeed, thanks, chatting with florian it seems like the callback will always be async since I change the DOM earlier.
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.
Attachment #8954704 - Flags: review?(florian) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f89d397ad02
Move getAnimations call out of the rAF for the WebRTC blinking animation. r=florian
https://hg.mozilla.org/mozilla-central/rev/3f89d397ad02
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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.
(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.
You need to log in before you can comment on or make changes to this bug.