1478637 - TrackingProtection.cancelAnimation (from browser-trackingprotection.js) does sync style flushes

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Florian Quèze [:florian]]

See this profile: https://perfht.ml/2LQnCUo

The sync flush happens when calling getAnimations at https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/browser/base/content/browser-trackingprotection.js#175

This is code from bug 1471713.

I wish this kind of regressions was covered by our automated perf tests. Currently we only cover layout flushes, not style flushes.

Comment 1

[Florian Quèze [:florian]]

In addition to causing a sync style flush (can probably be avoided with promiseDocumentFlushed), this code seems to be called too often (ie. unconditionally for every onSecurityChange call, which happens at each tab switch).

Comment 2

[Florian Quèze [:florian]]

Nominating for tracking63, as this is a serious performance regression in 63 (slowing down tab switching) and I think we should really fix this before shipping 63 to release.

Comment 3

[Johann Hofmann [:johannh]]

Attachment: Bug 1478637 - Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley

Comment 4

[Johann Hofmann [:johannh]]

Ah, right, florian is on PTO, let's reroute this...

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9001408 [details]
Bug 1478637 - Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley

Mike Conley (:mconley) (:⚙️) has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85496f02e642
Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley

Comment 7

[Andrei Ciure[:aciure]]

Backed out 1 changesets (bug 1478637) for browser_trackingUI_animation_2.js failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=85496f02e642767418641817493097099071e158&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&fromchange=45e540e5e0fc2bfca60fe015a546fdb6f2f01318&selectedJob=194436065&filter-searchStr=Linux+x64+debug+Mochitests+with+e10s+test-linux64%2Fdebug-mochitest-browser-chrome-e10s-2+M-e10s%28bc2%29

log: https://treeherder.mozilla.org/logviewer.html#?job_id=194436065&repo=autoland

backout: https://hg.mozilla.org/integration/autoland/rev/9961965127f838781dcb1a28633269078d25fb57

Comment 8

[Johann Hofmann [:johannh]]

Ok, yeah, I was afraid of that happening but didn't consider that this test would expose it. The pDF call is async and so makes cancelling the animation prone to race conditions with switching to the next tab. In this test it means that we switch to the tab without a shield icon and subsequently the shield stops animating, which means we don't remove its "animate" class anymore.

I think I should look at this from a different angle. Why am I even cancelling the animation "manually" in the first place, when removing the className should be enough, assuming that the same class is not added again in that tick? The latter may happen e.g. when navigating back and forth using the back button and the security information loads instantly.

Considering the alternatives (doing sync style flushes, racy unpredictable cancelling of the animation) I think we can live with the animation continuing instead of restarting in these edge cases. At least I hope that when I implemented this there weren't any other cases I had in mind.

So, assuming the above is all true, I'll just remove the cancelAnimation function and replace it with simply dropping the animate className. It should satisfy both perf needs and our tests for now, and we can observe it in Nightly.

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 9001408 [details]
Bug 1478637 - Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley

Mike, would you mind signing this off for me again?

Thanks!

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 9001408 [details]
Bug 1478637 - Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley

This seems okay, and is certainly simpler. Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a465e7bdc7a
Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5a465e7bdc7a

Comment 13

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

From what I understand this is actually a QE- bug, sorry for the spam.