Animation Inspector freezes the browser

RESOLVED FIXED in Firefox 59

Status

()

Firefox
Developer Tools: Animation Inspector
P1
normal
RESOLVED FIXED
a month ago
15 days ago

People

(Reporter: Kwan, Assigned: daisuke)

Tracking

({regression})

Trunk
Firefox 59
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a month ago
STR:
1) Go to https://duckduckgo.com/?q=css+animations
2) Open DevTools Inspector
3) Select the Animations panel
4) The browser is now frozen (page animations continue to visibly run though)

Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bb63705ec95e878172e8f4c4628e11814683ffdc&tochange=c3530ca8ede501e49fdd9efb7e5d7f72df7f522b

Regressed by bug 1422218.
Flags: needinfo?(dakatsuka)
(Assignee)

Comment 1

a month ago
Thank you for the reporting.
I could reproduce it.
Will fix as soon as possible, thanks!
Flags: needinfo?(dakatsuka)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a month ago
mozreview-review
Comment on attachment 8938273 [details]
Bug 1426194 - Part 2: Add test.

https://reviewboard.mozilla.org/r/209042/#review214762
Attachment #8938273 - Flags: review?(pbrosset) → review+

Comment 6

a month ago
mozreview-review
Comment on attachment 8938272 [details]
Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

https://reviewboard.mozilla.org/r/209040/#review214764

::: devtools/client/animationinspector/graph-helper.js:269
(Diff revision 1)
> +      // The computed value on the time which represents keyframe offset changes to the
> +      // value which is written in the keyframe.
> +      // However, we can set keyframes that have same offset.
> +      // In this case, keyframes A, B which is same offset animate as  the animation from
> +      // previous keyframe of A to A and from B to next B.
> +      // However, the both computed values will be value of B since the offsets are same.
> +      // To correspond this, each distance of keyframes calculate from time of start
> +      // keyframe to end keyframe - BOUND_EXCLUDING_TIME except same offset.

Sorry Daisuke, the code changes look good to me, but I don't understand this comment clearly enough to R+ this.
Could you try to explain to me again what's happening in the bug, and how do we solve it here. 
In fact, this explanation would be good to have as a multi-line commit message too.
I just want to be sure I R+ the right thing.
Thanks!
Attachment #8938272 - Flags: review?(pbrosset)
(Assignee)

Comment 7

a month ago
(In reply to Patrick Brosset <:pbro> from comment #6)
> Comment on attachment 8938272 [details]
> Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.
> 
> https://reviewboard.mozilla.org/r/209040/#review214764
> 
> ::: devtools/client/animationinspector/graph-helper.js:269
> (Diff revision 1)
> > +      // The computed value on the time which represents keyframe offset changes to the
> > +      // value which is written in the keyframe.
> > +      // However, we can set keyframes that have same offset.
> > +      // In this case, keyframes A, B which is same offset animate as  the animation from
> > +      // previous keyframe of A to A and from B to next B.
> > +      // However, the both computed values will be value of B since the offsets are same.
> > +      // To correspond this, each distance of keyframes calculate from time of start
> > +      // keyframe to end keyframe - BOUND_EXCLUDING_TIME except same offset.
> 
> Sorry Daisuke, the code changes look good to me, but I don't understand this
> comment clearly enough to R+ this.
> Could you try to explain to me again what's happening in the bug, and how do
> we solve it here. 
> In fact, this explanation would be good to have as a multi-line commit
> message too.
> I just want to be sure I R+ the right thing.
> Thanks!

Thanks Patrick! Okay!
Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
59 merges to beta next week I believe, Daisuke, do you want me to review the new commits you pushed?
Flags: needinfo?(dakatsuka)
(Assignee)

Comment 11

16 days ago
Yes, thanks!
Flags: needinfo?(dakatsuka)
(Assignee)

Updated

16 days ago
Attachment #8938272 - Flags: review?(pbrosset)

Comment 12

16 days ago
mozreview-review
Comment on attachment 8938272 [details]
Bug 1426194 - Part 1: Correspond to the keyframes which have same offset.

https://reviewboard.mozilla.org/r/209040/#review215760

Thank you for the long explanation :)
Attachment #8938272 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 13

16 days ago
Thank you very much!
I'll land soon.

Comment 14

16 days ago
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4a89ebfbb40
Part 1: Correspond to the keyframes which have same offset. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e4f2f93a78f0
Part 2: Add test. r=pbro

Comment 15

16 days ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/1230c90d4d99
Part 1: Correspond to the keyframes which have same offset. r=pbro
https://hg.mozilla.org/mozilla-central/rev/209dc7135305
Part 2: Add test. r=pbro

Comment 16

16 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1230c90d4d99
https://hg.mozilla.org/mozilla-central/rev/209dc7135305
Status: ASSIGNED → RESOLVED
Last Resolved: 16 days ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.