Bug 138777 - Crash when setting '-webkit-line-clamp' CSS property to a calculated value
Summary: Crash when setting '-webkit-line-clamp' 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 10:37 PST by Chris Dumez
Modified: 2014-11-16 17:18 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.29 KB, patch)
2014-11-16 12:15 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 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.