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.
There was a report on chromium at http://code.google.com/p/chromium/issues/detail?id=277799 as well.
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
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.
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
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
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
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
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
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
Created attachment 212805 [details] Patch Updated the expected results for /css3/calc/cssom.html. Fixed the LayoutTests ChangeLog entry.
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)
(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.
Created attachment 212834 [details] Patch Made the suggested changes.
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
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.
(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
Created attachment 212846 [details] Patch Added the FIXME bug URL.
Comment on attachment 212846 [details] Patch Clearing flags on attachment: 212846 Committed r156586: <http://trac.webkit.org/changeset/156586>
All reviewed patches have been landed. Closing bug.
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?
(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.