RESOLVED FIXED 65944
Crash in QuotesData::operator== when compiled with RVCT
https://bugs.webkit.org/show_bug.cgi?id=65944
Summary Crash in QuotesData::operator== when compiled with RVCT
Yong Li
Reported 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
Attachments
the patch (4.77 KB, patch)
2011-08-09 14:56 PDT, Yong Li
yong.li.webkit: review-
gyuyoung.kim: commit-queue-
updated (4.77 KB, patch)
2011-08-10 07:10 PDT, Yong Li
no flags
Fix style issue (4.87 KB, patch)
2011-08-10 07:36 PDT, Yong Li
no flags
Yong Li
Comment 1 2011-08-09 14:56:39 PDT
Created attachment 103403 [details] the patch
WebKit Review Bot
Comment 2 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.
Gyuyoung Kim
Comment 3 2011-08-09 15:02:16 PDT
WebKit Review Bot
Comment 4 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
WebKit Review Bot
Comment 5 2011-08-09 15:12:08 PDT
Gustavo Noronha (kov)
Comment 6 2011-08-09 15:13:02 PDT
WebKit Review Bot
Comment 7 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
Early Warning System Bot
Comment 8 2011-08-09 16:19:09 PDT
Yong Li
Comment 9 2011-08-10 07:10:20 PDT
WebKit Review Bot
Comment 10 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.
Yong Li
Comment 11 2011-08-10 07:36:10 PDT
Created attachment 103489 [details] Fix style issue No more "a"
Antonio Gomes
Comment 12 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?
Yong Li
Comment 13 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.
Alexey Proskuryakov
Comment 14 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.
Yong Li
Comment 15 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.
Yong Li
Comment 16 2011-08-11 11:22:44 PDT
Comment on attachment 103489 [details] Fix style issue Thanks Alexey!
WebKit Review Bot
Comment 17 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>
WebKit Review Bot
Comment 18 2011-08-11 12:23:15 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.