1424948 - Intermittent dom/animation/test/css-transitions/test_animation-pausing.html | pause() and play() a transition - pause() and play() a transition: assert_greater_than: margin-left increases after calling play() expected a no greater than 18.7 but got 18.7

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: ebalazs [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=151363954&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/PDgEvXxAQJCKe7jHuRtP2g/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I am pretty sure this was  caused by bug 1424644.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I did put a wrong  bug number.

Comment 3

[Intermittent Failures Robot]

23 failures in 92 pushes (0.25 failures/push) were associated with this bug yesterday.    

Repository breakdown:
* autoland: 11
* mozilla-inbound: 7
* mozilla-central: 3
* try: 2

Platform breakdown:
* android-4-3-armv7-api16: 10
* windows7-32: 4
* osx-10-10: 3
* linux64: 3
* android-api-16-gradle: 2
* linux32-stylo-disabled: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1424948&startday=2017-12-13&endday=2017-12-13&tree=all

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I was wondering why this failure happens without the Bevis' patch for the conformant Promise handling.

The failure actually happens after waiting for frame[1];

    return animation.ready.then(waitForFrame);
  })).then(t.step_func(function() {
    assert_greater_than(getMarginLeft(cs), previousAnimVal,
                        'margin-left increases after calling play()');

If the micro task in waitForFrame happens soon due to the new Promise handling, the assert_greater_than fails since we are still in the same tick.  So in any way we should use waitForNextFrame there. (I did miss this part in bug 1423078) But the Bevis' patch has not landed yet.

Finally I noticed that even if mPendingReadyTime is not clamped to the timeline current time, if the mPendingReadyTime is pretty close to the current time, then it's possible that the margin-left value becomes the same value in the previous tick.  So we should check iteration progress instead of margin-left value anyway.

[1] https://hg.mozilla.org/mozilla-central/file/8062887ff0d9/dom/animation/test/css-transitions/file_animation-pausing.html#l42

Comment 5

[Hiroyuki Ikezoe (:hiro)]

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1424948 - Use waitForNextFrame instead of waitForFrame.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1424948 - Check iteration progress value instead of animating computed style value.

It is possible that animating computed style becomes the same value as the
value in the previos tick if Animation.mPendingReadyTime is pretty close
to the timeline current time.

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

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8936816 [details]
Bug 1424948 - Use waitForNextFrame instead of waitForFrame.

https://reviewboard.mozilla.org/r/207516/#review213424

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8936817 [details]
Bug 1424948 - Check iteration progress value instead of animating computed style value.

https://reviewboard.mozilla.org/r/207518/#review213426

::: commit-message-a3f38:4
(Diff revision 1)
> +Bug 1424948 - Check iteration progress value instead of animating computed style value. r?birtles
> +
> +It is possible that animating computed style becomes the same value as the
> +value in the previos tick if Animation.mPendingReadyTime is pretty close

s/previos/previous/

::: dom/animation/test/css-transitions/file_animation-pausing.html:19
(Diff revision 1)
> -  assert_equals(getMarginLeft(cs), 0,
> -                'Initial value of margin-left is zero');
> -  var previousAnimVal = getMarginLeft(cs);
> +  assert_equals(animation.effect.getComputedTiming().progress, 0,
> +                'Initial value of progress is zero');
> +  var previousProgress = animation.effect.getComputedTiming().progress;

(Nit: If we swapped the order of these two lines we could just compare previousProgress with 0 which might be a little simpler.)

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8936817 [details]
Bug 1424948 - Check iteration progress value instead of animating computed style value.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e5bc4db7137
Use waitForNextFrame instead of waitForFrame. r=birtles
https://hg.mozilla.org/integration/autoland/rev/375327ad5708
Check iteration progress value instead of animating computed style value. r=birtles

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7e5bc4db7137
https://hg.mozilla.org/mozilla-central/rev/375327ad5708

Comment 13

[Intermittent Failures Robot]

56 failures in 423 pushes (0.132 failures/push) were associated with this bug in the last 7 days. 

This is the #13 most frequent failure this week.  

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **  

Repository breakdown:
* autoland: 28
* mozilla-inbound: 19
* mozilla-central: 6
* try: 3

Platform breakdown:
* android-4-3-armv7-api16: 19
* android-api-16-gradle: 17
* windows7-32: 6
* linux64: 5
* osx-10-10: 3
* linux32-stylo-disabled: 3
* linux64-stylo-disabled: 2
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1424948&startday=2017-12-11&endday=2017-12-17&tree=all

Comment 14

[Hiroyuki Ikezoe (:hiro)]

As far as I can tell there is no new failures the fix has landed.