Closed Bug 1462229 Opened 7 years ago Closed 7 years ago

Animation inspector becomes blank: "easing is undefined" in graph-helper.js

Categories

(DevTools :: Inspector: Animations, enhancement, P3)

61 Branch
enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: jdescottes, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

During today's demo: STRs: - open about:newtab - open inspector > animation inspector - hovering very quickly on the tiles of about:newtab console.error: (new TypeError("easing is undefined", "resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/animation/utils/graph-helper.js", 242)) Probably called from: getPreferredProgressThreshold, we should guard it with: if (!keyframe.easing) { continue; } (same as in getPreferredProgressThresholdByKeyframes)
Blocks: 1399830
Assignee: nobody → dakatsuka
I have updated the patch 1 since the last try had some problems in Windows. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3731faa879d839dbd8ea78b92d23476431d4a1a
Comment on attachment 8980171 [details] Bug 1462229 - Part 1: Avoid updating the state of removed animation. https://reviewboard.mozilla.org/r/246328/#review253124 ::: devtools/client/inspector/animation/animation.js:308 (Diff revision 2) > animations.splice(index, 1); > animation.off("changed", this.onAnimationStateChanged); > } > } > > - this.updateState(animations); > + // Update existing other animations as well since the currentTime would be proceeded. s/proceeded. Because/proceeded since ::: devtools/client/inspector/animation/animation.js:310 (Diff revision 2) > } > } > > - this.updateState(animations); > + // Update existing other animations as well since the currentTime would be proceeded. > + // Because the scrubber position is related the currentTime. > + // Also, should not update the state of removed animations since React components Also, don't update the state ... ::: devtools/client/inspector/animation/animation.js:311 (Diff revision 2) > } > > - this.updateState(animations); > + // Update existing other animations as well since the currentTime would be proceeded. > + // Because the scrubber position is related the currentTime. > + // Also, should not update the state of removed animations since React components > + // may refer same instance still. may refer to the same instance still
Attachment #8980171 - Flags: review?(gl) → review+
Comment on attachment 8980172 [details] Bug 1462229 - Part 2: Avoid updating SummaryGraphPath if the component have been destroyed or other node have been selected while calling async function. https://reviewboard.mozilla.org/r/246330/#review253126
Attachment #8980172 - Flags: review?(gl) → review+
Comment on attachment 8980173 [details] Bug 1462229 - Part 3: Guard that not touching null object during creating graph after animation inspector destroyed. https://reviewboard.mozilla.org/r/246332/#review253136 ::: devtools/client/inspector/animation/animation.js:536 (Diff revision 2) > * @return {Animation} > * https://drafts.csswg.org/web-animations/#the-animation-interface > */ > simulateAnimation(keyframes, effectTiming, isElementNeeded) { > + if (!this.win) { > + throw Error("Animation inspector already destroyed"); I think we should just return silently since throwing an error is unhelpful in this ase. // Don't simulate animation if the animation inspector is already destroyed. if (!this.win) { return; } ::: devtools/client/inspector/animation/animation.js:650 (Diff revision 2) > }); > } > > updateState(animations) { > + if (!this.inspector) { > + console.error("Animation inspector already destroyed"); Remove the console.error. Add a comment like above. ::: devtools/client/inspector/animation/components/graph/ComputedTimingPath.js:59 (Diff revision 2) > + let simulatedAnimation = null; > + > + try { > + simulatedAnimation = simulateAnimation(frames, effectTiming, true); > + } catch (e) { > + console.log(e); Remove this. ::: devtools/client/inspector/animation/components/graph/EffectTimingPath.js:43 (Diff revision 2) > - const simulatedAnimation = simulateAnimation(null, effectTiming, false); > + let simulatedAnimation = null; > + > + try { > + simulatedAnimation = simulateAnimation(null, effectTiming, false); > + } catch (e) { > + console.log(e); Remove this. ::: devtools/client/inspector/animation/components/graph/NegativePath.js:59 (Diff revision 2) > - const simulatedAnimation = simulateAnimation(frames, effectTiming, true); > + let simulatedAnimation = null; > + > + try { > + simulatedAnimation = simulateAnimation(frames, effectTiming, true); > + } catch (e) { > + console.log(e); Remove this. ::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:87 (Diff revision 2) > const effect = { > duration, > fill: "forwards", > }; > + > + // simulateAnimation throws error if animation inspector has destroyed already. Remove this. ::: devtools/client/inspector/animation/components/keyframes-graph/ComputedStylePath.js:190 (Diff revision 2) > const endKeyframe = keyframes[i + 1]; > + > + try { > - segments.push(...this.getPathSegments(startKeyframe, endKeyframe)); > + segments.push(...this.getPathSegments(startKeyframe, endKeyframe)); > + } catch (e) { > + console.error(e); Remove this.
Attachment #8980173 - Flags: review?(gl) → review+
Comment on attachment 8980174 [details] Bug 1462229 - Part 4: Add test for fast removal. https://reviewboard.mozilla.org/r/246334/#review253138 ::: devtools/client/inspector/animation/test/browser_animation_logic_mutations_fast.js:6 (Diff revision 2) > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Test whether the animation inspector will not crush when remove/add animations faster. s/crush/crash ::: devtools/client/inspector/animation/test/browser_animation_logic_mutations_fast.js:15 (Diff revision 2) > + const { inspector } = await openAnimationInspector(); > + > + info("Check state of the animation inspector after fast mutations"); > + await startFastMutations(tab); > + ok(inspector.panelWin.document.getElementById("animation-container"), > + "Animation inspector should live"); s/should/should be
Attachment #8980174 - Flags: review?(gl) → review+
Thank you for the reviewing, Gabriel! I have updated the patches. Will land if the try was green. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cb8a5c425b8580218c7ded7b36c368ed0766b80
Pushed by dakatsuka@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/453e9dd459a2 Part 1: Avoid updating the state of removed animation. r=gl https://hg.mozilla.org/integration/autoland/rev/452e577be2f4 Part 2: Avoid updating SummaryGraphPath if the component have been destroyed or other node have been selected while calling async function. r=gl https://hg.mozilla.org/integration/autoland/rev/6fa5b5963b06 Part 3: Guard that not touching null object during creating graph after animation inspector destroyed. r=gl https://hg.mozilla.org/integration/autoland/rev/443ff1adf8ab Part 4: Add test for fast removal. r=gl
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: