Closed Bug 1424948 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: hiro)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:timing])

Attachments

(2 files)

I am pretty sure this was  caused by bug 1424644.
Blocks: 1424644
Flags: needinfo?(hikezoe)
I did put a wrong  bug number.
Blocks: 1416966
No longer blocks: 1424644
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
Flags: needinfo?(hikezoe)
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8936816 [details]
Bug 1424948 - Use waitForNextFrame instead of waitForFrame.

https://reviewboard.mozilla.org/r/207516/#review213424
Attachment #8936816 - Flags: review?(bbirtles) → review+
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.)
Attachment #8936817 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7e5bc4db7137
https://hg.mozilla.org/mozilla-central/rev/375327ad5708
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
As far as I can tell there is no new failures the fix has landed.
Whiteboard: [stockwell fixed:timing]
You need to log in before you can comment on or make changes to this bug.