Bug 65944

Summary: Crash in QuotesData::operator== when compiled with RVCT
Product: WebKit Reporter: Yong Li <yong.li.webkit>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dglazkov, gns, jbedard, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch
yong.li.webkit: review-, gyuyoung.kim: commit-queue-
updated
none
Fix style issue none

Description Yong Li 2011-08-09 14:35:53 PDT
"if (!this || !&other)" in QuotesData::operator==() doesn't generate expected code when being compiled with RVCT. This can cause crashes
Comment 1 Yong Li 2011-08-09 14:56:39 PDT
Created attachment 103403 [details]
the patch
Comment 2 WebKit Review Bot 2011-08-09 14:58:36 PDT
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.
Comment 3 Gyuyoung Kim 2011-08-09 15:02:16 PDT
Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9340314
Comment 4 WebKit Review Bot 2011-08-09 15:02:18 PDT
Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9338347
Comment 5 WebKit Review Bot 2011-08-09 15:12:08 PDT
Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9332544
Comment 6 Gustavo Noronha (kov) 2011-08-09 15:13:02 PDT
Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9340320
Comment 7 WebKit Review Bot 2011-08-09 15:24:46 PDT
Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9338352
Comment 8 Early Warning System Bot 2011-08-09 16:19:09 PDT
Comment on attachment 103403 [details]
the patch

Attachment 103403 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9337415
Comment 9 Yong Li 2011-08-10 07:10:20 PDT
Created attachment 103484 [details]
updated
Comment 10 WebKit Review Bot 2011-08-10 07:12:02 PDT
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.
Comment 11 Yong Li 2011-08-10 07:36:10 PDT
Created attachment 103489 [details]
Fix style issue

No more "a"
Comment 12 Antonio Gomes 2011-08-10 07:43:44 PDT
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?
Comment 13 Yong Li 2011-08-10 08:02:24 PDT
(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.
Comment 14 Alexey Proskuryakov 2011-08-10 15:46:05 PDT
> 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.
Comment 15 Yong Li 2011-08-11 07:14:47 PDT
(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.
Comment 16 Yong Li 2011-08-11 11:22:44 PDT
Comment on attachment 103489 [details]
Fix style issue

Thanks Alexey!
Comment 17 WebKit Review Bot 2011-08-11 12:23:10 PDT
Comment on attachment 103489 [details]
Fix style issue

Clearing flags on attachment: 103489

Committed r92871: <http://trac.webkit.org/changeset/92871>
Comment 18 WebKit Review Bot 2011-08-11 12:23:15 PDT
All reviewed patches have been landed.  Closing bug.