1442150 - Enable test_running_on_compositor.html due to bug 1193394

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

It seems to start failing on Android.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=821ea90bc82a42f6df93b3aeded41b1dfe163a4f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=165108651

running on the compositor or not: assert_equals: Animation reports that it is running on the compositor during playback at 0ms expected true but got false

I will disable it on Android now. And will look at the failure reason after landing bug 1193394.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Disable it on Android

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8955037 [details] [diff] [review]
Disable it on Android

arai, please use this patch when landing the microtask change. Thanks!

Comment 3

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0f18ca10ec79c11a91091c501fd56e8d551f30
Bug 1442150 - Skip test_running_on_compositor.html on Android. r=arai

Comment 4

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5a0f18ca10ec

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Oops, I didn't set leave-open.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

The test has been disabled.  Reuse this bug for enabling it.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I have to check why the test failed on Android.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Checking the animation started at the current time fixes the failure [1].  I hope it needs only in the first test case.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1cdd0e9d9a1deb6f9d4040914160cb8275aadae

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1442150 - Rewrite test_running_on_compositor.html with async/await.

Review commit: https://reviewboard.mozilla.org/r/224948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224948/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1442150 - Move startsRightNow into testcommon.js and rename it to animationStartsRightNow.

Review commit: https://reviewboard.mozilla.org/r/224950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224950/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

Review commit: https://reviewboard.mozilla.org/r/224952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224952/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1442150 - Re-enable test_running_on_compositor.html on Android.

Now the test works fine with the new microtask handling.

Review commit: https://reviewboard.mozilla.org/r/224954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224954/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdd2e331ef291144ce295f354692244ea48ad765

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956005 [details]
Bug 1442150 - Rewrite test_running_on_compositor.html with async/await.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224948/diff/1-2/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956006 [details]
Bug 1442150 - Move startsRightNow into testcommon.js and rename it to animationStartsRightNow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224950/diff/1-2/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956007 [details]
Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224952/diff/1-2/

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956008 [details]
Bug 1442150 - Re-enable test_running_on_compositor.html on Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224954/diff/1-2/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956005 [details]
Bug 1442150 - Rewrite test_running_on_compositor.html with async/await.

https://reviewboard.mozilla.org/r/224948/#review231098

::: dom/animation/test/chrome/test_running_on_compositor.html:187
(Diff revision 2)
>    var div = addDiv(t, { style: 'animation: anim 100s' });
>    var animation = div.getAnimations()[0];
>  
> -  return waitForPaints().then(() => {
> -    return new Promise(resolve => {
> +  await waitForPaints();
> +
> +  await new Promise(resolve => {

Note that I did ask arai on IRC that whether we should do 'await new Promise' or 'return new Promise' if it's the final Promise in promise_test, the answer was 'await', so I did use it here and there.

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8956005 [details]
Bug 1442150 - Rewrite test_running_on_compositor.html with async/await.

https://reviewboard.mozilla.org/r/224948/#review231130

Thank you!

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8956006 [details]
Bug 1442150 - Move startsRightNow into testcommon.js and rename it to animationStartsRightNow.

https://reviewboard.mozilla.org/r/224950/#review231132

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8956007 [details]
Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

https://reviewboard.mozilla.org/r/224952/#review231138

::: dom/animation/test/chrome/test_running_on_compositor.html:71
(Diff revision 2)
> +  await animation.ready;
> +
> +  // If the animation starts at the current timeline time, we need to wait for
> +  // one more frame.
> +  if (animationStartsRightNow(animation)) {
> +    await waitForFrame();
> +  }
> +
>    await waitForPaints();
>  
>    assert_animation_is_running_on_compositor(animation,
>       'Animation reports that it is running on the compositor'
>       + ' during playback');

I don't quite understand this.

We send animations to the compositor as part of painting so it shouldn't matter whether or not the main thread is doing its vsync-fixup step or not, right?

Could you explain the failure case a bit more?

Does assert_animation_is_running_on_compositor look at the main thread state? Or does the compositor also suffer from the "vsync makes the time go backwards" thing?

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8956008 [details]
Bug 1442150 - Re-enable test_running_on_compositor.html on Android.

https://reviewboard.mozilla.org/r/224954/#review231140

Comment 23

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #21)
> Comment on attachment 8956007 [details]
> Bug 1442150 - Wait for one more frame if the animation starts at the current
> timeline time in the first test case.
> 
> https://reviewboard.mozilla.org/r/224952/#review231138
> 
> ::: dom/animation/test/chrome/test_running_on_compositor.html:71
> (Diff revision 2)
> > +  await animation.ready;
> > +
> > +  // If the animation starts at the current timeline time, we need to wait for
> > +  // one more frame.
> > +  if (animationStartsRightNow(animation)) {
> > +    await waitForFrame();
> > +  }
> > +
> >    await waitForPaints();
> >  
> >    assert_animation_is_running_on_compositor(animation,
> >       'Animation reports that it is running on the compositor'
> >       + ' during playback');
> 
> I don't quite understand this.
> 
> We send animations to the compositor as part of painting so it shouldn't
> matter whether or not the main thread is doing its vsync-fixup step or not,
> right?

Yes, that's right.  In this failure case I think it's somewhat related to the fake MozAfterPaint (bug 1439875).
What I am thinking is that when the animation started at the current timeline time, we skiped styling for the animation, at that time, we received the fake MozAfterPaint so that we don't yet send the animation to the compositor yet.

Anyway, as far as I can tell a proper solution is to land bug 1439875.

Comment 24

[Brian Birtles (:birtles)]

Ah, ok. If we're pretty sure that's the issue, could we add a comment to the patch to explain that?

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Sure will do.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956005 [details]
Bug 1442150 - Rewrite test_running_on_compositor.html with async/await.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224948/diff/2-3/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956006 [details]
Bug 1442150 - Move startsRightNow into testcommon.js and rename it to animationStartsRightNow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224950/diff/2-3/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956007 [details]
Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224952/diff/2-3/

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956008 [details]
Bug 1442150 - Re-enable test_running_on_compositor.html on Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224954/diff/2-3/

Comment 30

[Brian Birtles (:birtles)]

Comment on attachment 8956007 [details]
Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

https://reviewboard.mozilla.org/r/224952/#review231152

::: dom/animation/test/chrome/test_running_on_compositor.html:71
(Diff revision 3)
> +  await animation.ready;
> +
> +  // If the animation starts at the current timeline time, we need to wait for
> +  // one more frame to avoid receiving the fake timer-based MozAfterPaint event.
> +  // FIXME: Bug 1419226: Drop this 'waitForFrame'.  Once MozAfterPaint is fired
> +  // reliably, we just need to wait for a MozAfterPaint here.

Shouldn't this comment go before the `await animation.ready` too?

i.e. normally we only need to wait for paints to finish, right?

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #30)
> Comment on attachment 8956007 [details]
> Bug 1442150 - Wait for one more frame if the animation starts at the current
> timeline time in the first test case.
> 
> https://reviewboard.mozilla.org/r/224952/#review231152
> 
> ::: dom/animation/test/chrome/test_running_on_compositor.html:71
> (Diff revision 3)
> > +  await animation.ready;
> > +
> > +  // If the animation starts at the current timeline time, we need to wait for
> > +  // one more frame to avoid receiving the fake timer-based MozAfterPaint event.
> > +  // FIXME: Bug 1419226: Drop this 'waitForFrame'.  Once MozAfterPaint is fired
> > +  // reliably, we just need to wait for a MozAfterPaint here.
> 
> Shouldn't this comment go before the `await animation.ready` too?
> 
> i.e. normally we only need to wait for paints to finish, right?

Oh right. Good catch!  Thanks!

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956007 [details]
Bug 1442150 - Wait for one more frame if the animation starts at the current timeline time in the first test case.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224952/diff/3-4/

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8956008 [details]
Bug 1442150 - Re-enable test_running_on_compositor.html on Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224954/diff/3-4/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e277cff709f
Rewrite test_running_on_compositor.html with async/await. r=birtles
https://hg.mozilla.org/integration/autoland/rev/532ce5be3ae2
Move startsRightNow into testcommon.js and rename it to animationStartsRightNow. r=birtles
https://hg.mozilla.org/integration/autoland/rev/cbd57b06f2f1
Wait for one more frame if the animation starts at the current timeline time in the first test case. r=birtles
https://hg.mozilla.org/integration/autoland/rev/614d32cc3555
Re-enable test_running_on_compositor.html on Android. r=birtles

Comment 35

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/6e277cff709f
https://hg.mozilla.org/mozilla-central/rev/532ce5be3ae2
https://hg.mozilla.org/mozilla-central/rev/cbd57b06f2f1
https://hg.mozilla.org/mozilla-central/rev/614d32cc3555