Bug 138780 - Crash when setting 'order' CSS property to a calculated value
Summary: Crash when setting 'order' CSS property to a calculated value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 138778
  Show dependency treegraph
 
Reported: 2014-11-16 12:24 PST by Chris Dumez
Modified: 2014-11-16 21:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.93 KB, patch)
2014-11-16 13:29 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.92 KB, patch)
2014-11-16 20:54 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-11-16 12:24:07 PST
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
Comment 1 Chris Dumez 2014-11-16 13:29:27 PST
Created attachment 241684 [details]
Patch
Comment 2 Darin Adler 2014-11-16 19:53:26 PST
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.
Comment 3 Chris Dumez 2014-11-16 20:00:08 PST
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.
Comment 4 Chris Dumez 2014-11-16 20:54:55 PST
Created attachment 241689 [details]
Patch
Comment 5 WebKit Commit Bot 2014-11-16 21:41:05 PST
Comment on attachment 241689 [details]
Patch

Clearing flags on attachment: 241689

Committed r176171: <http://trac.webkit.org/changeset/176171>
Comment 6 WebKit Commit Bot 2014-11-16 21:41:14 PST
All reviewed patches have been landed.  Closing bug.