Closed Bug 1147491 Opened 5 years ago Closed 5 years ago

Slowing down an animation using animationPlayer.playbackRate crashes the tab

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
firefox44 --- verified

People

(Reporter: pbro, Assigned: jwatt)

References

Details

Attachments

(2 files)

STR:

- visit https://bgrins.github.io/devtools-demos/inspector/animation-timing.html
- open the devtools webconsole (ctrl/cmd+alt+K)
- enter:
document.querySelector("#box1").getAnimationPlayers()[0].playbackRate = 10
==> This works fine, the animation speeds up

- enter:
document.querySelector("#box1").getAnimationPlayers()[0].playbackRate = 0.5
==> BOOM! The tab crashes.
Blocks: 1120339
So in a debug build I get:

Assertion failure: aDivisor != 0 (Division by zero), at ../../dist/include/mozilla/TimeStamp.h:211
#01: mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>::operator/(long long) const[/Users/bzbarsky/mozilla/inbound/obj-firefox/dist/NightlyDebug.app/Contents/MacOS/XUL +0x15874b3]
#02: mozilla::dom::AnimationPlayer::SilentlySetCurrentTime(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator> const&)[/Users/bzbarsky/mozilla/inbound/obj-firefox/dist/NightlyDebug.app/Contents/MacOS/XUL +0x17cfd8f]
#03: mozilla::dom::AnimationPlayer::SetCurrentTime(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator> const&)[/Users/bzbarsky/mozilla/inbound/obj-firefox/dist/NightlyDebug.app/Contents/MacOS/XUL +0x17cfdff]
#04: mozilla::dom::AnimationPlayer::SetPlaybackRate(double)[/Users/bzbarsky/mozilla/inbound/obj-firefox/dist/NightlyDebug.app/Contents/MacOS/XUL +0x17cfee6]

The relevant code in AnimationPlayer::SilentlySetCurrentTime is:

112         mStartTime.SetValue(mTimeline->GetCurrentTime().Value() -
113                               (aSeekTime / mPlaybackRate));

here aSeekTime is a TimeDuration and the issue is that TimeDuration's operator/ takes an _integer_ argument.  So passing in 0.5 ends up passing floor(0.5), so 0.

This code has looked like this since bug 1127380 landed.

It seems like we should use aSeekTime.MultDouble(1 / mPlaybackRate) or something?
Flags: needinfo?(jwatt)
Assignee: nobody → jwatt
Flags: needinfo?(jwatt)
Thanks for the helpful debug notes.
Attachment #8586236 - Flags: review?(ehsan)
Comment on attachment 8586231 [details] [diff] [review]
part 1 - Fix playbackRate crash due to integer rounding causing divide-by-zero

>diff --git a/dom/animation/AnimationPlayer.cpp b/dom/animation/AnimationPlayer.cpp
>--- a/dom/animation/AnimationPlayer.cpp
>+++ b/dom/animation/AnimationPlayer.cpp
>@@ -101,17 +101,17 @@ AnimationPlayer::SilentlySetCurrentTime(
>       mPlaybackRate == 0.0
>       /*or, once supported, if we have a pending pause task*/) {
>     mHoldTime.SetValue(aSeekTime);
>     if (!mTimeline || mTimeline->GetCurrentTime().IsNull()) {
>       mStartTime.SetNull();
>     }
>   } else {
>     mStartTime.SetValue(mTimeline->GetCurrentTime().Value() -
>-                          (aSeekTime / mPlaybackRate));
>+                          (aSeekTime.MultDouble(1 / mPlaybackRate)));


If mPlaybackRate were *actually* zero here, then we'd still divide by 0 and run into trouble here.  Can you assert that it's nonzero here? (That's a real question; I'm not sure if we know it's nonzero).  If so, great; if not, we need to handle the 0 case.

(The other two chunks are fine; they've got explicit mPlaybackRate!=0 checks protecting this division.)
Comment on attachment 8586231 [details] [diff] [review]
part 1 - Fix playbackRate crash due to integer rounding causing divide-by-zero

Sorry, I got the logic confused; if mPlaybackRate is 0, we'll take the first case, and we won't hit this code. So, we should be fine. (and an assertion would probably be overkill given the "if" condition)

r=me
Attachment #8586231 - Flags: review?(dholbert) → review+
Attachment #8586236 - Flags: review?(ehsan) → review+
I was able to reproduce this issue on Mac OS X 10.10.5 on Nightly 39.0a1(2015-03-29) using STR from Comment 0.
The issue is no longer reproducible on Mac OS x 10.10.5, Ubuntu 14.04 x86 and Windows 10 x86 , using Latest Nightly 44.0a1 (2015-10-25) and STR from Comment 0 with the updated functions: 
- document.querySelector("#box1").getAnimations()[0].playbackRate = 10
- document.querySelector("#box1").getAnimations()[0].playbackRate = 0.5
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.