Closed Bug 1478637 Opened Last year Closed Last year

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

Categories

(Firefox :: Site Identity and Permission Panels, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 + fixed

People

(Reporter: florian, Assigned: johannh)

References

Details

(Keywords: regression, Whiteboard: [fxperf:p1][qf:p1:f64])

Attachments

(1 file)

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.
Whiteboard: [fxperf]
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).
Priority: -- → P2
Whiteboard: [fxperf][qf] → [fxperf][qf:p1:f64]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
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.
Ah, right, florian is on PTO, let's reroute this...
Attachment #9001408 - Attachment description: Bug 1478637 - Avoid sync style flushes when cancelling the shield animation in the identity block. r=florian → Bug 1478637 - Avoid sync style flushes when cancelling the shield animation in the identity block. r=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.
Attachment #9001408 - Flags: review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85496f02e642
Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley
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.
Flags: needinfo?(jhofmann)
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!
Attachment #9001408 - Flags: feedback?(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!
Attachment #9001408 - Flags: feedback?(mconley) → feedback+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a465e7bdc7a
Avoid sync style flushes when cancelling the shield animation in the identity block. r=mconley
Whiteboard: [fxperf][qf:p1:f64] → [fxperf:p1][qf:p1:f64]
https://hg.mozilla.org/mozilla-central/rev/5a465e7bdc7a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1485393
From what I understand this is actually a QE- bug, sorry for the spam.
Flags: qe-verify+ → qe-verify-
No longer depends on: 1485393
See Also: → 1485393
You need to log in before you can comment on or make changes to this bug.