Closed Bug 1467399 Opened 6 years ago Closed 6 years ago

Compositor sign is occasionally not correct when pause/resume

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1467399 - Part 1: Wait for next frame after pausing/resuming the animations.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1467399 - Part 2: Guard from removed animation during updating the animation state.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1467399 - Part 3: Modify a test for compositor sign.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
It seems the cause is bug 1456857. We introduced to use startTime from the bug so as to pause/resume.
Although we can not judge whether the animation is running on compositor or not after we have to wait next animation frame,  since the setting to startTime is sync, we are occasionally getting the state of same frame.  (Also, the document occasionally will be next frame during calling actor system.)

So as to resolve, in server, I'd like to try to introduce a function which waits the next frame.
Component: Developer Tools: Application Panel → Developer Tools: Animation Inspector
Product: Firefox → DevTools
Comment on attachment 8985392 [details]
Bug 1467399 - Part 1: Wait for next frame after pausing/resuming the animations.

https://reviewboard.mozilla.org/r/251014/#review257868

::: devtools/server/actors/animation.js:1004
(Diff revision 1)
>    },
> +
> +  /**
> +   * Wait for next animation frame.
> +   *
> +   * @param {Array} actors.

Missing `@return` jsdoc statement.

::: devtools/server/actors/animation.js:1007
(Diff revision 1)
> +    return Promise.all(
> +      actors.map(({ player }) => {

Can we split this in 2 to make it a little bit less nested please?

```
const promises = actors.map(...);
return Promise.all(promises);
```

::: devtools/server/actors/animation.js:1009
(Diff revision 1)
> +        const node = player.effect.target.ownerDocument
> +                       ? player.effect.target : player.effect.target.parentElement;
> +        const doc = node.ownerDocument;

There is a `window` getter, as well as a `node` getter on the `AnimationPlayerActor` class. Can't we use those instead of having to re-encode some complex logic to get a document when passed with an actor? It would be good to move this complexity where it belongs.
Attachment #8985392 - Flags: review?(pbrosset)
Comment on attachment 8985393 [details]
Bug 1467399 - Part 2: Guard from removed animation during updating the animation state.

https://reviewboard.mozilla.org/r/251016/#review257874

::: devtools/client/inspector/animation/animation.js:634
(Diff revision 1)
>      return new Promise((resolve, reject) => {
>        let count = 0;
>        let error = null;
>  
>        for (const animation of animations) {
>          animation.refreshState().catch(e => {
>            error = e;
>          }).finally(() => {
>            count += 1;
>  
> -          if (count === animations.length) {
> +          if (count !== animations.length) {
> +            return;
> +          }
> +
> -            if (error) {
> +          if (error) {
> -              reject(error);
> +            reject(error);
> -            } else {
> +          } else {
> -              resolve();
> -            }
> +            // Even when removal animation on inspected document, updateAnimations
> +            // might be called before onAnimationsMutation due to the async timing.
> +            // Return the animations as result of updateAnimations after getting rid of
> +            // the animations since they should not display.
> +            resolve(animations.filter(anim => !!anim.state.type));
>            }
>          });
>        }
>      });

This would be much easier to read if you made `updateAnimations` an async/await function.
Right now the `return` statement when `count` is not at `animations.length` yet feels weird, but in fact it's just to `continue` the `for` loop.
Could you please refactor this function?
Attachment #8985393 - Flags: review?(pbrosset)
Comment on attachment 8985394 [details]
Bug 1467399 - Part 3: Modify a test for compositor sign.

https://reviewboard.mozilla.org/r/251018/#review257876
Attachment #8985394 - Flags: review?(pbrosset) → review+
Indeed! All your comments are very reasonable, I thought. I will fix them.
Thanks Patrick!
Comment on attachment 8985392 [details]
Bug 1467399 - Part 1: Wait for next frame after pausing/resuming the animations.

https://reviewboard.mozilla.org/r/251014/#review258182

Much clearer. Thank you Daisuke!
Attachment #8985392 - Flags: review?(pbrosset) → review+
Comment on attachment 8985393 [details]
Bug 1467399 - Part 2: Guard from removed animation during updating the animation state.

https://reviewboard.mozilla.org/r/251016/#review258186

::: devtools/client/inspector/animation/animation.js:632
(Diff revision 2)
> -      for (const animation of animations) {
> +    const promises = animations.map(animation => {
> +      return new Promise(resolve => {
>          animation.refreshState().catch(e => {
>            error = e;
>          }).finally(() => {
> -          count += 1;
> -
> -          if (count === animations.length) {
> -            if (error) {
> -              reject(error);
> -            } else {
> -              resolve();
> +          resolve();
> -            }
> -          }
>          });
> -      }
> -    });
> +      });
> +    });
> +    await Promise.all(promises);

This would look much nicer as a `for` loop. Something like:

```
for (const animation of animations) {
  await animation.refreshState();
}
```

Also you wouldn't need to catch the error and then throw it. You'd just let the default JS error handling mechanism take care of it.

*But* there would be one difference I think. Here you're *not* waiting for each promise to resolve or reject before refreshing the state of the next animation.
And my understanding is that `Promise.all` races all promises.
So if that is an issue you want to avoid, i.e. if you want all of these `refreshState` calls to race, then forget about my proposal.
Attachment #8985393 - Flags: review?(pbrosset)
Comment on attachment 8985393 [details]
Bug 1467399 - Part 2: Guard from removed animation during updating the animation state.

https://reviewboard.mozilla.org/r/251016/#review258186

> This would look much nicer as a `for` loop. Something like:
> 
> ```
> for (const animation of animations) {
>   await animation.refreshState();
> }
> ```
> 
> Also you wouldn't need to catch the error and then throw it. You'd just let the default JS error handling mechanism take care of it.
> 
> *But* there would be one difference I think. Here you're *not* waiting for each promise to resolve or reject before refreshing the state of the next animation.
> And my understanding is that `Promise.all` races all promises.
> So if that is an issue you want to avoid, i.e. if you want all of these `refreshState` calls to race, then forget about my proposal.

Thanks Patrick!

Yeah, I changed to current code in bug 1445291 which resolved "no actor" error.
I had written originally as follows:

```
async updateAnimations(animations) {
  const promises = animations.map(animation => {
    return animation.refreshState();
  });
  
  await Promise.all(promises);    
```

However, in the above code, if one of the refreshState had error, Promise.all call reject(). Then we got that error from other refreshState which does not get response yet. So as to resolve this, I have written to wait for all of response of refreshState().
Also, it takes time a bit if we wait refreshState() one by one as you suggested.

What do you think?
Comment on attachment 8985393 [details]
Bug 1467399 - Part 2: Guard from removed animation during updating the animation state.

https://reviewboard.mozilla.org/r/251016/#review258198
Attachment #8985393 - Flags: review+
Thanks!
I make them to land!
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4dbd6590b235
Part 1: Wait for next frame after pausing/resuming the animations. r=pbro
https://hg.mozilla.org/integration/autoland/rev/ba634b003ba1
Part 2: Guard from removed animation during updating the animation state. r=pbro
https://hg.mozilla.org/integration/autoland/rev/ff4b9d0d9216
Part 3: Modify a test for compositor sign. r=pbro
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: