1291413 - Assertion failure: mHoldTime.IsNull() != mStartTime.IsNull() at dom/animation/Animation.cpp:994

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

Assertion failure: mHoldTime.IsNull() != mStartTime.IsNull() (An animation in the play-pending state should have either a resolved hold time or resolved start time (but not both)), at dom/animation/Animation.cpp:994

Stack:
    #0 0x7f20cd295ee2 in mozilla::dom::Animation::ResumeAt(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator> const&) dom/animation/Animation.cpp:990:3
    #1 0x7f20cd2931ee in mozilla::dom::Animation::Tick() dom/animation/Animation.cpp:520:5
    #2 0x7f20cd29ed61 in mozilla::dom::DocumentTimeline::WillRefresh(mozilla::TimeStamp) dom/animation/DocumentTimeline.cpp:163:5
    #3 0x7f20d0865d56 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1712:7
    #4 0x7f20d0865d56 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1712:7

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: stack

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Still reproduces on trunk.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Unravelling this test case based on what the behavior *should* be according to the spec:

> var a=e.animate([],{duration:1000,delay:150});

Animation is play-pending.
Hold time: 0
Start time: null

> setTimeout(boom,500,a);

Animation starts playback and is no longer play-pending.

After ~500ms:
Hold time: null
Start time: resolved (timeline time - ~500ms)
Calculated current time: ~350ms

> a.pause();

Animation becomes pause-pending.
No change to timing parameters.

> a.reverse();

Playback rate: -1

Silently set current time to ~350ms:
  -> Start time: resolved (timeline time + ~350ms)

Play an animation:
  Aborted paused = true
  Pause pending task is cancelled
  Resume task scheduled

i.e. at this point:

Hold time: null
Start time: resolved (timeline time + ~350ms)

> a.playbackRate=0;

Previous current time = ~350ms
Playback rate: 0

Set the current time to ~350ms
  -> Hold time: ~350ms

i.e. at this point:

Hold time: resolved ~350ms
Start time: resolved (timeline time + ~350ms)

> Resume task runs

PANIC: Both hold time and start time are set!


In this case I think the assertion is in error. It is normal for an animation with a zero playback rate to have *both* hold time and start time set.

The assertion was likely added[1] because of the following logic:

  if (mStartTime.IsNull()) {
    mStartTime = StartTimeFromReadyTime(aReadyTime);
    if (mPlaybackRate != 0) {
      mHoldTime.SetNull();
    }
  }

The thinking being that if both are set how can we know if it's ok to overwrite the mHoldTime? However, in the case of the mPlaybackRate being zero we don't overwrite the mHoldTime so this is ok.

I plan to simply update the assertion (after turning the test case into a suitable crashtest).

[1] http://searchfox.org/mozilla-central/diff/1060d42f7b2546054085facfad5bfcfbd5f4d149/dom/animation/AnimationPlayer.cpp#526

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hmm, even if we update the assertion to allow a zero playback rate we can create a related crashtest that will fail by simply changing the line:

> a.playbackRate=0;

to:

> a.playbackRate=0;
> a.playbackRate=1;

The reason is that when we update the playbackRate the second time we'll fail to clear the hold time.

That seems like a spec issue. Specifically the procedure to "silently set the current time" probably needs to identify when we are able to calculate an updated start time and clear the hold time in that case.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Specifically the procedure to "silently set
> the current time" probably needs to identify when we are able to calculate
> an updated start time and clear the hold time in that case.

However, it's not clear if that's the correct behavior for the "finished animation" case. It might be better to make the change in "Updating the playback rate of an animation" to make it clear the start time when we transition from zero playback rate in the middle of a play-pending task.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

After looking into this a bit further, I realize that the "update the finished state" task actually does clear the hold time as required. The problem is it doesn't do this when there is a pending task (as is the case here).

What we really need is for that behavior to still apply in the aborted pause case.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thinking about this further still, I'm not sure there's a problem after all. I think leaving both the hold and start time set until the pending play task runs is fine.

One tweak we could make, however, is that when we have *both* a resolved start and hold time, we could prefer using the hold time to calculate the start time. That would perform better.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1291413 - Fix assertion when resuming an Animation with both a start time and hold time;

When we set the playback rate to zero on a play-pending animation that is
resuming from an aborted pause we can arrive in a state where both the start
time and hold time are resolved. However, we previously added an assertion that
only one of these is ever set at a time.

Part of the assertion is warranted since that method contains the following
code:

  if (mStartTime.IsNull()) {
    mStartTime = StartTimeFromReadyTime(aReadyTime);
    if (mPlaybackRate != 0) {
      mHoldTime.SetNull();
    }
  }

Here StartTimeFromReadyTime requires a non-null hold time. So either mStartTime
or mHoldTime needs to be non-null. The requirement that only one or the other be
non-null, however, is not in the spec and not necessary (as the test cases in
this bug show).

What this assertion does bring to light, however, is that in the case where we
have *both* the start time and the hold time, we need to consider whether to use
the start time as-is, or calculate it from the hold time.

I have filed the following spec issue for this:

  https://github.com/w3c/web-animations/issues/200

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8915398 [details]
Bug 1291413 - Fix assertion when resuming an Animation with both a start time and hold time;

https://reviewboard.mozilla.org/r/186590/#review191676

::: dom/animation/Animation.cpp:1191
(Diff revision 1)
>    // This method is only expected to be called for an animation that is
>    // waiting to play. We can easily adapt it to handle other states
>    // but it's currently not necessary.
>    MOZ_ASSERT(mPendingState == PendingState::PlayPending,
>               "Expected to resume a play-pending animation");
> -  MOZ_ASSERT(mHoldTime.IsNull() != mStartTime.IsNull(),
> +  MOZ_ASSERT(!mHoldTime.IsNull() || !mStartTime.IsNull(),

Given that the fact you described in the commit messsage, i.e. if mStartTime is null then StartTimeFormReadyTime which requires non-null mHoldTime is called, I'd prefer to check !mStartTime.IsNull() first in this assertion.  It's just a my preference though.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yeah, I originally did that but then I tried to make the diff as minimal as possible so it was easier to see what changed, so I decided to keep the original order.

I'll update the order as you suggest and the attached assertion message too.

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

On further thought, I think I'll stick with the minimal diff.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/843d4559d591
Fix assertion when resuming an Animation with both a start time and hold time; r=hiro

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/843d4559d591