Simplify threading in browser thread by making only ChromeThread deal with di...

Simplify threading in browser thread by making only ChromeThread deal with different thread lifetimes.

The rest of the code doesn't get MessageLoop pointers since they're not thread-safe and instead just call PostTask on ChromeThread.  If the target thread is not alive, then the task is simply deleted.

In a followup change, I'll remove any remaining MessageLoop* caching.  With this change, there's little to be gained by caching since no locks are involved if the target MessageLoop is guaranteed to outlive the current thread (inferred automatically by the order of the chrome_threads_ array).

BUG=25354
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30163

[darin (slow to review), 2009-10-27 00:06:51]

LGTM... just some minor things:

http://codereview.chromium.org/306032/diff/16001/16016
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

http://codereview.chromium.org/306032/diff/16001/16016#newcode35
Line 35: io_thread_(ChromeThread::IO),
does this test thread need to be a real IO thread?  i.e., does it need to have a
MessageLoopForIO allocated on it?

http://codereview.chromium.org/306032/diff/16001/16034
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/306032/diff/16001/16034#newcode413
Line 413: // Start the database thread (the order doesn't really matter for it).
Q: i'm not sure what the second part means.  the order doesn't really matter for
it??

http://codereview.chromium.org/306032/diff/16001/16005
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/306032/diff/16001/16005#newcode143
Line 143: delete task;
maybe this should have a DLOG(INFO)?  or would that be too spammy for linux
developers?

http://codereview.chromium.org/306032/diff/16001/16031
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/306032/diff/16001/16031#newcode37
Line 37: enum ID {
maybe we should have an identifier named CURRENT, so that the same
ChromeThread::PostTask interfaces can be used for posting a task back to the
current thread.  that would help encourage people to write code that can live on
any thread instead of code that only runs on a specific thread.

http://codereview.chromium.org/306032/diff/16001/16031#newcode79
Line 79: // They return true iff the thread existed and the task was posted.
you should probably add a note here indicating that success in posting a task
does not ensure that it actually gets run.

you should probably also say that failure to post a task can only happen during
system shutdown.

http://codereview.chromium.org/306032/diff/16001/16057
File chrome/browser/chrome_thread_unittest.cc (left):

http://codereview.chromium.org/306032/diff/16001/16057#oldcode13
Line 13: TEST_F(ChromeThreadTest, Get) {
maybe there are still some interesting tests to write?

http://codereview.chromium.org/306032/diff/16001/16066
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/306032/diff/16001/16066#newcode622
Line 622: download->request_id()));
nit: indentation

http://codereview.chromium.org/306032/diff/16001/16064
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/306032/diff/16001/16064#newcode990
Line 990: NewRunnableFunction(&DownloadManager::OnPauseDownloadRequest,
nit: indentation

http://codereview.chromium.org/306032/diff/16001/16030
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/306032/diff/16001/16030#newcode24
Line 24: delete dom_storage_context;
are you sure it is safe to delete this object here?  i believe the
DOMStorageContext references WebKit API objects, which may not be threadsafe. 
you should check with jorlow.

[jam, 2009-10-27 02:38:14]

http://codereview.chromium.org/306032/diff/16001/16016
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

http://codereview.chromium.org/306032/diff/16001/16016#newcode35
Line 35: io_thread_(ChromeThread::IO),
On 2009/10/27 00:06:52, darin wrote:
> does this test thread need to be a real IO thread?  i.e., does it need to have
a
> MessageLoopForIO allocated on it?

It already had a MessageLoopForUI before.  I just added the ChromeThread to get
the thread tables the same as the browser process, but ChromeThread doesn't do
anything else.

http://codereview.chromium.org/306032/diff/16001/16034
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/306032/diff/16001/16034#newcode413
Line 413: // Start the database thread (the order doesn't really matter for it).
On 2009/10/27 00:06:52, darin wrote:
> Q: i'm not sure what the second part means.  the order doesn't really matter
for
> it??

I was trying to say that we just need to start the db thread, but it doesn't
matter where in this function we do it.  I'll clarify the comment more.

http://codereview.chromium.org/306032/diff/16001/16005
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/306032/diff/16001/16005#newcode143
Line 143: delete task;
On 2009/10/27 00:06:52, darin wrote:
> maybe this should have a DLOG(INFO)?  or would that be too spammy for linux
> developers?

I don't know how spammy it would be so I'm hesitant to add it.  But even if it's
not spammy, would there be any actionable thing that a developer would do?  If
it's harmless, it would just clog up the output and hide more important
statements, no?

http://codereview.chromium.org/306032/diff/16001/16031
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/306032/diff/16001/16031#newcode37
Line 37: enum ID {
On 2009/10/27 00:06:52, darin wrote:
> maybe we should have an identifier named CURRENT, so that the same
> ChromeThread::PostTask interfaces can be used for posting a task back to the
> current thread.  that would help encourage people to write code that can live
on
> any thread instead of code that only runs on a specific thread.

Wouldn't this be a little confusing, since in the browser process people would
use ChromeThread::PostTask(ChromeThread::CURRENT while in other processes it
would be MessageLoop::current()->PostTask?

http://codereview.chromium.org/306032/diff/16001/16031#newcode79
Line 79: // They return true iff the thread existed and the task was posted.
On 2009/10/27 00:06:52, darin wrote:
> you should probably add a note here indicating that success in posting a task
> does not ensure that it actually gets run.

Done.

> 
> you should probably also say that failure to post a task can only happen
during
> system shutdown.

This isn't always the case (i.e. the thread wasn't started yet, like the webkit
thread).

http://codereview.chromium.org/306032/diff/16001/16057
File chrome/browser/chrome_thread_unittest.cc (left):

http://codereview.chromium.org/306032/diff/16001/16057#oldcode13
Line 13: TEST_F(ChromeThreadTest, Get) {
On 2009/10/27 00:06:52, darin wrote:
> maybe there are still some interesting tests to write?

I couldn't think of any non-trivial tests.  I figured that the ui tests would be
strenuous enough?

http://codereview.chromium.org/306032/diff/16001/16066
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/306032/diff/16001/16066#newcode622
Line 622: download->request_id()));
On 2009/10/27 00:06:52, darin wrote:
> nit: indentation

Done.

http://codereview.chromium.org/306032/diff/16001/16064
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/306032/diff/16001/16064#newcode990
Line 990: NewRunnableFunction(&DownloadManager::OnPauseDownloadRequest,
On 2009/10/27 00:06:52, darin wrote:
> nit: indentation

Done.

http://codereview.chromium.org/306032/diff/16001/16030
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/306032/diff/16001/16030#newcode24
Line 24: delete dom_storage_context;
On 2009/10/27 00:06:52, darin wrote:
> are you sure it is safe to delete this object here?  i believe the
> DOMStorageContext references WebKit API objects, which may not be threadsafe. 
> you should check with jorlow.

This preserves the previous logic.

If there's no thread, then dom_storage_context_ was going to get deleted by the
scoped_ptr in this current thread since we're in the destructor.

[jam, 2009-10-27 02:46:29]

http://codereview.chromium.org/306032/diff/16001/16016
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

http://codereview.chromium.org/306032/diff/16001/16016#newcode35
Line 35: io_thread_(ChromeThread::IO),
On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> On 2009/10/27 00:06:52, darin wrote:
> > does this test thread need to be a real IO thread?  i.e., does it need to
have
> a
> > MessageLoopForIO allocated on it?
> 
> It already had a MessageLoopForUI before.  I just added the ChromeThread to
get
> the thread tables the same as the browser process, but ChromeThread doesn't do
> anything else.

oops, I realize what you mean now.  I needed a ChromeThread to get the thread
table, but I don't think the extra MessageLoop will hurt.

http://codereview.chromium.org/306032/diff/16001/16057
File chrome/browser/chrome_thread_unittest.cc (left):

http://codereview.chromium.org/306032/diff/16001/16057#oldcode13
Line 13: TEST_F(ChromeThreadTest, Get) {
On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> On 2009/10/27 00:06:52, darin wrote:
> > maybe there are still some interesting tests to write?
> 
> I couldn't think of any non-trivial tests.  I figured that the ui tests would
be
> strenuous enough?

On second thoughts, I'll leave the file with an empty test, since I can use it
in another upcoming patch.

[darin (slow to review), 2009-10-27 04:43:32]

http://codereview.chromium.org/306032/diff/16001/16016
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

http://codereview.chromium.org/306032/diff/16001/16016#newcode35
Line 35: io_thread_(ChromeThread::IO),
On 2009/10/27 02:46:29, John Abd-El-Malek wrote:
> On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> > On 2009/10/27 00:06:52, darin wrote:
> > > does this test thread need to be a real IO thread?  i.e., does it need to
> have
> > a
> > > MessageLoopForIO allocated on it?
> > 
> > It already had a MessageLoopForUI before.  I just added the ChromeThread to
> get
> > the thread tables the same as the browser process, but ChromeThread doesn't
do
> > anything else.
> 
> oops, I realize what you mean now.  I needed a ChromeThread to get the thread
> table, but I don't think the extra MessageLoop will hurt.

OK

http://codereview.chromium.org/306032/diff/16001/16034
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/306032/diff/16001/16034#newcode413
Line 413: // Start the database thread (the order doesn't really matter for it).
On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> On 2009/10/27 00:06:52, darin wrote:
> > Q: i'm not sure what the second part means.  the order doesn't really matter
> for
> > it??
> 
> I was trying to say that we just need to start the db thread, but it doesn't
> matter where in this function we do it.  I'll clarify the comment more.

OK

http://codereview.chromium.org/306032/diff/16001/16005
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/306032/diff/16001/16005#newcode143
Line 143: delete task;
On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> On 2009/10/27 00:06:52, darin wrote:
> > maybe this should have a DLOG(INFO)?  or would that be too spammy for linux
> > developers?
> 
> I don't know how spammy it would be so I'm hesitant to add it.  But even if
it's
> not spammy, would there be any actionable thing that a developer would do?  If
> it's harmless, it would just clog up the output and hide more important
> statements, no?

True... I think we'll likely end up with some valgrind / purify suppressions as
a result of this.  That kind of sucks.  I wish we had a better solution that
didn't leak memory on shutdown.

http://codereview.chromium.org/306032/diff/16001/16031
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/306032/diff/16001/16031#newcode37
Line 37: enum ID {
On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> On 2009/10/27 00:06:52, darin wrote:
> > maybe we should have an identifier named CURRENT, so that the same
> > ChromeThread::PostTask interfaces can be used for posting a task back to the
> > current thread.  that would help encourage people to write code that can
live
> on
> > any thread instead of code that only runs on a specific thread.
> 
> Wouldn't this be a little confusing, since in the browser process people would
> use ChromeThread::PostTask(ChromeThread::CURRENT while in other processes it
> would be MessageLoop::current()->PostTask?

True, good point.

This sort of makes me wish that base/ provided the mechanism here, and that we
just had a way like we do with PathService to allow modules to extend the set of
items.  Then, we could use a consistent mechanism everywhere.

http://codereview.chromium.org/306032/diff/16001/16031#newcode79
Line 79: // They return true iff the thread existed and the task was posted.
On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> On 2009/10/27 00:06:52, darin wrote:
> > you should probably add a note here indicating that success in posting a
task
> > does not ensure that it actually gets run.
> 
> Done.
> 
> > 
> > you should probably also say that failure to post a task can only happen
> during
> > system shutdown.
> 
> This isn't always the case (i.e. the thread wasn't started yet, like the
webkit
> thread).

OK

http://codereview.chromium.org/306032/diff/16001/16057
File chrome/browser/chrome_thread_unittest.cc (left):

http://codereview.chromium.org/306032/diff/16001/16057#oldcode13
Line 13: TEST_F(ChromeThreadTest, Get) {
On 2009/10/27 02:46:29, John Abd-El-Malek wrote:
> On 2009/10/27 02:38:18, John Abd-El-Malek wrote:
> > On 2009/10/27 00:06:52, darin wrote:
> > > maybe there are still some interesting tests to write?
> > 
> > I couldn't think of any non-trivial tests.  I figured that the ui tests
would
> be
> > strenuous enough?
> 
> On second thoughts, I'll leave the file with an empty test, since I can use it
> in another upcoming patch.

OK... I was thinking that you might be able to test the behavior of PostTask
when the target thread does not exist, etc.

[jorlow, 2009-10-27 21:37:25]

http://codereview.chromium.org/306032/diff/16001/16026
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/306032/diff/16001/16026#newcode350
Line 350: ChromeThread::PostTask(ChromeThread::WEBKIT, FROM_HERE, task);
use from_here, not FROM_HERE

http://codereview.chromium.org/306032/diff/16001/16028
File chrome/browser/in_process_webkit/webkit_thread.cc (left):

http://codereview.chromium.org/306032/diff/16001/16028#oldcode22
Line 22: DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::IO));
Why did you remove this?