Bug 119165 - REGRESSION(r153380): Can't open messages on Gmail
Summary: REGRESSION(r153380): Can't open messages on Gmail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Major
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar, Regression
: 119176 119193 119202 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-07-26 14:49 PDT by Ryosuke Niwa
Modified: 2013-07-29 10:31 PDT (History)
13 users (show)

See Also:


Attachments
Disables final on clang 4.0 (2.94 KB, patch)
2013-07-26 18:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed a typo (2.95 KB, patch)
2013-07-26 18:06 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (2.98 KB, patch)
2013-07-26 18:12 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Make the fix Apple specific since __clang_major__ and __clang_minor__ are vendor dependent (3.11 KB, patch)
2013-07-26 18:55 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Use ifdef instead of if defined (3.11 KB, patch)
2013-07-26 19:01 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-07-26 14:49:55 PDT
Reproduction steps:
1. Login to Gmail
2. Click on any message.

Expected result:
The message is open on a pane

Actual result:
Nothing happens.

I don't see any error messages in the console either.  It appears as if click events are never fired?
Comment 1 Radar WebKit Bug Importer 2013-07-26 14:50:37 PDT
<rdar://problem/14564599>
Comment 2 Filip Pizlo 2013-07-26 14:52:15 PDT
It worked fine when I was landing http://trac.webkit.org/changeset/153381; after all, to get the crash to repro you had to click on messages.
Comment 3 Ryosuke Niwa 2013-07-26 14:52:23 PDT
I guess the regression range is somewhat misleading since WebKit nightly used to crash prior to http://trac.webkit.org/changeset/153381
Comment 4 Radar WebKit Bug Importer 2013-07-26 14:52:30 PDT
<rdar://problem/14564630>
Comment 5 Radar WebKit Bug Importer 2013-07-26 14:52:33 PDT
<rdar://problem/14564632>
Comment 6 Ryosuke Niwa 2013-07-26 14:54:40 PDT
(In reply to comment #2)
> It worked fine when I was landing http://trac.webkit.org/changeset/153381; after all, to get the crash to repro you had to click on messages.

Do you remember the revision you were able to open a message?
Comment 7 Ryosuke Niwa 2013-07-26 15:35:14 PDT
This is probably caused by http://trac.webkit.org/changeset/153380.
Comment 8 Ryosuke Niwa 2013-07-26 16:30:58 PDT
I've confirmed that the regression is caused by http://trac.webkit.org/changeset/153380 by manually bisecting the issue.
Comment 9 Ryosuke Niwa 2013-07-26 16:31:37 PDT
kling points out that the issue doesn't reproduce on this local machine. We're suspecting it's yet another instance of an old version of clang not being able to compile FINAL correctly.
Comment 10 Ryosuke Niwa 2013-07-26 16:46:40 PDT
It appears that this bug reproduces whenever we compile WebKit with clang 4 but not when we compile it with clang 5.
Comment 11 Ryosuke Niwa 2013-07-26 18:06:09 PDT
Created attachment 207569 [details]
Disables final on clang 4.0
Comment 12 Ryosuke Niwa 2013-07-26 18:06:56 PDT
Created attachment 207570 [details]
Fixed a typo
Comment 13 Ryosuke Niwa 2013-07-26 18:08:51 PDT
Comment on attachment 207570 [details]
Fixed a typo

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

> Source/WTF/wtf/Compiler.h:44
> +#define GCC_VERSION_AT_LEAST(major, minor) (__clang_major__ >= major && __clang_major__ >= minor)

Ugh... clearly this needs to be CLANG_VERSION_AT_LEAST instead.

> Source/WTF/wtf/Compiler.h:65
> +#define WTF_COMPILER_SUPPORTS_CXX_FINAL_CONTROL WTF_COMPILER_SUPPORTS_CXX_OVERRIDE_CONTROL && GCC_VERSION_AT_LEAST(4, 2)

Ditto here.
Comment 14 Ryosuke Niwa 2013-07-26 18:12:45 PDT
Created attachment 207571 [details]
Patch for landing
Comment 15 Ryosuke Niwa 2013-07-26 18:13:24 PDT
Comment on attachment 207571 [details]
Patch for landing

I'm gonna wait for EWS bots to process this patch.
Comment 16 Andreas Kling 2013-07-26 18:41:51 PDT
Comment on attachment 207571 [details]
Patch for landing

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

> Source/WTF/wtf/Compiler.h:44
> +#define CLANG_VERSION_AT_LEAST(major, minor) (defined(__clang_major__) && __clang_major__ >= major && __clang_major__ >= minor)

Typo at the end, __clang_major__ >= minor
Should be __clang_minor__ >= minor
Comment 17 Ryosuke Niwa 2013-07-26 18:55:34 PDT
Created attachment 207574 [details]
Make the fix Apple specific since __clang_major__ and __clang_minor__ are vendor dependent
Comment 18 WebKit Commit Bot 2013-07-26 18:57:32 PDT
Attachment 207574 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Compiler.h']" exit_code: 1
Source/WTF/wtf/Compiler.h:63:  One space before end of line comments  [whitespace/comments] [5]
Source/WTF/wtf/Compiler.h:63:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Ryosuke Niwa 2013-07-26 19:01:04 PDT
Created attachment 207575 [details]
Use ifdef instead of if defined
Comment 20 WebKit Commit Bot 2013-07-26 19:02:26 PDT
Attachment 207575 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Compiler.h']" exit_code: 1
Source/WTF/wtf/Compiler.h:63:  One space before end of line comments  [whitespace/comments] [5]
Source/WTF/wtf/Compiler.h:63:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Andreas Kling 2013-07-27 14:37:31 PDT
Comment on attachment 207575 [details]
Use ifdef instead of if defined

Looks like a reasonable solution. I'll land this patch right away to get nightlies back in shape on ML.
Comment 22 Andreas Kling 2013-07-27 14:39:13 PDT
Comment on attachment 207575 [details]
Use ifdef instead of if defined

Clearing flags on attachment: 207575

Committed r153400: <http://trac.webkit.org/changeset/153400>
Comment 23 Andreas Kling 2013-07-27 14:39:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Ryosuke Niwa 2013-07-28 17:17:57 PDT
(In reply to comment #21)
> (From update of attachment 207575 [details])
> Looks like a reasonable solution. I'll land this patch right away to get nightlies back in shape on ML.

Thanks!
Comment 25 Alexey Proskuryakov 2013-07-29 10:30:08 PDT
*** Bug 119176 has been marked as a duplicate of this bug. ***
Comment 26 Alexey Proskuryakov 2013-07-29 10:31:06 PDT
*** Bug 119193 has been marked as a duplicate of this bug. ***
Comment 27 Alexey Proskuryakov 2013-07-29 10:31:43 PDT
*** Bug 119202 has been marked as a duplicate of this bug. ***