Closed Bug 1472076 Opened 6 years ago Closed 6 years ago

Update animation state and do relevant stuff whenever the refresh driver's time is changed

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files)

Bug 1472076 - Introduce nsATimerAdjustmentObserver in nsRefreshDriver.
59 bytes, text/x-review-board-request
birtles
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1472076 - Update animations when the refresh driver's time changed due to the active timer changes.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
This is another potential fix for bug 1466010.

As we (:mattwoodrow, :birtles and I) discussed on IRC, it's possible that IsCurrent() and IsInEffect() change the return value during a single tick.  That's because when the refresh driver changes to the active timer, the most recent refresh time might be changed, thus it's possible that the time is advanced a bit.  We should prevent such cases somehow.  Matt suggested me to pass the time value to the functions instead of getting the time stamp from the refresh driver each time we call the functions.

Here is a try with the approach;

https://treeherder.mozilla.org/#/jobs?repo=try&revision=797f0f5a141495fa434624256eb30782bc9a9ffc

I did initially tweak DocumentTimelint::GetCurrentTimeStamp directly, but it didn't work as expected, since that one is called without animations (i.e. script can call document.timeline.currentTime with documents having no animations, that means that WillRefresh() is not called at all).  This problem can probably be solved by introducing another TimeStamp variable in DocumentTimeStamp for animations, but instead, I did introduce another variant of DocumentTimeline::GetCurrentTime() there (named GetCurrentTimeForAnimation).
Hmm, I start thinking document.timeline.currentTime shouldn't change the return value during the same tick.
An annoying point to make the document.timeline.currentTime stable on the same tick, it means we skip calling nsRefreshDriver::MostRecentRefresh() in DocumentTimeline::GetCurrentTimeStamp(), but it seems CSS animations/transitions events rely on this call, I should have finished bug 1354501. :/
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> This is another potential fix for bug 1466010.
> 
> As we (:mattwoodrow, :birtles and I) discussed on IRC, it's possible that
> IsCurrent() and IsInEffect() change the return value during a single tick. 
> That's because when the refresh driver changes to the active timer, the most
> recent refresh time might be changed, thus it's possible that the time is
> advanced a bit.  We should prevent such cases somehow.  Matt suggested me to
> pass the time value to the functions instead of getting the time stamp from
> the refresh driver each time we call the functions.
> 
> Here is a try with the approach;
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=797f0f5a141495fa434624256eb30782bc9a9ffc

I make a mistake there (I did forget to add '!').

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4504f3b34dccced601754af850e3b6be6811af5
This is tricky. I think it would be nice to avoid having a separate time for animations than what we report via the API (since it will surely introduce other bugs).

Can we detect when the refresh driver time has changed and notify animations accordingly? (That is, if we must have two separate times to avoid dispatching events, then I'd prefer to make the event time the special case and then call a function that does some subset of the work Tick does--but simply skips events.)

But yes, we should fix bug 1354501!
(In reply to Brian Birtles (:birtles) from comment #4)
> This is tricky. I think it would be nice to avoid having a separate time for
> animations than what we report via the API (since it will surely introduce
> other bugs).
> 
> Can we detect when the refresh driver time has changed and notify animations
> accordingly?

I believe we can do so.  That is the exactly what initially I though of.  After I was told the approach in comment 0 by Matt, I thought it's probably simpler than we detect the refresh driver's time changed.  But... actually I think I can't do easier fix without bug 1354501. :/

I will try to the other approach.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=087e9638c4553b9fefb1de53548db27901320577

I couldn't name a good one for the notification function, but it should work!
Summary: Ensure IsCurrent() and IsInEffect() are unchanged during a single tick → Update animation state and do relevant stuff whenever the refresh driver's time is changed
Priority: -- → P2
Comment on attachment 8988886 [details]
Bug 1472076 - Introduce nsATimerAdjustmentObserver in nsRefreshDriver.

https://reviewboard.mozilla.org/r/254040/#review260870

::: commit-message-6041c:11
(Diff revision 1)
> +The reason why we have another observer array in parallel with existing array
> +(mObservers) is to avoid enumerating whole existing array and calling the new
> +virtual function since most of observers don't need the new notification at
> +all.

Thanks for the explanation. It feels a little bit odd to register two types of observers using the one method (AddRefreshObserver). It raises questions like, should the ObserverCount() count these twice? And it feels a little bit fragile--we might introduce bugs in future if we deal with one array but forget the other.

So, just at a first glance, it seems to me like we should either:

a) Let AddRefreshObserver store the observer once, but store a tuple for each
   observer: observer + types of refreshes they're subscribed to?

   You would still need to iterate the whole array but that wouldn't increase
   the O complexity. Also, you wouldn't need to call a virtual method since
   you'd just check the refresh types it's listening for.

b) Have a separate callback for registering for these, e.g.
   Add/RemoveTimerAdjustmentObserver.

   We already have a bunch of similar orthogonal observers like this:

   Add/RemovePostRefreshObserver
   Add/RemoveResizeEventFlushObserver
   Add/RemoveStyleFlushObserver etc.
   etc. etc.


Fleshing out (a) I think it might have an API something like this:

// The reason for which WillRefresh is being called.
enum class RefreshType {
  Tick,
  TimerAdjustment,
}

// (Note we'd only call this for observers who registered to listen to timer
// adjustments.)
virtual void WillRefresh(mozilla::TimeStamp aTime, RefreshType aRefreshType);

// (i.e. no separate NotifyTimeAdvanced callback.)

using RefreshTypes = EnumSet<RefreshTypes>;

bool AddRefreshObserver(
  nsARefreshObserver *aObserver,
  mozilla::FlushType aFlushType,
  RefreshTypes aRefreshTypes = RefreshTypes(RefreshType::Tick)
);
bool RemoveRefreshObserver(nsARefreshObserver *aObserver,
                           mozilla::FlushType aFlushType);
// (i.e. no need to pass aRefreshTypes to RemoveRefreshObserver)

I guess there will be a bit of work to make observer arrays store tuples however.
Thinking about this further, I lean more towards (a) since, supposing we have more call sites that want to listen to timer adjustments, we should ideally preserve the same ordering we call them in between regular ticks and timer adjustments and I think (a) lets us do that (since presumably when we handle timer adjustments we will iterate over all 4 arrays and call them in the same order).
(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #9)
> Comment on attachment 8988886 [details]
> Bug 1472076 - Introduce NotifyTimeAdvanced() into nsARefreshObserver.
> 
> https://reviewboard.mozilla.org/r/254040/#review260870
> 
> ::: commit-message-6041c:11
> (Diff revision 1)
> > +The reason why we have another observer array in parallel with existing array
> > +(mObservers) is to avoid enumerating whole existing array and calling the new
> > +virtual function since most of observers don't need the new notification at
> > +all.
> 
> Thanks for the explanation. It feels a little bit odd to register two types
> of observers using the one method (AddRefreshObserver). It raises questions
> like, should the ObserverCount() count these twice?

Thanks.  This question noticed me an important fact.  The observers for this shouldn't be counted in ObserverCount(), since we don't normally stop the timer if ObserverCount() returns 0.  So,

> b) Have a separate callback for registering for these, e.g.
>    Add/RemoveTimerAdjustmentObserver.

We should take this b).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from
> comment #9)
> > Comment on attachment 8988886 [details]
> > Bug 1472076 - Introduce NotifyTimeAdvanced() into nsARefreshObserver.
> > 
> > https://reviewboard.mozilla.org/r/254040/#review260870
> > 
> > ::: commit-message-6041c:11
> > (Diff revision 1)
> > > +The reason why we have another observer array in parallel with existing array
> > > +(mObservers) is to avoid enumerating whole existing array and calling the new
> > > +virtual function since most of observers don't need the new notification at
> > > +all.
> > 
> > Thanks for the explanation. It feels a little bit odd to register two types
> > of observers using the one method (AddRefreshObserver). It raises questions
> > like, should the ObserverCount() count these twice?
> 
> Thanks.  This question noticed me an important fact.  The observers for this
> shouldn't be counted in ObserverCount(), since we don't normally stop the
> timer if ObserverCount() returns 0.  So,

if ObserverCount() *doesn't* returns 0.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from
> > comment #9)
> > > Comment on attachment 8988886 [details]
> > > Bug 1472076 - Introduce NotifyTimeAdvanced() into nsARefreshObserver.
> > > 
> > > https://reviewboard.mozilla.org/r/254040/#review260870
> > > 
> > > ::: commit-message-6041c:11
> > > (Diff revision 1)
> > > > +The reason why we have another observer array in parallel with existing array
> > > > +(mObservers) is to avoid enumerating whole existing array and calling the new
> > > > +virtual function since most of observers don't need the new notification at
> > > > +all.
> > > 
> > > Thanks for the explanation. It feels a little bit odd to register two types
> > > of observers using the one method (AddRefreshObserver). It raises questions
> > > like, should the ObserverCount() count these twice?
> > 
> > Thanks.  This question noticed me an important fact.  The observers for this
> > shouldn't be counted in ObserverCount(), since we don't normally stop the
> > timer if ObserverCount() returns 0.  So,
> 
> if ObserverCount() *doesn't* returns 0.

Can you explain?
(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #15)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > > (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from
> > > comment #9)
> > > > Comment on attachment 8988886 [details]
> > > > Bug 1472076 - Introduce NotifyTimeAdvanced() into nsARefreshObserver.
> > > > 
> > > > https://reviewboard.mozilla.org/r/254040/#review260870
> > > > 
> > > > ::: commit-message-6041c:11
> > > > (Diff revision 1)
> > > > > +The reason why we have another observer array in parallel with existing array
> > > > > +(mObservers) is to avoid enumerating whole existing array and calling the new
> > > > > +virtual function since most of observers don't need the new notification at
> > > > > +all.
> > > > 
> > > > Thanks for the explanation. It feels a little bit odd to register two types
> > > > of observers using the one method (AddRefreshObserver). It raises questions
> > > > like, should the ObserverCount() count these twice?
> > > 
> > > Thanks.  This question noticed me an important fact.  The observers for this
> > > shouldn't be counted in ObserverCount(), since we don't normally stop the
> > > timer if ObserverCount() returns 0.  So,
> > 
> > if ObserverCount() *doesn't* returns 0.
> 
> Can you explain?

The correct sentence is 'HasObservers()' instead of ObserverCount() though.
We do stop the timer in nsRefreshDriver::Tick() if HasObservers() returns false, that means that if the timer adjust observer is counted in HasObservers(), it interferes stopping the timer.
I don't follow why that implies (b) is necessary? Can't you do (a) but require refresh observers observe ticks?
(In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from comment #17)
> I don't follow why that implies (b) is necessary? Can't you do (a) but
> require refresh observers observe ticks?

Of course we can but can we avoid the mistake the new timer adjustment observer class didn't observe refresh driver tick?  (We can do it with assertions in AddXXObserver, but it needs an assumption which one should be called first).  To be honest, I don't have any clear ideas how other observers use this new observer, and if someone wants to observe without normal observers, I don't know..
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> (In reply to Brian Birtles (:birtles, traveling 29 June - 6 July) from
> comment #17)
> > I don't follow why that implies (b) is necessary? Can't you do (a) but
> > require refresh observers observe ticks?
> 
> Of course we can but can we avoid the mistake the new timer adjustment
> observer class didn't observe refresh driver tick?  (We can do it with
> assertions in AddXXObserver, but it needs an assumption which one should be
> called first). 

Never mind about this.  I was confused with the new Add/Remove methods.
Comment on attachment 8988886 [details]
Bug 1472076 - Introduce nsATimerAdjustmentObserver in nsRefreshDriver.

https://reviewboard.mozilla.org/r/254040/#review261184

This seems ok to me but I'd like someone else who is more familiar with nsRefreshDriver to do an additional review.

::: layout/base/nsRefreshDriver.h:77
(Diff revision 2)
> + * timer changes. Callers must ensure an observer is removed before it is
> + * destroyed.
> + */
> +class nsATimerAdjustmentObserver {
> +public:
> +  NS_INLINE_DECL_PURE_VIRTUAL_REFCOUNTING

Do we need this? Or should we let the caller be responsible for lifetime management?

::: layout/base/nsRefreshDriver.h:530
(Diff revision 2)
> +  // Note: These observers should NOT be counted in HasObservers() since they
> +  // should exist exactly when we stop the timers, freeze or something like
> +  // that.

Does this comment make sense:

"These observers should NOT be included in HasObservers() since that method is used to determine whether or not to stop the timer, or restore it when thawing the refresh driver. On the other hand these observers are intended to be called when the timer is re-started and should not influence its starting or stopping."

[ It seems a little odd to include these observers in ObserverCount but NOT include it in HasObservers. I wonder if we should rename HasObservers to NeedsTicks()? ]

::: layout/base/nsRefreshDriver.cpp:1420
(Diff revision 2)
>                 mMostRecentRefreshEpochTime);
> +
> +  if (mMostRecentRefresh != newMostRecentRefresh) {
> +    mMostRecentRefresh = newMostRecentRefresh;
> +
> +    nsTObserverArray<nsATimerAdjustmentObserver*>::ForwardIterator

Should this be EndLimitedIterator?

::: layout/base/nsRefreshDriver.cpp:1481
(Diff revision 2)
>           !mAnimationEventFlushObservers.IsEmpty() ||
>           !mResizeEventFlushObservers.IsEmpty() ||
>           !mPendingEvents.IsEmpty() ||
>           !mFrameRequestCallbackDocs.IsEmpty() ||
>           !mThrottledFrameRequestCallbackDocs.IsEmpty() ||
>           !mEarlyRunners.IsEmpty();

Perhaps add a comment here as well to say, "We should NOT count mTimerAdjustmentObservers here since this this method is used to determine whether or not to stop the timer or re-start it and timer adjustment observers should not influence timer starting or stopping."
Attachment #8988886 - Flags: review?(bbirtles) → review+
Comment on attachment 8988887 [details]
Bug 1472076 - Update animations when the refresh driver's time changed due to the active timer changes.

https://reviewboard.mozilla.org/r/254042/#review261188

::: dom/animation/DocumentTimeline.cpp:224
(Diff revision 2)
> +
> +void
> +DocumentTimeline::ObserveRefreshDriver(nsRefreshDriver* aDriver)
> +{
> +  MOZ_ASSERT(!mIsObservingRefreshDriver);
> +  // Set the flag before calling AddRefreshObserver since the function might

s/Set the flag/Set the mIsObservingRefreshDriver flag/
s/since the function/since it/
s/, thus/which calls/

::: layout/base/nsRefreshDriver.h:531
(Diff revision 2)
>    mozilla::TimeStamp mNextRecomputeVisibilityTick;
>  
>    // separate arrays for each flush type we support
>    ObserverArray mObservers[4];
>    // Note: These observers should NOT be counted in HasObservers() since they
> -  // should exist exactly when we stop the timers, freeze or something like
> +  // should exist exactly when we stop/freeze the timer.

(I suspect this change was supposed to be part of the underlying patch.)
Attachment #8988887 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1788c8f229c1
Introduce nsATimerAdjustmentObserver in nsRefreshDriver. r=birtles
https://hg.mozilla.org/integration/autoland/rev/01f4eab134da
Update animations when the refresh driver's time changed due to the active timer changes. r=birtles
https://hg.mozilla.org/mozilla-central/rev/1788c8f229c1
https://hg.mozilla.org/mozilla-central/rev/01f4eab134da
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8988886 [details]
Bug 1472076 - Introduce nsATimerAdjustmentObserver in nsRefreshDriver.

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: Crash on beta
[Is this code covered by automated tests?]: No, unfortunately
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: It would be nice if someone can reproduce the crash bug 1466010 reliably, the most reliable way to reproduce the crash is to make youtube video fullscreen, this bug should fix the crash on youtube
[List of other uplifts needed for the feature/fix]: No, just needs two patches in this bug
[Is the change risky?]: Low
[Why is the change risky/not risky?]: I did carefully add this stuff not to affect existing parts,  the stuff works only in very rare conditions.
[String changes made/needed]: None
Attachment #8988886 - Flags: approval-mozilla-beta?
See Also: → 1473172
It looks like we are still seeing the crashes from bug 1466010 in Nightly builds after this landed on Nightly - even crashes on YouTube.  Do you think that this might be different on beta 62 and fix the crashes there?  We could try it speculatively on beta if you think it might work even though it may not have worked in Nightly.
Andrei, can your team try to help out here by reproducing the crash(es) from bug 1466010? Thanks!
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Flags: needinfo?(andrei.vaida) → needinfo?(roxana.leitan)
I believe this is one of the cause of bug 1466010, but I now think this doesn't fix the youtube case.  The case on youtube is caused by other reason(s) we are not aware of.
I didn't manage to reproduce the crash on Windows 10 x64 (Windows build 16299 and 17134), Ubuntu 16.04 x64 and Mac OS x 10.12, on Firefox Nightly 62.0a1 (2018-06-01) and (2018-05-15) 2018-06-16) or Firefox Beta 62.0b5. I tested on youtube videos ( full screen/theatre mode and browser full-screen F11) and with NVDA enabled/disabled.

Turning "gfx.webrender.enabled" I reproduced the following crash: https://crash-stats.mozilla.com/report/index/4a4ab4f1-dd11-42fc-8cfd-d034d0180709 covered in bug 1452513.

Should I continue the investigation on this?
Flags: needinfo?(roxana.leitan) → needinfo?(hikezoe)
Comment on attachment 8988886 [details]
Bug 1472076 - Introduce nsATimerAdjustmentObserver in nsRefreshDriver.

Speculative crash fix, let's uplift for beta 7.
Attachment #8988886 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to roxana.leitan@softvision.ro from comment #31)
> I didn't manage to reproduce the crash on Windows 10 x64 (Windows build
> 16299 and 17134), Ubuntu 16.04 x64 and Mac OS x 10.12, on Firefox Nightly
> 62.0a1 (2018-06-01) and (2018-05-15) 2018-06-16) or Firefox Beta 62.0b5. I
> tested on youtube videos ( full screen/theatre mode and browser full-screen
> F11) and with NVDA enabled/disabled.
> 
> Turning "gfx.webrender.enabled" I reproduced the following crash:
> https://crash-stats.mozilla.com/report/index/4a4ab4f1-dd11-42fc-8cfd-
> d034d0180709 covered in bug 1452513.
> 
> Should I continue the investigation on this?

No, it takes too much time.  Thanks roxana.
Flags: needinfo?(hikezoe)
Gave this qe verify bug another spin on Win 10/Win8.1 using Fx 62.0b3 20180625141512 and Fx 62.0b8 20180712042337. I've added to the STR from comment 31 different streaming sites, multiple windows/ dual screens, different hardware and driver versions machines and still no luck reproducing the crash. 

Given the above + comment 34, I'm removing the qe+ flag for the time being.
Let's add it back if the crash count on beta with this signature is still present or/and new details on its  reproducibility are found.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: