Bug 138777

Summary: Crash when setting '-webkit-line-clamp' CSS property to a calculated value
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: CSSAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, dino, kling
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 138778    
Attachments:
Description Flags
Patch none

Description Chris Dumez 2014-11-16 10:37:48 PST 
Crash when setting '-webkit-line-clamp' CSS property to a calculated value, e.g. 'calc(100% * 2)'.

Backtrace:
SHOULD NEVER BE REACHED
/Users/chris/WebKit/OpenSource/Source/WebCore/css/CSSPrimitiveValueMappings.h(142) : WebCore::LineClampValue WebCore::CSSPrimitiveValue::operator LineClampValue() const
1   0x10f752770 WTFCrash
2   0x11293f879 WebCore::CSSPrimitiveValue::operator WebCore::LineClampValue<WebCore::LineClampValue>() const
3   0x11293890e WebCore::StyleBuilderFunctions::applyValueWebkitLineClamp(WebCore::StyleResolver&, WebCore::CSSValue&)
4   0x11293064a WebCore::StyleBuilder::applyProperty(WebCore::CSSPropertyID, WebCore::StyleResolver&, WebCore::CSSValue&, bool, bool)
5   0x112978f03 WebCore::StyleResolver::applyProperty(WebCore::CSSPropertyID, WebCore::CSSValue*)
6   0x1129867e7 WebCore::StyleResolver::CascadedProperties::Property::apply(WebCore::StyleResolver&)
7   0x1129789ea WebCore::StyleResolver::applyCascadedProperties(WebCore::StyleResolver::CascadedProperties&, int, int)
8   0x112977137 WebCore::StyleResolver::applyMatchedProperties(WebCore::StyleResolver::MatchResult const&, WebCore::Element const*, WebCore::StyleResolver::ShouldUseMatchedPropertiesCache)
9   0x112974dc3 WebCore::StyleResolver::styleForElement(WebCore::Element*, WebCore::RenderStyle*, WebCore::StyleSharingBehavior, WebCore::RuleMatchingBehavior, WebCore::RenderRegion const*)
10  0x1129a6513 WebCore::Style::styleForElement(WebCore::Element&, WebCore::RenderStyle&)
11  0x1129a4482 WebCore::Style::resolveLocal(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
12  0x1129a1ebd WebCore::Style::resolveTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
13  0x1129a211b WebCore::Style::resolveTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
14  0x1129a211b WebCore::Style::resolveTree(WebCore::Element&, WebCore::RenderStyle&, WebCore::Style::RenderTreePosition&, WebCore::Style::Change)
15  0x1129a1d78 WebCore::Style::resolveTree(WebCore::Document&, WebCore::Style::Change)
16  0x11122a516 WebCore::Document::recalcStyle(WebCore::Style::Change)
17  0x1112266ff WebCore::Document::updateStyleIfNeeded()
18  0x1112209e9 WebCore::Document::styleRecalcTimerFired(WebCore::Timer&)
Comment 1 Chris Dumez 2014-11-16 12:15:36 PST 
Created attachment 241683 [details]
Patch
Comment 2 Benjamin Poulain 2014-11-16 16:09:36 PST 
Comment on attachment 241683 [details]
Patch

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

> LayoutTests/fast/css/webkit-line-clamp-calculated-value.html:16
> +shouldBeEmptyString("testDiv.style['-webkit-line-clamp']");
> +evalAndLog("testDiv.style['-webkit-line-clamp'] = 'calc(10% * 2)'");
> +shouldBeEqualToString("testDiv.style['-webkit-line-clamp']", "calc(20%)");
> +shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('-webkit-line-clamp')", "20%");
> +evalAndLog("testDiv.style['-webkit-line-clamp'] = 'calc(2 * 3)'");
> +shouldBeEqualToString("testDiv.style['-webkit-line-clamp']", "calc(6)");
> +shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('-webkit-line-clamp')", "6");

I suggest building this by having an array [input, expectedStyle, computedStyle] and stress test the hell out of it (all operator and units combinations).

Since this is returning and integer, I would also test: boundaries integers values. Boundary floating point values, NaN, etc.
Comment 3 Chris Dumez 2014-11-16 16:12:14 PST 
Comment on attachment 241683 [details]
Patch

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

>> LayoutTests/fast/css/webkit-line-clamp-calculated-value.html:16
>> +shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('-webkit-line-clamp')", "6");
> 
> I suggest building this by having an array [input, expectedStyle, computedStyle] and stress test the hell out of it (all operator and units combinations).
> 
> Since this is returning and integer, I would also test: boundaries integers values. Boundary floating point values, NaN, etc.

We already have extensive testing for calc() support (operators, ...).
Comment 4 Benjamin Poulain 2014-11-16 16:19:17 PST 
(In reply to comment #3)
> We already have extensive testing for calc() support (operators, ...).

I certainly hope so :)

But apparently LineClampValue's testing is lacking or we would have discovered this problem sooner.
Comment 5 Chris Dumez 2014-11-16 16:21:42 PST 
(In reply to comment #4)
> (In reply to comment #3)
> > We already have extensive testing for calc() support (operators, ...).
> 
> I certainly hope so :)
> 
> But apparently LineClampValue's testing is lacking or we would have
> discovered this problem sooner.

Right, the issue is with the *type* of value that is expected for each CSS property. This is why I am fuzzy testing all CSS properties an finding these bugs. Testing support for different calc operators or corner cases is out-of-scope here IMHO, this is an orthogonal issue and already well tested.
Comment 6 Benjamin Poulain 2014-11-16 16:28:05 PST 
(In reply to comment #5)
> Right, the issue is with the *type* of value that is expected for each CSS
> property. This is why I am fuzzy testing all CSS properties an finding these
> bugs. Testing support for different calc operators or corner cases is
> out-of-scope here IMHO, this is an orthogonal issue and already well tested.

You are right, testing all cases of calc() is not enough.

Ideally we should test all keywords in addition to calc, and do that for every property.

Maybe you should write a test script listing all CSS keyword, some useful values (boundaries, negative, etc) and all CSS property. Then have tests trying all combinations (ideally split on multiple files or it will timeout).
Comment 7 Chris Dumez 2014-11-16 16:30:18 PST 
(In reply to comment #6)
> (In reply to comment #5)
> > Right, the issue is with the *type* of value that is expected for each CSS
> > property. This is why I am fuzzy testing all CSS properties an finding these
> > bugs. Testing support for different calc operators or corner cases is
> > out-of-scope here IMHO, this is an orthogonal issue and already well tested.
> 
> You are right, testing all cases of calc() is not enough.
> 
> Ideally we should test all keywords in addition to calc, and do that for
> every property.
> 
> Maybe you should write a test script listing all CSS keyword, some useful
> values (boundaries, negative, etc) and all CSS property. Then have tests
> trying all combinations (ideally split on multiple files or it will timeout).

This sounds like the test I am currently running locally to find all these. However, it is crashing like crazy at the moment.
Comment 8 Benjamin Poulain 2014-11-16 16:37:38 PST 
Comment on attachment 241683 [details]
Patch

Let's fix all the bugs and land your test afterwards then.
Comment 9 Chris Dumez 2014-11-16 16:39:27 PST 
Comment on attachment 241683 [details]
Patch

Thanks, I am trying to file more bugs now in case people want to help.
Comment 10 WebKit Commit Bot 2014-11-16 17:18:27 PST 
Comment on attachment 241683 [details]
Patch

Clearing flags on attachment: 241683

Committed r176165: <http://trac.webkit.org/changeset/176165>
Comment 11 WebKit Commit Bot 2014-11-16 17:18:33 PST 
All reviewed patches have been landed.  Closing bug.