1430975 - Don't need to reduce time precision for all animation event, it's needed only for animation cancel event

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

We've reduced the precision for the elapsed time of all animation event [1],  but as far as I can tell, the elapsed time is generally calculated from given parameters for animations properties, i.e. animation duration or delay etc. So it's not necessary to reduce them.  The one exception is animationcancel event, it's calculated from current time line time. So I think the place we need to call ReduceTimePrecisionAsSecs is here [2] in CSSAnimation::QueueEvents().

Brian, is that right?

[1] https://hg.mozilla.org/mozilla-central/file/b2cb61e83ac5/layout/style/nsAnimationManager.h#l58
[2] https://hg.mozilla.org/mozilla-central/file/b2cb61e83ac5/layout/style/nsAnimationManager.cpp#l261

Comment 1

[Brian Birtles (:birtles)]

Yes, looking at the tables in:

  https://drafts.csswg.org/css-animations-2/#event-dispatch
  https://drafts.csswg.org/css-transitions-2/#event-dispatch

It seems only animationcancel and transitioncancel use the current time.

Comment 2

[Brian Birtles (:birtles)]

(And on an unrelated note, with all your refactoring of event dispatch, please bear in mind that we want to fix bug 1354501 in the near future -- I hope all this refactoring helps us get closer to being able to sort events from CSS animations, CSS transitions, and from Web Animations at once.)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #1)
> Yes, looking at the tables in:
> 
>   https://drafts.csswg.org/css-animations-2/#event-dispatch
>   https://drafts.csswg.org/css-transitions-2/#event-dispatch
> 
> It seems only animationcancel and transitioncancel use the current time.

Oh dear.  The argument name for TransitionEventInfo is |aDuration|, I did miss the part.  So unfortunately we don't reduce it so far.

(In reply to Brian Birtles (:birtles) from comment #2)
> (And on an unrelated note, with all your refactoring of event dispatch,
> please bear in mind that we want to fix bug 1354501 in the near future -- I
> hope all this refactoring helps us get closer to being able to sort events
> from CSS animations, CSS transitions, and from Web Animations at once.)

Yeah, I am currently thinking we should unify DelayedEventDispatcher, I mean we should have a single DelayedEventDispatcher for each document, and the class should handle all the types of animation events.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c6186ad6b10243b41de3d462b59d756a1e2d4ed

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1430975 - Rename |aDuration| argument to |aElapsedTime|.

It's not actually duration.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1430975 - Don't pass a copy of StickyTimeDuration.

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1430975 - Make zero duration as a static constexpr.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1430975 - Reduce elapsed time precision only for animationcancel event.

Only animationcancel event uses current time, the elapsed time for other
animation events are simply calculated from given animation properties (e.g.
animation-duration, animation-delay, etc.).

See the table in https://drafts.csswg.org/css-animations-2/#event-dispatch

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1430975 - Reduce elapsed time precision for transitioncancel event.

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8943187 [details]
Bug 1430975 - Don't pass a copy of StickyTimeDuration.

https://reviewboard.mozilla.org/r/213536/#review219230

::: layout/style/nsAnimationManager.cpp:238
(Diff revision 1)
> -                                  StickyTimeDuration aElapsedTime,
> -                                  TimeStamp aTimeStamp) {
> +                                  const StickyTimeDuration& aElapsedTime,
> +                                  const TimeStamp& aTimeStamp) {

I have mended here before landing bug 1430924, but I forgot to push it. :/

Comment 11

[Tom Ritter [:tjr]]

Thank you for structuring the patches so well!

Something I am concerned about - our goal in rounding (clamping) timers is not just to reduce the precision of the current time of the system. We also want to disguise the precision of elapsed time between two events.

The ultimate goal of this effort is to prevent an attacker from having fine-grained timing information about an operation. So if animations providing fine-grained timing information - even if it was completely disconnected from what the actual system time was - then animations would be a vector to time the operation of interest.

So at first glance I think we do need to reduce the precision on all animation events, not just cancel.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Tom Ritter [:tjr] from comment #11)
> So if animations
> providing fine-grained timing information - even if it was completely
> disconnected from what the actual system time was - then animations would be
> a vector to time the operation of interest.

I don't quite understand this part.  Could you please elaborate the reason?  As I commented,  the elapsed time for animation events are just calculated from CSS property values, so we need to modify the values themselves?, say, animation-duration, animation-delay, animation-iterations, etc. etc?  If the 'calculation' is a problem, the calculated value may depend on system (the calculation uses std::min, I not really sure though, but I believe there no timing specific values), then we also need to modify getComputedStyle() values?

Comment 13

[Hiroyuki Ikezoe (:hiro)]

:tjr ^?

Comment 14

[Tom Ritter [:tjr]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> (In reply to Tom Ritter [:tjr] from comment #11)
> > So if animations
> > providing fine-grained timing information - even if it was completely
> > disconnected from what the actual system time was - then animations would be
> > a vector to time the operation of interest.
> 
> I don't quite understand this part.  Could you please elaborate the reason? 
> As I commented,  the elapsed time for animation events are just calculated
> from CSS property values, so we need to modify the values themselves?, say,
> animation-duration, animation-delay, animation-iterations, etc. etc?  

Hm. When no timer reduction is turned on, https://jsfiddle.net/jt7g2p41/1/ shows me that a 0s animation is taking a few tens of microseconds. But I can't actually turn that into a useful timer. A duration of .1 is ~.1 seconds, a duration of .01 seconds is the same as a duration of 0 seconds.

> If the
> 'calculation' is a problem, the calculated value may depend on system (the
> calculation uses std::min, I not really sure though, but I believe there no
> timing specific values), 

So what I'm imagining is something like this: https://jsfiddle.net/shtxy1p9/1/

I didn't spend a lot of time digging into the animations API to figure out how to make it really powerful, but the basic idea is that I start an animation, get a timestamp before the 'work' (represented by the while loop), do the work, and then get a timestamp afterwards. Subtract the two, and I get an approximation of the time it took to do the work.

Now it seems like elapsedTime (which I think is what you're modifying right?) isn't giving me any granularity (compared to .timeStamp). Like you said, it's coming from the animation-duration, etc. (I also briefly tested animation iteration: https://jsfiddle.net/0vfhscwq/1/ )

So... it's looking like I'm wrong. (I'm still nervous though)

> then we also need to modify getComputedStyle() values?

Hm. I'm not sure how that comes into this. 

Do you mean if I have an animation that goes from Red to Green, I could use getComputedStyle to determine the exact shade of color being shown, and reverse that into a fine-grained timestamp? If so, yea, ideally we would figure out how to obfuscate that as well, but for now I have not considered how to deal with implicit clocks, only with the explicit places we give times out.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Thanks for clarification!

(In reply to Tom Ritter [:tjr] from comment #14)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> > (In reply to Tom Ritter [:tjr] from comment #11)
> > > So if animations
> > > providing fine-grained timing information - even if it was completely
> > > disconnected from what the actual system time was - then animations would be
> > > a vector to time the operation of interest.
> > 
> > I don't quite understand this part.  Could you please elaborate the reason? 
> > As I commented,  the elapsed time for animation events are just calculated
> > from CSS property values, so we need to modify the values themselves?, say,
> > animation-duration, animation-delay, animation-iterations, etc. etc?  
> 
> Hm. When no timer reduction is turned on, https://jsfiddle.net/jt7g2p41/1/
> shows me that a 0s animation is taking a few tens of microseconds. But I
> can't actually turn that into a useful timer. A duration of .1 is ~.1
> seconds, a duration of .01 seconds is the same as a duration of 0 seconds.
> 
> > If the
> > 'calculation' is a problem, the calculated value may depend on system (the
> > calculation uses std::min, I not really sure though, but I believe there no
> > timing specific values), 
> 
> So what I'm imagining is something like this:
> https://jsfiddle.net/shtxy1p9/1/
> 
> I didn't spend a lot of time digging into the animations API to figure out
> how to make it really powerful, but the basic idea is that I start an
> animation, get a timestamp before the 'work' (represented by the while
> loop), do the work, and then get a timestamp afterwards. Subtract the two,
> and I get an approximation of the time it took to do the work.
> 
> Now it seems like elapsedTime (which I think is what you're modifying
> right?) isn't giving me any granularity (compared to .timeStamp). Like you
> said, it's coming from the animation-duration, etc. (I also briefly tested
> animation iteration: https://jsfiddle.net/0vfhscwq/1/ )
> 
> So... it's looking like I'm wrong. (I'm still nervous though)

Good to hear. :)

> > then we also need to modify getComputedStyle() values?
> 
> Hm. I'm not sure how that comes into this. 
> 
> Do you mean if I have an animation that goes from Red to Green, I could use
> getComputedStyle to determine the exact shade of color being shown, and
> reverse that into a fine-grained timestamp? If so, yea, ideally we would
> figure out how to obfuscate that as well, but for now I have not considered
> how to deal with implicit clocks, only with the explicit places we give
> times out.

I meant static values, like percentage or something, I mean system dependent values.  But.. yeah, animating value is likely to... It's another story as you mentioned but we should deeply consider it might expose timing specific information to some extent.

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8943186 [details]
Bug 1430975 - Rename |aDuration| argument to |aElapsedTime|.

https://reviewboard.mozilla.org/r/213534/#review219568

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8943187 [details]
Bug 1430975 - Don't pass a copy of StickyTimeDuration.

https://reviewboard.mozilla.org/r/213536/#review219570

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8943188 [details]
Bug 1430975 - Make zero duration as a static constexpr.

https://reviewboard.mozilla.org/r/213538/#review219582

::: layout/style/nsAnimationManager.cpp:196
(Diff revision 1)
>    nsPresContext* presContext = mOwningElement.GetPresContext();
>    if (!presContext) {
>      return;
>    }
>  
> -  const StickyTimeDuration zeroDuration;
> +  static const StickyTimeDuration zeroDuration = StickyTimeDuration();

nit:
constexpr ??

Comment 19

[Boris Chiou [:boris]]

Comment on attachment 8943189 [details]
Bug 1430975 - Reduce elapsed time precision only for animationcancel event.

https://reviewboard.mozilla.org/r/213540/#review219592

Comment 20

[Boris Chiou [:boris]]

Comment on attachment 8943190 [details]
Bug 1430975 - Reduce elapsed time precision for transitioncancel event.

https://reviewboard.mozilla.org/r/213542/#review219594

::: layout/style/nsTransitionManager.h:324
(Diff revision 1)
>      , mTimeStamp(aTimeStamp)
>    {
>      // XXX Looks like nobody initialize WidgetEvent::time
>      mEvent.mPropertyName =
>        NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(aProperty));
> -    mEvent.mElapsedTime = aElapsedTime.ToSeconds();
> +    mEvent.mElapsedTime = aElapsedTime;

Interesting, we don't call `ReduceTimePrecisionAsSecs(...)` in the current m-c. I assume the current m-c has this bug. :)

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #18)
> Comment on attachment 8943188 [details]
> Bug 1430975 - Make zero duration as a static constexpr.
> 
> https://reviewboard.mozilla.org/r/213538/#review219582
> 
> ::: layout/style/nsAnimationManager.cpp:196
> (Diff revision 1)
> >    nsPresContext* presContext = mOwningElement.GetPresContext();
> >    if (!presContext) {
> >      return;
> >    }
> >  
> > -  const StickyTimeDuration zeroDuration;
> > +  static const StickyTimeDuration zeroDuration = StickyTimeDuration();
> 
> nit:
> constexpr ??

Yes, indeed!  What did I do actually?? :/


(In reply to Boris Chiou [:boris] from comment #20)
> Comment on attachment 8943190 [details]
> Bug 1430975 - Reduce elapsed time precision for transitioncancel event.
> 
> https://reviewboard.mozilla.org/r/213542/#review219594
> 
> ::: layout/style/nsTransitionManager.h:324
> (Diff revision 1)
> >      , mTimeStamp(aTimeStamp)
> >    {
> >      // XXX Looks like nobody initialize WidgetEvent::time
> >      mEvent.mPropertyName =
> >        NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(aProperty));
> > -    mEvent.mElapsedTime = aElapsedTime.ToSeconds();
> > +    mEvent.mElapsedTime = aElapsedTime;
> 
> Interesting, we don't call `ReduceTimePrecisionAsSecs(...)` in the current
> m-c. I assume the current m-c has this bug. :)

That's right.  Brian noticed it to me in comment 1.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8943188 [details]
Bug 1430975 - Make zero duration as a static constexpr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213538/diff/1-2/

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8943189 [details]
Bug 1430975 - Reduce elapsed time precision only for animationcancel event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213540/diff/1-2/

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8943190 [details]
Bug 1430975 - Reduce elapsed time precision for transitioncancel event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213542/diff/1-2/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/890bae053e53
Rename |aDuration| argument to |aElapsedTime|. r=boris
https://hg.mozilla.org/integration/autoland/rev/bdfe507f5294
Don't pass a copy of StickyTimeDuration. r=boris
https://hg.mozilla.org/integration/autoland/rev/da6ed0639905
Make zero duration as a static constexpr. r=boris
https://hg.mozilla.org/integration/autoland/rev/3141e18a84e7
Reduce elapsed time precision only for animationcancel event. r=boris
https://hg.mozilla.org/integration/autoland/rev/bf3a9047ffca
Reduce elapsed time precision for transitioncancel event. r=boris

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/890bae053e53
https://hg.mozilla.org/mozilla-central/rev/bdfe507f5294
https://hg.mozilla.org/mozilla-central/rev/da6ed0639905
https://hg.mozilla.org/mozilla-central/rev/3141e18a84e7
https://hg.mozilla.org/mozilla-central/rev/bf3a9047ffca