Bug 139162

Summary: Crash when setting 'flex' CSS property to 'calc(2 * 3) calc(2 * 3)'
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: CSSAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, kling, koivisto, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 138778    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2014-12-01 16:32:32 PST 
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
Comment 1 Chris Dumez 2014-12-01 17:07:46 PST 
Created attachment 242364 [details]
Patch
Comment 2 Darin Adler 2014-12-02 09:02:53 PST 
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.
Comment 3 Chris Dumez 2014-12-02 10:24:12 PST 
> 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.
Comment 4 Chris Dumez 2014-12-02 10:40:18 PST 
Created attachment 242426 [details]
Patch
Comment 5 Chris Dumez 2014-12-02 10:57:00 PST 
Created attachment 242429 [details]
Patch
Comment 6 WebKit Commit Bot 2014-12-02 11:15:07 PST 
Comment on attachment 242429 [details]
Patch

Clearing flags on attachment: 242429

Committed r176674: <http://trac.webkit.org/changeset/176674>
Comment 7 WebKit Commit Bot 2014-12-02 11:15:11 PST 
All reviewed patches have been landed.  Closing bug.