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().

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

[Jeffrey Yasskin, 2014-05-22 00:57:58]

This depends on http://crrev.com/292903002.

[Jeffrey Yasskin, 2014-05-22 01:22:28]

Actually, ->falken to aid in his getting OWNERS here.

[michaeln, 2014-05-22 01:43:41]

drive-by-comments

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (left):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:115: return
process_manager_.get();
if this was implemented as return wrapper_->process_manager_get(), we wouldn't
need to expose the wrapper() getter. it might be nice to keep that out of the
public section unless/until we see more need to get at that class from the bulk
of the system

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.h (right):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:25: //
ServiceWorkerProcessManager is destroyed on the UI thread shortly after its
stale comment, 'shortly after' s/b 'when the'

[falken, 2014-05-22 02:43:19]

lgtm

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:178:
base::WeakPtrFactory<ServiceWorkerContextCore> weak_factory_;
It's not from this patch, but shouldn't WeakPtrFactory come at the end? I see it
can't because we use it to initialize the following members, but how does this
square with the usage comment in weak_ptr.h?

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:50: }
nit: newline here?

[Jeffrey Yasskin, 2014-05-22 02:47:54]

Thanks.

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (left):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:115: return
process_manager_.get();
On 2014/05/22 01:43:42, michaeln wrote:
> if this was implemented as return wrapper_->process_manager_get(), we wouldn't
> need to expose the wrapper() getter. it might be nice to keep that out of the
> public section unless/until we see more need to get at that class from the
bulk
> of the system

Sure, done.

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_core.h:178:
base::WeakPtrFactory<ServiceWorkerContextCore> weak_factory_;
On 2014/05/22 02:43:19, falken wrote:
> It's not from this patch, but shouldn't WeakPtrFactory come at the end? I see
it
> can't because we use it to initialize the following members, but how does this
> square with the usage comment in weak_ptr.h?

The reason to put it at the end is to get it invalidated before any of the
members are destroyed. ContextCore handles that by calling
"weak_factory_.InvalidateWeakPtrs();" explicitly in its destructor.

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:50: }
On 2014/05/22 02:43:19, falken wrote:
> nit: newline here?

Done.

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
File content/browser/service_worker/service_worker_process_manager.h (right):

https://codereview.chromium.org/292973003/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:25: //
ServiceWorkerProcessManager is destroyed on the UI thread shortly after its
On 2014/05/22 01:43:42, michaeln wrote:
> stale comment, 'shortly after' s/b 'when the'

Unfortunately, it's not stale. The SWContextWrapper is refcounted, so it can be
destroyed on any thread, not just the UI thread. To protect against that, I kept
the DefaultDeleter specialization. I could replace that with a use of
BrowserThread::DeleteOnUIThread in the SWContextWrapper definition if you
prefer.

(It's still important to destroy SWProcessManager on the UI thread because
that's where its WeakPtr is used.)

[michaeln1, 2014-05-22 02:56:37]

> Unfortunately, it's not stale. The SWContextWrapper is refcounted, so it can
be
> destroyed on any thread, not just the UI thread. To protect against that, I
kept
> the DefaultDeleter specialization. I could replace that with a use of
> BrowserThread::DeleteOnUIThread in the SWContextWrapper definition if you
> prefer.
> 
> (It's still important to destroy SWProcessManager on the UI thread because
> that's where its WeakPtr is used.)

i see, DefaultDeleter sneakery applies when scoped_ptr<> is used!

[Jeffrey Yasskin, 2014-05-23 07:07:36]

+Kinuko for owners.

[Jeffrey Yasskin, 2014-05-25 19:58:05]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-05-25 19:58:24]

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

[commit-bot: I haz the power, 2014-05-25 21:51:30]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-25 21:54:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-25 21:54:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Jeffrey Yasskin, 2014-05-25 21:58:06]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-05-25 21:58:16]

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

[commit-bot: I haz the power, 2014-05-25 22:01:42]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[commit-bot: I haz the power, 2014-05-25 22:05:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-25 22:05:25]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[Jeffrey Yasskin, 2014-05-25 22:55:37]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-05-25 22:55:59]

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

[commit-bot: I haz the power, 2014-05-26 16:15:10]

Change committed as 272839

[jochen (gone - plz use gerrit), 2014-05-26 18:29:33]

A revert of this CL has been created in
https://codereview.chromium.org/307443003/ by jochen@chromium.org.

The reason for reverting is: I suspect this broke
ServiceWorkerDispatcherHostTest.EarlyContextDeletion on linux.

[jam, 2014-05-26 19:24:12]

On 2014/05/26 18:29:33, jochen wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/307443003/ by mailto:jochen@chromium.org.
> 
> The reason for reverting is: I suspect this broke
> ServiceWorkerDispatcherHostTest.EarlyContextDeletion on linux.

just to confirm since I was looking at the tree, this revert did indeed fix the
test.