Bug 121020 – Crash on shape-outside when using calc()

• Component: CSS
• Status: RESOLVED FIXED

[Dirk Schulze]

Created attachment 211010 [details]
Crash test

When a users specifies the shape-outside property with calc(), the browser crashes:

-webkit-shape-outside: circle(50%, 50%, calc(50% - 20px));

Stack:

1   0x10778a4d0 WTFCrash
2   0x109664cfd WebCore::floatValueForLength(WebCore::Length const&, float, WebCore::RenderView*)
3   0x109c08029 WebCore::Shape::createShape(WebCore::BasicShape const*, WebCore::LayoutSize const&, WebCore::WritingMode, WebCore::Length, WebCore::Length)
4   0x109c0b115 WebCore::ShapeInfo<WebCore::RenderBlock, &(WebCore::RenderStyle::resolvedShapeInside() const), &(WebCore::Shape::getIncludedIntervals(WebCore::LayoutUnit, WebCore::LayoutUnit, WTF::Vector<WebCore::LineSegment, 0ul, WTF::CrashOnOverflow>&) const)>::computedShape() const
5   0x1098712b1 WebCore::ShapeInsideInfo::computedShapeLogicalBoundingBox() const
6   0x10986d251 WebCore::ShapeInfo<WebCore::RenderBlock, &(WebCore::RenderStyle::resolvedShapeInside() const), &(WebCore::Shape::getIncludedIntervals(WebCore::LayoutUnit, WebCore::LayoutUnit, WTF::Vector<WebCore::LineSegment, 0ul, WTF::CrashOnOverflow>&) const)>::shapeLogicalTop() const
7   0x10987881b WebCore::RenderBlock::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int)

BasicShapeFunction.cpp:114 static Length convertToLength(...

does not have CalculatedConversion as possible type.

[Dirk Schulze]

There was a report on chromium at http://code.google.com/p/chromium/issues/detail?id=277799 as well.

[Hans Muller]

As Dirk pointed out, the fix for the immediate problem is trivial. Just add CalculatedConversion to the convertToLength() supportTypes template parameter in BasicShapeFunctions.cpp/convertToLength(). And similarly, this exposes a lacuna in CSSPrimitiveValue::CSSPrimitiveValue(const Length&)

window.getComputedStyle(document.getElementById("shape-inside"))["-webkit-shape-inside"]

[CRASH]

frame #6: 0x0000000104c9777f WebCore`WebCore::ComputedStyleExtractor::propertyValue(WebCore::CSSPropertyID, WebCore::EUpdateLayout) const + 47759 at CSSComputedStyleDeclaration.cpp:2847
   2844	                return cssValuePool().createIdentifierValue(CSSValueNone);
   2845	            }
   2846	            ASSERT(style->shapeInside()->type() == ShapeValue::Shape);
-> 2847	            return valueForBasicShape(style->shapeInside()->shape());
   2848	        case CSSPropertyWebkitShapeOutside:
   2849	            if (!style->shapeOutside())
   2850	                return cssValuePool().createIdentifierValue(CSSValueAuto);

The valueForBasicShape() implementation uses CSSValuePool.createValue() which uses the aforementioned CSSPrimitiveValue() constructor.
The constructor fails because it's currently unsupported for Lengths whose type is CALCULATED.

CSSPrimitiveValue::CSSPrimitiveValue(const Length& length)
{
...
        case Calculated:
        case Relative:
        case Undefined:
            ASSERT_NOT_REACHED();
            break;
}

This issue appears to have been addressed in Blink, see http://src.chromium.org/viewvc/blink?view=revision&revision=156683

[Hans Muller]

Created attachment 212774 [details]
Patch

This change prevents a crash caused by specifying a CSS Shape geometry Length attribute with a calc() expression. It adds support for converting Lengths to CSSPrimitive Values, in large part by migrating Blink changes made to the calc classes since the split. Doing so required a few supporting changes in some related classes, notably CSSPrimitiveValue.

[Build Bot]

Comment on attachment 212774 [details]
Patch

Attachment 212774 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2629043

New failing tests:
css3/calc/cssom.html

[Build Bot]

Created attachment 212778 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212774 [details]
Patch

Attachment 212774 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2632042

New failing tests:
css3/calc/cssom.html

[Build Bot]

Created attachment 212785 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 212774 [details]
Patch

Attachment 212774 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2652011

New failing tests:
css3/calc/cssom.html

[Build Bot]

Created attachment 212786 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Hans Muller]

Created attachment 212805 [details]
Patch

Updated the expected results for /css3/calc/cssom.html. Fixed the LayoutTests ChangeLog entry.

[Dirk Schulze]

Comment on attachment 212805 [details]
Patch

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

Thanks for all the work Hans! Great back porting patch. And thanks for cleaning up the source code as well. Just some snippets inside. Really just minor things but I would like to look over the patch again.

> LayoutTests/css3/calc/cssom-expected.txt:12
> -100px * (1 + 2 * 3 - 4 / 5) => calc(100px * ((1 + (2 * 3)) - (4 / 5)))
> -(100px) + 200px => calc(100px + 200px)
> +100px * (1 + 2 * 3 - 4 / 5) => calc(620px)
> +(100px) + 200px => calc(300px)

Do we have nested calc's as well? This is important if you animate from one calc function to a real value on CSS Shapes. So we need a test for that. I believe a small test calc(5px + calc(3px * 5%)) is sufficient enough.

> Source/WebCore/css/BasicShapeFunctions.cpp:41
> +static PassRefPtr<CSSPrimitiveValue> convertToCSSPrimitiveValue(const RenderStyle* style, const Length& length)

This function actually makes the calls longer. Compare the both:
 
pool.createValue(rectangle->x(), style);
convertToCSSPrimitiveValue(style, rectangle->x())

It doesn't look like it is worth it and you have to look at another place to see what is going on as well. I suggest removing this function again.

> Source/WebCore/css/BasicShapeFunctions.cpp:121
> +    return value->convertToLength<CalculatedConversion | FixedIntegerConversion | FixedFloatConversion | PercentConversion | ViewportPercentageConversion>(style, rootStyle, style->effectiveZoom());

I think CalculatedConversion is lined up after PercentConversion every where else. Just a snippet, but makes search and replace easier later when we refactor the code :D

> Source/WebCore/css/CSSCalculationValue.cpp:273
> +

One newline too many.

> Source/WebCore/css/CSSCalculationValue.cpp:292
> +        ASSERT_NOT_REACHED();

We already have the assertion in the switch, you'll never reach it.

> Source/WebCore/css/CSSCalculationValue.cpp:326
> +/* CalcNumber */        { CalcNumber,        CalcOther,         CalcPercentNumber, CalcPercentNumber, CalcOther },
> +/* CalcLength */        { CalcOther,         CalcLength,        CalcPercentLength, CalcOther,         CalcPercentLength },
> +/* CalcPercent */       { CalcPercentNumber, CalcPercentLength, CalcPercent,       CalcPercentNumber, CalcPercentLength },
> +/* CalcPercentNumber */ { CalcPercentNumber, CalcOther,         CalcPercentNumber, CalcPercentNumber, CalcOther },
> +/* CalcPercentLength */ { CalcOther,         CalcPercentLength, CalcPercentLength, CalcOther,         CalcPercentLength },

Do you mind if you add these comments at the end of each line?

> Source/WebCore/css/CSSCalculationValue.cpp:362
> +    // Not testing for actual integer values.

I don't understand this comment.

> Source/WebCore/css/CSSCalculationValue.cpp:383
> +    static PassRefPtr<CSSCalcExpressionNode> createSimplified(PassRefPtr<CSSCalcExpressionNode> leftSide, PassRefPtr<CSSCalcExpressionNode> rightSide, CalcOperator op)

Wow! This function is massive! Do we have tests for special cases like calc(a + b / calc(d * calc(e + f))) ?

> Source/WebCore/css/CSSCalculationValue.cpp:748
> +        // FIXME(crbug.com/269320): Create a CSSCalcExpressionNode equivalent of CalcExpressionBlendLength.

Hmmm. I am not sure if we should leave that in WebKit source code. I think we have a WebKit bug for it already. Can you search it and replace the comment with the webkit bug please?

> Source/WebCore/css/CSSCalculationValue.cpp:795
> +    ASSERT_NOT_REACHED();

I am unsure if we should leave these assertions. But maybe we should if someone forgets to add an return. But then we should remove the assertion and return in case Undefined:. (Same with the other switches)

[Hans Muller]

(In reply to comment #11)
> (From update of attachment 212805 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212805&action=review
> 
> > LayoutTests/css3/calc/cssom-expected.txt:12
> > -100px * (1 + 2 * 3 - 4 / 5) => calc(100px * ((1 + (2 * 3)) - (4 / 5)))
> > -(100px) + 200px => calc(100px + 200px)
> > +100px * (1 + 2 * 3 - 4 / 5) => calc(620px)
> > +(100px) + 200px => calc(300px)
> 
> Do we have nested calc's as well? This is important if you animate from one calc function to a real value on CSS Shapes. So we need a test for that. I believe a small test calc(5px + calc(3px * 5%)) is sufficient enough.

We don't have nested calc's yet.  

> > Source/WebCore/css/CSSCalculationValue.cpp:292
> > +        ASSERT_NOT_REACHED();
> 
> We already have the assertion in the switch, you'll never reach it.

I agree however I think it's there to keep the Windows compiler from failing.  I think there was a discussion about this on webkit-dev recently although I haven't been able to find it.

> > Source/WebCore/css/CSSCalculationValue.cpp:362
> > +    // Not testing for actual integer values.
> 
> I don't understand this comment.

I think it's drawing a distinction between the calc() "resolved type" of the expression and the actual type of the result.  For example the result of 4/2 is not an integer per the spec. I don't think this part of the comment is useful (the spec reference below it is), so I've removed it.

> > Source/WebCore/css/CSSCalculationValue.cpp:383
> > +    static PassRefPtr<CSSCalcExpressionNode> createSimplified(PassRefPtr<CSSCalcExpressionNode> leftSide, PassRefPtr<CSSCalcExpressionNode> rightSide, CalcOperator op)
> 
> Wow! This function is massive! Do we have tests for special cases like calc(a + b / calc(d * calc(e + f))) ?

Good point.  I'd forgotten to migrate the test added to check the new simplification code.

> 
> > Source/WebCore/css/CSSCalculationValue.cpp:748
> > +        // FIXME(crbug.com/269320): Create a CSSCalcExpressionNode equivalent of CalcExpressionBlendLength.
> 
> Hmmm. I am not sure if we should leave that in WebKit source code. I think we have a WebKit bug for it already. Can you search it and replace the comment with the webkit bug please?

I wasn't able to locate an existing bug for this.  I've removed the Blink bug reference for now.

[Hans Muller]

Created attachment 212834 [details]
Patch

Made the suggested changes.

[Bem Jones-Bey]

Comment on attachment 212805 [details]
Patch

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

>>> Source/WebCore/css/CSSCalculationValue.cpp:292
>>> +        ASSERT_NOT_REACHED();
>> 
>> We already have the assertion in the switch, you'll never reach it.
> 
> I agree however I think it's there to keep the Windows compiler from failing.  I think there was a discussion about this on webkit-dev recently although I haven't been able to find it.

It's Qt, actually. Here's the thread: https://lists.webkit.org/pipermail/webkit-dev/2013-September/025422.html

[Dirk Schulze]

Comment on attachment 212834 [details]
Patch

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

Great patch. Just a snippet. r+

> Source/WebCore/css/CSSCalculationValue.cpp:746
> +        // FIXME: Create a CSSCalcExpressionNode equivalent of CalcExpressionBlendLength.

If this bug does not exist yet, please create it and add the link in the comment.

[Hans Muller]

(In reply to comment #15)
> (From update of attachment 212834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212834&action=review
> 
> Great patch. Just a snippet. r+
> 
> > Source/WebCore/css/CSSCalculationValue.cpp:746
> > +        // FIXME: Create a CSSCalcExpressionNode equivalent of CalcExpressionBlendLength.
> 
> If this bug does not exist yet, please create it and add the link in the comment.

I created a WebKit version of the Bink bug, it's https://bugs.webkit.org/show_bug.cgi?id=122036

[Hans Muller]

Created attachment 212846 [details]
Patch

Added the FIXME bug URL.

[WebKit Commit Bot]

Comment on attachment 212846 [details]
Patch

Clearing flags on attachment: 212846

Committed r156586: <http://trac.webkit.org/changeset/156586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 212846 [details]
Patch

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

> Source/WebCore/css/CSSValuePool.h:55
> +    PassRefPtr<CSSPrimitiveValue> createValue(const Length& value, const RenderStyle* style) { return CSSPrimitiveValue::create(value, style); }

A value with style? How does that make sense?

[Hans Muller]

(In reply to comment #20)
> (From update of attachment 212846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212846&action=review
> 
> > Source/WebCore/css/CSSValuePool.h:55
> > +    PassRefPtr<CSSPrimitiveValue> createValue(const Length& value, const RenderStyle* style) { return CSSPrimitiveValue::create(value, style); }
> 
> A value with style? How does that make sense?

To c(In reply to comment #20)
> (From update of attachment 212846 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212846&action=review
> 
> > Source/WebCore/css/CSSValuePool.h:55
> > +    PassRefPtr<CSSPrimitiveValue> createValue(const Length& value, const RenderStyle* style) { return CSSPrimitiveValue::create(value, style); }
> 
> A value with style? How does that make sense?

It's really just there to provide access to the style's effectiveZoom, which is needed to compute the value of pixel type lengths. The CSSPrimitiveValue doesn't actually hang on to the style.