Change the lifetime of the WebKit thread to be a subset of the IO thread's li...

Simplify the WebKit thread model.  It's now created/destroyed on the UI thread (before/after the IO thread is started/stopped).  The WebKit thread is created lazily as needed (while on the IO thread).TEST=noneBUG=none


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

[jorlow, 2009-07-07 02:25:37]

I think this is ready for review.

[darin (slow to review), 2009-07-07 06:06:00]

http://codereview.chromium.org/149238/diff/1039/44
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/1039/44#newcode22
Line 22: // We absolutely cannot be deleted on the WebKit because it's possible
the
nit: "on the WebKit [thread] because..."

http://codereview.chromium.org/149238/diff/1039/44#newcode30
Line 30: return;  // Leaking is better than crashing.
actually, it is better to crash.  that way we get a report sent to the crash
server, and armed with that information, we can learn that we have more work to
do.  if we just silently leak, then other more subtle badness may ensue, which
might manifest itself in much harder to diagnose symptoms.

please don't fear a judiciously placed CHECK(...)!

one question:  why is it useful to support multiple WebKitThread instances?

http://codereview.chromium.org/149238/diff/1039/45
File chrome/browser/in_process_webkit/webkit_thread.h (right):

http://codereview.chromium.org/149238/diff/1039/45#newcode40
Line 40: static bool IsOnIOThread() {
this method feels like it belongs in a more general place.  perhaps ChromeThread
should just have a method to test if the calling thread is a particular known
thread?

if you make the WebKit thread be a ChromeThread, then it could also partake. 
being a member of ChromeThread doesn't imply any kind of special lifetime.

so you could perhaps replace both of these methods with calls like:

  ChromeThread::CalledOn(ChromeThread::IO)
  ChromeThread::CalledOn(ChromeThread::WEBKIT)

how does that strike you?

http://codereview.chromium.org/149238/diff/1039/48
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/149238/diff/1039/48#newcode532
Line 532: scoped_ptr<WebKitThread> webkit_thread_;
i was expecting to see some code changes in resource_dispatcher_host.cc too.

[jorlow, 2009-07-07 08:40:14]

Will update the code in the morning.

http://codereview.chromium.org/149238/diff/1039/44
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/1039/44#newcode22
Line 22: // We absolutely cannot be deleted on the WebKit because it's possible
the
On 2009/07/07 06:06:00, darin wrote:
> nit: "on the WebKit [thread] because..."

Done.

http://codereview.chromium.org/149238/diff/1039/44#newcode30
Line 30: return;  // Leaking is better than crashing.
On 2009/07/07 06:06:00, darin wrote:
> actually, it is better to crash.  that way we get a report sent to the crash
> server, and armed with that information, we can learn that we have more work
to
> do.  if we just silently leak, then other more subtle badness may ensue, which
> might manifest itself in much harder to diagnose symptoms.
> 
> please don't fear a judiciously placed CHECK(...)!
> 
> one question:  why is it useful to support multiple WebKitThread instances?

I debated back and forth.  Fine with me, though.  I'll just make the DCHECK
above a CHECK.

http://codereview.chromium.org/149238/diff/1039/45
File chrome/browser/in_process_webkit/webkit_thread.h (right):

http://codereview.chromium.org/149238/diff/1039/45#newcode40
Line 40: static bool IsOnIOThread() {
On 2009/07/07 06:06:00, darin wrote:
> this method feels like it belongs in a more general place.  perhaps
ChromeThread
> should just have a method to test if the calling thread is a particular known
> thread?
> 
> if you make the WebKit thread be a ChromeThread, then it could also partake. 
> being a member of ChromeThread doesn't imply any kind of special lifetime.
> 
> so you could perhaps replace both of these methods with calls like:
> 
>   ChromeThread::CalledOn(ChromeThread::IO)
>   ChromeThread::CalledOn(ChromeThread::WEBKIT)
> 
> how does that strike you?

It's ironic you say that since this code started out as WEBKIT being a
ChromeThread.  :-)

I think the lifetime and constraints on the WebKit thread (and there only being
one) make it wise to keep it isolated in this class.  Especially since we're
going to be so strict about what threads you can access it on.

IsOnIOThread seems like a good candidate for being moved back into ChromeThread.
 It just needs to be something where a DCHECK of your current thread can be
expressed in <80 chars.  :-)

http://codereview.chromium.org/149238/diff/1039/45#newcode70
Line 70: // Returns a the WebKit thread's message loop or NULL if we're in
Oops..english issue here.

http://codereview.chromium.org/149238/diff/1039/48
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/149238/diff/1039/48#newcode532
Line 532: scoped_ptr<WebKitThread> webkit_thread_;
On 2009/07/07 06:06:00, darin wrote:
> i was expecting to see some code changes in resource_dispatcher_host.cc too.

The only code related to this is instantiation, and that's done the same whether
it's a scoped_ptr or scoped_refptr.

[jorlow, 2009-07-07 08:46:52]

Ooops...forgot one thing.

http://codereview.chromium.org/149238/diff/1039/44
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/1039/44#newcode30
Line 30: return;  // Leaking is better than crashing.
On 2009/07/07 08:40:15, Jeremy Orlow wrote:
> On 2009/07/07 06:06:00, darin wrote:
> > actually, it is better to crash.  that way we get a report sent to the crash
> > server, and armed with that information, we can learn that we have more work
> to
> > do.  if we just silently leak, then other more subtle badness may ensue,
which
> > might manifest itself in much harder to diagnose symptoms.
> > 
> > please don't fear a judiciously placed CHECK(...)!
> > 
> > one question:  why is it useful to support multiple WebKitThread instances?
> 
> I debated back and forth.  Fine with me, though.  I'll just make the DCHECK
> above a CHECK.

Oh, and we allow multiple WebKitThread instances because it's the cleanest way
to handle testing.

No matter what, we need to track whether there's multiple instances so we only
instantiate one at a time.  Whether we assert, only spin up the thread for the
first requester, etc we'll have to somehow deal with the situation.  It actually
isn't much more code to make it handle multiple instances cleanly, rather than
doing something hacky for tests.

[darin (slow to review), 2009-07-07 17:32:38]

http://codereview.chromium.org/149238/diff/1039/45
File chrome/browser/in_process_webkit/webkit_thread.h (right):

http://codereview.chromium.org/149238/diff/1039/45#newcode40
Line 40: static bool IsOnIOThread() {
On 2009/07/07 08:40:15, Jeremy Orlow wrote:
> I think the lifetime and constraints on the WebKit thread (and there only
being
> one) make it wise to keep it isolated in this class.  Especially since we're
> going to be so strict about what threads you can access it on.

It's a good point, but know that we basically have the same situation with all
of the other threads too.  I'm not sure how the WEBKIT thread is special in this
regard.


> IsOnIOThread seems like a good candidate for being moved back into
ChromeThread.
>  It just needs to be something where a DCHECK of your current thread can be
> expressed in <80 chars.  :-)

yeah!

[jam, 2009-07-07 18:21:59]

http://codereview.chromium.org/149238/diff/1039/44
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/1039/44#newcode30
Line 30: return;  // Leaking is better than crashing.
On 2009/07/07 08:46:53, Jeremy Orlow wrote:
> On 2009/07/07 08:40:15, Jeremy Orlow wrote:
> > On 2009/07/07 06:06:00, darin wrote:
> > > actually, it is better to crash.  that way we get a report sent to the
crash
> > > server, and armed with that information, we can learn that we have more
work
> > to
> > > do.  if we just silently leak, then other more subtle badness may ensue,
> which
> > > might manifest itself in much harder to diagnose symptoms.
> > > 
> > > please don't fear a judiciously placed CHECK(...)!
> > > 
> > > one question:  why is it useful to support multiple WebKitThread
instances?
> > 
> > I debated back and forth.  Fine with me, though.  I'll just make the DCHECK
> > above a CHECK.
> 
> Oh, and we allow multiple WebKitThread instances because it's the cleanest way
> to handle testing.

Can you explain this a bit more?  I think it'd make the production code simpler
to not have a wrapper object and support multiple instances.  Why do multiple
instances need to be created in the test code?  I'd lean towards making that
complicated over the non-testing code.

> 
> No matter what, we need to track whether there's multiple instances so we only
> instantiate one at a time.  Whether we assert, only spin up the thread for the
> first requester, etc we'll have to somehow deal with the situation.  It
actually
> isn't much more code to make it handle multiple instances cleanly, rather than
> doing something hacky for tests.

[jorlow, 2009-07-07 18:29:46]

This is getting really frustrating. I'm going in circles here. I had simpler
"production" logic and had the WebKit thread being a chrome thread at one
point, but was pushed away from both.
I'm going to make a new patch based on these suggestions and hack up the
tests if I need to. Hopefully we can be done with this soon...



On Tue, Jul 7, 2009 at 11:22 AM, <jam@chromium.org> wrote:

>
> http://codereview.chromium.org/149238/diff/1039/44
> File chrome/browser/in_process_webkit/webkit_thread.cc (right):
>
> http://codereview.chromium.org/149238/diff/1039/44#newcode30
> Line 30: return;  // Leaking is better than crashing.
> On 2009/07/07 08:46:53, Jeremy Orlow wrote:
>
>> On 2009/07/07 08:40:15, Jeremy Orlow wrote:
>> > On 2009/07/07 06:06:00, darin wrote:
>> > > actually, it is better to crash.  that way we get a report sent to
>>
> the crash
>
>> > > server, and armed with that information, we can learn that we have
>>
> more work
>
>> > to
>> > > do.  if we just silently leak, then other more subtle badness may
>>
> ensue,
>
>> which
>> > > might manifest itself in much harder to diagnose symptoms.
>> > >
>> > > please don't fear a judiciously placed CHECK(...)!
>> > >
>> > > one question:  why is it useful to support multiple WebKitThread
>>
> instances?
>
>> >
>> > I debated back and forth.  Fine with me, though.  I'll just make the
>>
> DCHECK
>
>> > above a CHECK.
>>
>
>  Oh, and we allow multiple WebKitThread instances because it's the
>>
> cleanest way
>
>> to handle testing.
>>
>
> Can you explain this a bit more?  I think it'd make the production code
> simpler to not have a wrapper object and support multiple instances.
> Why do multiple instances need to be created in the test code?  I'd lean
> towards making that complicated over the non-testing code.
>
>
>  No matter what, we need to track whether there's multiple instances so
>>
> we only
>
>> instantiate one at a time.  Whether we assert, only spin up the thread
>>
> for the
>
>> first requester, etc we'll have to somehow deal with the situation.
>>
> It actually
>
>> isn't much more code to make it handle multiple instances cleanly,
>>
> rather than
>
>> doing something hacky for tests.
>>
>
> http://codereview.chromium.org/149238
>

[jorlow, 2009-07-07 21:58:51]

Ready for another review.  I think all comments have been addressed.

[jam, 2009-07-07 22:14:40]

http://codereview.chromium.org/149238/diff/101/1060
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/149238/diff/101/1060#newcode60
Line 60: return  == message_loop;
?

http://codereview.chromium.org/149238/diff/101/1054
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/101/1054#newcode17
Line 17: : single_process_(CommandLine::ForCurrentProcess()->HasSwitch(
nit: why is this variable being cached?  CommandLine::ForCurrentProcess() is
threadsafe.

http://codereview.chromium.org/149238/diff/101/1055
File chrome/browser/in_process_webkit/webkit_thread.h (right):

http://codereview.chromium.org/149238/diff/101/1055#newcode20
Line 20: // thread.  If you create multiple instances of WebKitThread, they'll
I think this comment is stale

http://codereview.chromium.org/149238/diff/101/1055#newcode64
Line 64: static InternalWebKitThread* webkit_thread_;
It doesn't look like this needs to be static anymore.

http://codereview.chromium.org/149238/diff/101/1055#newcode67
Line 67: static int ref_count_;
This doesn't look like it's needed anymore.

[jorlow, 2009-07-07 22:31:02]

Just uploaded a new version.

http://codereview.chromium.org/149238/diff/101/1060
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/149238/diff/101/1060#newcode60
Line 60: return  == message_loop;
On 2009/07/07 22:14:40, John Abd-El-Malek wrote:
> ?

Forgot to hit save/compile before I uploaded.  It's fixed.

http://codereview.chromium.org/149238/diff/101/1054
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/101/1054#newcode17
Line 17: : single_process_(CommandLine::ForCurrentProcess()->HasSwitch(
On 2009/07/07 22:14:40, John Abd-El-Malek wrote:
> nit: why is this variable being cached?  CommandLine::ForCurrentProcess() is
> threadsafe.

Speed.  I don't want to do a couple string copies + a map lookup every time we
call WebKitThread every time GetMessageLoop is called when --single-process is
specified.  This is an optimization for a condition that doesn't exist yet, but
it seems simple and lightweight enough.

Should I take it out?

http://codereview.chromium.org/149238/diff/101/1055
File chrome/browser/in_process_webkit/webkit_thread.h (right):

http://codereview.chromium.org/149238/diff/101/1055#newcode20
Line 20: // thread.  If you create multiple instances of WebKitThread, they'll
On 2009/07/07 22:14:40, John Abd-El-Malek wrote:
> I think this comment is stale

Done.

http://codereview.chromium.org/149238/diff/101/1055#newcode64
Line 64: static InternalWebKitThread* webkit_thread_;
On 2009/07/07 22:14:40, John Abd-El-Malek wrote:
> It doesn't look like this needs to be static anymore.

All right...I deleted any traces of code to handle multiple WebKitThread
instances.  There are asserts in the WebKit api, so I suppose I should just let
them do their job.

http://codereview.chromium.org/149238/diff/101/1055#newcode67
Line 67: static int ref_count_;
On 2009/07/07 22:14:40, John Abd-El-Malek wrote:
> This doesn't look like it's needed anymore.

Done.

[jam, 2009-07-07 22:43:52]

lgtm with these two nits

http://codereview.chromium.org/149238/diff/101/1054
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/101/1054#newcode17
Line 17: : single_process_(CommandLine::ForCurrentProcess()->HasSwitch(
On 2009/07/07 22:31:02, Jeremy Orlow wrote:
> On 2009/07/07 22:14:40, John Abd-El-Malek wrote:
> > nit: why is this variable being cached?  CommandLine::ForCurrentProcess() is
> > threadsafe.
> 
> Speed.  I don't want to do a couple string copies + a map lookup every time we
> call WebKitThread every time GetMessageLoop is called when --single-process is
> specified.  This is an optimization for a condition that doesn't exist yet,
but
> it seems simple and lightweight enough.
> 
> Should I take it out?

Yeah I think it'd be better to take it out.  Since it only speeds up
--single-process mode which is just a testing mode, better to have less code and
let it be a little bit slower.

http://codereview.chromium.org/149238/diff/1092/116
File chrome/browser/in_process_webkit/webkit_thread.h (right):

http://codereview.chromium.org/149238/diff/1092/116#newcode62
Line 62: InternalWebKitThread* webkit_thread_;
nit: This should be inside a scoped_ptr.

[jorlow, 2009-07-07 23:36:37]

Done

Darin, can you take a look.

http://codereview.chromium.org/149238/diff/101/1054
File chrome/browser/in_process_webkit/webkit_thread.cc (right):

http://codereview.chromium.org/149238/diff/101/1054#newcode17
Line 17: : single_process_(CommandLine::ForCurrentProcess()->HasSwitch(
On 2009/07/07 22:43:52, John Abd-El-Malek wrote:
> On 2009/07/07 22:31:02, Jeremy Orlow wrote:
> > On 2009/07/07 22:14:40, John Abd-El-Malek wrote:
> > > nit: why is this variable being cached?  CommandLine::ForCurrentProcess()
is
> > > threadsafe.
> > 
> > Speed.  I don't want to do a couple string copies + a map lookup every time
we
> > call WebKitThread every time GetMessageLoop is called when --single-process
is
> > specified.  This is an optimization for a condition that doesn't exist yet,
> but
> > it seems simple and lightweight enough.
> > 
> > Should I take it out?
> 
> Yeah I think it'd be better to take it out.  Since it only speeds up
> --single-process mode which is just a testing mode, better to have less code
and
> let it be a little bit slower.

Done.

http://codereview.chromium.org/149238/diff/1092/116
File chrome/browser/in_process_webkit/webkit_thread.h (right):

http://codereview.chromium.org/149238/diff/1092/116#newcode62
Line 62: InternalWebKitThread* webkit_thread_;
On 2009/07/07 22:43:52, John Abd-El-Malek wrote:
> nit: This should be inside a scoped_ptr.

Done.

[darin (slow to review), 2009-07-07 23:43:03]

LGTM

http://codereview.chromium.org/149238/diff/130/1104
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/149238/diff/130/1104#newcode25
Line 25: NULL,  // WebKit
nit: WEBKIT

http://codereview.chromium.org/149238/diff/130/1105
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/149238/diff/130/1105#newcode82
Line 82: static bool CurrentlyOn(ID identifier);
nit: I suggested CalledOn to be consistent with base/non_thread_safe.h.  But,
CurrentlyOn reads well, so maybe base/non_thread_safe.h should change :) 
(doesn't have to be part of this CL.)

The behavior of returning true when not actually called on the specified thread
seems like it will lead to bugs.  But, maybe I'm over-thinking things.

http://codereview.chromium.org/149238/diff/130/1103
File chrome/browser/in_process_webkit/webkit_thread_unittest.cc (right):

http://codereview.chromium.org/149238/diff/130/1103#newcode8
Line 8: // TODO(jorlow): Write some tests.
file a bug and reference it here?

[jorlow, 2009-07-08 00:06:46]

Done...will commit soon.

http://codereview.chromium.org/149238/diff/130/1104
File chrome/browser/chrome_thread.cc (right):

http://codereview.chromium.org/149238/diff/130/1104#newcode25
Line 25: NULL,  // WebKit
On 2009/07/07 23:43:03, darin wrote:
> nit: WEBKIT

Done.

http://codereview.chromium.org/149238/diff/130/1105
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/149238/diff/130/1105#newcode82
Line 82: static bool CurrentlyOn(ID identifier);
On 2009/07/07 23:43:03, darin wrote:
> nit: I suggested CalledOn to be consistent with base/non_thread_safe.h.  But,
> CurrentlyOn reads well, so maybe base/non_thread_safe.h should change :) 
> (doesn't have to be part of this CL.)
> 
> The behavior of returning true when not actually called on the specified
thread
> seems like it will lead to bugs.

Possibly...but the way it is currently is definitely the common case.  In
addition, the only time you should see weirdness is during testing...so I'm not
too concerned.

> But, maybe I'm over-thinking things.

http://codereview.chromium.org/149238/diff/130/1103
File chrome/browser/in_process_webkit/webkit_thread_unittest.cc (right):

http://codereview.chromium.org/149238/diff/130/1103#newcode8
Line 8: // TODO(jorlow): Write some tests.
On 2009/07/07 23:43:03, darin wrote:
> file a bug and reference it here?

Done.

[huanr, 2009-07-14 20:27:32]

With the change, in DOMStorageDispatcherHost::Send, does message get deleted
twice?

[jorlow, 2009-07-14 20:40:55]

Actually, before the change it got deleted twice.  I believe it's correct
now.

Note that no one is actually using the thread yet (and that use will be
behind a flag for a bit).

J

On Tue, Jul 14, 2009 at 1:27 PM, <huanr@chromium.org> wrote:

> With the change, in DOMStorageDispatcherHost::Send, does message get
> deleted twice?
>
>
> http://codereview.chromium.org/149238
>

[huanr, 2009-07-15 05:07:23]

You are right. The old code has the bug. I was reviewing Coverity
defect and found this issue. It is based on an out of date source
check out.

Huan

On Tue, Jul 14, 2009 at 1:40 PM, Jeremy Orlow<jorlow@chromium.org> wrote:
> Actually, before the change it got deleted twice.=A0 I believe it's corre=
ct
> now.
>
> Note that no one is actually using the thread yet (and that use will be
> behind a flag for a bit).
>
> J
>
> On Tue, Jul 14, 2009 at 1:27 PM, <huanr@chromium.org> wrote:
>>
>> With the change, in DOMStorageDispatcherHost::Send, does message get
>> deleted twice?
>>
>> http://codereview.chromium.org/149238
>
>