ServiceWorker: Send state change events via SWProviderHost

ServiceWorker: Send state change events via SWProviderHost

This CL refactors relationship among SW(Registration)Handle, SWProviderHost and
SWDispatcherHost.

Before this CL, the handle directly asks SWDispatcherHost to send an IPC
message when it receives an state change notification. After this CL, the
handle retains a weakptr to SWProviderHost and asks it to send the message.

This change enables that...

- the handles don't have to take care of receiver's thread id anymore.
It's managed by SWProviderHost instead. (Currently the thread id is hard-coded
as kDocumentMainThreadId, but I'll make it injectable in a subsequent CL.)

- IPC messages could be queued up when the receiver's thread hasn't been ready
to receive the messages. This would be useful when we support the worker thread
as a receiver's thread.

BUG=437677
TEST=content_unittests --gtest_filter=ServiceWorker*
TEST=run-webkit-tests http/tests/serviceworker/

Committed: https://crrev.com/084b4564fc3f39352e9c394fd803a9fb248e2efb
Cr-Commit-Position: refs/heads/master@{#312592}

[nhiroki, 2015-01-20 05:59:58]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-01-20 06:00:04]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-01-20 06:00:10]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-01-20 06:00:16]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2015-01-20 06:19:16]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2015-01-20 06:19:21]

Patchset #1 (id:100001) has been deleted

[nhiroki, 2015-01-20 06:37:15]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org, kinuko@chromium.org

[nhiroki, 2015-01-20 06:37:16]

Hi, can you review this?

I'd like to use this change in my separate CL[1] so that the browser process can
directly send state change events to the worker thread. Please feel free to ask
me if it's difficult to understand an intention of this CL.

Thanks.

[1] https://codereview.chromium.org/849163002/

[falken, 2015-01-21 04:59:29]

Quick comment, is the design doc up-to-date:
https://docs.google.com/document/d/1qDGbMlwKOXxCRBlw9IirK8Qmna3JqvnO3Aogkfu9U...

Where does this CL fit into the design doc?

Also, WebServiceWorkerImpl has its own logic for queuing up state change events.
It seems like it's starting to get complex with queuing in various places.
Ideally somehow there's just one queuing spot. I don't totally have it all in my
head though about how these fit together.

[nhiroki, 2015-01-21 06:42:42]

On 2015/01/21 04:59:29, falken wrote:
> Quick comment, is the design doc up-to-date:
>
https://docs.google.com/document/d/1qDGbMlwKOXxCRBlw9IirK8Qmna3JqvnO3Aogkfu9U...
> 
> Where does this CL fit into the design doc?

This CL is necessary for new step3 which will be described in the design doc
(sorry, I'm now writing it).
An actual (WIP) CL for the step 3 is available here:
https://codereview.chromium.org/855383006/

> Also, WebServiceWorkerImpl has its own logic for queuing up state change
events.
> It seems like it's starting to get complex with queuing in various places.
> Ideally somehow there's just one queuing spot. I don't totally have it all in
my
> head though about how these fit together.

IMO, I'd prefer to keep multiple queuing mechanisms because their purposes would
be different and merging them seems to make the implementation more complicated.
The existing queuing mechanism is used to mediate communication between Blink
and Renderer (waiting for initializing Blink's worker/registration proxies when
new worker/registration object is created) and that's NOT related to the browser
process. On the other hand, the new queuing mechanism will be used to mediate
communication Renderer(WorkerThread) and Browser (waiting for initializing
worker thread in order to directly communicate with the worker thread).

[kinuko, 2015-01-21 06:47:30]

(Sorry for slow review) I agree that having a separate queue is ok (will need to
think twice if it's optimal or not, but anyway).  Afair the state change queue
in WebServiceWorkerImpl was for working around the ordering issue due  to
asynchronous promise resolution with micro tasks, which is not in the browser
process.

[kinuko, 2015-01-21 06:48:23]

> asynchronous promise resolution with micro tasks, which is not in the browser
> process.

which is not in the... -> which is not visible in the...

[nhiroki, 2015-01-21 07:39:17]

On 2015/01/21 06:42:42, nhiroki wrote:
> On 2015/01/21 04:59:29, falken wrote:
> > Quick comment, is the design doc up-to-date:
> >
>
https://docs.google.com/document/d/1qDGbMlwKOXxCRBlw9IirK8Qmna3JqvnO3Aogkfu9U...
> > 
> > Where does this CL fit into the design doc?
> 
> This CL is necessary for new step3 which will be described in the design doc
> (sorry, I'm now writing it).
> An actual (WIP) CL for the step 3 is available here:
> https://codereview.chromium.org/855383006/

FYI: Wrote the new step3 in the design doc.

[kinuko, 2015-01-21 10:11:00]

I think this is looking good.

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:612:
base::ASCIIToUTF16(kShutdownErrorMessage)));
Why do you add this error message? (It doesn't look really related to this
change, and when provider host's gone the receiver document's probably not there
either?)

[nhiroki, 2015-01-22 01:59:56]

Patchset #2 (id:140001) has been deleted

[nhiroki, 2015-01-22 03:34:15]

Thank you for reviewing. Comment reply only.

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:612:
base::ASCIIToUTF16(kShutdownErrorMessage)));
On 2015/01/21 10:11:00, kinuko wrote:
> Why do you add this error message? (It doesn't look really related to this

|provider_host->IsContextAlive()| may return false due to
|SWContextWrapper::DeleteAndStartOver()|, that is, transient system shutdown. In
this case, |provider_host| (receiver document) is still valid but the associated
context is not, so the provider_host should not work with it anymore. This is
the reason why I added the error message here.

Now I'm wondering if we can just ignore the case because the registration itself
was successfully completed at this point. But in this case an empty registration
info will be created based on the invalid context at line 627 and will be
sent... hmmm...

> change, and when provider host's gone the receiver document's probably not
there
> either?)

This case just return without sending a message at line 606.

[nhiroki, 2015-01-22 04:12:51]

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:612:
base::ASCIIToUTF16(kShutdownErrorMessage)));
On 2015/01/22 03:34:14, nhiroki wrote:
> On 2015/01/21 10:11:00, kinuko wrote:
> > Why do you add this error message? (It doesn't look really related to this
> 
> |provider_host->IsContextAlive()| may return false due to
> |SWContextWrapper::DeleteAndStartOver()|, that is, transient system shutdown.
In
> this case, |provider_host| (receiver document) is still valid but the
associated
> context is not, so the provider_host should not work with it anymore. This is
> the reason why I added the error message here.
> 
> Now I'm wondering if we can just ignore the case because the registration
itself
> was successfully completed at this point. But in this case an empty
registration
> info will be created based on the invalid context at line 627 and will be
> sent... hmmm...

If |DeleteAndStartOver()| happens between OnRegisterServiceWorker() and
RegistrationCompeltion(), probably |status| might not be OK and handled by line
616. So we could skip the IsContextAlive() check and don't take care of the
empty registration info case I mentioned.

Okay, I'll remove the check and the message.

> > change, and when provider host's gone the receiver document's probably not
> there
> > either?)
> 
> This case just return without sending a message at line 606.

[nhiroki, 2015-01-22 04:33:43]

Removed the unnecessary error handling. Can you take another look?

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/825383004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:612:
base::ASCIIToUTF16(kShutdownErrorMessage)));
On 2015/01/22 04:12:50, nhiroki wrote:
> On 2015/01/22 03:34:14, nhiroki wrote:
> > On 2015/01/21 10:11:00, kinuko wrote:
> > > Why do you add this error message? (It doesn't look really related to this
> > 
> > |provider_host->IsContextAlive()| may return false due to
> > |SWContextWrapper::DeleteAndStartOver()|, that is, transient system
shutdown.
> In
> > this case, |provider_host| (receiver document) is still valid but the
> associated
> > context is not, so the provider_host should not work with it anymore. This
is
> > the reason why I added the error message here.
> > 
> > Now I'm wondering if we can just ignore the case because the registration
> itself
> > was successfully completed at this point. But in this case an empty
> registration
> > info will be created based on the invalid context at line 627 and will be
> > sent... hmmm...
> 
> If |DeleteAndStartOver()| happens between OnRegisterServiceWorker() and
> RegistrationCompeltion(), probably |status| might not be OK and handled by
line
> 616. So we could skip the IsContextAlive() check and don't take care of the
> empty registration info case I mentioned.
> 
> Okay, I'll remove the check and the message.
> 
> > > change, and when provider host's gone the receiver document's probably not
> > there
> > > either?)
> > 
> > This case just return without sending a message at line 606.
> 

Done.

[kinuko, 2015-01-22 06:30:52]

lgtm

https://codereview.chromium.org/825383004/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/825383004/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:121: //
OnDecrementServiceWorkerRefCount.
nit: 'via OnDecrementServiceWorkerRefCount' part could be probably dropped
(feels a bit out of context) now that this method's no longer on DispatcherHost,
where the deref method is defined.

[nhiroki, 2015-01-22 07:05:59]

https://codereview.chromium.org/825383004/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/825383004/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.h:121: //
OnDecrementServiceWorkerRefCount.
On 2015/01/22 06:30:52, kinuko wrote:
> nit: 'via OnDecrementServiceWorkerRefCount' part could be probably dropped
> (feels a bit out of context) now that this method's no longer on
DispatcherHost,
> where the deref method is defined.

Done.

[nhiroki, 2015-01-22 08:08:56]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-01-22 08:09:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/825383004/200001

[commit-bot: I haz the power, 2015-01-22 08:59:59]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2015-01-22 09:01:49]

Patchset 4 (id:??) landed as
https://crrev.com/084b4564fc3f39352e9c394fd803a9fb248e2efb
Cr-Commit-Position: refs/heads/master@{#312592}