Call ResetPendingTasks when we set null target for pending animations

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(6 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
(Assignee)

Comment 1

10 months ago
I did write more tests yesterday, it noticed me that there are other bunch of issue for PendingAnimationTracker.  I think this should block shipping setting target/effect features.
Blocks: 1398037
(Assignee)

Comment 3

9 months ago
It turned out the particular case in bug 1474213 comment 14 like this;

>   const anim = element.animate(...);
>   // Now play-pending
>   anim.effect.target = null;

This animation in PendingAnimationTracker will be discarded from TriggerPendingAnimations() in nsDisplayList.cpp, but if the presshell is suppressing paint (i.g. IsPaintingSuppressed() returns true), the animation persists in the next tick.  So anyway, I am going to discard eagerly here in this bug.

Also note that I am concerned that we flush layout [1] if there is any pending animations in the tracker, this might case some bad interactions with retained display list stuff,  I am totally unsure, but I do want to avoid the unnecessary explicit flush.

[1] https://hg.mozilla.org/mozilla-central/file/3edc9c3ae818/layout/base/nsRefreshDriver.cpp#l1957
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

9 months ago
mozreview-review
Comment on attachment 8991183 [details]
Bug 1474247 - Introduce nsDOMWindowUtils.isAnimationPending().

https://reviewboard.mozilla.org/r/256144/#review263030

Apart from the comments below this looks fine, but I should probably read through the whole patch series before setting r+ to make sure I understand why we need this.

::: dom/base/nsDOMWindowUtils.cpp:3810
(Diff revision 1)
> +NS_IMETHODIMP
> +nsDOMWindowUtils::IsAnimationPending(dom::Animation* aAnimation, bool* aRetVal)
> +{

MOZ_ASSERT that aRetVal is not null?

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1711
(Diff revision 1)
> +  /*
> +   * Returns true if the given animation is being tracked by the pending
> +   * animation tracker for the current document.
> +   */
> +  bool isAnimationPending(in Animation aAnimation);

Can we call this isAnimationInPendingTracker ?

The reason is that an animation can be pending but not in the tracker (as I'm sure you know).

Also, there should be a space after this.

Comment 14

9 months ago
mozreview-review
Comment on attachment 8991185 [details]
Bug 1474247 - Factor out Animation::ReschedulePendingAnimations and make it public.

https://reviewboard.mozilla.org/r/256148/#review263288

::: dom/animation/Animation.cpp
(Diff revision 1)
> -    // Reschedule pending pause or pending play tasks.
> -    // If we have a pending animation, it will either be registered
> -    // in the pending animation tracker and have a null pending ready time,
> -    // or, after it has been painted, it will be removed from the tracker
> -    // and assigned a pending ready time.
>      // After updating the effect we'll typically need to repaint so if we've
>      // already been assigned a pending ready time, we should clear it and put
>      // the animation back in the tracker.

I don't think this second part of the comment makes a lot of sense without the first part. Can we drop the comment here and move the whole thing to ReschedulePendingTasks, perhaps adapting it as follows:

    // Reschedule pending pause or pending play tasks when updating the target
    // effect.
    //
    // If we are pending, we will either be registered in the pending animation
    // tracker and have a null pending ready time, or, after it our effect has
    // been painted, we will be removed from the tracker and assigned a pending
    // ready time.
    //
    // When the target effect is updated, we'll typically need to repaint so for
    // the latter case where we already have a pending ready time, clear it and
    // put ourselves back in the pending animation tracker.
Attachment #8991185 - Flags: review?(bbirtles) → review+

Comment 15

9 months ago
mozreview-review
Comment on attachment 8991186 [details]
Bug 1474247 - Check mPendingState instead of mPendingReadyTime in Animation::ReschedulePendingTasks().

https://reviewboard.mozilla.org/r/256150/#review263292

r=me for the code changes although I'm still thinking about the best way to test this.

::: commit-message-dd3de:1
(Diff revision 1)
> +Bug 1474247 - Check mPendingState instead of mPemdingReadyTime in Animation::ReschedulePendingTasks(). r?birtles

mPendingReadyTime

::: dom/animation/Animation.cpp:1470
(Diff revision 1)
> -  if (mPendingReadyTime.IsNull()) {
> +  if (mPendingState == PendingState::NotPending) {
>      return;
>    }

(For my own notes, what this patch is doing is changing this method to simply ensure that the animation is in the pending animation tracker since it's possible we might not be in a tracker nor have a pending ready time.)

::: dom/animation/Animation.cpp:1477
(Diff revision 1)
>    if (doc) {
>      PendingAnimationTracker* tracker =
>        doc->GetOrCreatePendingAnimationTracker();
>      if (mPendingState == PendingState::PlayPending) {
>        tracker->AddPlayPending(*this);
>      } else {
>        tracker->AddPausePending(*this);
>      }
>    }

Does it make sense to call tracker->IsWaitingToPlay / IsWaitingToPause first to avoid the call to EnsurePaintIsScheduled in the case where we are already in the tracker?
Attachment #8991186 - Flags: review?(bbirtles) → review+

Comment 16

9 months ago
mozreview-review
Comment on attachment 8991190 [details]
Bug 1474247 - Call ReschedulePendingtasks when we set a new target to KeyframeEffect.

https://reviewboard.mozilla.org/r/256158/#review263298

This appears to be almost testable using the public API.

For example, we could do:

const effect = new KeyframeEffect(null, null);
const anim = new Animation(effect);
anim.play();
anim.effect.target = div;
anim.ready.then(() => {
  assert_less_than(anim.startTime, anim.timeline.currentTime,
    'The ready time from the pending animation tracker, not the'
    + ' timeline current time was used');
});

However, I think that due to vsync adjustments and timer clamping that assertion might sometimes fail. So maybe the way you wrote this is best.
Attachment #8991190 - Flags: review?(bbirtles) → review+

Comment 17

9 months ago
mozreview-review
Comment on attachment 8991183 [details]
Bug 1474247 - Introduce nsDOMWindowUtils.isAnimationPending().

https://reviewboard.mozilla.org/r/256144/#review263300

::: dom/base/nsDOMWindowUtils.cpp:3817
(Diff revision 1)
> +{
> +  if (!aAnimation) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  nsIDocument* doc = GetDocument();

Can this be null?
Attachment #8991183 - Flags: review?(bbirtles) → review+

Comment 18

9 months ago
mozreview-review
Comment on attachment 8991187 [details]
Bug 1474247 - Add a test case that the target element changed to another element in the same document.

https://reviewboard.mozilla.org/r/256152/#review263296

Is this patch actually necessary? I mean, does anything bad happen if we don't remove ourselves from the pending animation tracker of the old document?

Comment 19

9 months ago
mozreview-review
Comment on attachment 8991189 [details]
Bug 1474247 - Reset pending tasks in KeyframeEffect::SetTarget if no new target sets.

https://reviewboard.mozilla.org/r/256156/#review263302

Likewise with this patch, I'm not sure if this is actually necessary. Also, even if we do want to remove ourselves from the pending animation tracker, I don't think we want to reset pending tasks here.
Attachment #8991189 - Flags: review?(bbirtles)

Comment 20

9 months ago
mozreview-review
Comment on attachment 8991184 [details]
Bug 1474247 - Test cases for animations tracked by the pending animation tracker for Animation::SetEffect, Animation::Cancel() and .

https://reviewboard.mozilla.org/r/256146/#review263304

::: dom/animation/test/mozilla/test_pending_animation_tracker.html:14
(Diff revision 1)
> +test(t => {
> +  const target = addDiv(t);
> +  const anim = target.animate(null, 100 * MS_PER_SEC);
> +  assert_true(anim.pending, 'The animation should be pending');
> +  assert_true(SpecialPowers.DOMWindowUtils.isAnimationPending(anim),
> +              'The animation should be tracked by tracker');
> +
> +  anim.effect = null;
> +
> +  assert_false(anim.pending, 'The animation should not be pending');
> +  assert_false(SpecialPowers.DOMWindowUtils.isAnimationPending(anim),
> +               'The animation should NOT be tracked by the tracker');
> +}, 'Setting null effect removes the animation from the tracker');

As per [1], I want to change this behavior but I think it's ok for now.

Although, could we just make these tests test whether the animation is in the pending tracker or not and not testing the pending state too?

That way we won't have to update it when we change the behavior later.

[1] https://github.com/w3c/csswg-drafts/issues/2077#issuecomment-404383899

::: dom/animation/test/mozilla/test_pending_animation_tracker.html:76
(Diff revision 1)
> +
> +  target.remove();
> +
> +  assert_true(anim.pending, 'The animation should be still pending');
> +  assert_true(SpecialPowers.DOMWindowUtils.isAnimationPending(anim),
> +              'The animation is still bing tracked by the tracker');

s/bing/being/
Attachment #8991184 - Flags: review?(bbirtles) → review+

Comment 21

9 months ago
mozreview-review
Comment on attachment 8991188 [details]
Bug 1474247 - Call ReschedulePendingtasks when we set a new target to KeyframeEffect.

https://reviewboard.mozilla.org/r/256154/#review263306
Attachment #8991188 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 22

9 months ago
(In reply to Brian Birtles (:birtles) from comment #15)

> ::: dom/animation/Animation.cpp:1477
> (Diff revision 1)
> >    if (doc) {
> >      PendingAnimationTracker* tracker =
> >        doc->GetOrCreatePendingAnimationTracker();
> >      if (mPendingState == PendingState::PlayPending) {
> >        tracker->AddPlayPending(*this);
> >      } else {
> >        tracker->AddPausePending(*this);
> >      }
> >    }
> 
> Does it make sense to call tracker->IsWaitingToPlay / IsWaitingToPause first
> to avoid the call to EnsurePaintIsScheduled in the case where we are already
> in the tracker?

Sure, that makes sense, will do.

Comment 23

9 months ago
mozreview-review
Comment on attachment 8991191 [details]
Bug 1474247 - Remove the animation from the pending tracker if the new target element is in a different document.

https://reviewboard.mozilla.org/r/256160/#review263308

I think this is probably fine but I'm just a bit unsure if we need to do all this removing. The original design here was supposed to be "add and forget" but maybe we do need to do this after all.

I'd like to have another look at this after rebasing without the code where we reset pending tasks when the target is set to null.

::: dom/animation/KeyframeEffect.cpp:955
(Diff revision 1)
> +        nsIDocument* newRenderedDocument =
> +          newTarget->mElement->GetComposedDoc();
> +        if (newRenderedDocument &&
> +            newRenderedDocument != GetRenderedDocument()) {

I think this logic will change based on my feedback about not resetting tasks when the target is null so perhaps I should check this bit again after rebasing.
Attachment #8991191 - Flags: review?(bbirtles)
I think this is mostly fine and certainly fixes a few problems. I think I'm persuaded about added the DOMWindowUtils method and I think I might be persuaded about the active removal from the pending tracker although I'd like to hear your thoughts about that.

I've reviewed as much as I can for now and will wait for the updated patches to check the two patches I've yet to review.

One general bit of feedback is that it might be good to drop the checks for the 'pending' member from the tests since I think that behavior, while related, is not exactly what we need to test here, and will likely change. It's up to you though.
(Assignee)

Comment 25

9 months ago
(In reply to Brian Birtles (:birtles) from comment #18)
> Comment on attachment 8991187 [details]
> Bug 1474247 - Remove the animation from the pending animation tracker if the
> new effect's target element is in a different document.
> 
> https://reviewboard.mozilla.org/r/256152/#review263296
> 
> Is this patch actually necessary? I mean, does anything bad happen if we
> don't remove ourselves from the pending animation tracker of the old
> document?
(In reply to Brian Birtles (:birtles) from comment #19)
> Comment on attachment 8991189 [details]
> Bug 1474247 - Reset pending tasks in KeyframeEffect::SetTarget if no new
> target sets.
> 
> https://reviewboard.mozilla.org/r/256156/#review263302
> 
> Likewise with this patch, I'm not sure if this is actually necessary. Also,
> even if we do want to remove ourselves from the pending animation tracker, I
> don't think we want to reset pending tasks here.

These two patches what I *WAS* concerned in bug 1466010.  Actually these Web Animations API are not exposed in content yet, so it's not actually the case on youtube.com though, the PendingAnimationTracker for the old document keeps the pending animation which had been moved into the other document until the next tick happens, which means that we end up calling FlushPendingNotifications(Layout) in refresh driver's tick, in the worst case, the pending animation in the PendingAnimationTracker remains after the next tick if the paint is suppressed by the pres shell.   That's said, that's fair to drop these patches here in this bug since it's not related to the youtube case.
(Assignee)

Comment 26

9 months ago
(In reply to Brian Birtles (:birtles) from comment #20)
> Comment on attachment 8991184 [details]
> Bug 1474247 - Test cases for animations tracked by the pending animation
> tracker for Animation::SetEffect, Animation::Cancel() and .
> 
> https://reviewboard.mozilla.org/r/256146/#review263304
> 
> ::: dom/animation/test/mozilla/test_pending_animation_tracker.html:14
> (Diff revision 1)
> > +test(t => {
> > +  const target = addDiv(t);
> > +  const anim = target.animate(null, 100 * MS_PER_SEC);
> > +  assert_true(anim.pending, 'The animation should be pending');
> > +  assert_true(SpecialPowers.DOMWindowUtils.isAnimationPending(anim),
> > +              'The animation should be tracked by tracker');
> > +
> > +  anim.effect = null;
> > +
> > +  assert_false(anim.pending, 'The animation should not be pending');
> > +  assert_false(SpecialPowers.DOMWindowUtils.isAnimationPending(anim),
> > +               'The animation should NOT be tracked by the tracker');
> > +}, 'Setting null effect removes the animation from the tracker');
> 
> As per [1], I want to change this behavior but I think it's ok for now.
> 
> Although, could we just make these tests test whether the animation is in
> the pending tracker or not and not testing the pending state too?
> 
> That way we won't have to update it when we change the behavior later.
> 
> [1] https://github.com/w3c/csswg-drafts/issues/2077#issuecomment-404383899

Sure.  I will drop all the pending state check.  (The spec change makes sense to me)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8991189 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8991190 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8991191 - Attachment is obsolete: true
(Assignee)

Comment 33

9 months ago
mozreview-review
Comment on attachment 8991187 [details]
Bug 1474247 - Add a test case that the target element changed to another element in the same document.

https://reviewboard.mozilla.org/r/256152/#review263316

I think MozReview got confused about this patch.  (I think this patch already got r+)

Comment 35

9 months ago
mozreview-review
Comment on attachment 8991185 [details]
Bug 1474247 - Factor out Animation::ReschedulePendingAnimations and make it public.

https://reviewboard.mozilla.org/r/256148/#review263332

::: dom/animation/Animation.h:388
(Diff revision 2)
>    /**
> +   * Reschedule pending pause or pending play tasks when updating the target
> +   * effect.
> +   *
> +   * If we are pending, we will either be registered in the pending animation
> +   * tracker and have a null pending ready time, or, after it our effect has

s/it our effect/out effect/

Probably my typo. Sorry.

::: dom/animation/Animation.h:393
(Diff revision 2)
> +   * tracker and have a null pending ready time, or, after it our effect has
> +   * been painted, we will be removed from the tracker and assigned a pending
> +   * ready time.
> +   *
> +   * When the target effect is updated, we'll typically need to repaint so for
> +   * the later case where we already have a pending ready time, clear it and put

Sorry, this was probably my typo but: s/later/latter/

Comment 36

9 months ago
mozreview-review
Comment on attachment 8991186 [details]
Bug 1474247 - Check mPendingState instead of mPendingReadyTime in Animation::ReschedulePendingTasks().

https://reviewboard.mozilla.org/r/256150/#review263334

::: dom/animation/Animation.cpp:1477
(Diff revision 2)
> -    if (mPendingState == PendingState::PlayPending) {
> +    if (mPendingState == PendingState::PlayPending &&
> +        !tracker->IsWaitingToPlay(*this)) {
>        tracker->AddPlayPending(*this);
> -    } else {
> +    } else if (!tracker->IsWaitingToPause(*this)) {

I think this logic is wrong, isn't it?

If we are play-pending but AND in the tracker then won't this mean we get added as a pause-pending animation too?

Comment 37

9 months ago
mozreview-review
Comment on attachment 8991187 [details]
Bug 1474247 - Add a test case that the target element changed to another element in the same document.

https://reviewboard.mozilla.org/r/256152/#review263336
Attachment #8991187 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 38

9 months ago
(In reply to Brian Birtles (:birtles) from comment #36)
> Comment on attachment 8991186 [details]
> Bug 1474247 - Check mPendingState instead of mPendingReadyTime in
> Animation::ReschedulePendingTasks().
> 
> https://reviewboard.mozilla.org/r/256150/#review263334
> 
> ::: dom/animation/Animation.cpp:1477
> (Diff revision 2)
> > -    if (mPendingState == PendingState::PlayPending) {
> > +    if (mPendingState == PendingState::PlayPending &&
> > +        !tracker->IsWaitingToPlay(*this)) {
> >        tracker->AddPlayPending(*this);
> > -    } else {
> > +    } else if (!tracker->IsWaitingToPause(*this)) {
> 
> I think this logic is wrong, isn't it?
> 
> If we are play-pending but AND in the tracker then won't this mean we get
> added as a pause-pending animation too?

Gah error-prone.  Thanks for the catch. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> (In reply to Brian Birtles (:birtles) from comment #18)
> > Comment on attachment 8991187 [details]
> > Bug 1474247 - Remove the animation from the pending animation tracker if the
> > new effect's target element is in a different document.
> > 
> > https://reviewboard.mozilla.org/r/256152/#review263296
> > 
> > Is this patch actually necessary? I mean, does anything bad happen if we
> > don't remove ourselves from the pending animation tracker of the old
> > document?
> (In reply to Brian Birtles (:birtles) from comment #19)
> > Comment on attachment 8991189 [details]
> > Bug 1474247 - Reset pending tasks in KeyframeEffect::SetTarget if no new
> > target sets.
> > 
> > https://reviewboard.mozilla.org/r/256156/#review263302
> > 
> > Likewise with this patch, I'm not sure if this is actually necessary. Also,
> > even if we do want to remove ourselves from the pending animation tracker, I
> > don't think we want to reset pending tasks here.
> 
> These two patches what I *WAS* concerned in bug 1466010.

Sorry, my wording there wasn't clear. I was just trying to convince myself that it was necessary. If you want to file a separate bug to re-add those patches that would be fine.

(Although for this particular patch I was mostly concerned about the additional behavior where it seemed like we would cancel the pending tasks if the target effect was set to null. Maybe I misunderstood, but that seemed like an observable change that wasn't desirable.)
(Assignee)

Comment 44

9 months ago
(In reply to Brian Birtles (:birtles) from comment #43)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> > (In reply to Brian Birtles (:birtles) from comment #18)
> > > Comment on attachment 8991187 [details]
> > > Bug 1474247 - Remove the animation from the pending animation tracker if the
> > > new effect's target element is in a different document.
> > > 
> > > https://reviewboard.mozilla.org/r/256152/#review263296
> > > 
> > > Is this patch actually necessary? I mean, does anything bad happen if we
> > > don't remove ourselves from the pending animation tracker of the old
> > > document?
> > (In reply to Brian Birtles (:birtles) from comment #19)
> > > Comment on attachment 8991189 [details]
> > > Bug 1474247 - Reset pending tasks in KeyframeEffect::SetTarget if no new
> > > target sets.
> > > 
> > > https://reviewboard.mozilla.org/r/256156/#review263302
> > > 
> > > Likewise with this patch, I'm not sure if this is actually necessary. Also,
> > > even if we do want to remove ourselves from the pending animation tracker, I
> > > don't think we want to reset pending tasks here.
> > 
> > These two patches what I *WAS* concerned in bug 1466010.
> 
> Sorry, my wording there wasn't clear. I was just trying to convince myself
> that it was necessary. If you want to file a separate bug to re-add those
> patches that would be fine.
> 
> (Although for this particular patch I was mostly concerned about the
> additional behavior where it seemed like we would cancel the pending tasks
> if the target effect was set to null. Maybe I misunderstood, but that seemed
> like an observable change that wasn't desirable.)

Thanks! I will probably open a new bug if bug 1475155 didn't fix the crash on youtube.com (which means there is no other hope.).

Comment 45

9 months ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed9f66da4732
Introduce nsDOMWindowUtils.isAnimationPending(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/12b9745a2fd0
Test cases for animations tracked by the pending animation tracker for Animation::SetEffect, Animation::Cancel() and . r=birtles
https://hg.mozilla.org/integration/autoland/rev/2507e1b2e3a4
Factor out Animation::ReschedulePendingAnimations and make it public. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b55121332ceb
Check mPendingState instead of mPendingReadyTime in Animation::ReschedulePendingTasks(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/a79a5c81b3b5
Add a test case that the target element changed to another element in the same document. r=birtles
https://hg.mozilla.org/integration/autoland/rev/5eea82f8593d
Call ReschedulePendingtasks when we set a new target to KeyframeEffect. r=birtles
You need to log in before you can comment on or make changes to this bug.