1373712 - Assertion failure: SpecifiedKeyframeArraysAreEqual(mKeyframes, keyframesCopy) with large color value

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase fails the assertion in mozilla-central rev fe809f57bf22


Assertion failure: SpecifiedKeyframeArraysAreEqual(mKeyframes, keyframesCopy) (Apart from the computed offset members, the keyframes array should not be modified), at /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cp
p:937
#01: mozilla::dom::KeyframeEffectReadOnly::DoUpdateProperties<nsStyleContext> at dom/animation/KeyframeEffectReadOnly.cpp:336
#02: mozilla::dom::KeyframeEffectReadOnly::DoSetKeyframes<nsStyleContext> at dom/animation/KeyframeEffectReadOnly.cpp:231
#03: mozilla::dom::KeyframeEffectReadOnly::SetKeyframes at dom/animation/KeyframeEffectReadOnly.cpp:188
#04: mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions> at dom/animation/KeyframeEffectReadOnly.cpp:858
#05: mozilla::dom::KeyframeEffect::Constructor at dom/animation/KeyframeEffect.cpp:65
#06: mozilla::dom::Element::Animate at dom/base/Element.cpp:3600
#07: mozilla::dom::Element::Animate at mfbt/Maybe.h:445
#08: mozilla::dom::ElementBinding::animate at mfbt/AlreadyAddRefed.h:143
#09: mozilla::dom::GenericBindingMethod at dom/bindings/BindingUtils.cpp:2962
#10: js::CallJSNative at js/src/jscntxtinlines.h:293
#11: js::InternalCallOrConstruct at js/src/vm/Interpreter.cpp:470
#12: Interpret at js/src/vm/Interpreter.cpp:3067
#13: js::RunScript at js/src/vm/Interpreter.cpp:381

Comment 1

[Brian Birtles (:birtles)]

Given the timing of this, I wonder if this is a regression from bug 1339690 (dropping paced timing). Perhaps the move semantics introduced in:

  https://hg.mozilla.org/mozilla-central/rev/8a0c89fee7aa

Boris, do you want to have a look?

Comment 2

[Boris Chiou [:boris]]

OK. This is P1. wow~~

Comment 3

[Boris Chiou [:boris]]

Looks like |mPropertyValues|s of the 1st Keyframes and the 2nd Keyframes are different, but their serialization are identical. :(

Comment 4

[Boris Chiou [:boris]]

OK. I just verified this by an older version of Gecko (before we dropped spacing mode), and still can reproduce this bug. The root cause is:

In this example, we use "hsl(63e292, 41%, 34%)" as a property value of a keyframe. After parsing it [1] and creating the PropertyValuePair, the nsCSSValue in PropertyValuePair is nsCSSValueFloatColor, with values (nan, 0.41, 0.34). When trying to build properties, we copy the keyframes and do a comparison between the original one and the copied one:
* original:
  mProperty: color, mValue: (nan, 0.41, 0.34)
* copied:
  mProperty: color, mValue: (nan, 0.41, 0.34)

So, nsCSSValueFloatColor::operator==() returns false because the first component is "nan".


Therefore, there are some possible ways to fix this
a. Fix parser: if degreeAngle is "inf" [1], we should make aHue as a max value of float, or just give it "0" because it is a degree?
b. Fix the constructor of nsCSSValueFloatColor. Only store finite values in nsCSSValueFloatColor.
c. Fix nsCSSValueFloatColor::operator==(). If this and aOther are "nan" or "infinite", we return true.

What do you think, Brian? I'm not sure which way is better for this.

[1] We parse "63e292" as inf, and wrap it as nan by "aHue = aHue - floor(aHue)".
(http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/layout/style/nsCSSParser.cpp#7006)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Pretty similar to bug 1290994.

Comment 6

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #2)
> OK. This is P1. wow~~

Yeah, I was concerned that this was a regression with security implications (since that assertion is checking we don't modify the array while iterating over it except for a very limited set of safe mutations). Also, if it was a recent regression, it might block other fuzzing. Since neither of those things appear to be the case, it's not such high priority.

(In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #4)
> OK. I just verified this by an older version of Gecko (before we dropped
> spacing mode), and still can reproduce this bug. The root cause is:
> 
> In this example, we use "hsl(63e292, 41%, 34%)" as a property value of a
> keyframe. After parsing it [1] and creating the PropertyValuePair, the
> nsCSSValue in PropertyValuePair is nsCSSValueFloatColor, with values (nan,
> 0.41, 0.34). When trying to build properties, we copy the keyframes and do a
> comparison between the original one and the copied one:
> * original:
>   mProperty: color, mValue: (nan, 0.41, 0.34)
> * copied:
>   mProperty: color, mValue: (nan, 0.41, 0.34)
> 
> So, nsCSSValueFloatColor::operator==() returns false because the first
> component is "nan".
> 
> 
> Therefore, there are some possible ways to fix this
> a. Fix parser: if degreeAngle is "inf" [1], we should make aHue as a max
> value of float, or just give it "0" because it is a degree?

The parser seems buggy here. In particular, in CSSParserImpl::ParseHue we clamp angle values to the finite range but not number values. It's probably most consistent to detect the infinite case in ParseHue and clamp it there.

Ideally it would be good to clamp to multiples of 360 but I don't think we can do that since the double to float conversion happens in nsCSSScanner::ScanNumber. For now, let's just be consistent with the angle case and clamp to min/max float.

(Looking at https://jsfiddle.net/shbu89L9/ in other browsers I see Chrome goes lime and Edge goes pale blue. However 63e292 % 360 is 264 which is purple so I don't think they're doing anything sensible there.)

> b. Fix the constructor of nsCSSValueFloatColor. Only store finite values in
> nsCSSValueFloatColor.

I guess if we do (a) we could add an assertion here? Unless there are other code paths that might trigger this?

Comment 7

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #6)
> The parser seems buggy here. In particular, in CSSParserImpl::ParseHue we
> clamp angle values to the finite range but not number values. It's probably
> most consistent to detect the infinite case in ParseHue and clamp it there.
> 
> Ideally it would be good to clamp to multiples of 360 but I don't think we
> can do that since the double to float conversion happens in
> nsCSSScanner::ScanNumber. For now, let's just be consistent with the angle
> case and clamp to min/max float.

Understood. I will fix the parser.

> 
> (Looking at https://jsfiddle.net/shbu89L9/ in other browsers I see Chrome
> goes lime and Edge goes pale blue. However 63e292 % 360 is 264 which is
> purple so I don't think they're doing anything sensible there.)
> 
> > b. Fix the constructor of nsCSSValueFloatColor. Only store finite values in
> > nsCSSValueFloatColor.
> 
> I guess if we do (a) we could add an assertion here? Unless there are other
> code paths that might trigger this?

Sure. Will add an assertion to make sure this shouldn't happen. If we always use nsCSSParser to parser color property value, we may not trigger this. Besides, still need to check Servo css-parser also handle this properly. Thanks for the suggestion.

Comment 8

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #6)
> The parser seems buggy here. In particular, in CSSParserImpl::ParseHue we
> clamp angle values to the finite range but not number values. It's probably
> most consistent to detect the infinite case in ParseHue and clamp it there.

And it seems that we start to parse this as a number without any clamps in Bug 1295456.

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1373712 - Part 1: Fix ParseHue() for inf value.

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

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1373712 - Part 2: Add a crashtest with large color value.

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

Comment 11

[Brian Birtles (:birtles)]

Comment on attachment 8879823 [details]
Bug 1373712 - Part 2: Add a crashtest with large color value.

https://reviewboard.mozilla.org/r/151218/#review156054

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

https://reviewboard.mozilla.org/r/151216/#review156056

::: layout/style/nsCSSValue.h:1831
(Diff revision 1)
> -  {}
> +  {
> +    MOZ_ASSERT(mozilla::IsFinite(aComponent1) &&
> +               mozilla::IsFinite(aComponent2) &&
> +               mozilla::IsFinite(aComponent3) &&
> +               mozilla::IsFinite(aAlpha),
> +               "Only accept finite float");

Nit: I think it generally makes more sense to write assertion messages as positive statements we expect to be true, e.g. "Each color/alpha component should be a finite float"

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

https://reviewboard.mozilla.org/r/151216/#review156056

> Nit: I think it generally makes more sense to write assertion messages as positive statements we expect to be true, e.g. "Each color/alpha component should be a finite float"

I see. I will update this description.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

https://reviewboard.mozilla.org/r/151216/#review156058

::: layout/style/nsCSSParser.cpp:6959
(Diff revision 1)
>    if (!GetToken(true)) {
>      REPORT_UNEXPECTED_EOF(PEColorHueEOF);
>      return false;
>    }
>  
> -  // <number>
> -  if (mToken.mType == eCSSToken_Number) {
> -    aAngle = mToken.mNumber;
> -    return true;
> -  }
>    UngetToken();

Can we get rid of this whole chunk (from GetToken up to UngetToken) now that you've removed the usages of mToken her?  (Right now this ends up being basically "GetToken(); UngetToken();" which feels a parser anti-pattern.)

It does still kinda handle the no-more-input scenario, but it doesn't *need* to be responsible for that.  Your ParseSingleTokenVariant failure case below can handle that more gracefully, I think.

::: layout/style/nsCSSParser.cpp:6966
(Diff revision 1)
> -    aAngle = mToken.mNumber;
> -    return true;
> -  }
>    UngetToken();
>  
> -  // <angle>
> +  float result = 0.0;

Move the "result" declaration closer to where it's used, please. (Probably right before the GetUnit check.)

And don't bother giving it a default 0.0 value, because that 0.0 value is never used. (All codepaths assign it to something else before bothering to use its value.)

And ideally it'd also be nice to name it something like "unclampedResult" to make it clearer that it's *not quite* our result (and to indicate what differentiates it from our actual result).

::: layout/style/nsCSSValue.h:1831
(Diff revision 1)
> -  {}
> +  {
> +    MOZ_ASSERT(mozilla::IsFinite(aComponent1) &&
> +               mozilla::IsFinite(aComponent2) &&
> +               mozilla::IsFinite(aComponent3) &&
> +               mozilla::IsFinite(aAlpha),
> +               "Only accept finite float");

Agreed with birtles on this -- I think this assertion message would be a bit clearer:
 "Caller must ensure color components are finite"

It might also be helpful to include a comment (or a mention in this message) about *why we care* about these args being finite. (It's not at all clear from the contextual code here; clearly the constructor doesn't do anything with these args that depends on them being finite.)

Comment 15

[Boris Chiou [:boris]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

https://reviewboard.mozilla.org/r/151216/#review156058

> Can we get rid of this whole chunk (from GetToken up to UngetToken) now that you've removed the usages of mToken her?  (Right now this ends up being basically "GetToken(); UngetToken();" which feels a parser anti-pattern.)
> 
> It does still kinda handle the no-more-input scenario, but it doesn't *need* to be responsible for that.  Your ParseSingleTokenVariant failure case below can handle that more gracefully, I think.

Oh yes. ParseSingleTokenVariant also checks EOF. I will remove this chunk.

> Agreed with birtles on this -- I think this assertion message would be a bit clearer:
>  "Caller must ensure color components are finite"
> 
> It might also be helpful to include a comment (or a mention in this message) about *why we care* about these args being finite. (It's not at all clear from the contextual code here; clearly the constructor doesn't do anything with these args that depends on them being finite.)

I see. I will add a comment to mention about why we need this assertion.

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

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

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8879823 [details]
Bug 1373712 - Part 2: Add a crashtest with large color value.

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

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

https://reviewboard.mozilla.org/r/151216/#review156058

> Oh yes. ParseSingleTokenVariant also checks EOF. I will remove this chunk.

Besides, I will also remove "PEColorHueEOF" from dom/locales/en-US/chrome/layout/css.properties.

Comment 19

[Boris Chiou [:boris]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

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

Comment 20

[Boris Chiou [:boris]]

Comment on attachment 8879823 [details]
Bug 1373712 - Part 2: Add a crashtest with large color value.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aee4c19e88ff
Part 1: Fix ParseHue() for inf value. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/924e83ab9a55
Part 2: Add a crashtest with large color value. r=birtles

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/aee4c19e88ff
https://hg.mozilla.org/mozilla-central/rev/924e83ab9a55

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here that would justify uplifting this to Beta or can it ride the 56 train?

Comment 24

[Boris Chiou [:boris]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Is there a user impact here that would justify uplifting this to Beta or can
> it ride the 56 train?

This bug clamps inf value to avoid undefined behaviors, so it would be better to uplift to beta.

Comment 25

[Boris Chiou [:boris]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

Approval Request Comment
[Feature/Bug causing the regression]: feature bug: Bug 1295456
[User impact if declined]: This causes assertions for debug build and may cause undefined behaviors for large hsl color.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Nope
[List of other uplifts needed for the feature/fix]: This patch and the following crashtest
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only clamp the inf value, so this is safe.
[String changes made/needed]: No.

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8879823 [details]
Bug 1373712 - Part 2: Add a crashtest with large color value.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1295456
[User impact if declined]: This is the test case of the fix.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: The part 1 patch in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is just an valid internal crash test.
[String changes made/needed]: No.

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8879822 [details]
Bug 1373712 - Part 1: Fix ParseHue() for inf value.

avoid overflow in hue parsing, beta55+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a93656f44a2
https://hg.mozilla.org/releases/mozilla-beta/rev/7c7bf5a6aebf