Bug 139162 – Crash when setting 'flex' CSS property to 'calc(2 * 3) calc(2 * 3)'

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

[Chris Dumez]

Crash when setting 'flex' CSS property to 'calc(2 * 3) calc(2 * 3)'

Trace:
ASSERTION FAILED: !m_parsedCalculation
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010f7280fa WTFCrash + 42
1   com.apple.WebCore             	0x000000011124cc3b WebCore::CSSParser::parseCalculation(WebCore::CSSParserValue*, WebCore::CalculationPermittedValueRange) + 235 (CSSParser.cpp:10000)
2   com.apple.WebCore             	0x000000011124c899 WebCore::CSSParser::validCalculationUnit(WebCore::CSSParserValue*, WebCore::CSSParser::Units, WebCore::CSSParser::ReleaseParsedCalcValueCondition) + 105 (CSSParser.cpp:1588)
3   com.apple.WebCore             	0x000000011124cd2e WebCore::CSSParser::validUnit(WebCore::CSSParserValue*, WebCore::CSSParser::Units, WebCore::CSSParserMode, WebCore::CSSParser::ReleaseParsedCalcValueCondition) + 78 (CSSParser.cpp:1640)
4   com.apple.WebCore             	0x0000000111288d28 WebCore::CSSParser::validUnit(WebCore::CSSParserValue*, WebCore::CSSParser::Units, WebCore::CSSParser::ReleaseParsedCalcValueCondition) + 56 (CSSParser.h:622)
5   com.apple.WebCore             	0x000000011125e355 WebCore::CSSParser::parseFlex(WebCore::CSSParserValueList*, bool) + 213 (CSSParser.cpp:7363)
6   com.apple.WebCore             	0x0000000111250d6a WebCore::CSSParser::parseValue(WebCore::CSSPropertyID, bool) + 14666 (CSSParser.cpp:2491)
7   com.apple.WebCore             	0x00000001112150e3 cssyyparse(WebCore::CSSParser*) + 2771 (.CSSGrammar.y:389)
8   com.apple.WebCore             	0x000000011124a6ae WebCore::CSSParser::parseValue(WebCore::MutableStyleProperties*, WebCore::CSSPropertyID, WTF::String const&, bool, WebCore::StyleSheetContents*) + 110 (CSSParser.cpp:1314)
9   com.apple.WebCore             	0x00000001112498e7 WebCore::CSSParser::parseValue(WebCore::MutableStyleProperties*, WebCore::CSSPropertyID, WTF::String const&, bool, WebCore::CSSParserMode, WebCore::StyleSheetContents*) + 471 (CSSParser.cpp:1302)
10  com.apple.WebCore             	0x0000000112bb040f WebCore::MutableStyleProperties::setProperty(WebCore::CSSPropertyID, WTF::String const&, bool, WebCore::StyleSheetContents*) + 159 (StyleProperties.cpp:682)
11  com.apple.WebCore             	0x0000000112552d2b WebCore::PropertySetCSSStyleDeclaration::setPropertyInternal(WebCore::CSSPropertyID, WTF::String const&, bool, int&) + 171 (PropertySetCSSStyleDeclaration.cpp:265)
12  com.apple.WebCore             	0x0000000111d22ec8 WebCore::JSCSSStyleDeclaration::putDelegate(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) + 392 (JSCSSStyleDeclarationCustom.cpp:342)
13  com.apple.WebCore             	0x0000000111d1e079 WebCore::JSCSSStyleDeclaration::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) + 329 (JSCSSStyleDeclaration.cpp:284)
14  com.apple.JavaScriptCore      	0x000000010f0a1252 JSC::JSValue::put(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) + 210
15  com.apple.JavaScriptCore      	0x000000010f073cf6 void JSC::DFG::operationPutByValInternal<false, false>(JSC::ExecState*, long long, long long, long long) + 678

[Chris Dumez]

Created attachment 242364 [details]
Patch

[Darin Adler]

Comment on attachment 242364 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:6948
>      const double result = m_parsedCalculation ? m_parsedCalculation->doubleValue() : v->fValue;

Seems overkill to say const here. Just to emphasize that it’s not changed in the next three lines of code in this tiny function?

> Source/WebCore/css/CSSParser.cpp:6950
> +        m_parsedCalculation.clear();

Or maybe just = nullptr.

Also, the argument name here is “v” and the definition of the argument is formatted incorrectly (with a space before the *), and the argument should be a reference not a pointer.

And passing an argument to tell this to release the parsed calculation is just strange. Two separate functions would be much better.

[Chris Dumez]

> And passing an argument to tell this to release the parsed calculation is
> just strange. Two separate functions would be much better.

I agree this needs refactoring. However, I'd like to handle this separately because it does not only affect this function. The same pattern is used for:
- validCalculationUnit()
- validUnit()
- parsedDouble()

I need to think a bit more about this to come up with a nicer proposal.

[Chris Dumez]

Created attachment 242426 [details]
Patch

[Chris Dumez]

Created attachment 242429 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 242429 [details]
Patch

Clearing flags on attachment: 242429

Committed r176674: <http://trac.webkit.org/changeset/176674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.