Closed Bug 1291413 Opened 3 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox51 --- wontfix
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: truber, Assigned: birtles)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file 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
Attached file stack
Flags: in-testsuite?
Priority: -- → P3
Blocks: 1334591
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
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
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.
(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.
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.
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 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.
Attachment #8915398 - Flags: review?(hikezoe) → review+
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.
On further thought, I think I'll stick with the minimal diff.
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/843d4559d591
Fix assertion when resuming an Animation with both a start time and hold time; r=hiro
https://hg.mozilla.org/mozilla-central/rev/843d4559d591
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.