ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size.

https://bugs.webkit.org/show_bug.cgi?id=88866

Reviewed by Alexander Pavlov.

Source/WebCore:

StylePropertySet::fontValue always assumed that it was called using
style.font after a subsequent call which set the shorthand font. The
ASSERT_NOT_REACHED assumed that all longhands of the font shorthand not
set by the shorthand itself were set to initial. While it's true when
we set the font shorthand (i.e all longhands are set to implicit initial)
it is not true when you set the longhands individually. For example setting
font-size will not set other font properties to initial. It is the behavior of all
other shorthands in WebKit. When reconstructing the shorthand other
properties tests whether the value of each longhands is initial or not
(if not then we omit the value, as we should always construct the
shortest shorthand possible) or if the value is set or not (if set then
we include it in the shorthand if not then we omit it). The comment
removed was also talking about invalid font property potentially built
by fontValue(). So far appendFontLonghandValueIfExplicit will always
construct a valid value as it takes care of adding ' ' or '/' when
needed, so the return value is parsable and correct.

Test: fast/css/font-shorthand-from-longhands.html

* css/StylePropertySet.cpp:
(WebCore::StylePropertySet::appendFontLonghandValueIfExplicit):
(WebCore::StylePropertySet::fontValue):
* css/StylePropertySet.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@139313 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Change-Id: I46a4b1d023f3cb8f4e7b4c0b7f176b5b9f850870
Reviewed-by: Simon Hausmann <simon.hausmann@digia.com>

3 files changed, 48 insertions, 22 deletions

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 7b3d70b5a..3f75558f6 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2013-01-10  Alexis Menard  <alexis@webkit.org>
+
+        ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size.
+        https://bugs.webkit.org/show_bug.cgi?id=88866
+
+        Reviewed by Alexander Pavlov.
+
+        StylePropertySet::fontValue always assumed that it was called using
+        style.font after a subsequent call which set the shorthand font. The
+        ASSERT_NOT_REACHED assumed that all longhands of the font shorthand not
+        set by the shorthand itself were set to initial. While it's true when
+        we set the font shorthand (i.e all longhands are set to implicit initial)
+        it is not true when you set the longhands individually. For example setting
+        font-size will not set other font properties to initial. It is the behavior of all
+        other shorthands in WebKit. When reconstructing the shorthand other
+        properties tests whether the value of each longhands is initial or not
+        (if not then we omit the value, as we should always construct the
+        shortest shorthand possible) or if the value is set or not (if set then
+        we include it in the shorthand if not then we omit it). The comment
+        removed was also talking about invalid font property potentially built
+        by fontValue(). So far appendFontLonghandValueIfExplicit will always
+        construct a valid value as it takes care of adding ' ' or '/' when
+        needed, so the return value is parsable and correct.
+
+        Test: fast/css/font-shorthand-from-longhands.html
+
+        * css/StylePropertySet.cpp:
+        (WebCore::StylePropertySet::appendFontLonghandValueIfExplicit):
+        (WebCore::StylePropertySet::fontValue):
+        * css/StylePropertySet.h:
+
 2012-12-19  Simon Hausmann  <simon.hausmann@digia.com>, Jedrzej Nowacki <jedrzej.nowacki@digia.com>
 
         [Qt] JS bridge does not transmit QVariants anymore in Qt5
diff --git a/Source/WebCore/css/StylePropertySet.cpp b/Source/WebCore/css/StylePropertySet.cpp
index f662ccacf..ccd7d8a62 100644
--- a/Source/WebCore/css/StylePropertySet.cpp
+++ b/Source/WebCore/css/StylePropertySet.cpp
@@ -225,15 +225,15 @@ String StylePropertySet::borderSpacingValue(const StylePropertyShorthand& shorth
     return horizontalValueCSSText + ' ' + verticalValueCSSText;
 }
 
-bool StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID, StringBuilder& result, String& commonValue) const
+void StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID, StringBuilder& result, String& commonValue) const
 {
     int foundPropertyIndex = findPropertyIndex(propertyID);
     if (foundPropertyIndex == -1)
-        return false; // All longhands must have at least implicit values if "font" is specified.
+        return; // All longhands must have at least implicit values if "font" is specified.
 
     if (propertyAt(foundPropertyIndex).isImplicit()) {
         commonValue = String();
-        return true;
+        return;
     }
 
     char prefix = '\0';
@@ -258,37 +258,32 @@ bool StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyI
     result.append(value);
     if (!commonValue.isNull() && commonValue != value)
         commonValue = String();
-
-    return true;
 }
 
 String StylePropertySet::fontValue() const
 {
-    int foundPropertyIndex = findPropertyIndex(CSSPropertyFontSize);
-    if (foundPropertyIndex == -1)
+    int fontSizePropertyIndex = findPropertyIndex(CSSPropertyFontSize);
+    int fontFamilyPropertyIndex = findPropertyIndex(CSSPropertyFontFamily);
+    if (fontSizePropertyIndex == -1 || fontFamilyPropertyIndex == -1)
         return emptyString();
 
-    PropertyReference fontSizeProperty = propertyAt(foundPropertyIndex);
-    if (fontSizeProperty.isImplicit())
+    PropertyReference fontSizeProperty = propertyAt(fontSizePropertyIndex);
+    PropertyReference fontFamilyProperty = propertyAt(fontFamilyPropertyIndex);
+    if (fontSizeProperty.isImplicit() || fontFamilyProperty.isImplicit())
         return emptyString();
 
     String commonValue = fontSizeProperty.value()->cssText();
     StringBuilder result;
-    bool success = true;
-    success &= appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue);
-    success &= appendFontLonghandValueIfExplicit(CSSPropertyFontVariant, result, commonValue);
-    success &= appendFontLonghandValueIfExplicit(CSSPropertyFontWeight, result, commonValue);
+    appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue);
+    appendFontLonghandValueIfExplicit(CSSPropertyFontVariant, result, commonValue);
+    appendFontLonghandValueIfExplicit(CSSPropertyFontWeight, result, commonValue);
     if (!result.isEmpty())
         result.append(' ');
     result.append(fontSizeProperty.value()->cssText());
-    success &= appendFontLonghandValueIfExplicit(CSSPropertyLineHeight, result, commonValue);
-    success &= appendFontLonghandValueIfExplicit(CSSPropertyFontFamily, result, commonValue);
-    if (!success) {
-        // An invalid "font" value has been built (should never happen, as at least implicit values
-        // for mandatory longhands are always found in the style), report empty value instead.
-        ASSERT_NOT_REACHED();
-        return emptyString();
-    }
+    appendFontLonghandValueIfExplicit(CSSPropertyLineHeight, result, commonValue);
+    if (!result.isEmpty())
+        result.append(' ');
+    result.append(fontFamilyProperty.value()->cssText());
     if (isInitialOrInherit(commonValue))
         return commonValue;
     return result.toString();
diff --git a/Source/WebCore/css/StylePropertySet.h b/Source/WebCore/css/StylePropertySet.h
index 456f7ef2e..40eab1d13 100644
--- a/Source/WebCore/css/StylePropertySet.h
+++ b/Source/WebCore/css/StylePropertySet.h
@@ -197,7 +197,7 @@ private:
     String get4Values(const StylePropertyShorthand&) const;
     String borderSpacingValue(const StylePropertyShorthand&) const;
     String fontValue() const;
-    bool appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const;
+    void appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const;
 
     bool removeShorthandProperty(CSSPropertyID);
     bool propertyMatches(const PropertyReference&) const;