Bug 145254 – Fix logical-not-parentheses warning in CachedScript.cpp

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

../../Source/WebCore/loader/cache/CachedScript.cpp:98:12: warning: logical not is only applied to the left hand side of this comparison [-Wlogical-not-parentheses]
    return !parseContentTypeOptionsHeader(m_response.httpHeaderField(HTTPHeaderName::XContentTypeOptions)) == ContentTypeOptionsNosniff || MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType());
           ^                                                                                               ~~
../../Source/WebCore/loader/cache/CachedScript.cpp:98:12: note: add parentheses after the '!' to evaluate the comparison first
    return !parseContentTypeOptionsHeader(m_response.httpHeaderField(HTTPHeaderName::XContentTypeOptions)) == ContentTypeOptionsNosniff || MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType());
           ^
            (                                                                                                                          )
../../Source/WebCore/loader/cache/CachedScript.cpp:98:12: note: add parentheses around left hand side expression to silence this warning
    return !parseContentTypeOptionsHeader(m_response.httpHeaderField(HTTPHeaderName::XContentTypeOptions)) == ContentTypeOptionsNosniff || MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType());
           ^
           (                                                                                              )
1 warning generated.

----

It seems this warning found a real bug should be fixed.

[Csaba Osztrogonác]

Created attachment 253515 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 253515 [details]
Patch

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

> Source/WebCore/loader/cache/CachedScript.cpp:98
> +    return !(parseContentTypeOptionsHeader(m_response.httpHeaderField(HTTPHeaderName::XContentTypeOptions)) == ContentTypeOptionsNosniff) || MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType());

Shouldn't this just use != ?

[Darin Adler]

Comment on attachment 253515 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedScript.cpp:98
>> +    return !(parseContentTypeOptionsHeader(m_response.httpHeaderField(HTTPHeaderName::XContentTypeOptions)) == ContentTypeOptionsNosniff) || MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType());
> 
> Shouldn't this just use != ?

I agree that it should.

[Darin Adler]

Also, we should test this feature. Now that we know it was broken.

[Csaba Osztrogonác]

Comment on attachment 253515 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedScript.cpp:98
>>> +    return !(parseContentTypeOptionsHeader(m_response.httpHeaderField(HTTPHeaderName::XContentTypeOptions)) == ContentTypeOptionsNosniff) || MIMETypeRegistry::isSupportedJavaScriptMIMEType(mimeType());
>> 
>> Shouldn't this just use != ?
> 
> I agree that it should.

Good point, I'll change it before landing.

[Csaba Osztrogonác]

(In reply to comment #4)
> Also, we should test this feature. Now that we know it was broken.

Source/WebCore/platform/network/HTTPParsers.h:
-----------------------------------------------
...
enum ContentTypeOptionsDisposition {
    ContentTypeOptionsNone,
    ContentTypeOptionsNosniff
};
...

It is already covered by testcases and this code isn't broken
functionally, because ContentTypeOptionsNosniff gets 1 value in 
this enum and we were lucky and got the same results in both cases.

           X: ContentTypeOptionsNone (0)    X: ContentTypeOptionsNosniff (1)
 !X==1               true                            false
!(X==1)              true                            false
  X!=1               true                            false

[Csaba Osztrogonác]

Created attachment 253991 [details]
Patch for landing

[Csaba Osztrogonác]

( just a note, it is already fixed in blink near 2 years ago :)
  https://codereview.chromium.org/18753002 )

[WebKit Commit Bot]

Comment on attachment 253991 [details]
Patch for landing

Clearing flags on attachment: 253991

Committed r185057: <http://trac.webkit.org/changeset/185057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.