Reparent SWProcessManager onto SWContextWrapper.

Reparent SWProcessManager onto SWContextWrapper.

This will allow Wrapper::Shutdown to drop all process references synchronously
in a future change, which will make it possible to shutdown Chrome with service
workers running and without hitting the DCHECK in
ProfileDestroyer::DestroyProfileWhenAppropriate().

This relands http://crrev.com/272839 (reverted by http://crrev.com/272858), and
fixes the breakage in
ServiceWorkerDispatcherHostTest.EarlyContextDeletion. The breakage happened
because I changed ContextWrapper::Shutdown to always PostTask instead of
running inline when CurrentlyOn(BrowserThread::IO). I haven't uncovered why the
breakage only happened on slow bots.

BUG=368570
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273184

[Jeffrey Yasskin, 2014-05-27 23:33:25]

PTAL.

[dominicc (has gone to gerrit), 2014-05-27 23:57:11]

On 2014/05/27 23:33:25, Jeffrey Yasskin wrote:
> PTAL.

Informally LGTM.

Is reaching into the context_core_ and resetting its reference to the wrapper
before destroying it too belt-and-suspenders? If the context_core_ destructor
gets more complicated later it would make detecting bugs easy.

[Jeffrey Yasskin, 2014-05-28 00:13:02]

On 2014/05/27 23:57:11, dominicc wrote:
> On 2014/05/27 23:33:25, Jeffrey Yasskin wrote:
> > PTAL.
> 
> Informally LGTM.
> 
> Is reaching into the context_core_ and resetting its reference to the wrapper
> before destroying it too belt-and-suspenders? If the context_core_ destructor
> gets more complicated later it would make detecting bugs easy.

We can't have the UI-thread part of ContextWrapper::Shutdown change the
IO-thread ContextCore::wrapper_. (without a lock, which Chrome doesn't really
use for this) We could do that from the test, but I'm not sure that's better
than running the event loop. Sorry if I'm misunderstanding.

[kinuko, 2014-05-28 00:20:42]

(+horo, I think he can / may want to also review this)

[dominicc (has gone to gerrit), 2014-05-28 01:19:00]

On 2014/05/28 00:13:02, Jeffrey Yasskin wrote:
> On 2014/05/27 23:57:11, dominicc wrote:
> > On 2014/05/27 23:33:25, Jeffrey Yasskin wrote:
> > > PTAL.
> > 
> > Informally LGTM.
> > 
> > Is reaching into the context_core_ and resetting its reference to the
wrapper
> > before destroying it too belt-and-suspenders? If the context_core_
destructor
> > gets more complicated later it would make detecting bugs easy.
> 
> We can't have the UI-thread part of ContextWrapper::Shutdown change the
> IO-thread ContextCore::wrapper_. (without a lock, which Chrome doesn't really
> use for this) We could do that from the test, but I'm not sure that's better
> than running the event loop. Sorry if I'm misunderstanding.

Ah, of course. The part of my brain cognizant of shared-memory multithreaded
programming has apparently completely atrophied. No wonder I'm happier lately.
:)

[horo, 2014-05-28 01:45:02]

On 2014/05/28 00:13:02, Jeffrey Yasskin wrote:
> On 2014/05/27 23:57:11, dominicc wrote:
> > On 2014/05/27 23:33:25, Jeffrey Yasskin wrote:
> > > PTAL.
> > 
> > Informally LGTM.
> > 
> > Is reaching into the context_core_ and resetting its reference to the
wrapper
> > before destroying it too belt-and-suspenders? If the context_core_
destructor
> > gets more complicated later it would make detecting bugs easy.
> 
> We can't have the UI-thread part of ContextWrapper::Shutdown change the
> IO-thread ContextCore::wrapper_. (without a lock, which Chrome doesn't really
> use for this) We could do that from the test, but I'm not sure that's better
> than running the event loop. Sorry if I'm misunderstanding.

What will happen if ServiceWorkerContextWrapper is deleted right after
Shutdown() is called without hopping to IO-thread?
This case doesn't occur in Chrome browser?

[horo, 2014-05-28 01:51:17]

On 2014/05/28 01:45:02, horo wrote:
> On 2014/05/28 00:13:02, Jeffrey Yasskin wrote:
> > On 2014/05/27 23:57:11, dominicc wrote:
> > > On 2014/05/27 23:33:25, Jeffrey Yasskin wrote:
> > > > PTAL.
> > > 
> > > Informally LGTM.
> > > 
> > > Is reaching into the context_core_ and resetting its reference to the
> wrapper
> > > before destroying it too belt-and-suspenders? If the context_core_
> destructor
> > > gets more complicated later it would make detecting bugs easy.
> > 
> > We can't have the UI-thread part of ContextWrapper::Shutdown change the
> > IO-thread ContextCore::wrapper_. (without a lock, which Chrome doesn't
really
> > use for this) We could do that from the test, but I'm not sure that's better
> > than running the event loop. Sorry if I'm misunderstanding.
> 
> What will happen if ServiceWorkerContextWrapper is deleted right after
> Shutdown() is called without hopping to IO-thread?
> This case doesn't occur in Chrome browser?

Oh, sorry I understood!
BrowserThread::PostTask in Shutdown() increments the refcount of
ServiceWorkerContextWrapper.
lgtm

[Jeffrey Yasskin, 2014-05-28 01:58:17]

On 2014/05/28 01:51:17, horo wrote:
> On 2014/05/28 01:45:02, horo wrote:
> > On 2014/05/28 00:13:02, Jeffrey Yasskin wrote:
> > > On 2014/05/27 23:57:11, dominicc wrote:
> > > > On 2014/05/27 23:33:25, Jeffrey Yasskin wrote:
> > > > > PTAL.
> > > > 
> > > > Informally LGTM.
> > > > 
> > > > Is reaching into the context_core_ and resetting its reference to the
> > wrapper
> > > > before destroying it too belt-and-suspenders? If the context_core_
> > destructor
> > > > gets more complicated later it would make detecting bugs easy.
> > > 
> > > We can't have the UI-thread part of ContextWrapper::Shutdown change the
> > > IO-thread ContextCore::wrapper_. (without a lock, which Chrome doesn't
> really
> > > use for this) We could do that from the test, but I'm not sure that's
better
> > > than running the event loop. Sorry if I'm misunderstanding.
> > 
> > What will happen if ServiceWorkerContextWrapper is deleted right after
> > Shutdown() is called without hopping to IO-thread?
> > This case doesn't occur in Chrome browser?
> 
> Oh, sorry I understood!
> BrowserThread::PostTask in Shutdown() increments the refcount of
> ServiceWorkerContextWrapper.
> lgtm

Yeah, it's subtle. I'm happy to see that I actually added a comment for this,
although possibly not in the right place since you didn't find it:
https://codereview.chromium.org/303733002/diff/20001/content/browser/service_...

[falken, 2014-05-28 02:24:38]

On 2014/05/28 01:58:17, Jeffrey Yasskin wrote:
> On 2014/05/28 01:51:17, horo wrote:
> > On 2014/05/28 01:45:02, horo wrote:
> > > On 2014/05/28 00:13:02, Jeffrey Yasskin wrote:
> > > > On 2014/05/27 23:57:11, dominicc wrote:
> > > > > On 2014/05/27 23:33:25, Jeffrey Yasskin wrote:
> > > > > > PTAL.
> > > > > 
> > > > > Informally LGTM.
> > > > > 
> > > > > Is reaching into the context_core_ and resetting its reference to the
> > > wrapper
> > > > > before destroying it too belt-and-suspenders? If the context_core_
> > > destructor
> > > > > gets more complicated later it would make detecting bugs easy.
> > > > 
> > > > We can't have the UI-thread part of ContextWrapper::Shutdown change the
> > > > IO-thread ContextCore::wrapper_. (without a lock, which Chrome doesn't
> > really
> > > > use for this) We could do that from the test, but I'm not sure that's
> better
> > > > than running the event loop. Sorry if I'm misunderstanding.
> > > 
> > > What will happen if ServiceWorkerContextWrapper is deleted right after
> > > Shutdown() is called without hopping to IO-thread?
> > > This case doesn't occur in Chrome browser?
> > 
> > Oh, sorry I understood!
> > BrowserThread::PostTask in Shutdown() increments the refcount of
> > ServiceWorkerContextWrapper.
> > lgtm
> 
> Yeah, it's subtle. I'm happy to see that I actually added a comment for this,
> although possibly not in the right place since you didn't find it:
>
https://codereview.chromium.org/303733002/diff/20001/content/browser/service_...

lgtm
may be nice to wrap the long line in the CL description more nicely

[Jeffrey Yasskin, 2014-05-28 04:05:00]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-05-28 04:05:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/303733002/20001

[commit-bot: I haz the power, 2014-05-28 04:30:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-28 08:48:07]

Change committed as 273184