Bug 138780 – Crash when setting 'order' CSS property to a calculated value

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Dumez]

Crash when setting 'order' CSS property to a calculated value, e.g. 'calc(2 * 3)'.

Backtrace:
ASSERTION FAILED: !m_parsedCalculation
/Users/chris/WebKit/OpenSource/Source/WebCore/css/CSSParser.cpp(3114) : bool WebCore::CSSParser::parseValue(WebCore::CSSPropertyID, bool)
1   0x105a17770 WTFCrash
2   0x1073666e9 WebCore::CSSParser::parseValue(WebCore::CSSPropertyID, bool)
3   0x10732c793 cssyyparse(WebCore::CSSParser*)
4   0x10735daee WebCore::CSSParser::parseValue(WebCore::MutableStyleProperties*, WebCore::CSSPropertyID, WTF::String const&, bool, WebCore::StyleSheetContents*)
5   0x10735cd27 WebCore::CSSParser::parseValue(WebCore::MutableStyleProperties*, WebCore::CSSPropertyID, WTF::String const&, bool, WebCore::CSSParserMode, WebCore::StyleSheetContents*)
6   0x108c2110f WebCore::MutableStyleProperties::setProperty(WebCore::CSSPropertyID, WTF::String const&, bool, WebCore::StyleSheetContents*)
7   0x1085f8a47 WebCore::PropertySetCSSStyleDeclaration::setPropertyInternal(WebCore::CSSPropertyID, WTF::String const&, bool, int&)
8   0x107dea0fb WebCore::JSCSSStyleDeclaration::putDelegate(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
9   0x107de52a9 WebCore::JSCSSStyleDeclaration::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
10  0x1053e6d92 JSC::JSValue::put(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)
11  0x1053bae26 void JSC::DFG::operationPutByValInternal<false, false>(JSC::ExecState*, long long, long long, long long)
12  0x1053bab6b operationPutByValNonStrict

[Chris Dumez]

Created attachment 241684 [details]
Patch

[Darin Adler]

Comment on attachment 241684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241684&action=review

> Source/WebCore/ChangeLog:15
> +        valid in the CSS Parser intead but this would have brought issues:

Typo: instead

> Source/WebCore/ChangeLog:16
> +        - The calculated value needs to be adjused to INT_MIN + 2 if it is less

typo: adjusted

> Source/WebCore/css/CSSParser.cpp:2507
> +            double result = std::max<double>(std::numeric_limits<int>::min() + 2, parsedDouble(value, ReleaseParsedCalcValue));

I find the parsedDouble function confusing. It seems hard to use it correctly. Also, none of the callers use DoNotReleaseParsedCalcValue. I think the function should be renamed to takeParsedDouble, the argument should be changed to a CSSParserValue&, and the enum should be removed.

And even after those fixes, I think it remains a function that implements a confusing pattern. For some reason we put the calculated value into CSSParser state, but the non-calculate value into a local variable; this is a messy pattern.

[Chris Dumez]

Comment on attachment 241684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241684&action=review

>> Source/WebCore/css/CSSParser.cpp:2507
>> +            double result = std::max<double>(std::numeric_limits<int>::min() + 2, parsedDouble(value, ReleaseParsedCalcValue));
> 
> I find the parsedDouble function confusing. It seems hard to use it correctly. Also, none of the callers use DoNotReleaseParsedCalcValue. I think the function should be renamed to takeParsedDouble, the argument should be changed to a CSSParserValue&, and the enum should be removed.
> 
> And even after those fixes, I think it remains a function that implements a confusing pattern. For some reason we put the calculated value into CSSParser state, but the non-calculate value into a local variable; this is a messy pattern.

Ok, I will look into this in a follow-up.

[Chris Dumez]

Created attachment 241689 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 241689 [details]
Patch

Clearing flags on attachment: 241689

Committed r176171: <http://trac.webkit.org/changeset/176171>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.