1272475 - CSS Transform: Assertion failure: !mozilla::IsNaN(mValue.mFloat)

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Raymond Forbes[:rforbes]]

void nsCSSValue::SetFloatValue(float aValue, nsCSSUnit aUnit)
{
  MOZ_ASSERT(eCSSUnit_Number <= aUnit, "not a float value");
  Reset();
  if (eCSSUnit_Number <= aUnit) {
    mUnit = aUnit;
    mValue.mFloat = aValue;
    MOZ_ASSERT(!mozilla::IsNaN(mValue.mFloat));
  }
}

Assertion failure: !mozilla::IsNaN(mValue.mFloat), at c:/src/mozilla-source/mozilla-central/layout/style/nsCSSValue.cpp:406
#01: AddTransformScale (c:\src\mozilla-source\mozilla-central\layout\style\styleanimationvalue.cpp:1256)
#02: AddTransformLists (c:\src\mozilla-source\mozilla-central\layout\style\styleanimationvalue.cpp:2103)
#03: mozilla::StyleAnimationValue::AddWeighted (c:\src\mozilla-source\mozilla-central\layout\style\styleanimationvalue.cpp:2706)
#04: mozilla::StyleAnimationValue::Interpolate (c:\src\mozilla-source\mozilla-central\obj-i686-pc-mingw32\dist\include\mozilla\styleanimationvalue.h:105)
#05: mozilla::dom::KeyframeEffectReadOnly::ComposeStyle (c:\src\mozilla-source\mozilla-central\dom\animation\keyframeeffect.cpp:643)
#06: mozilla::dom::Animation::ComposeStyle (c:\src\mozilla-source\mozilla-central\dom\animation\animation.cpp:821)
#07: mozilla::EffectCompositor::ComposeAnimationRule (c:\src\mozilla-source\mozilla-central\dom\animation\effectcompositor.cpp:598)
#08: mozilla::EffectCompositor::MaybeUpdateAnimationRule (c:\src\mozilla-source\mozilla-central\dom\animation\effectcompositor.cpp:259)
#09: mozilla::EffectCompositor::GetAnimationRule (c:\src\mozilla-source\mozilla-central\dom\animation\effectcompositor.cpp:295)
#10: mozilla::EffectCompositor::AnimationStyleRuleProcessor::RulesMatching (c:\src\mozilla-source\mozilla-central\dom\animation\effectcompositor.cpp:811)
#11: EnumRulesMatching<ElementRuleProcessorData> (c:\src\mozilla-source\mozilla-central\layout\style\nsstyleset.cpp:780)
#12: nsStyleSet::FileRules (c:\src\mozilla-source\mozilla-central\layout\style\nsstyleset.cpp:1171)
#13: nsStyleSet::ResolveStyleFor (c:\src\mozilla-source\mozilla-central\layout\style\nsstyleset.cpp:1342)
#14: nsStyleSet::ResolveStyleFor (c:\src\mozilla-source\mozilla-central\layout\style\nsstyleset.cpp:1323)
#15: mozilla::StyleSetHandle::Ptr::ResolveStyleFor (c:\src\mozilla-source\mozilla-central\obj-i686-pc-mingw32\dist\include\mozilla\stylesethandleinlines.h:85)
#16: nsComputedDOMStyle::GetStyleContextForElementNoFlush (c:\src\mozilla-source\mozilla-central\layout\style\nscomputeddomstyle.cpp:502)
#17: nsComputedDOMStyle::GetStyleContextForElement (c:\src\mozilla-source\mozilla-central\layout\style\nscomputeddomstyle.cpp:429)
#18: mozilla::dom::KeyframeEffectReadOnly::SetFrames (c:\src\mozilla-source\mozilla-central\dom\animation\keyframeeffect.cpp:449)
#19: mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect,mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions> (c:\src\mozilla-source\mozilla-central\dom\animation\keyframeeffect.cpp:746)
#20: mozilla::dom::KeyframeEffect::Constructor (c:\src\mozilla-source\mozilla-central\dom\animation\keyframeeffect.cpp:1367)
#21: mozilla::dom::Element::Animate (c:\src\mozilla-source\mozilla-central\dom\base\element.cpp:3356)
#22: mozilla::dom::Element::Animate (c:\src\mozilla-source\mozilla-central\dom\base\element.cpp:3313)
#23: mozilla::dom::ElementBinding::animate (c:\src\mozilla-source\mozilla-central\obj-i686-pc-mingw32\dom\bindings\elementbinding.cpp:3414)
#24: mozilla::dom::GenericBindingMethod (c:\src\mozilla-source\mozilla-central\dom\bindings\bindingutils.cpp:2781)
#25: js::CallJSNative (c:\src\mozilla-source\mozilla-central\js\src\jscntxtinlines.h:235)
#26: js::InternalCallOrConstruct (c:\src\mozilla-source\mozilla-central\js\src\vm\interpreter.cpp:480)
#27: InternalCall (c:\src\mozilla-source\mozilla-central\js\src\vm\interpreter.cpp:525)
#28: Interpret (c:\src\mozilla-source\mozilla-central\js\src\vm\interpreter.cpp:2831)
#29: js::RunScript (c:\src\mozilla-source\mozilla-central\js\src\vm\interpreter.cpp:426)
#30: js::ExecuteKernel (c:\src\mozilla-source\mozilla-central\js\src\vm\interpreter.cpp:704)
#31: js::Execute (c:\src\mozilla-source\mozilla-central\js\src\vm\interpreter.cpp:736)
#32: Evaluate (c:\src\mozilla-source\mozilla-central\js\src\jsapi.cpp:4519)
#33: Evaluate (c:\src\mozilla-source\mozilla-central\js\src\jsapi.cpp:4546)
#34: JS::Evaluate (c:\src\mozilla-source\mozilla-central\js\src\jsapi.cpp:4607)
#35: nsJSUtils::EvaluateString (c:\src\mozilla-source\mozilla-central\dom\base\nsjsutils.cpp:212)
#36: nsJSUtils::EvaluateString (c:\src\mozilla-source\mozilla-central\dom\base\nsjsutils.cpp:279)
#37: nsScriptLoader::EvaluateScript (c:\src\mozilla-source\mozilla-central\dom\base\nsscriptloader.cpp:2023)
#38: nsScriptLoader::ProcessRequest (c:\src\mozilla-source\mozilla-central\dom\base\nsscriptloader.cpp:1821)
#39: nsScriptLoader::ProcessScriptElement (c:\src\mozilla-source\mozilla-central\dom\base\nsscriptloader.cpp:1558)
#40: nsScriptElement::MaybeProcessScript (c:\src\mozilla-source\mozilla-central\dom\base\nsscriptelement.cpp:142)
#41: nsIScriptElement::AttemptToExecute (c:\src\mozilla-source\mozilla-central\dom\base\nsiscriptelement.h:222)
#42: nsHtml5TreeOpExecutor::RunFlushLoop (c:\src\mozilla-source\mozilla-central\parser\html\nshtml5treeopexecutor.cpp:491)
#43: nsHtml5ExecutorFlusher::Run (c:\src\mozilla-source\mozilla-central\parser\html\nshtml5streamparser.cpp:130)
#44: nsThread::ProcessNextEvent (c:\src\mozilla-source\mozilla-central\xpcom\threads\nsthread.cpp:991)
#45: NS_ProcessNextEvent (c:\src\mozilla-source\mozilla-central\xpcom\glue\nsthreadutils.cpp:290)
#46: mozilla::ipc::MessagePump::Run (c:\src\mozilla-source\mozilla-central\ipc\glue\messagepump.cpp:98)
#47: MessageLoop::RunInternal (c:\src\mozilla-source\mozilla-central\ipc\chromium\src\base\message_loop.cc:233)
#48: MessageLoop::RunHandler (c:\src\mozilla-source\mozilla-central\ipc\chromium\src\base\message_loop.cc:227)
#49: MessageLoop::Run (c:\src\mozilla-source\mozilla-central\ipc\chromium\src\base\message_loop.cc:207)
#50: nsBaseAppShell::Run (c:\src\mozilla-source\mozilla-central\widget\nsbaseappshell.cpp:158)
#51: nsAppShell::Run (c:\src\mozilla-source\mozilla-central\widget\windows\nsappshell.cpp:264)
#52: nsAppStartup::Run (c:\src\mozilla-source\mozilla-central\toolkit\components\startup\nsappstartup.cpp:285)
#53: XREMain::XRE_mainRun (c:\src\mozilla-source\mozilla-central\toolkit\xre\nsapprunner.cpp:4368)
#54: XREMain::XRE_main (c:\src\mozilla-source\mozilla-central\toolkit\xre\nsapprunner.cpp:4472)
#55: XRE_main (c:\src\mozilla-source\mozilla-central\toolkit\xre\nsapprunner.cpp:4580)
#56: do_main (c:\src\mozilla-source\mozilla-central\browser\app\nsbrowserapp.cpp:220)
#57: NS_internal_main (c:\src\mozilla-source\mozilla-central\browser\app\nsbrowserapp.cpp:360)
#58: wmain (c:\src\mozilla-source\mozilla-central\toolkit\xre\nswindowswmain.cpp:138)
#59: invoke_main (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:79)
#60: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:255)
#61: __scrt_common_main (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:300)
#62: wmainCRTStartup (f:\dd\vctools\crt\vcstartup\src\startup\exe_wmain.cpp:17)
#63: BaseThreadInitThunk[C:\Windows\SYSTEM32\KERNEL32.DLL +0x138f4]
#64: RtlUnicodeStringToInteger[C:\Windows\SYSTEM32\ntdll.dll +0x65de3]
#65: RtlUnicodeStringToInteger[C:\Windows\SYSTEM32\ntdll.dll +0x65dae]

Comment 1

[Raymond Forbes[:rforbes]]

Attachment: testcase.html

Comment 2

[Andrew McCreight [:mccr8]]

Brian, is there somebody who can look at this? Is this going to be a security issue possibly?

Comment 3

[Brian Birtles (:birtles)]

Not likely a security issue. Boris has done some work on range checking these values so he might be interested in looking at this.

Boris, care to take a look?

Comment 4

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #3)
> Not likely a security issue. Boris has done some work on range checking
> these values so he might be interested in looking at this.
> 
> Boris, care to take a look?

OK.

Raymond, does this assertion only happen in Window? Thanks.

Comment 5

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #4)
> (In reply to Brian Birtles (:birtles) from comment #3)
> > Not likely a security issue. Boris has done some work on range checking
> > these values so he might be interested in looking at this.
> > 
> > Boris, care to take a look?
> 
> OK.
> 
> Raymond, does this assertion only happen in Window? Thanks.

I can reproduce this on mac. Thanks.

Comment 6

[Boris Chiou [:boris]]

Attachment: simplified test

Comment 7

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #6)
> Created attachment 8772333 [details]
> simplified test

I think this assertion happens because we use an infinity value (e.g. 9.5e+307, a very large number) in the scale function. I dumped the properties value of this test case:
[log] Dump Animation Properties
[log] transform
[log]   0.000000..0.200000: scale(0.1)..scale(8)
[log]   0.200000..0.400000: scale(8)..scale(0.7)
[log]   0.400000..0.600000: scale(0.7)..scale(16)
[log]   0.600000..0.800000: scale(16)..scale(Infinity)
[log]   0.800000..1.000000: scale(Infinity)..scale(32)

When we try to interpolate the values between scale(16) and scale(Infinity), this assertion happens.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

This must be another variant of bug 1278485.  The scale value must be parsed as float.

Comment 9

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> This must be another variant of bug 1278485.  The scale value must be parsed
> as float.

Thanks, Hiro. You're right. I'm thinking where is the best place to clamp the +/-infinity CSS value to max/min float value. We can clamp it in nsCSSParser::CSSParserImpl::ParseFunctionInternals(), or StyleAnimationValue::AddTransformScale(). I may try to do this in nsCSSParser because StyleAnimationValue::AddTransformScale() only fix this problem for scale() and we may have the same problem for other transform functions or other functions I haven't noticed.

Comment 10

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #9)
> Thanks, Hiro. You're right. I'm thinking where is the best place to clamp
> the +/-infinity CSS value to max/min float value. We can clamp it in
> nsCSSParser::CSSParserImpl::ParseFunctionInternals(), or
> StyleAnimationValue::AddTransformScale(). I may try to do this in
> nsCSSParser because StyleAnimationValue::AddTransformScale() only fix this
> problem for scale() and we may have the same problem for other transform
> functions or other functions I haven't noticed.

Looks like other transform functions, e.g. translate, don't have this problem because nsCSSParser still returns a valid length value even if the input is a very large pixel value, so I think we only need to clamp eCSSUnit_Number for transform functions.

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1272475 - Part 1: Clamp max/min float value in the parser of CSS Transform function.

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

Comment 12

[Boris Chiou [:boris]]

Attachment: Bug 1272475 - Part 2: Add crashtests and mochitests.

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

Comment 13

[Cameron McCormack (:heycam)]

I don't understand how length values remain finite but number values get rounded to infinity.  Does the rounding to infinity happen when we call nsCSSValue::SetFloatValue in ParseVariant?  If so, why doesn't this happen when ParseVariant calls TranslateDimension, which also calls SetFloatValue?

Comment 14

[Boris Chiou [:boris]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> I don't understand how length values remain finite but number values get
> rounded to infinity.  Does the rounding to infinity happen when we call
> nsCSSValue::SetFloatValue in ParseVariant?  If so, why doesn't this happen
> when ParseVariant calls TranslateDimension, which also calls SetFloatValue?

Oops. Sorry, I think I'm wrong. For pixel values, SetFloatValue still set "inf", but we convert it into app unit somewhere I guess, so it is rounded, and so StyleAnimationValue::AddedWeight() can calculate this without IsNaN assertion.

Therefore, I should clamp the float values for |unit >= eCSSUnit_Number| in ParseFunctionInternal(). I will update this soon. Thank you.

Comment 15

[Boris Chiou [:boris]]

Comment on attachment 8772801 [details]
Bug 1272475 - Part 1: Clamp max/min float value in the parser of CSS Transform function.

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

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8772802 [details]
Bug 1272475 - Part 2: Add crashtests and mochitests.

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

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8772801 [details]
Bug 1272475 - Part 1: Clamp max/min float value in the parser of CSS Transform function.

https://reviewboard.mozilla.org/r/65528/#review63484

::: layout/style/nsCSSParser.cpp:15420
(Diff revision 2)
>      uint32_t m = aVariantMaskAll ? aVariantMaskAll : aVariantMask[index];
>      if (ParseVariant(newValue, m, nullptr) != CSSParseResult::Ok) {
>        break;
>      }
>  
> +    if (newValue.GetUnit() >= eCSSUnit_Number) {

This is fine, but can you factor out this check into a static method on nsCSSValue (maybe called IsFloatUnit) so that it doesn't get out of sync with the corresponding check we do in the assertion in nsCSSValue::GetFloatValue?  r=me with that.

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8772802 [details]
Bug 1272475 - Part 2: Add crashtests and mochitests.

https://reviewboard.mozilla.org/r/65530/#review63486

::: dom/animation/test/crashtests/1272475-1.html:14
(Diff revision 2)
> +                                "background: red;");
> +      document.body.appendChild(div);
> +      div.animate([{"transform":"scale(8)"},
> +                   {"transform":"scale(9.5e+307)"},
> +                   {"transform":"scale(32)"}],
> +                  {"duration":1000, "fill":"both"});

Nit: align this with the previous line.

::: dom/animation/test/crashtests/1272475-2.html:14
(Diff revision 2)
> +                                "background: red;");
> +      document.body.appendChild(div);
> +      div.animate([{"transform":"rotate(8rad)"},
> +                   {"transform":"rotate(9.5e+307rad)"},
> +                   {"transform":"rotate(32rad)"}],
> +                  {"duration":1000, "fill":"both"});

Nit: align this with the previous line.

::: dom/animation/test/mozilla/file_transform_limits.html:9
(Diff revision 2)
> +<body>
> +<script>
> +'use strict';
> +
> +// We clamp +infinity or -inifinity value in floating point to
> +// maximum floating point value or -maxinum floating point value.

s/maxinum/maximum/

Comment 19

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/65528/#review63484

> This is fine, but can you factor out this check into a static method on nsCSSValue (maybe called IsFloatUnit) so that it doesn't get out of sync with the corresponding check we do in the assertion in nsCSSValue::GetFloatValue?  r=me with that.

OK. Thanks for your suggestion.

Comment 20

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/65530/#review63486

> Nit: align this with the previous line.

Previous lines are an array which is the first parameter of animate(), and this line (i.e. duraion: xxxx) is the 2nd parameter. However, I think I should add an extra space after '[' and before ']' to make it easier to read. Thanks.

Comment 21

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/65530/#review63486

> Previous lines are an array which is the first parameter of animate(), and this line (i.e. duraion: xxxx) is the 2nd parameter. However, I think I should add an extra space after '[' and before ']' to make it easier to read. Thanks.

Oh, I misread.  You are right.

Comment 22

[Boris Chiou [:boris]]

Comment on attachment 8772801 [details]
Bug 1272475 - Part 1: Clamp max/min float value in the parser of CSS Transform function.

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

Comment 23

[Boris Chiou [:boris]]

Comment on attachment 8772802 [details]
Bug 1272475 - Part 2: Add crashtests and mochitests.

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5598fd36f3d4
Part 1: Clamp max/min float value in the parser of CSS Transform function. r=heycam
https://hg.mozilla.org/integration/autoland/rev/538b3f9574b7
Part 2: Add crashtests and mochitests. r=heycam

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5598fd36f3d4
https://hg.mozilla.org/mozilla-central/rev/538b3f9574b7