ASSERTION FAILED: !valueWithCalculation.calculation() in WebCore::CSSParser::validate...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Jan 2015 05:29:09 +0000 (05:29 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Jan 2015 05:29:09 +0000 (05:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=140251

Reviewed by Darin Adler.

Source/WebCore:

Using a calculated value for text-shadow's blur-radius was hitting an
assertion in CSSParser::validateCalculationUnit() because validUnit()
is called twice, first with 'FLength' unit, then more stricly with
'FLength|FNonNeg' if parsing the blur-radius as it cannot be negative
as per the specification:
- http://dev.w3.org/csswg/css-text-decor-3/#text-shadow-property
- http://dev.w3.org/csswg/css-backgrounds-3/#shadow

On the second call, the ValueWithCalculation's m_calculation member
was already initialized and the code did not handle this. This patch
updates validateCalculationUnit() to teach it to reuse the previously
parsed calculation in this case. All it needs to do is to update the
existing CSSCalcValue's range to allow negative values or not.

When writing the layout test for this, I also noticed that the CSS
parser was not rejecting negative calculated values for blur-radius
(only negative non-calculated ones). This is because
validateCalculationUnit() was ignoring FNonNeg if the calculated
value is a Length. This patch also addresses the issue.

Test: fast/css/text-shadow-calc-value.html

* css/CSSCalculationValue.h:
(WebCore::CSSCalcValue::setPermittedValueRange):
Add a setter to update the CSSCalculationValue's permitted value range
so that the CSS parser does not need to fully reparse the calculation
only to update the permitted value range.

* css/CSSParser.cpp:
(WebCore::CSSParser::validateCalculationUnit):
- Teach the code to reuse the previously parsed calculation value.
- Do the FNonNeg check for Length calculations as well.

LayoutTests:

Add a layout test to check that using calculated values for
'text-shadow' CSS doesn't crash and works as intended. Also check
that the CSS parser is correctly validating the blur-radius, which
is supposed to be non-negative, as per the specification:
- http://dev.w3.org/csswg/css-text-decor-3/#text-shadow-property
- http://dev.w3.org/csswg/css-backgrounds-3/#shadow

* fast/css/text-shadow-calc-value-expected.txt: Added.
* fast/css/text-shadow-calc-value.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@178156 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css/text-shadow-calc-value-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/text-shadow-calc-value.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSCalculationValue.h
Source/WebCore/css/CSSParser.cpp

index c93b4f1..337f0f2 100644 (file)
@@ -1,3 +1,20 @@
+2015-01-08  Chris Dumez  <cdumez@apple.com>
+
+        ASSERTION FAILED: !valueWithCalculation.calculation() in WebCore::CSSParser::validateCalculationUnit
+        https://bugs.webkit.org/show_bug.cgi?id=140251
+
+        Reviewed by Darin Adler.
+
+        Add a layout test to check that using calculated values for
+        'text-shadow' CSS doesn't crash and works as intended. Also check
+        that the CSS parser is correctly validating the blur-radius, which
+        is supposed to be non-negative, as per the specification:
+        - http://dev.w3.org/csswg/css-text-decor-3/#text-shadow-property
+        - http://dev.w3.org/csswg/css-backgrounds-3/#shadow
+
+        * fast/css/text-shadow-calc-value-expected.txt: Added.
+        * fast/css/text-shadow-calc-value.html: Added.
+
 2015-01-08  Benjamin Poulain  <bpoulain@apple.com>
 
         Make better use of the stack when compiling selectors
diff --git a/LayoutTests/fast/css/text-shadow-calc-value-expected.txt b/LayoutTests/fast/css/text-shadow-calc-value-expected.txt
new file mode 100644 (file)
index 0000000..3126bf1
--- /dev/null
@@ -0,0 +1,19 @@
+Tests assigning a calculated value to 'text-shadow' CSS property.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS testDiv.style['text-shadow'] is ""
+testDiv.style['text-shadow'] = 'calc(1 * 3px) calc(2 * 3px) calc(3 * 3px) rgb(255, 255, 255)'
+PASS testDiv.style['text-shadow'] is "rgb(255, 255, 255) calc(3px) calc(6px) calc(9px)"
+PASS window.getComputedStyle(testDiv).getPropertyValue('text-shadow') is "rgb(255, 255, 255) 3px 6px 9px"
+testDiv.style['text-shadow'] = 'calc(-1 * 3px) calc(-2 * 3px) calc(3 * 3px) rgb(255, 255, 255)'
+PASS testDiv.style['text-shadow'] is "rgb(255, 255, 255) calc(-3px) calc(-6px) calc(9px)"
+PASS window.getComputedStyle(testDiv).getPropertyValue('text-shadow') is "rgb(255, 255, 255) -3px -6px 9px"
+testDiv.style['text-shadow'] = 'calc(1 * 3px) calc(2 * 3px) calc(-3 * 3px) rgb(255, 255, 255)'
+PASS testDiv.style['text-shadow'] is previousStyle
+PASS window.getComputedStyle(testDiv).getPropertyValue('text-shadow') is previousComputedStyle
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/text-shadow-calc-value.html b/LayoutTests/fast/css/text-shadow-calc-value.html
new file mode 100644 (file)
index 0000000..517b72c
--- /dev/null
@@ -0,0 +1,31 @@
+<!DOCTYPE html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<div id="testDiv" style="position: absolute;"></div>
+<script>
+description("Tests assigning a calculated value to 'text-shadow' CSS property.");
+
+var testDiv = document.getElementById("testDiv");
+
+shouldBeEmptyString("testDiv.style['text-shadow']");
+evalAndLog("testDiv.style['text-shadow'] = 'calc(1 * 3px) calc(2 * 3px) calc(3 * 3px) rgb(255, 255, 255)'");
+shouldBeEqualToString("testDiv.style['text-shadow']", "rgb(255, 255, 255) calc(3px) calc(6px) calc(9px)");
+shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('text-shadow')", "rgb(255, 255, 255) 3px 6px 9px");
+
+// Negative h-shadow and v-shadow are allowed.
+evalAndLog("testDiv.style['text-shadow'] = 'calc(-1 * 3px) calc(-2 * 3px) calc(3 * 3px) rgb(255, 255, 255)'");
+shouldBeEqualToString("testDiv.style['text-shadow']", "rgb(255, 255, 255) calc(-3px) calc(-6px) calc(9px)");
+shouldBeEqualToString("window.getComputedStyle(testDiv).getPropertyValue('text-shadow')", "rgb(255, 255, 255) -3px -6px 9px")
+
+var previousStyle = testDiv.style['text-shadow'];
+var previousComputedStyle = window.getComputedStyle(testDiv).getPropertyValue('text-shadow');
+
+// Negative blur-radius is not allowed.
+evalAndLog("testDiv.style['text-shadow'] = 'calc(1 * 3px) calc(2 * 3px) calc(-3 * 3px) rgb(255, 255, 255)'");
+// text-shadow should not be updated.
+shouldBe("testDiv.style['text-shadow']", "previousStyle");
+shouldBe("window.getComputedStyle(testDiv).getPropertyValue('text-shadow')", "previousComputedStyle")
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
index 1f99b16..27769d0 100644 (file)
@@ -1,3 +1,43 @@
+2015-01-08  Chris Dumez  <cdumez@apple.com>
+
+        ASSERTION FAILED: !valueWithCalculation.calculation() in WebCore::CSSParser::validateCalculationUnit
+        https://bugs.webkit.org/show_bug.cgi?id=140251
+
+        Reviewed by Darin Adler.
+
+        Using a calculated value for text-shadow's blur-radius was hitting an
+        assertion in CSSParser::validateCalculationUnit() because validUnit()
+        is called twice, first with 'FLength' unit, then more stricly with
+        'FLength|FNonNeg' if parsing the blur-radius as it cannot be negative
+        as per the specification:
+        - http://dev.w3.org/csswg/css-text-decor-3/#text-shadow-property
+        - http://dev.w3.org/csswg/css-backgrounds-3/#shadow
+
+        On the second call, the ValueWithCalculation's m_calculation member
+        was already initialized and the code did not handle this. This patch
+        updates validateCalculationUnit() to teach it to reuse the previously
+        parsed calculation in this case. All it needs to do is to update the
+        existing CSSCalcValue's range to allow negative values or not.
+
+        When writing the layout test for this, I also noticed that the CSS
+        parser was not rejecting negative calculated values for blur-radius
+        (only negative non-calculated ones). This is because
+        validateCalculationUnit() was ignoring FNonNeg if the calculated
+        value is a Length. This patch also addresses the issue.
+
+        Test: fast/css/text-shadow-calc-value.html
+
+        * css/CSSCalculationValue.h:
+        (WebCore::CSSCalcValue::setPermittedValueRange):
+        Add a setter to update the CSSCalculationValue's permitted value range
+        so that the CSS parser does not need to fully reparse the calculation
+        only to update the permitted value range.
+
+        * css/CSSParser.cpp:
+        (WebCore::CSSParser::validateCalculationUnit):
+        - Teach the code to reuse the previously parsed calculation value.
+        - Do the FNonNeg check for Length calculations as well.
+
 2015-01-08  Darin Adler  <darin@apple.com>
 
         Modernize and streamline HTMLTokenizer
index 3c74d3a..c43e17f 100644 (file)
@@ -97,6 +97,7 @@ public:
     double computeLengthPx(const CSSToLengthConversionData&) const;
 
     Ref<CalculationValue> createCalculationValue(const CSSToLengthConversionData&) const;
+    void setPermittedValueRange(CalculationPermittedValueRange);
 
     String customCSSText() const;
     bool equals(const CSSCalcValue&) const;
@@ -107,7 +108,7 @@ private:
     double clampToPermittedRange(double) const;
 
     const Ref<CSSCalcExpressionNode> m_expression;
-    const bool m_shouldClampToNonNegative;
+    bool m_shouldClampToNonNegative;
 };
 
 inline CSSCalcValue::CSSCalcValue(Ref<CSSCalcExpressionNode>&& expression, bool shouldClampToNonNegative)
@@ -123,6 +124,11 @@ inline Ref<CalculationValue> CSSCalcValue::createCalculationValue(const CSSToLen
         m_shouldClampToNonNegative ? CalculationRangeNonNegative : CalculationRangeAll);
 }
 
+inline void CSSCalcValue::setPermittedValueRange(CalculationPermittedValueRange range)
+{
+    m_shouldClampToNonNegative = range != CalculationRangeAll;
+}
+
 } // namespace WebCore
 
 SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSCalcValue, isCalcValue())
index 64964ba..c9dc4c6 100644 (file)
@@ -1586,10 +1586,17 @@ bool CSSParser::validateCalculationUnit(ValueWithCalculation& valueWithCalculati
 {
     bool mustBeNonNegative = unitFlags & FNonNeg;
 
-    ASSERT(!valueWithCalculation.calculation());
-    RefPtr<CSSCalcValue> calculation = parseCalculation(valueWithCalculation, mustBeNonNegative ? CalculationRangeNonNegative : CalculationRangeAll);
-    if (!calculation)
-        return false;
+    RefPtr<CSSCalcValue> calculation;
+    if (valueWithCalculation.calculation()) {
+        // The calculation value was already parsed so we reuse it. However, we may need to update its range.
+        calculation = valueWithCalculation.calculation();
+        calculation->setPermittedValueRange(mustBeNonNegative ? CalculationRangeNonNegative : CalculationRangeAll);
+    } else {
+        valueWithCalculation.setCalculation(parseCalculation(valueWithCalculation, mustBeNonNegative ? CalculationRangeNonNegative : CalculationRangeAll));
+        calculation = valueWithCalculation.calculation();
+        if (!calculation)
+            return false;
+    }
 
     bool isValid = false;
     switch (calculation->category()) {
@@ -1604,6 +1611,8 @@ bool CSSParser::validateCalculationUnit(ValueWithCalculation& valueWithCalculati
         break;
     case CalcLength:
         isValid = (unitFlags & FLength);
+        if (isValid && mustBeNonNegative && calculation->isNegative())
+            isValid = false;
         break;
     case CalcPercent:
         isValid = (unitFlags & FPercent);
@@ -1628,8 +1637,7 @@ bool CSSParser::validateCalculationUnit(ValueWithCalculation& valueWithCalculati
     case CalcOther:
         break;
     }
-    if (isValid)
-        valueWithCalculation.setCalculation(WTF::move(calculation));
+
     return isValid;
 }