Teach EmbeddedWorkerInstance to create a process when it needs one.

Teach EmbeddedWorkerInstance to create a process when it needs one.

This saves the BrowserContext into the ServiceWorkerContextWrapper, and uses it when no process is available. We always choose a renderer process on the UI thread and take a reference to it before asking the renderer to start the SW thread. This also fixes a race where a process might die before we have a chance to use it.

We'll probably need to thread more process IDs through in cases where there might not be a ProviderHost, but that can wait for a subsequent CL.

BUG=359811, 362060, 362058
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266825

[Jeffrey Yasskin, 2014-04-25 04:40:32]

I haven't fully proofread this yet, so apologies if it has some obvious
mistakes.

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:608:
shell()->web_contents()->GetBrowserContext(),
Since the new ServiceWorker has no ProviderHosts, this should test that it can
be created on the UI thread.

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:238:
SetStatus(ACTIVATING);
This fixes the url_request_job_unittest, which was hitting the DCHECK in
ServiceWorkerFetchDispatcher::Run once StartWorker became less synchronous.

https://codereview.chromium.org/238043002/diff/30001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/238043002/diff/30001/content/renderer/service...
content/renderer/service_worker/embedded_worker_dispatcher.cc:66:
RenderThread::Get()->EnsureWebKitInitialized();
This is needed if the SW is the first code run in a new process.

[jam, 2014-04-25 04:59:30]

https://codereview.chromium.org/238043002/diff/30001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/238043002/diff/30001/content/public/browser/s...
content/public/browser/service_worker_context.h:40: BrowserContext* context,
this seems redundant since ServiceWorkerContext should know its own
browsercontext?

[kinuko, 2014-04-25 13:46:34]

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:20:
base::LazyInstance<base::Callback<bool(int)> > s_decrement_worker_refcount;
Whoa... ah ok it's for testing.

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:27: if (rph)
Probably also check if (!rph->FastShutdownStarted()) ?

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:30: return rph;
I prefer explicitly return a boolean value.

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.h:56: void
IncrementWorkerRef(
Could these method be factored out into its own class?  It's Start/Run method or
ctor could take a refptr to ContextWrapper to access the browser_context_ if
necessary.  These don't seem necessary to be public methods at the context layer
to me, and it'd be more readable if such process allocation part is factored out
into its own file.

Also the calling convention feels slightly weird to me, i.e. this is expected to
be called on UI thread and calls the callback on IO thread, while I think most
regular convention would be to dispatch callback on the original thread where
the method is called.

And one more nit.. since this also creates a new process if not available could
this be named better to indicate that too, e.g. ProcessAllocator or
ProcessReservationHelper or something like that?

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:80: }
Thanks for fixing this part... (should all message filters that inherit from
BrowserMessageFilter do something like this?)

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:145: switch
(running_status()) {
This looks nicer, thanks.

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:238:
SetStatus(ACTIVATING);
On 2014/04/25 04:40:33, Jeffrey Yasskin wrote:
> This fixes the url_request_job_unittest, which was hitting the DCHECK in
> ServiceWorkerFetchDispatcher::Run once StartWorker became less synchronous.

Uh oh I see. Thanks for noticing, this was sketchy.
Is it possible not to expose the 'second_call' param on the public method
signature?  I don't have a great solution, but say, could we have another member
field like pending_operation_ and set it to ACTIVATING while we're starting a
worker, and check if 'status() == INSTALLED || (status() == ACTIVATING &&
pending_operation_ == ACTIVATING)'?

[Jeffrey Yasskin, 2014-04-26 03:52:18]

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:20:
base::LazyInstance<base::Callback<bool(int)> > s_decrement_worker_refcount;
On 2014/04/25 13:46:35, kinuko wrote:
> Whoa... ah ok it's for testing.

And gone now.

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:27: if (rph)
On 2014/04/25 13:46:35, kinuko wrote:
> Probably also check if (!rph->FastShutdownStarted()) ?

Done.

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.cc:30: return rph;
On 2014/04/25 13:46:35, kinuko wrote:
> I prefer explicitly return a boolean value.

Done.

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_context_wrapper.h:56: void
IncrementWorkerRef(
On 2014/04/25 13:46:35, kinuko wrote:
> Could these method be factored out into its own class?  It's Start/Run method
or
> ctor could take a refptr to ContextWrapper to access the browser_context_ if
> necessary.  These don't seem necessary to be public methods at the context
layer
> to me, and it'd be more readable if such process allocation part is factored
out
> into its own file.

Done.

> Also the calling convention feels slightly weird to me, i.e. this is expected
to
> be called on UI thread and calls the callback on IO thread, while I think most
> regular convention would be to dispatch callback on the original thread where
> the method is called.

I can make these callable from the IO thread. It takes a surprising amount of
extra code to forward arguments back to another thread, so I'd rather not have
this call the callback on UI.

> And one more nit.. since this also creates a new process if not available
could
> this be named better to indicate that too, e.g. ProcessAllocator or
> ProcessReservationHelper or something like that?

I think I'll call the new class "ServiceWorkerProcessManager", and the
"increment" function "AllocateWorkerProcess", which probably implies that the
"decrement" function should be "ReleaseWorkerProcess".

Or I could define a ScopedGeneric typedef
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/scoped_gener...)
to hold the reference...

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:80: }
On 2014/04/25 13:46:35, kinuko wrote:
> Thanks for fixing this part... (should all message filters that inherit from
> BrowserMessageFilter do something like this?)

According to jam@, "historically, message filters have been used to respond to
IPCs from the renderer", and after an IPC is received, the channel is guaranteed
to be ready. I think this should be documented better, and perhaps
BrowserMessageFilter should DCHECK its Send() result instead of passing it
through, but it's not clear that "all message filters" are going to run into
this problem.

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

https://codereview.chromium.org/238043002/diff/30001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:238:
SetStatus(ACTIVATING);
On 2014/04/25 13:46:35, kinuko wrote:
> On 2014/04/25 04:40:33, Jeffrey Yasskin wrote:
> > This fixes the url_request_job_unittest, which was hitting the DCHECK in
> > ServiceWorkerFetchDispatcher::Run once StartWorker became less synchronous.
> 
> Uh oh I see. Thanks for noticing, this was sketchy.
> Is it possible not to expose the 'second_call' param on the public method
> signature?  I don't have a great solution, but say, could we have another
member
> field like pending_operation_ and set it to ACTIVATING while we're starting a
> worker, and check if 'status() == INSTALLED || (status() == ACTIVATING &&
> pending_operation_ == ACTIVATING)'?

Using a new field for this will allow the user to call DispatchActivateEvent()
twice in a row, which the existing checks would forbid.
I think the right way to hide the new argument is to use a private function for
the "AfterStartWorker" part. I'll switch to that.

https://codereview.chromium.org/238043002/diff/30001/content/public/browser/s...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/238043002/diff/30001/content/public/browser/s...
content/public/browser/service_worker_context.h:40: BrowserContext* context,
On 2014/04/25 04:59:30, jam wrote:
> this seems redundant since ServiceWorkerContext should know its own
> browsercontext?

At the moment, ServiceWorkerContext doesn't know its own BrowserContext, but I
can thread that through at construction instead.

[kinuko, 2014-04-28 06:58:15]

This looks much nicer to me now, a few more nits.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:51: void
EmbeddedWorkerRegistry::StartWorkerWithProcessId(
nit: method order's different from .h

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:57:
DCHECK(ContainsKey(worker_map_, embedded_worker_id));
Technically ContainsKey() could be false if all worker reference is freed before
AllocateWorkerProcess returns.

Should we call context_->process_manager()->ReleaseWorkerProcess(process_id),
fire callback with ABORT error and return immediately in the case?

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

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:51: weak_this_,
nit: This doesn't seem necessary given that it's posted from IO -> UI as in the
same order when we destruct this instance, but well that's probably why you
commented 'double-check' in the header..

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:70: // Shutdown
has started.
Should this be checked before we loop over process_ids?

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:124: return rph
!= NULL;
Since we only increment worker-ref when line 121 is true, maybe we should

if (rph && !rph->FastShutdownStarted()) {
  rph->IncrementWorkerRefCount();
  return true;
}
return false;

(or vice versa)

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

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:23: //
ServiceWorker system is using them.
Can you also add a comment to note that an instance of this always outlives
ServiceWorkerContextCore (though we have a similar comment at ctor) and is
destructed on UI thread?

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:26: // |*this|
must be scoped inside |context|. |increment_for_test| and
nit: |context| is a bit unclear in this context, |context_wrapper|->context() ?

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:34:
base::Callback<bool(int)>());
In chromium we don't use optional parameters.. could we have a separate
test-only method to override these?

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:76: void
operator()(content::ServiceWorkerProcessManager* ptr) const;
Aha, I'd just call DeleteSoon(BrowserThread::UI, FROM_HERE,
process_manager_.release()) in context core's dtor, but well this'd work too and
might be more explicit.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:218:
DispatchInstallEventAfterStartWorker(active_version_id, callback);
Thx, using private methods look nicer to me.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:122: const
std::vector<int>& potential_process_ids = std::vector<int>());
Again, no optional parameters in chromium...  Could we have another method like,
StartWorkerWithCandidateProcesses() (as function overloading is not really
preferred either), which takes process_ids param?  StartWorker can internally
call it then.

[Jeffrey Yasskin, 2014-04-28 20:47:40]

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:51: void
EmbeddedWorkerRegistry::StartWorkerWithProcessId(
On 2014/04/28 06:58:16, kinuko wrote:
> nit: method order's different from .h

Done.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:57:
DCHECK(ContainsKey(worker_map_, embedded_worker_id));
On 2014/04/28 06:58:16, kinuko wrote:
> Technically ContainsKey() could be false if all worker reference is freed
before
> AllocateWorkerProcess returns.
> 
> Should we call context_->process_manager()->ReleaseWorkerProcess(process_id),
> fire callback with ABORT error and return immediately in the case?

Good catch. I think I'd gotten confused between worker_map_ and
worker_process_map_, where a missing entry in the process_map would indicate a
bug here. Fixed and tested.

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

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:51: weak_this_,
On 2014/04/28 06:58:16, kinuko wrote:
> nit: This doesn't seem necessary given that it's posted from IO -> UI as in
the
> same order when we destruct this instance, but well that's probably why you
> commented 'double-check' in the header..

Yeah, I believe we don't need the WeakPtrFactory at all with the current
implementation, but I always hate tracing other people's code when they need the
order of calls to match in two separate parts of the code, so I didn't want to
cause anyone that problem in the future.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:70: // Shutdown
has started.
On 2014/04/28 06:58:16, kinuko wrote:
> Should this be checked before we loop over process_ids?

No, if we do that, a lot of the unittests would need to create a BrowserContext.
With the current ordering, they can override increment_for_test_, and cause the
allocation to succeed.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.cc:124: return rph
!= NULL;
On 2014/04/28 06:58:16, kinuko wrote:
> Since we only increment worker-ref when line 121 is true, maybe we should
> 
> if (rph && !rph->FastShutdownStarted()) {
>   rph->IncrementWorkerRefCount();
>   return true;
> }
> return false;
> 
> (or vice versa)

Yep, I forgot to update enough of this when I added FastShutdownStarted.

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

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:23: //
ServiceWorker system is using them.
On 2014/04/28 06:58:16, kinuko wrote:
> Can you also add a comment to note that an instance of this always outlives
> ServiceWorkerContextCore (though we have a similar comment at ctor) and is
> destructed on UI thread?

Done.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:26: // |*this|
must be scoped inside |context|. |increment_for_test| and
On 2014/04/28 06:58:16, kinuko wrote:
> nit: |context| is a bit unclear in this context, |context_wrapper|->context()
?

Whoops. That comment must be from when the ProcessManager just took a
BrowserContext.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:34:
base::Callback<bool(int)>());
On 2014/04/28 06:58:16, kinuko wrote:
> In chromium we don't use optional parameters.. could we have a separate
> test-only method to override these?

Done. Note that this makes it more complicated to specify how the fields are
accessed in a thread-safe way.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:76: void
operator()(content::ServiceWorkerProcessManager* ptr) const;
On 2014/04/28 06:58:16, kinuko wrote:
> Aha, I'd just call DeleteSoon(BrowserThread::UI, FROM_HERE,
> process_manager_.release()) in context core's dtor, but well this'd work too
and
> might be more explicit.

I was nervous about using a scoped_ptr that didn't actually include the
instructions for how to destroy its contents. It kind of breaks the contract for
scoped_ptr.

I considered beefing up BrowserThread::DeleteOnThread<UI> to work as a
scoped_ptr deleter, but then I realized that would make the code that creates
ProcessManagers significantly more verbose. Since we have the pattern that
refcounted objects can explicitly state the thread they get destroyed on, and
the definition of this class assumes that it's destroyed on UI, overriding the
default deleter seems like the right way to enforce that.

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:122: const
std::vector<int>& potential_process_ids = std::vector<int>());
On 2014/04/28 06:58:16, kinuko wrote:
> Again, no optional parameters in chromium...  Could we have another method
like,
> StartWorkerWithCandidateProcesses() (as function overloading is not really
> preferred either), which takes process_ids param?  StartWorker can internally
> call it then.

Sure, done.

[kinuko, 2014-04-29 01:06:51]

lgtm

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

https://codereview.chromium.org/238043002/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_process_manager.h:76: void
operator()(content::ServiceWorkerProcessManager* ptr) const;
On 2014/04/28 20:47:40, Jeffrey Yasskin wrote:
> On 2014/04/28 06:58:16, kinuko wrote:
> > Aha, I'd just call DeleteSoon(BrowserThread::UI, FROM_HERE,
> > process_manager_.release()) in context core's dtor, but well this'd work too
> and
> > might be more explicit.
> 
> I was nervous about using a scoped_ptr that didn't actually include the
> instructions for how to destroy its contents. It kind of breaks the contract
for
> scoped_ptr.
> 
> I considered beefing up BrowserThread::DeleteOnThread<UI> to work as a
> scoped_ptr deleter, but then I realized that would make the code that creates
> ProcessManagers significantly more verbose. Since we have the pattern that
> refcounted objects can explicitly state the thread they get destroyed on, and
> the definition of this class assumes that it's destroyed on UI, overriding the
> default deleter seems like the right way to enforce that.

Yep, I can see some benefits here.

https://codereview.chromium.org/238043002/diff/130001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/238043002/diff/130001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.cc:182:
context_->process_manager()->ReleaseWorkerProcess(process_id);
Check if (context_) before accessing it here too?

[jam, 2014-04-29 01:07:54]

gypi & public lgtm

[Jeffrey Yasskin, 2014-04-29 01:37:29]

Thanks for the review!

https://codereview.chromium.org/238043002/diff/130001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/238043002/diff/130001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.cc:182:
context_->process_manager()->ReleaseWorkerProcess(process_id);
On 2014/04/29 01:06:52, kinuko wrote:
> Check if (context_) before accessing it here too?

Done, here and in StartWorker itself.

[Jeffrey Yasskin, 2014-04-29 01:37:35]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-04-29 01:38:41]

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

[commit-bot: I haz the power, 2014-04-29 02:59:03]

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

[commit-bot: I haz the power, 2014-04-29 02:59:04]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Jeffrey Yasskin, 2014-04-29 05:21:06]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-04-29 05:21:53]

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

[commit-bot: I haz the power, 2014-04-29 10:15:16]

Change committed as 266825

[michaeln, 2014-04-29 19:08:44]

https://codereview.chromium.org/238043002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_process_manager.h (right):

https://codereview.chromium.org/238043002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_process_manager.h:28: // |*this|
must be owned by |context_wrapper|->context().
Hmmm... does this create a reference cycle?

Wrapper owns Core owns Mgr refs Wrapper

[michaeln, 2014-04-29 19:10:51]

I don't see how that cycle is broken.

> must be owned by |context_wrapper|->context().
> Hmmm... does this create a reference cycle?
> 
> Wrapper owns Core owns Mgr refs Wrapper

[Jeffrey Yasskin, 2014-04-29 19:13:24]

https://codereview.chromium.org/238043002/diff/150001/content/browser/service...
File content/browser/service_worker/service_worker_process_manager.h (right):

https://codereview.chromium.org/238043002/diff/150001/content/browser/service...
content/browser/service_worker/service_worker_process_manager.h:28: // |*this|
must be owned by |context_wrapper|->context().
On 2014/04/29 19:08:45, michaeln wrote:
> Hmmm... does this create a reference cycle?
> 
> Wrapper owns Core owns Mgr refs Wrapper

Yes. The cycle is broken in Wrapper::Shutdown(). I forgot to add that comment
back in when I re-introduced the cycle.

We could also break the cycle by adding a WeakPtr to Wrapper. I don't think
there's a way to use just the WeakPtr to Core because we need to null out
browser_context_ when Shutdown() is called, before hopping threads.

[michaeln1, 2014-04-29 22:14:58]

Ah... i see, thanx!

> Yes. The cycle is broken in Wrapper::Shutdown(). I forgot to add that comment
> back in when I re-introduced the cycle.
> 
> We could also break the cycle by adding a WeakPtr to Wrapper. I don't think
> there's a way to use just the WeakPtr to Core because we need to null out
> browser_context_ when Shutdown() is called, before hopping threads.

nah... what you have looks great, now that i see it;)

[horo, 2014-04-30 03:12:29]

After this patch is landed, when I close the Chrome browser after registering a
ServiceWorker, DCHECK in ProfileDestroyer::DestroyProfileWhenAppropriate() fails
because content::RenderProcessHost::run_renderer_in_process() is 1.

I think this is because the registered ServiceWorker doesn't stop and
DecrementWorkerRefcount() is not called when I click the close button of Chrome.

Is this intended or a bug?

[kinuko, 2014-04-30 05:31:19]

On 2014/04/30 03:12:29, horo wrote:
> After this patch is landed, when I close the Chrome browser after registering
a
> ServiceWorker, DCHECK in ProfileDestroyer::DestroyProfileWhenAppropriate()
fails
> because content::RenderProcessHost::run_renderer_in_process() is 1.
> 
> I think this is because the registered ServiceWorker doesn't stop and
> DecrementWorkerRefcount() is not called when I click the close button of
Chrome.
> 
> Is this intended or a bug?

Can we file a separate bug for this? Though I don't find a proper way to fix
this yet.
There're multiple things happening,

1. We don't have a proper code to explicitly stop the worker yet except for
EmbeddedWorkerRegistry::Shutdown()
2. ProfileDstroyer::DestroyProfileWhenAppropriate() is called much ealier than
ServiceWorkerContextWrapper::Shutdown() is called
3. EmbeddedWorkerRegistry's ref-counted, and its Shutdown() may not be called
when ServiceWorkerContextWrapper's shutdown() is called. (This can be easily
fixed by explicitly calling EmbeddedWorkerRegistry::Shutdown() in
SWContextCore's dtor)

Also stopping a worker currently crashes, but it's another issue (and already
filed, to be fixed by branch cut)

[kinuko, 2014-04-30 05:32:25]

On 2014/04/30 05:31:19, kinuko wrote:
> On 2014/04/30 03:12:29, horo wrote:
> > After this patch is landed, when I close the Chrome browser after
registering
> a
> > ServiceWorker, DCHECK in ProfileDestroyer::DestroyProfileWhenAppropriate()
> fails
> > because content::RenderProcessHost::run_renderer_in_process() is 1.
> > 
> > I think this is because the registered ServiceWorker doesn't stop and
> > DecrementWorkerRefcount() is not called when I click the close button of
> Chrome.
> > 
> > Is this intended or a bug?
> 
> Can we file a separate bug for this? Though I don't find a proper way to fix
> this yet.
> There're multiple things happening,
> 
> 1. We don't have a proper code to explicitly stop the worker yet except for
> EmbeddedWorkerRegistry::Shutdown()
> 2. ProfileDstroyer::DestroyProfileWhenAppropriate() is called much ealier than
> ServiceWorkerContextWrapper::Shutdown() is called
> 3. EmbeddedWorkerRegistry's ref-counted, and its Shutdown() may not be called
> when ServiceWorkerContextWrapper's shutdown() is called. (This can be easily
> fixed by explicitly calling EmbeddedWorkerRegistry::Shutdown() in
> SWContextCore's dtor)

And in our case 2. looks to be most problematic.

> Also stopping a worker currently crashes, but it's another issue (and already
> filed, to be fixed by branch cut)

[horo, 2014-04-30 05:55:50]

Filed a bug http://crbug.com/368570.

[Jeffrey Yasskin, 2014-04-30 07:27:20]

Sorry for the trouble. I'll look into getting this into the right
order tomorrow, unless you've figured it out or rolled it back first.

On Tue, Apr 29, 2014 at 8:12 PM,  <horo@chromium.org> wrote:
> After this patch is landed, when I close the Chrome browser after
> registering a
> ServiceWorker, DCHECK in ProfileDestroyer::DestroyProfileWhenAppropriate()
> fails
> because content::RenderProcessHost::run_renderer_in_process() is 1.
>
> I think this is because the registered ServiceWorker doesn't stop and
> DecrementWorkerRefcount() is not called when I click the close button of
> Chrome.
>
> Is this intended or a bug?
>
> https://codereview.chromium.org/238043002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.