Bug 138777 – Crash when setting '-webkit-line-clamp' CSS property to a calculated value

• Component: CSS
• Status: RESOLVED FIXED

[Chris Dumez]

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&)

[Chris Dumez]

Created attachment 241683 [details]
Patch

[Benjamin Poulain]

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.

[Chris Dumez]

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, ...).

[Benjamin Poulain]

(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.

[Chris Dumez]

(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.

[Benjamin Poulain]

(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).

[Chris Dumez]

(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.

[Benjamin Poulain]

Comment on attachment 241683 [details]
Patch

Let's fix all the bugs and land your test afterwards then.

[Chris Dumez]

Comment on attachment 241683 [details]
Patch

Thanks, I am trying to file more bugs now in case people want to help.

[WebKit Commit Bot]

Comment on attachment 241683 [details]
Patch

Clearing flags on attachment: 241683

Committed r176165: <http://trac.webkit.org/changeset/176165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.