Closed Bug 1462229 Opened 6 years ago Closed 6 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: