Bug 73937

Summary: [chromium] Guard access to WebKitPlatformSupport::currentThread with a null check
Product: WebKit Reporter: Adam Klein <adamk>
Component: New BugsAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: fishd, levin, ojan, rafaelw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73851    
Attachments:
Description Flags
Patch
none
Patch for landing none

Adam Klein
Reported 2011-12-06 11:18:29 PST
[chromium] Guard access to WebKitPlatformSupport::currentThread with a null check
Attachments
Patch (2.09 KB, patch)
2011-12-06 11:20 PST, Adam Klein 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.18 KB, patch)
2011-12-06 11:44 PST, Adam Klein 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adam Klein
Comment 1 2011-12-06 11:20:49 PST
Created attachment 118076 [details] Patch
Adam Barth
Comment 2 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.
David Levin
Comment 3 2011-12-06 11:40:42 PST
Why is it a bad thing to construct a MessageLoop in the test code?
Adam Klein
Comment 4 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.
Adam Klein
Comment 5 2011-12-06 11:44:17 PST
Created attachment 118081 [details] Patch for landing
David Levin
Comment 6 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.
Adam Klein
Comment 7 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).
David Levin
Comment 8 2011-12-06 11:52:56 PST
Comment on attachment 118081 [details] Patch for landing Thanks! Sorry for the hold up.
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2011-12-06 13:06:44 PST
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.