Bug 83544

Summary: Make CSSParser::parseValue()'s handling of CSSPropertyCursor more obviously correct.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: CSSAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, haraken, macpherson, menard, mikelawther, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Luke Macpherson 2012-04-09 20:55:44 PDT
Make CSSParser::parseValue()'s handling of CSSPropertyCursor more obviously correct.
Comment 1 Luke Macpherson 2012-04-09 20:59:31 PDT
Created attachment 136389 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-04-10 04:33:48 PDT
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?
Comment 3 Kentaro Hara 2012-04-10 05:11:33 PDT
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.
Comment 4 Alexis Menard (darktears) 2012-04-10 06:33:54 PDT
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.
Comment 5 Kentaro Hara 2012-04-10 06:41:14 PDT
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.
Comment 6 Luke Macpherson 2012-04-10 15:52:29 PDT
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.
Comment 7 Kentaro Hara 2012-04-11 00:11:41 PDT
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?
Comment 8 Luke Macpherson 2012-04-11 16:42:35 PDT
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.
Comment 9 Kentaro Hara 2012-04-11 20:39:01 PDT
(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.
Comment 10 Luke Macpherson 2012-04-17 13:39:59 PDT
Created attachment 137593 [details]
Patch
Comment 11 Kentaro Hara 2012-04-17 13:46:39 PDT
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?
Comment 12 Luke Macpherson 2012-04-17 14:04:41 PDT
(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.
Comment 13 WebKit Review Bot 2012-04-17 15:56:40 PDT
Comment on attachment 137593 [details]
Patch

Clearing flags on attachment: 137593

Committed r114455: <http://trac.webkit.org/changeset/114455>
Comment 14 WebKit Review Bot 2012-04-17 15:56:46 PDT
All reviewed patches have been landed.  Closing bug.