Crash in QuotesData::operator== when compiled with RVCT

[Yong Li]

"if (!this || !&other)" in QuotesData::operator==() doesn't generate expected code when being compiled with RVCT. This can cause crashes

[Yong Li]

Created attachment 103403 [details]
the patch

[WebKit Review Bot]

Attachment 103403 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/QuotesData.h:37:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9340314

[WebKit Review Bot]

Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9338347

[WebKit Review Bot]

Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9332544

[Gustavo Noronha (kov)]

Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9340320

[WebKit Review Bot]

Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9338352

[Early Warning System Bot]

Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9337415

[Yong Li]

Created attachment 103484 [details]
updated

[WebKit Review Bot]

Attachment 103484 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/QuotesData.h:37:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yong Li]

Created attachment 103489 [details]
Fix style issue

No more "a"

[Antonio Gomes]

Comment on attachment 103489 [details]
Fix style issue

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

> Source/WebCore/rendering/style/QuotesData.h:37
> -    bool operator==(const QuotesData&) const;
>      void operator delete(void* p) { delete[] static_cast<char*>(p); }
> +    static bool equal(const QuotesData*, const QuotesData*);

Should we add a comment about avoiding "==" so no one tries to get it back in?

[Yong Li]

(In reply to comment #12)
> (From update of attachment 103489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103489&action=review
> 
> > Source/WebCore/rendering/style/QuotesData.h:37
> > -    bool operator==(const QuotesData&) const;
> >      void operator delete(void* p) { delete[] static_cast<char*>(p); }
> > +    static bool equal(const QuotesData*, const QuotesData*);
> 
> Should we add a comment about avoiding "==" so no one tries to get it back in?

operator== is not a problem. The problem is the old operator== is bad written. It checks "this" pointer and the address of a reference (QuotesData&), and expects to work with null "this" and a reference deferencing a null pointer.

If someone in the future wants to write a normal operator==, and doesn't use operator== on null pointers, it should be fine.

[Alexey Proskuryakov]

> The problem is the old operator== is bad written 

Are you sure that the blame is pointed correctly here? Seems like a bug in this particular compiler to me.

[Yong Li]

(In reply to comment #14)
> > The problem is the old operator== is bad written 
> 
> Are you sure that the blame is pointed correctly here? Seems like a bug in this particular compiler to me.

This was discussed @ https://bugs.webkit.org/show_bug.cgi?id=36901

from [Daniel Bates]:

By 8.3.2 (5) of the C++ standard <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3092.pdf>:

A reference shall be initialized to refer to a valid object or function. [ Note: in particular, a null reference cannot exist in a well-defined program, because the only way to create such a reference would be to bind it to the “object” obtained by dereferencing a null pointer, which causes undefined behavior....]

RVCT uses a strict interpretation of the standard (compared to GCC) and assumes a reference must always be initialized with a valid object. Hence it optimizes away !&other.

Another one is "!this". Although RVCT has a compiler option "--allow_null_this", we should still avoid writing such code.

[Yong Li]

Comment on attachment 103489 [details]
Fix style issue

Thanks Alexey!

[WebKit Review Bot]

Comment on attachment 103489 [details]
Fix style issue

Clearing flags on attachment: 103489

Committed r92871: <http://trac.webkit.org/changeset/92871>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.