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)
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)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dakatsuka
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-review |
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 12•7 years ago
|
||
mozreview-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 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
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
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/453e9dd459a2
https://hg.mozilla.org/mozilla-central/rev/452e577be2f4
https://hg.mozilla.org/mozilla-central/rev/6fa5b5963b06
https://hg.mozilla.org/mozilla-central/rev/443ff1adf8ab
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•