Bug 83544 – Make CSSParser::parseValue()'s handling of CSSPropertyCursor more obviously correct.

• Component: CSS
• Status: RESOLVED FIXED

[Luke Macpherson]

Make CSSParser::parseValue()'s handling of CSSPropertyCursor more obviously correct.

[Luke Macpherson]

Created attachment 136389 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 136389 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:1595
> +        if (value) {

Why you assert here if you check with an if?

[Kentaro Hara]

Comment on attachment 136389 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:1581
> +        ASSERT(list || value);

I am not sure what the ASSERT() is for. It seems that the ASSERT(list || value) has the same meaning as the ASSERT(value) in the line 1594.

Other changes looks OK to me.

>> Source/WebCore/css/CSSParser.cpp:1595
>> +        if (value) {
> 
> Why you assert here if you check with an if?

Maybe because ASSERT() is ignored in the Release build.

[Alexis Menard (darktears)]

Comment on attachment 136389 [details]
Patch

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

>>> Source/WebCore/css/CSSParser.cpp:1595
>>> +        if (value) {
>> 
>> Why you assert here if you check with an if?
> 
> Maybe because ASSERT() is ignored in the Release build.

I know it is but what's the point of asserting here in debug only if you protect the code with the if after, just avoiding the crash in Release but you may get erratic behavior. An example in WebKit code :
bool AccessibilityRenderObject::isPasswordField() const
{
    ASSERT(m_renderer);
    if (!m_renderer->node() || !m_renderer->node()->isHTMLElement())

...

m_renderer validity is not tested.

To me assert means that if the assert is true well we're in bad shape, it should not happen. It's a nit pick maybe.

[Kentaro Hara]

Comment on attachment 136389 [details]
Patch

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

>>>> Source/WebCore/css/CSSParser.cpp:1595
>>>> +        if (value) {
>>> 
>>> Why you assert here if you check with an if?
>> 
>> Maybe because ASSERT() is ignored in the Release build.
> 
> I know it is but what's the point of asserting here in debug only if you protect the code with the if after, just avoiding the crash in Release but you may get erratic behavior. An example in WebKit code :
> bool AccessibilityRenderObject::isPasswordField() const
> {
>     ASSERT(m_renderer);
>     if (!m_renderer->node() || !m_renderer->node()->isHTMLElement())
> 
> ...
> 
> m_renderer validity is not tested.
> 
> To me assert means that if the assert is true well we're in bad shape, it should not happen. It's a nit pick maybe.

Ah, makes sense. Maybe removing the if statement is a solution, since we are assuming that 'value' should not be 0 here.

[Luke Macpherson]

Comment on attachment 136389 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:1581
>> +        ASSERT(list || value);
> 
> I am not sure what the ASSERT() is for. It seems that the ASSERT(list || value) has the same meaning as the ASSERT(value) in the line 1594.
> 
> Other changes looks OK to me.

This is to document (and enforce) the expected invariant that after the loop has executed, either value is set, or a list of values has been created. If neither of these are true, you have a problem.

>>>>> Source/WebCore/css/CSSParser.cpp:1595
>>>>> +        if (value) {
>>>> 
>>>> Why you assert here if you check with an if?
>>> 
>>> Maybe because ASSERT() is ignored in the Release build.
>> 
>> I know it is but what's the point of asserting here in debug only if you protect the code with the if after, just avoiding the crash in Release but you may get erratic behavior. An example in WebKit code :
>> bool AccessibilityRenderObject::isPasswordField() const
>> {
>>     ASSERT(m_renderer);
>>     if (!m_renderer->node() || !m_renderer->node()->isHTMLElement())
>> 
>> ...
>> 
>> m_renderer validity is not tested.
>> 
>> To me assert means that if the assert is true well we're in bad shape, it should not happen. It's a nit pick maybe.
> 
> Ah, makes sense. Maybe removing the if statement is a solution, since we are assuming that 'value' should not be 0 here.

What's going on here is that static analysis tools look at the "while (value && value->unit == CSSPrimitiveValue::CSS_URI) {" above, and deduce that a possible exit condition of the loop is that value is null. It turns out that we expect that if that has happened we would have handled it in the if (list) case above. Again, this is non-obvious to a cursory reading, and too convoluted for coverity to pick up. So here, the ASSERT is to document the expectation that if value was null, it should have been handled above in the if (list) case, and the if (value) was added to reassure any static analysis tools that we really are not going to do a null-pointer dereference here.

[Kentaro Hara]

Comment on attachment 136389 [details]
Patch

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

>>> Source/WebCore/css/CSSParser.cpp:1581
>>> +        ASSERT(list || value);
>> 
>> I am not sure what the ASSERT() is for. It seems that the ASSERT(list || value) has the same meaning as the ASSERT(value) in the line 1594.
>> 
>> Other changes looks OK to me.
> 
> This is to document (and enforce) the expected invariant that after the loop has executed, either value is set, or a list of values has been created. If neither of these are true, you have a problem.

Think this way:

Basically, ASSERT(... || ...) is not a good form since it is not intuitive. We want to write ASSERT(...) for one variable at the exact place. So let's consider how we can split ASSERT(list || value) into ASSERT(list) and ASSERT(value). Then the code will look like this:

if (list) {
    if (!value) {
        ...;
    }
}
ASSERT(value);    // already written
if (value) {
    ...;
}

This means that the original ASSERT(list || value) is not needed.

>>>>>> Source/WebCore/css/CSSParser.cpp:1595
>>>>>> +        if (value) {
>>>>> 
>>>>> Why you assert here if you check with an if?
>>>> 
>>>> Maybe because ASSERT() is ignored in the Release build.
>>> 
>>> I know it is but what's the point of asserting here in debug only if you protect the code with the if after, just avoiding the crash in Release but you may get erratic behavior. An example in WebKit code :
>>> bool AccessibilityRenderObject::isPasswordField() const
>>> {
>>>     ASSERT(m_renderer);
>>>     if (!m_renderer->node() || !m_renderer->node()->isHTMLElement())
>>> 
>>> ...
>>> 
>>> m_renderer validity is not tested.
>>> 
>>> To me assert means that if the assert is true well we're in bad shape, it should not happen. It's a nit pick maybe.
>> 
>> Ah, makes sense. Maybe removing the if statement is a solution, since we are assuming that 'value' should not be 0 here.
> 
> What's going on here is that static analysis tools look at the "while (value && value->unit == CSSPrimitiveValue::CSS_URI) {" above, and deduce that a possible exit condition of the loop is that value is null. It turns out that we expect that if that has happened we would have handled it in the if (list) case above. Again, this is non-obvious to a cursory reading, and too convoluted for coverity to pick up. So here, the ASSERT is to document the expectation that if value was null, it should have been handled above in the if (list) case, and the if (value) was added to reassure any static analysis tools that we really are not going to do a null-pointer dereference here.

I think that WebKit normally does not write such "reassurance".

abarth@, darin@ (who will know much about WebKit codebase): Any idea?

[Luke Macpherson]

Hi Kentaro, I still don't feel like we're on the same page here. 

This is hard to do in the context of a code review / bug tracker, but I think it would be helpful if you took a look at the original code, and try to explain why it is safe to dereference value on line 1593 "id = value->id;"

I think once you go through that exercise it will become much clearer what this patch is trying to achieve.

[Kentaro Hara]

(In reply to comment #8)
> Hi Kentaro, I still don't feel like we're on the same page here. 
> 
> This is hard to do in the context of a code review / bug tracker, but I think it would be helpful if you took a look at the original code, and try to explain why it is safe to dereference value on line 1593 "id = value->id;"
> 
> I think once you go through that exercise it will become much clearer what this patch is trying to achieve.

As darktears@ pointed out, I think

- If value->id should be always safe, put 'ASSERT(value)' and remove 'if(value)'.
- If value->id can be unsafe, put 'if(value)' and remove 'ASSERT(value)'.

The point is that having both 'ASSERT(value)' and 'if(value)' sounds strange.

[Luke Macpherson]

Created attachment 137593 [details]
Patch

[Kentaro Hara]

Comment on attachment 137593 [details]
Patch

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

OK

> Source/WebCore/css/CSSParser.cpp:1641
> +            return false;

Nit: What happens if we do not write 'return false'? Compile error?

[Luke Macpherson]

(In reply to comment #11)
> (From update of attachment 137593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137593&action=review
> 
> OK
> 
> > Source/WebCore/css/CSSParser.cpp:1641
> > +            return false;
> 
> Nit: What happens if we do not write 'return false'? Compile error?

The safest thing to do is to immediately stop treat the property as invalid if we did get to this state somehow.

[WebKit Review Bot]

Comment on attachment 137593 [details]
Patch

Clearing flags on attachment: 137593

Committed r114455: <http://trac.webkit.org/changeset/114455>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.