Bug 67139 – Early return in CSSPrimitiveValue::getDoubleValueInternal() omits additional invalid enums

• Component: CSS
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

* SUMMARY
The early return code in CSSPrimitiveValue::getDoubleValueInternal() in CSSPrimitiveValue.cpp ignores the newly added CSS_COUNTER_NAME enum because it checks for currently-known illegal values instead of currently-known legal values.

Current code:

bool CSSPrimitiveValue::getDoubleValueInternal(UnitTypes requestedUnitType, double* result) const
{
    if (m_type < CSS_NUMBER || (m_type > CSS_DIMENSION && m_type < CSS_TURN) || requestedUnitType < CSS_NUMBER || (requestedUnitType > CSS_DIMENSION && requestedUnitType < CSS_TURN))
        return false;

Instead it should do something like this:

bool CSSPrimitiveValue::getDoubleValueInternal(UnitTypes requestedUnitType, double* result) const
{
    if (!isValidCSSUnitTypeForDoubleConversion(m_type) || !isValidCSSUnitTypeForDoubleConversion(requestedUnitType))
        return false;

And then have a static inline method that handles all of them with a switch statement (so that you get a compiler warning if a new CSSPrimitiveValue::UnitTypes enum is added that isn't in the list:

static inline bool isValidCSSUnitTypeForDoubleConversion(UnitTypes unitType)
{
    switch (unitType) {
    case CSS_UNKNOWN:
        return false;
    case CSS_NUMBER:
    ...
    case CSS_DIMENSION:
        return true;
    case CSS_STRING:
    ...
    case CSS_PARSER_IDENTIFIER:
        return false;
    case CSS_TURN:
    case CSS_REMS:
        return true;
    case CSS_COUNTER_NAME:
    case CSS_FROM_FLOW:
    case CSS_SHAPE:
        return false;
    }

    ASSERT_NOT_REACHED();
    return false;
}

The original code was added in ToT WebKit r72189.  <http://trac.webkit.org/changeset/72189>

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> * SUMMARY
> The early return code in CSSPrimitiveValue::getDoubleValueInternal() in CSSPrimitiveValue.cpp ignores the newly added CSS_COUNTER_NAME enum because it checks for currently-known illegal values instead of currently-known legal values.

As well as CSS_FROM_FLOW and CSS_SHAPE now.

[Alexander Pavlov (apavlov)]

Created attachment 105643 [details]
[PATCH] Suggested fix

Thanks for the tip David.

[Darin Adler]

Comment on attachment 105643 [details]
[PATCH] Suggested fix

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

r=me even though I think we can make some small improvements

> Source/WebCore/css/CSSPrimitiveValue.cpp:53
> +    switch (unitType) {

Using a switch is a great technique to make sure we make a conscious decision for each enum value. But I’d suggest just having one block for false and one for true, sorted alphabetically. The order now may have some logic to it, but it’s not obvious and I think it would be better to not have that.

> Source/WebCore/css/CSSPrimitiveValue.cpp:86
> +    case CSSPrimitiveValue:: CSS_UNICODE_RANGE:
> +
> +    case CSSPrimitiveValue:: CSS_PARSER_OPERATOR:

This blank line looks like a mistake.

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> (From update of attachment 105643 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105643&action=review
> 
> r=me even though I think we can make some small improvements
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:53
> > +    switch (unitType) {
> 
> Using a switch is a great technique to make sure we make a conscious decision for each enum value. But I’d suggest just having one block for false and one for true, sorted alphabetically. The order now may have some logic to it, but it’s not obvious and I think it would be better to not have that.
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:86
> > +    case CSSPrimitiveValue:: CSS_UNICODE_RANGE:
> > +
> > +    case CSSPrimitiveValue:: CSS_PARSER_OPERATOR:
> 
> This blank line looks like a mistake.

Thanks Darin, I will land the patch tomorrow with these nits fixed.

[Alexander Pavlov (apavlov)]

Committed r94169: <http://trac.webkit.org/changeset/94169>

[David Kilzer (:ddkilzer)]

FWIW, I think a test could have been written for this using the fact that certain CSS properties would return a value instead of throwing an exception.