1278485 - "Assertion failure: IsFinite(progress) (Progress value should be finite)" with DOM animation, easing: cubic-bezier

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: IsFinite(progress) (Progress value should be finite), at dom/animation/KeyframeEffect.cpp:363

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Hiroyuki Ikezoe (:hiro)]

It turns out the progress value was NaN there.  We should avoid return the NaN.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51308d860ebef12a7f123a94b62bb5f263d49ee6
I did mark a wrong bug number in commit message...

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1278485 - Part 1: Limit y1 and y2 control points for cubic-bezier to avoid overflows.

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

Comment 5

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

Comment on attachment 8764755 [details]
Bug 1278485 - Part 1: Limit y1 and y2 control points for cubic-bezier to avoid overflows.

https://reviewboard.mozilla.org/r/60100/#review57354

::: dom/animation/KeyframeEffect.cpp:363
(Diff revision 1)
>    // Apply the easing.
>    if (aTiming.mFunction) {
>      progress = aTiming.mFunction->GetValue(progress, result.mBeforeFlag);
>    }
>  
> -  MOZ_ASSERT(IsFinite(progress), "Progress value should be finite");
> +  MOZ_ASSERT(!IsNaN(progress), "Progress value should be a valid number");

Progress should always be finite, right? And IsFinite should return false for NaN, right?

(If progress can be infinite, we will need to update the spec and WebIDL.)

::: dom/smil/nsSMILKeySpline.cpp:64
(Diff revision 1)
> +  // We should return zero if aT is zero to avoid returning NaN.
> +  if (aT == 0) {
> +    return 0;
> +  }

How does the function below end up returning NaN?

Also, do we need something similar for aT == 1?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/effect-easing.html:397
(Diff revision 1)
> +  // The cubic function yields zero * -infinity, the result
> +  // should be zero.

What about it yields zero * -infinity? I think I don't understand this comment.

(Also, zero * infinity does not equal zero.)

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/effect-easing.html:401
(Diff revision 1)
> +
> +  // The cubic function yields zero * -infinity, the result
> +  // should be zero.
> +  assert_equals(anim.effect.getComputedTiming().progress, 0,
> +                "The result of calculation which causes -infinity " +
> +                "multiplied by zeo should be zero");

Nit: s/zeo/zero/ here and below.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> Comment on attachment 8764755 [details]
> Bug 1278485 - Avoid calculations of positive/negative infinity multiplied by
> zero in nsSMILKeySpline::CalcBezier.
> 
> https://reviewboard.mozilla.org/r/60100/#review57354
> 
> ::: dom/animation/KeyframeEffect.cpp:363
> (Diff revision 1)
> >    // Apply the easing.
> >    if (aTiming.mFunction) {
> >      progress = aTiming.mFunction->GetValue(progress, result.mBeforeFlag);
> >    }
> >  
> > -  MOZ_ASSERT(IsFinite(progress), "Progress value should be finite");
> > +  MOZ_ASSERT(!IsNaN(progress), "Progress value should be a valid number");
> 
> Progress should always be finite, right? And IsFinite should return false
> for NaN, right?
> (If progress can be infinite, we will need to update the spec and WebIDL.)

Actually in the test case, some variables in nsSMILKeySpline::CalcBezier were overflowed.
As a result, nsSMILKeySpline::CalcBezier returned NaN because it calculated 0 * "double's" infinity.
So, it's not *real* infinity there, it's just a value we can not handle properly.

> ::: dom/smil/nsSMILKeySpline.cpp:64
> (Diff revision 1)
> > +  // We should return zero if aT is zero to avoid returning NaN.
> > +  if (aT == 0) {
> > +    return 0;
> > +  }
> 
> How does the function below end up returning NaN?

As I described above, A(aA1, aA2), B(aA1, aA2) or C(aA1) can be positive/negative infinity of double.

> Also, do we need something similar for aT == 1?

I don't think nsSMILKeySpline::CalcBezier does return NaN in such case.

> :::
> testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/effect-
> easing.html:397
> (Diff revision 1)
> > +  // The cubic function yields zero * -infinity, the result
> > +  // should be zero.
> 
> What about it yields zero * -infinity? I think I don't understand this
> comment.
> 
> (Also, zero * infinity does not equal zero.)

I meant the 'infinity' is a value over the boundary of double.

Oh, I checked now chromium behavior.  I did confirm they validate easing function parameters and throw an exception like this:

 Uncaught TypeError: Failed to execute 'animate' on 'Element': 'cubic-bezier(0, -1e+308, 0, 0)' is not a valid value for easing

Should we follow them?

Comment 7

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > Comment on attachment 8764755 [details]
> > Bug 1278485 - Avoid calculations of positive/negative infinity multiplied by
> > zero in nsSMILKeySpline::CalcBezier.
> > 
> > https://reviewboard.mozilla.org/r/60100/#review57354
> > 
> > ::: dom/animation/KeyframeEffect.cpp:363
> > (Diff revision 1)
> > >    // Apply the easing.
> > >    if (aTiming.mFunction) {
> > >      progress = aTiming.mFunction->GetValue(progress, result.mBeforeFlag);
> > >    }
> > >  
> > > -  MOZ_ASSERT(IsFinite(progress), "Progress value should be finite");
> > > +  MOZ_ASSERT(!IsNaN(progress), "Progress value should be a valid number");
> > 
> > Progress should always be finite, right? And IsFinite should return false
> > for NaN, right?
> > (If progress can be infinite, we will need to update the spec and WebIDL.)
> 
> Actually in the test case, some variables in nsSMILKeySpline::CalcBezier
> were overflowed.
> As a result, nsSMILKeySpline::CalcBezier returned NaN because it calculated
> 0 * "double's" infinity.
> So, it's not *real* infinity there, it's just a value we can not handle
> properly.

Ok, so is this change still necessary? If we make GetValue() return a 0 in that case, IsFinite() should still be correct, right? And should fail if GetValue() returns NaN, right?

> > Also, do we need something similar for aT == 1?
> 
> I don't think nsSMILKeySpline::CalcBezier does return NaN in such case.

Does it correctly return 1?

> > :::
> > testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/effect-
> > easing.html:397
> > (Diff revision 1)
> > > +  // The cubic function yields zero * -infinity, the result
> > > +  // should be zero.
> > 
> > What about it yields zero * -infinity? I think I don't understand this
> > comment.
> > 
> > (Also, zero * infinity does not equal zero.)
> 
> I meant the 'infinity' is a value over the boundary of double.

I wonder if this is an implementation detail and should be put in a 'mozilla' test?

> Oh, I checked now chromium behavior.  I did confirm they validate easing
> function parameters and throw an exception like this:
> 
>  Uncaught TypeError: Failed to execute 'animate' on 'Element':
> 'cubic-bezier(0, -1e+308, 0, 0)' is not a valid value for easing
> 
> Should we follow them?

Are they failing to parse it? Or are they actually evaluating the curve and ensuring it is within some limits?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > (In reply to Brian Birtles (:birtles) from comment #5)
> > > Comment on attachment 8764755 [details]
> > > Bug 1278485 - Avoid calculations of positive/negative infinity multiplied by
> > > zero in nsSMILKeySpline::CalcBezier.
> > > 
> > > https://reviewboard.mozilla.org/r/60100/#review57354
> > > 
> > > ::: dom/animation/KeyframeEffect.cpp:363
> > > (Diff revision 1)
> > > >    // Apply the easing.
> > > >    if (aTiming.mFunction) {
> > > >      progress = aTiming.mFunction->GetValue(progress, result.mBeforeFlag);
> > > >    }
> > > >  
> > > > -  MOZ_ASSERT(IsFinite(progress), "Progress value should be finite");
> > > > +  MOZ_ASSERT(!IsNaN(progress), "Progress value should be a valid number");
> > > 
> > > Progress should always be finite, right? And IsFinite should return false
> > > for NaN, right?
> > > (If progress can be infinite, we will need to update the spec and WebIDL.)
> > 
> > Actually in the test case, some variables in nsSMILKeySpline::CalcBezier
> > were overflowed.
> > As a result, nsSMILKeySpline::CalcBezier returned NaN because it calculated
> > 0 * "double's" infinity.
> > So, it's not *real* infinity there, it's just a value we can not handle
> > properly.
> 
> Ok, so is this change still necessary? If we make GetValue() return a 0 in
> that case, IsFinite() should still be correct, right? And should fail if
> GetValue() returns NaN, right?

Ah, right.  GetValue should have returned 0 in that case.

> > > Also, do we need something similar for aT == 1?
> > 
> > I don't think nsSMILKeySpline::CalcBezier does return NaN in such case.
> 
> Does it correctly return 1?

I did not check it at all but I think it returns positive or negative infinity.

> > > testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/effect-
> > > easing.html:397
> > > (Diff revision 1)
> > > > +  // The cubic function yields zero * -infinity, the result
> > > > +  // should be zero.
> > > 
> > > What about it yields zero * -infinity? I think I don't understand this
> > > comment.
> > > 
> > > (Also, zero * infinity does not equal zero.)
> > 
> > I meant the 'infinity' is a value over the boundary of double.
> 
> I wonder if this is an implementation detail and should be put in a
> 'mozilla' test?
> 
> > Oh, I checked now chromium behavior.  I did confirm they validate easing
> > function parameters and throw an exception like this:
> > 
> >  Uncaught TypeError: Failed to execute 'animate' on 'Element':
> > 'cubic-bezier(0, -1e+308, 0, 0)' is not a valid value for easing
> > 
> > Should we follow them?
> 
> Are they failing to parse it? Or are they actually evaluating the curve and
> ensuring it is within some limits?

Yes, failed to parse. 
https://chromium.googlesource.com/chromium/src/+/42dd9774c080e909f2d0646b3282b2d666b99ca2/third_party/WebKit/Source/core/animation/AnimationInputHelpers.cpp#247

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > > Oh, I checked now chromium behavior.  I did confirm they validate easing
> > > function parameters and throw an exception like this:
> > > 
> > >  Uncaught TypeError: Failed to execute 'animate' on 'Element':
> > > 'cubic-bezier(0, -1e+308, 0, 0)' is not a valid value for easing
> > > 
> > > Should we follow them?
> > 
> > Are they failing to parse it? Or are they actually evaluating the curve and
> > ensuring it is within some limits?
> 
> Yes, failed to parse. 
> https://chromium.googlesource.com/chromium/src/+/
> 42dd9774c080e909f2d0646b3282b2d666b99ca2/third_party/WebKit/Source/core/
> animation/AnimationInputHelpers.cpp#247

Oops, I should not that I did check in the parser code, but I could not find what is the limit value of it.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > > > Oh, I checked now chromium behavior.  I did confirm they validate easing
> > > > function parameters and throw an exception like this:
> > > > 
> > > >  Uncaught TypeError: Failed to execute 'animate' on 'Element':
> > > > 'cubic-bezier(0, -1e+308, 0, 0)' is not a valid value for easing
> > > > 
> > > > Should we follow them?
> > > 
> > > Are they failing to parse it? Or are they actually evaluating the curve and
> > > ensuring it is within some limits?
> > 
> > Yes, failed to parse. 
> > https://chromium.googlesource.com/chromium/src/+/
> > 42dd9774c080e909f2d0646b3282b2d666b99ca2/third_party/WebKit/Source/core/
> > animation/AnimationInputHelpers.cpp#247
> 
> Oops, I should not that I did check in the parser code, but I could not find
> what is the limit value of it.

should *note*

Comment 11

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> (In reply to Brian Birtles (:birtles) from comment #7)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > > (In reply to Brian Birtles (:birtles) from comment #5)
> > > > Comment on attachment 8764755 [details]
> > > > Bug 1278485 - Avoid calculations of positive/negative infinity multiplied by
> > > > zero in nsSMILKeySpline::CalcBezier.
> > > > 
> > > > https://reviewboard.mozilla.org/r/60100/#review57354
> > > > 
> > > > ::: dom/animation/KeyframeEffect.cpp:363
> > > > (Diff revision 1)
> > > > >    // Apply the easing.
> > > > >    if (aTiming.mFunction) {
> > > > >      progress = aTiming.mFunction->GetValue(progress, result.mBeforeFlag);
> > > > >    }
> > > > >  
> > > > > -  MOZ_ASSERT(IsFinite(progress), "Progress value should be finite");
> > > > > +  MOZ_ASSERT(!IsNaN(progress), "Progress value should be a valid number");
> > > > 
> > > > Progress should always be finite, right? And IsFinite should return false
> > > > for NaN, right?
> > > > (If progress can be infinite, we will need to update the spec and WebIDL.)
> > > 
> > > Actually in the test case, some variables in nsSMILKeySpline::CalcBezier
> > > were overflowed.
> > > As a result, nsSMILKeySpline::CalcBezier returned NaN because it calculated
> > > 0 * "double's" infinity.
> > > So, it's not *real* infinity there, it's just a value we can not handle
> > > properly.
> > 
> > Ok, so is this change still necessary? If we make GetValue() return a 0 in
> > that case, IsFinite() should still be correct, right? And should fail if
> > GetValue() returns NaN, right?
> 
> Ah, right.  GetValue should have returned 0 in that case.
> 
> > > > Also, do we need something similar for aT == 1?
> > > 
> > > I don't think nsSMILKeySpline::CalcBezier does return NaN in such case.
> > 
> > Does it correctly return 1?
> 
> I did not check it at all but I think it returns positive or negative
> infinity.

We should check that. It seems like it should return 1, so if it's returning infinity, we might need special handling for aT == 1.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > > > Oh, I checked now chromium behavior.  I did confirm they validate easing
> > > > function parameters and throw an exception like this:
> > > > 
> > > >  Uncaught TypeError: Failed to execute 'animate' on 'Element':
> > > > 'cubic-bezier(0, -1e+308, 0, 0)' is not a valid value for easing
> > > > 
> > > > Should we follow them?
> > > 
> > > Are they failing to parse it? Or are they actually evaluating the curve and
> > > ensuring it is within some limits?
> > 
> > Yes, failed to parse. 
> > https://chromium.googlesource.com/chromium/src/+/
> > 42dd9774c080e909f2d0646b3282b2d666b99ca2/third_party/WebKit/Source/core/
> > animation/AnimationInputHelpers.cpp#247
> 
> Oops, I should not that I did check in the parser code, but I could not find
> what is the limit value of it.

Looks to be between -1e38 and -1e39. Also, I notice the release channel doesn't have this limitation so it must be a fairly recent change. It would be interesting to look up when the change happened and the rationale for it.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #11)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > > > > Oh, I checked now chromium behavior.  I did confirm they validate easing
> > > > > function parameters and throw an exception like this:
> > > > > 
> > > > >  Uncaught TypeError: Failed to execute 'animate' on 'Element':
> > > > > 'cubic-bezier(0, -1e+308, 0, 0)' is not a valid value for easing
> > > > > 
> > > > > Should we follow them?
> > > > 
> > > > Are they failing to parse it? Or are they actually evaluating the curve and
> > > > ensuring it is within some limits?
> > > 
> > > Yes, failed to parse. 
> > > https://chromium.googlesource.com/chromium/src/+/
> > > 42dd9774c080e909f2d0646b3282b2d666b99ca2/third_party/WebKit/Source/core/
> > > animation/AnimationInputHelpers.cpp#247
> > 
> > Oops, I should not that I did check in the parser code, but I could not find
> > what is the limit value of it.
> 
> Looks to be between -1e38 and -1e39. Also, I notice the release channel
> doesn't have this limitation so it must be a fairly recent change. It would
> be interesting to look up when the change happened and the rationale for it.

Oh, great info.  That must be float limit.  And the changeset is:
 https://chromium.googlesource.com/chromium/src/+/84a58b756e4c8c2746cdfc8f326c11f06d0b2865
I guess before this change, it maybe silently fail.

Comment 13

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

Hiro, is it ok to assign this to you?

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Sure!

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I was wondering why the same cubic-bezier function for CSS animations does not hit any assertions.

That's because we don't assert for keyframe easing function there.  Should we assert as well?
http://hg.mozilla.org/mozilla-central/file/e45890951ce7/dom/animation/KeyframeEffect.cpp#l654

And, I think this problem is fundamentally a problem in 
CSSParserImpl::ParseTransitionTimingFunctionValueComponent.  We use float there and stores the value as float in nsTimingFunction, so if a value greater than float max, it will be inf of float.  I think we should check that the value is not inf of -inf there.
http://hg.mozilla.org/mozilla-central/file/e45890951ce7/layout/style/nsCSSParser.cpp#l16391

Brian, what do you think?

Comment 16

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> I was wondering why the same cubic-bezier function for CSS animations does
> not hit any assertions.
> 
> That's because we don't assert for keyframe easing function there.  Should
> we assert as well?
> http://hg.mozilla.org/mozilla-central/file/e45890951ce7/dom/animation/
> KeyframeEffect.cpp#l654

Yes I guess we should.

> And, I think this problem is fundamentally a problem in 
> CSSParserImpl::ParseTransitionTimingFunctionValueComponent.  We use float
> there and stores the value as float in nsTimingFunction, so if a value
> greater than float max, it will be inf of float.  I think we should check
> that the value is not inf of -inf there.
> http://hg.mozilla.org/mozilla-central/file/e45890951ce7/layout/style/
> nsCSSParser.cpp#l16391

That sounds reasonable. The other question is what we should do when we detect infinity. I'm not sure what our normal behavior is when we encounter a value we can't represent internally. It's not strictly an authoring error but a limitation of our implementation. We should check what we do for other properties, I guess.

Then we can decide if we should:

a) Report an error and fall back to linear
b) Silently fall back to linear
c) Silently clamp the value to something we can represent

Are there cases where the control points are within the range we can represent as a float but the timing function produces values outside that range? I guess there are and perhaps we should just clamp those values.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #16)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> > I was wondering why the same cubic-bezier function for CSS animations does
> > not hit any assertions.
> > 
> > That's because we don't assert for keyframe easing function there.  Should
> > we assert as well?
> > http://hg.mozilla.org/mozilla-central/file/e45890951ce7/dom/animation/
> > KeyframeEffect.cpp#l654
> 
> Yes I guess we should.

Note that I had checked StyleAnimationValue::Interpolate, and as far as I can tell, there is no security risk with inf or + inf values.

> > And, I think this problem is fundamentally a problem in 
> > CSSParserImpl::ParseTransitionTimingFunctionValueComponent.  We use float
> > there and stores the value as float in nsTimingFunction, so if a value
> > greater than float max, it will be inf of float.  I think we should check
> > that the value is not inf of -inf there.
> > http://hg.mozilla.org/mozilla-central/file/e45890951ce7/layout/style/
> > nsCSSParser.cpp#l16391
> 
> That sounds reasonable. The other question is what we should do when we
> detect infinity. I'm not sure what our normal behavior is when we encounter
> a value we can't represent internally. It's not strictly an authoring error
> but a limitation of our implementation. We should check what we do for other
> properties, I guess.
> 
> Then we can decide if we should:
> 
> a) Report an error and fall back to linear
> b) Silently fall back to linear
> c) Silently clamp the value to something we can represent

I found a similar case about Date object.
http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.1

It restricts 100,000,000 days, so if we pass 100,000,000 * 86,400,000 + 1 to 'new Date()', we get an invalid Date object (not throwing exceptions).

So, my opinion is we should define a reasonable range for y1/y2 control points and add it to the spec, then throw an exception for the value of out of range just like chrome does.
We already throw an exception for x control point which is out of its range [0, 1].
https://drafts.csswg.org/css-transitions-1/#funcdef-transition-timing-function-cubic-bezier

I've already written tests for those x control points.

> Are there cases where the control points are within the range we can
> represent as a float but the timing function produces values outside that
> range? I guess there are and perhaps we should just clamp those values.

In my approximate calculation, the max value of CalcBezier is approximately 12*3.4*(10^38)^2, it will be within the range of double (double max is 10^308 order).

I'd say the reasonable range is [-1e+38, 1e+38].

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> (In reply to Brian Birtles (:birtles) from comment #16)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> > > I was wondering why the same cubic-bezier function for CSS animations does
> > > not hit any assertions.
> > > 
> > > That's because we don't assert for keyframe easing function there.  Should
> > > we assert as well?
> > > http://hg.mozilla.org/mozilla-central/file/e45890951ce7/dom/animation/
> > > KeyframeEffect.cpp#l654
> > 
> > Yes I guess we should.
> 
> Note that I had checked StyleAnimationValue::Interpolate, and as far as I
> can tell, there is no security risk with inf or + inf values.

Of course I meant '-inf'.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)

> > Are there cases where the control points are within the range we can
> > represent as a float but the timing function produces values outside that
> > range? I guess there are and perhaps we should just clamp those values.
> 
> In my approximate calculation, the max value of CalcBezier is approximately
> 12*3.4*(10^38)^2, it will be within the range of double (double max is
> 10^308 order).
> 
> I'd say the reasonable range is [-1e+38, 1e+38].

After writing some test cases, it revealed that these boundary values are not suitable because of float precision.  For example, I wrote test cases to check values near by boundaries, e.g. 1e+38, 1e+38 + 1, 1e+38 - 1.  We can't tell all of these values in C++.  All values look the same due to floating point precision.  More worse case, for more smaller values, we can't tell 100000 and 100000.001.  So if we define the limit, we should choose more smaller values.

Just FYI: cubic-bezier(0, -1000, 1, 1000); http://cubic-bezier.com/#0,-1000,1,1000

Comment 20

[Hiroyuki Ikezoe (:hiro)]

This morning, I and Brian discussed about this.

The conclusion of the discussion is we'd take c) in comment 16 and ensure that calculated value on each boundary should be 0 or 1.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1278485 - Part 2: ComputedTimingFunction::GetValue ensures 0 or 1 on both edges.

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1278485 - Part 3: Add test cases to check x value of cubic-bezier is in [0, 1] and step numbers of steps function.

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

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8764755 [details]
Bug 1278485 - Part 1: Limit y1 and y2 control points for cubic-bezier to avoid overflows.

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

Comment 24

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

Comment on attachment 8764755 [details]
Bug 1278485 - Part 1: Limit y1 and y2 control points for cubic-bezier to avoid overflows.

https://reviewboard.mozilla.org/r/60100/#review60528

r=me with comments addressed

::: dom/animation/test/mozilla/file_cubic_bezier_limits.html:73
(Diff revision 2)
> +test(function(t) {
> +  var div = addDiv(t);
> +  var anim = div.animate({ }, 100 * MS_PER_SEC);
> +
> +  anim.effect.setKeyframes([ { easing: 'cubic-bezier(0, 1e+39, 0, 0)' }]);
> +  assert_equals(anim.effect.getKeyframes()[0].easing,
> +    'cubic-bezier(0, ' + max_float + ', 0, 0)',
> +    'y1 control point for keyframe easing on upper boundary');
> +
> +  anim.effect.setKeyframes([ { easing: 'cubic-bezier(0, 0, 0, 1e+39)' }]);
> +  assert_equals(anim.effect.getKeyframes()[0].easing,
> +    'cubic-bezier(0, 0, 0, ' + max_float + ')',
> +    'y2 control point for keyframe easing on upper boundary');
> +
> +  anim.effect.setKeyframes([ { easing: 'cubic-bezier(0, -1e+39, 0, 0)' }]);
> +  assert_equals(anim.effect.getKeyframes()[0].easing,
> +    'cubic-bezier(0, ' + -max_float + ', 0, 0)',
> +    'y1 control point for keyframe easing on lower boundary');
> +
> +  anim.effect.setKeyframes([ { easing: 'cubic-bezier(0, 0, 0, -1e+39)' }]);
> +  assert_equals(anim.effect.getKeyframes()[0].easing,
> +    'cubic-bezier(0, 0, 0, ' + -max_float + ')',
> +    'y2 control point for keyframe easing on lower boundary');
> +
> +}, 'Clamp y1 and y2 control point on for keyframe easing' );

This test appears to be identical to the above test except for the test descriptions.

::: layout/style/nsCSSParser.cpp:16395
(Diff revision 2)
> +    if (num == NegativeInfinity<float>()) {
> +      num = -std::numeric_limits<float>::max();
> +    } else if (num == PositiveInfinity<float>()) {
> +      num = std::numeric_limits<float>::max();
> +    }

Would the following work?

  num = mozilla::clamped(num, -std::numeric_limits<float>::max(),
                               std::numeric_limits<float>::max());

Comment 25

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

(In reply to Brian Birtles (:birtles) from comment #24)
> ::: layout/style/nsCSSParser.cpp:16395
> (Diff revision 2)
> > +    if (num == NegativeInfinity<float>()) {
> > +      num = -std::numeric_limits<float>::max();
> > +    } else if (num == PositiveInfinity<float>()) {
> > +      num = std::numeric_limits<float>::max();
> > +    }
> 
> Would the following work?
> 
>   num = mozilla::clamped(num, -std::numeric_limits<float>::max(),
>                                std::numeric_limits<float>::max());

Oh, we also need to #include <limits> and "nsAlgorithm.h"

Comment 26

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

Comment on attachment 8769981 [details]
Bug 1278485 - Part 2: ComputedTimingFunction::GetValue ensures 0 or 1 on both edges.

https://reviewboard.mozilla.org/r/63614/#review60540

::: dom/animation/ComputedTimingFunction.cpp:76
(Diff revision 1)
> +    if (aPortion == 0.0) {
> +      return 0.0;
> +    } else if (aPortion == 1.0) {
> +      return 1.0;
> +    }

We have a policy in our coding style that says, "Don't put an else right after a return" which I think most people interpret as meaning we should write:

if (aPortion == 0.0) {
  return 0.0;
}
if (aPortion == 1.0) {
  return 1.0;
}

::: dom/animation/test/mozilla/file_cubic_bezier_limits.html:176
(Diff revision 1)
> +  assert_equals(anim.effect.getComputedTiming().progress, 0.0,
> +    'progress on lower edge for the highest value of y1 and y2 control points');
> +
> +  anim.finish();
> +  assert_equals(anim.effect.getComputedTiming().progress, 1.0,
> +    'progress on upper edger for the highest value of y1 and y2 control points');

Nit: s/edger/edge/

Comment 27

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

Comment on attachment 8769982 [details]
Bug 1278485 - Part 3: Add test cases to check x value of cubic-bezier is in [0, 1] and step numbers of steps function.

https://reviewboard.mozilla.org/r/63616/#review60554

::: testing/web-platform/tests/web-animations/resources/effect-easing-tests.js:54
(Diff revision 1)
> +  {
> +    desc: 'invalid function name',
> +    easing: 'test'
> +  },
> +  {
> +    desc: 'x1 control point for cubic-bezier is out of [0, 1]',

Nit: s/out of [0, 1]/out of the range [0, 1]/

Here and a few places below

::: testing/web-platform/tests/web-animations/resources/effect-easing-tests.js:54
(Diff revision 1)
> +    desc: 'x1 control point for cubic-bezier is out of [0, 1]',
> +    easing: 'cubic-bezier(1.1, 0, 1, 1)'
> +  },
> +  {
> +    desc: 'x2 control point for cubic-bezier is out of [0, 1]',
> +    easing: 'cubic-bezier(0, 0, 1.1, 1)'
> +  },
> +  {
> +    desc: 'x1 control point for cubic-bezier is out of [0, 1]',
> +    easing: 'cubic-bezier(-0.1, 0, 1, 1)'
> +  },
> +  {
> +    desc: 'x2 control point for cubic-bezier is out of [0, 1]',

It looks like we repeat the same test description twice -- that will make it harder to tell which test fail or mark one as a known failure. Can we change the test description so that it is unique in each case?

Better still, why not just drop 'desc' altogether and make this a simple array and then just use the actual easing string in the test description?

Comment 28

[Hiroyuki Ikezoe (:hiro)]

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

Comment 29

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/63616/#review60554

> It looks like we repeat the same test description twice -- that will make it harder to tell which test fail or mark one as a known failure. Can we change the test description so that it is unique in each case?
> 
> Better still, why not just drop 'desc' altogether and make this a simple array and then just use the actual easing string in the test description?

It's a really nice idea! Thanks!

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8764755 [details]
Bug 1278485 - Part 1: Limit y1 and y2 control points for cubic-bezier to avoid overflows.

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

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8769981 [details]
Bug 1278485 - Part 2: ComputedTimingFunction::GetValue ensures 0 or 1 on both edges.

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

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8769982 [details]
Bug 1278485 - Part 3: Add test cases to check x value of cubic-bezier is in [0, 1] and step numbers of steps function.

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac72085aab9d
Part 1: Limit y1 and y2 control points for cubic-bezier to avoid overflows. r=birtles
https://hg.mozilla.org/integration/autoland/rev/21146d7fda46
Part 2: ComputedTimingFunction::GetValue ensures 0 or 1 on both edges. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4f7bcfbda34b
Part 3: Add test cases to check x value of cubic-bezier is in [0, 1] and step numbers of steps function. r=birtles

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ac72085aab9d
https://hg.mozilla.org/mozilla-central/rev/21146d7fda46
https://hg.mozilla.org/mozilla-central/rev/4f7bcfbda34b