Bug 73937 - [chromium] Guard access to WebKitPlatformSupport::currentThread with a null check
Summary: [chromium] Guard access to WebKitPlatformSupport::currentThread with a null c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 73851
  Show dependency tree / graph
 
Reported: 2011-12-06 11:18 PST by Adam Klein
Modified: 2011-12-06 13:06 PST (History)
5 users (show)

See Also:

Attachments
Patch (2.09 KB, patch)
2011-12-06 11:20 PST, Adam Klein 		no flags Details | Formatted Diff | Diff
Patch for landing (2.18 KB, patch)
2011-12-06 11:44 PST, Adam Klein 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-12-06 11:18:29 PST 
[chromium] Guard access to WebKitPlatformSupport::currentThread with a null check
Comment 1 Adam Klein 2011-12-06 11:20:49 PST 
Created attachment 118076 [details]
Patch
Comment 2 Adam Barth 2011-12-06 11:33:21 PST 
Comment on attachment 118076 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:10
> +        Though |currentThread| is never null in production code, it is null in
> +        unit tests (such as Chromium's unit_tests) that call WebKit::initialize()
> +        without first constructing a MessageLoop.

Can you add this comment to the code as well?  We don't want folks cargo-cult copying this null check.
Comment 3 David Levin 2011-12-06 11:40:42 PST 
Why is it a bad thing to construct a MessageLoop in the test code?
Comment 4 Adam Klein 2011-12-06 11:44:00 PST 
Comment on attachment 118076 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:10
>> +        without first constructing a MessageLoop.
> 
> Can you add this comment to the code as well?  We don't want folks cargo-cult copying this null check.

Done.
Comment 5 Adam Klein 2011-12-06 11:44:17 PST 
Created attachment 118081 [details]
Patch for landing
Comment 6 David Levin 2011-12-06 11:45:15 PST 
Comment on attachment 118081 [details]
Patch for landing

Just blocking for the moment since there is an unanswered question.
Comment 7 Adam Klein 2011-12-06 11:50:14 PST 
(In reply to comment #3)
> Why is it a bad thing to construct a MessageLoop in the test code?

It's not so much that tests don't construct MessageLoops, but that each test case inside, e.g., unit_tests wants to have control over its own MessageLoop. For example, some tests need an IO loop, others might need a UI loop.  And only one can exist per thread. So at initialization time (presumably during test startup), there's no loop, even if each test case creates a loop later (in its GTest test fixture).
Comment 8 David Levin 2011-12-06 11:52:56 PST 
Comment on attachment 118081 [details]
Patch for landing

Thanks! Sorry for the hold up.
Comment 9 WebKit Review Bot 2011-12-06 13:06:39 PST 
Comment on attachment 118081 [details]
Patch for landing

Clearing flags on attachment: 118081

Committed r102171: <http://trac.webkit.org/changeset/102171>
Comment 10 WebKit Review Bot 2011-12-06 13:06:44 PST 
All reviewed patches have been landed.  Closing bug.