Bug 104131 – REGRESSION (r136683): css3/calc/background-position-parsing.html failing on EFL Linux 64-bit Debug WK2

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Thiago Marcos P. Santos]

Created attachment 177770 [details]
backtrace

css3/calc/background-position-parsing.html started failing on EFL Linux 64-bit Debug WK2 in r136683 <http://trac.webkit.org/changeset/136683>.

http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r136682%20(6697)/results.html passed
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r136683%20(6698)/results.html failed

[Alexis Menard (darktears)]

*** Bug 104133 has been marked as a duplicate of this bug. ***

[Alexis Menard (darktears)]

Created attachment 177779 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #2)
> Created an attachment (id=177779) [details]
> Patch

I'm wondering if the fix should go in isPositionValue instead. Thoughts?

[Alexis Menard (darktears)]

Created attachment 177782 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 177787 [details]
Fix comment

[Anders Carlsson]

Comment on attachment 177787 [details]
Fix comment

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

> Source/WebCore/css/CSSParser.cpp:3865
> +    m_parsedCalculation.release();

This should be m_parsedCalculation = nullptr;

[Antti Koivisto]

Comment on attachment 177787 [details]
Fix comment

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

> Source/WebCore/css/CSSParser.cpp:3867
>  inline bool CSSParser::isPositionValue(CSSParserValue* value)
>  {
> -    return isBackgroundPositionKeyword(value->id) || validUnit(value, FPercent | FLength);
> +    bool ret = isBackgroundPositionKeyword(value->id) || validUnit(value, FPercent | FLength);
> +    // validUnit may encounter a CSS calc() and could potentially set m_parsedCalculation to a value. As we may call later isPositionValue again
> +    // we need to unset it otherwise the next call to validUnit will assert when it will check a calc again. parseFillPositionComponent also workaround this behavior.
> +    m_parsedCalculation.release();
> +    return ret;
>  }

It seems wrong that a isFoo function has a side effect like this.

[Alexis Menard (darktears)]

Created attachment 177811 [details]
Patch

[WebKit Review Bot]

Comment on attachment 177811 [details]
Patch

Clearing flags on attachment: 177811

Committed r136754: <http://trac.webkit.org/changeset/136754>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.