Closed Bug 1458268 Opened Last year Closed Last year

Blank inspector after using animation inspector

Categories

(DevTools :: Inspector: Animations, defect, P2)

59 Branch
defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

Details

Attachments

(4 files)

After hovering a node while the animation panel is selected, the whole inspector goes blank. Need to close and reopen devtools to restore it. See attached GIF.

Seems to happen on about:newtab, not sure how frequently it can happen. 

In the console I can see:
(new TypeError("Value being assigned to Animation.currentTime is not a finite floating-point value.", "resource://devtools/shared/base-loader.js -> resource://devtools/client/inspector/animation/components/KeyframesProgressBar.js", 69))
When this happens the current animation for KeyframesProgressBar has playState = "idle" and all other fields to undefined including "playbackRate":

  this.simulatedAnimation.currentTime = (timeScale.minStartTime + currentTime - previousStartTime) * playbackRate;
  
  -> NaN

We can simply guard here, but there might be a better way to handle this.
Attached file testcase.html
Can be easily reproduced with the attachment:
- open inspector
- select animation tab
=> Inspector goes blank
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review246790


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/test/browser_animation_css-transition-with-playstate-idle.js:32
(Diff revision 1)
> +</body>
> +</html>`;
> +
> +add_task(async function() {
> +  const tab = await addTab(PAGE_URL);
> +  const { animationInspector, inspector, panel } = await openAnimationInspector();

Error: 'inspector' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/inspector/animation/test/browser_animation_css-transition-with-playstate-idle.js:61
(Diff revision 1)
> +
> +  // Query the scrubber element again to check that the UI is still rendered.
> +  ok(!!panel.querySelector(".current-time-scrubber"),
> +    "The scrubber element is still rendered in the animation inspector panel");
> +
> +  info("Wait for the keyframes graph to be updated before ending the test.")

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review246872

::: devtools/client/inspector/animation/components/KeyframesProgressBar.js:64
(Diff revision 1)
>      const {
>        animation,
>        timeScale,
>      } = this.props;
>      const {
> -      playbackRate,
> +      playbackRate = 1,

Thank you for finding and fixing this, Julian!

Please let me cancel this review.
I investigated this one, the cause is that we were referring invalid animation in onCurrentTimeUpdated.
The function is called not only as current time listener but also from KeyframesProgressBar itself as well. The problem was second case. Though we call from componentWillReceiveProps, actually we needed to refer the animation in nextProps. However, for now, in onCurrentTimeUpdated, we refer to the animation of this.props. The animation was already invalid( previous different animation actor ).
(It's my fault, sorry..)

I think, one of solutions is that we give the nextProps to onCurrentTimeUpdated. In the function, if the given props exists, refer that. Else, refer this.props.

::: devtools/client/inspector/animation/test/browser_animation_css-transition-with-playstate-idle.js:12
(Diff revision 1)
> +// transitions from the playState "idle".
> +
> +const PAGE_URL = `data:text/html,
> +<!DOCTYPE html>
> +<html>
> +<head>

Please insert meta element for charset in <head> like below:
  `<meta charset="utf-8">`

::: devtools/client/inspector/animation/test/browser_animation_css-transition-with-playstate-idle.js:16
(Diff revision 1)
> +<html>
> +<head>
> +  <style type="text/css">
> +    div {
> +      opacity: 0;
> +      transition-duration: 200ms;

This duration is too short, because we tests on very slow envrionment as well. I think, the animation might  finish before calling waitUntilAnimationsPaused. Maybe 5s?
If you think this test takes too long time, we can set the current time ahead by clickOnCurrentTimeScrubberController[1].
Then, resume[2].

[1] https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation/test/head.js#189
[2] https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation/test/head.js#151
Attachment #8972385 - Flags: review?(dakatsuka)
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review246908


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/components/KeyframesProgressBar.js:69
(Diff revision 2)
> -    const {
>        playbackRate,
>        previousStartTime = 0,
>      } = animation.state;
>  
> -    this.simulatedAnimation.currentTime =
> +    const time = (timeScale.minStartTime + currentTime - previousStartTime) * playbackRate;

Error: Line 69 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/inspector/animation/components/KeyframesProgressBar.js:76
(Diff revision 2)
> +      // Setting an invalid currentTime will throw so bail out if time is not a number for
> +      // any reason.
> +      return;
> +    }
> +
> +    this.simulatedAnimation.currentTime = time

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review246872

Thanks for the review Daisuke, some questions for you in the comments below!

> Thank you for finding and fixing this, Julian!
> 
> Please let me cancel this review.
> I investigated this one, the cause is that we were referring invalid animation in onCurrentTimeUpdated.
> The function is called not only as current time listener but also from KeyframesProgressBar itself as well. The problem was second case. Though we call from componentWillReceiveProps, actually we needed to refer the animation in nextProps. However, for now, in onCurrentTimeUpdated, we refer to the animation of this.props. The animation was already invalid( previous different animation actor ).
> (It's my fault, sorry..)
> 
> I think, one of solutions is that we give the nextProps to onCurrentTimeUpdated. In the function, if the given props exists, refer that. Else, refer this.props.

Thanks, I followed your suggestion although I extracted the common part to a "updateOffset" method in order to avoid optional arguments. This way the onCurrentTimeUpdated callback is only ever called as a callback and not directly from this component. Let me know if that's ok with you.

I also added a guard against a NaN value for the computed time. The reason for that is that even with the suggested approach, some transitions will still fail with this code. You can try  to tweak my test case and set the transition time to 1 or 2 ms. This would still give a blank inspector. I guess that's because we get into edge cases where the "next" animation is already in idle state because the animations are so short. So I think it's better to be extra careful here and if we have such an edge case, just return early.

> Please insert meta element for charset in <head> like below:
>   `<meta charset="utf-8">`

Done, thanks!

> This duration is too short, because we tests on very slow envrionment as well. I think, the animation might  finish before calling waitUntilAnimationsPaused. Maybe 5s?
> If you think this test takes too long time, we can set the current time ahead by clickOnCurrentTimeScrubberController[1].
> Then, resume[2].
> 
> [1] https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation/test/head.js#189
> [2] https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation/test/head.js#151

Sorry for not applying this one, but I want to make sure this is needed first :) All my waitUntil* tests are expecting what should be the final state of the UI. So I think even on very slow platforms, the waitUntil should succeed? Am I missing something? 

Started a try run on all platforms at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0174e87f13508857481ed165df8ae7a5816141ad
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review246910

::: devtools/client/inspector/animation/test/browser_animation_css-transition-with-playstate-idle.js:62
(Diff revision 2)
> +  // Query the scrubber element again to check that the UI is still rendered.
> +  ok(!!panel.querySelector(".current-time-scrubber"),
> +    "The scrubber element is still rendered in the animation inspector panel");
> +
> +  info("Wait for the keyframes graph to be updated before ending the test.");
> +  await waitForAnimationDetail(animationInspector);

I am a bit concerned with this. I added it to avoid errors when stopping the test before the animation inspector has finished all rendering. Otherwise, we get errors in various rendering callbacks, because the window is no longer available.

I don't think we have a good event to wait for to know that the animation-inspector is fully "done". The current approach feels fragile and I am afraid it will be racy and intermittent. I tried to listen to a precise number of events. It seems that right now, during the whole test we fire 4 "animation-keyframes-rendered" events. But this feels very implementation specific. 

I see two options here: 
1/ introduce a helper that waits either for rendering events, but at most for X milliseconds (to avoid timeouts if the events have already been fired) 
2/ make the animation inspector more stable and guard the various callbacks that can be triggered after the destruction of the panel

What do you think?
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review246872

> Thanks, I followed your suggestion although I extracted the common part to a "updateOffset" method in order to avoid optional arguments. This way the onCurrentTimeUpdated callback is only ever called as a callback and not directly from this component. Let me know if that's ok with you.
> 
> I also added a guard against a NaN value for the computed time. The reason for that is that even with the suggested approach, some transitions will still fail with this code. You can try  to tweak my test case and set the transition time to 1 or 2 ms. This would still give a blank inspector. I guess that's because we get into edge cases where the "next" animation is already in idle state because the animations are so short. So I think it's better to be extra careful here and if we have such an edge case, just return early.

Thank you for the updating, Julian!

The function updateOffset looks good to me, thanks!
And, regarding to the guard, I could reproduce the issue.  As you said, the reason was getAnimationsCurrentTime returned undefined because the animation already finished.
I'll fix this in another bug. Then we can drop the guard.

> Sorry for not applying this one, but I want to make sure this is needed first :) All my waitUntil* tests are expecting what should be the final state of the UI. So I think even on very slow platforms, the waitUntil should succeed? Am I missing something? 
> 
> Started a try run on all platforms at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0174e87f13508857481ed165df8ae7a5816141ad

Yes, it looks that most of all work well. Also, because the updated patch checks "paused" and "finished", waitUntilAnimationsPaused should work as well.
However, if the animation finished before the below code, the waitUntil() never return true.

  `info("Wait until the scrubber starts moving");`
  `await waitUntil(() => scrubberEl.getBoundingClientRect().x != scrubberX);`

The try had the error[1].

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=176525657&repo=try&lineNumber=2903
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review246910

> I am a bit concerned with this. I added it to avoid errors when stopping the test before the animation inspector has finished all rendering. Otherwise, we get errors in various rendering callbacks, because the window is no longer available.
> 
> I don't think we have a good event to wait for to know that the animation-inspector is fully "done". The current approach feels fragile and I am afraid it will be racy and intermittent. I tried to listen to a precise number of events. It seems that right now, during the whole test we fire 4 "animation-keyframes-rendered" events. But this feels very implementation specific. 
> 
> I see two options here: 
> 1/ introduce a helper that waits either for rendering events, but at most for X milliseconds (to avoid timeouts if the events have already been fired) 
> 2/ make the animation inspector more stable and guard the various callbacks that can be triggered after the destruction of the panel
> 
> What do you think?

Thank you for the sugestion!
1 looks easier but 2 looks better.. 
I wonder if we better to modify animation actor, in case of 2 ??
(In reply to Daisuke Akatsuka (:daisuke) from comment #11)
> Comment on attachment 8972385 [details]
> Bug 1458268 - Avoid empty inspector when inspecting css transition;
> 
> https://reviewboard.mozilla.org/r/241002/#review246872
> 
> > Thanks, I followed your suggestion although I extracted the common part to a "updateOffset" method in order to avoid optional arguments. This way the onCurrentTimeUpdated callback is only ever called as a callback and not directly from this component. Let me know if that's ok with you.
> > 
> > I also added a guard against a NaN value for the computed time. The reason for that is that even with the suggested approach, some transitions will still fail with this code. You can try  to tweak my test case and set the transition time to 1 or 2 ms. This would still give a blank inspector. I guess that's because we get into edge cases where the "next" animation is already in idle state because the animations are so short. So I think it's better to be extra careful here and if we have such an edge case, just return early.
> 
> Thank you for the updating, Julian!
> 
> The function updateOffset looks good to me, thanks!
> And, regarding to the guard, I could reproduce the issue.  As you said, the
> reason was getAnimationsCurrentTime returned undefined because the animation
> already finished.
> I'll fix this in another bug. Then we can drop the guard.
> 
> > Sorry for not applying this one, but I want to make sure this is needed first :) All my waitUntil* tests are expecting what should be the final state of the UI. So I think even on very slow platforms, the waitUntil should succeed? Am I missing something? 
> > 
> > Started a try run on all platforms at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0174e87f13508857481ed165df8ae7a5816141ad
> 
> Yes, it looks that most of all work well. Also, because the updated patch
> checks "paused" and "finished", waitUntilAnimationsPaused should work as
> well.
> However, if the animation finished before the below code, the waitUntil()
> never return true.
> 
>   `info("Wait until the scrubber starts moving");`
>   `await waitUntil(() => scrubberEl.getBoundingClientRect().x != scrubberX);`
> 
> The try had the error[1].
> 
> [1]
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=176525657&repo=try&lineNumber=2903

Indeed! So the issue is that the scrubber position at the end of the first animation is the same as at the end of the last animation. I tried updating it to move the scrubber midway before toggling the class but I realized that the test would still potentially fail on the last `await waitForAnimationDetail(animationInspector);` is the animation completed before the test reaches this statement. So in the end I applied your initial suggestion and bumped the animation duration to 5seconds.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Thanks for the feedback about avoiding errors after the destroy of the panel. I spent a bit more time with it. Main issue is with SummaryGraphPath::updateState which is async and calls `await getAnimatedPropertyMap(animation);`. This call is wrapped in a try/catch:

    try {
      animatedPropertyMap = await getAnimatedPropertyMap(animation);
    } catch (e) {
      // Expected if we've already been destroyed or other node have been selected
      // in the meantime.
      console.error(e);
      return;
    }

But getAnimatedPropertyMap is not throwing if the panel is destroyed. And throwing an error from there if the panel was destroyed seems to fix the issues I had in the test. This could be an option here.
Attachment #8972615 - Flags: feedback?(dakatsuka)
Comment on attachment 8972615 [details] [diff] [review]
avoid_errors_after_destroy.patch

Thank you very much!
It looks good for this time!

(I am wondering if we can reject the promises that are connecting to the server at same time when destroying. But please let me think this in another bug.)
Attachment #8972615 - Flags: feedback?(dakatsuka) → feedback+
Comment on attachment 8972385 [details]
Bug 1458268 - Avoid empty inspector when inspecting css transition;

https://reviewboard.mozilla.org/r/241002/#review247048

Thank you for the updating!
Attachment #8972385 - Flags: review?(dakatsuka) → review+
Thanks for the feedback! As discussed, let's move the patch and discussion to another bug.

Waiting for all jobs on https://treeherder.mozilla.org/#/jobs?repo=try&revision=733649994276de9f829edd06c4c1c62f5a853090 to finish and will land if everything is green.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 3 changes to 3 files
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook is invalid: import of "mozhghooks.prevent_webidl_changes" failed
remote: (run with --traceback for stack trace)
abort: push failed on remote
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3206de2e2437
Avoid empty inspector when inspecting css transition;r=daisuke
https://hg.mozilla.org/mozilla-central/rev/3206de2e2437
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
QA Whiteboard: [good first verify]
I have reproduced this bug with Nightly 61.0a1 (2018-05-01) on Windows 10, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID   : 20180517141400   
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
QA Whiteboard: [good first verify] → [good first verify] [bugday-20180516]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.