Implement ServiceWorkerVersion::SendMessage() (for dispatching events etc)

Implement ServiceWorkerVersion::SendMessage()

As a preparation to implement DispatchInstallEvent(), this implements
following two methods:
- SendMessage()
- SendMessageAndRegisterCallback()

Unlike my previous patches, this supports:
- Sending messages without explicitly starting the worker
- Sending multiple messages concurrently
- Receiving a response for each message

Now each message can be sent with a unique 'request_id', which can be
used to send back a response to a particular message.

BUG=313530
TEST=ServiceWorkerVersionTest.\*
R=alecflett@chromium.org

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

[kinuko, 2014-01-31 10:32:14]

This patch tries to support much more generic messaging than my previous
patches.  PTL

[alecflett, 2014-02-04 20:53:24]

overall I like this - a few nits and one issue with requests.

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

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:205: // Let all message
callbacks fail.
lets add a TODO that indicates that in the future we'll likely re-send the
messages. (i.e. requeue them for the next call to StartWorker)

This also makes me wonder if there are certain messages that we shouldn't
re-send? But lets cross that bridge when we get to it.

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:219:
message_callbacks_.Remove(request_id);
This is going to be a little tricky - it's going to be possible to get multiple
responses back from an outgoing message. For fetch, this is an error, I think
it's catch-able on the renderer side:

addEventListener("fetch", function(e) { e.respondWith(fetch(e.request))});
addEventListener("fetch", function(e) { e.respondWith(fetch(e.request))});

(the 2nd event handler should fail when calling e.respondWith())

But at the same time, the install event could be waiting on multiple things:

addEventListener("install", function(e) { e.waitFor(caches.get('foo').add(url1);
});
addEventListener("install", function(e) { e.waitFor(caches.get('foo').add(url2);
});

I'm honestly not quite sure what the right thing to do here is - it seems like
the callback implementation itself needs to be the one to clear the request. We
could add 'this' (i.e. the ServiceWorkerVersion instance) as a parameter to
MessageCallback, and then MessageCallback's implementation could call BACK into
the version to clear it?

That said, I think we should keep what you have but add a TODO to let the
callback clear the request id.

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:192: //
SendMessage should return START_WORKER_FAILED error since it tried to
I think you should also add a test for letting the worker stop, and then sending
it a new message - to make sure stopping a worker doesn't wedge it in some
permanent way.

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:28:
current_request_id_ = request_id;
lets zero this out (or set it to -1) in the handlers, and then DCHECK that it
isn't set, to make sure we don't accidentally have reentrant handlers or
something.

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.h (right):

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.h:34: void
OnMessageReceived(int request_id, const IPC::Message& message);
I'm starting to think this should return a bool (for 'handled') and propagate
that as far as it can go - giving us a backstop to catch bad messages (i.e.
security checks)

[alecflett, 2014-02-04 21:09:34]

On 2014/02/04 20:53:24, alecflett wrote:
> I'm honestly not quite sure what the right thing to do here is - it seems like
> the callback implementation itself needs to be the one to clear the request.
We
> could add 'this' (i.e. the ServiceWorkerVersion instance) as a parameter to
> MessageCallback, and then MessageCallback's implementation could call BACK
into
> the version to clear it?

In continuing to think about this, I asked myself why the browser process would
even want to do this accounting.. what I came up with was that if the
promise-being-waited-on is truly based on some backend operation (like
cache.add() or something, then it sure would be nice if we could bypass the
renderer, or let the renderer get killed, and still finish the installation.
(Because as it stands, if the renderer is killed during installation, the
installation will fail, and I'm not sure if that's the right behavior or not.)

But maybe that's a more advanced case then we really need to worry about here?
Maybe for now the renderer really can do all the accounting about pending
install responses?

[kinuko, 2014-02-04 22:17:12]

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

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:219:
message_callbacks_.Remove(request_id);
On 2014/02/04 20:53:24, alecflett wrote:
> This is going to be a little tricky - it's going to be possible to get
multiple
> responses back from an outgoing message. For fetch, this is an error, I think
> it's catch-able on the renderer side:
> 
> addEventListener("fetch", function(e) { e.respondWith(fetch(e.request))});
> addEventListener("fetch", function(e) { e.respondWith(fetch(e.request))});
> 
> (the 2nd event handler should fail when calling e.respondWith())
> 
> But at the same time, the install event could be waiting on multiple things:
> 
> addEventListener("install", function(e) {
e.waitFor(caches.get('foo').add(url1);
> });
> addEventListener("install", function(e) {
e.waitFor(caches.get('foo').add(url2);
> });

I'm planning to wait ('join') all these promises in the child process side, so
this should just get one response.

Or will there be events we need to wait for in the browser side?  (I'm afriad
it'll make things way more complex)

> I'm honestly not quite sure what the right thing to do here is - it seems like
> the callback implementation itself needs to be the one to clear the request.
We
> could add 'this' (i.e. the ServiceWorkerVersion instance) as a parameter to
> MessageCallback, and then MessageCallback's implementation could call BACK
into
> the version to clear it?
> 
> That said, I think we should keep what you have but add a TODO to let the
> callback clear the request id.

[kinuko, 2014-02-04 22:50:24]

Updated, or added response.

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

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:205: // Let all message
callbacks fail.
On 2014/02/04 20:53:24, alecflett wrote:
> lets add a TODO that indicates that in the future we'll likely re-send the
> messages. (i.e. requeue them for the next call to StartWorker)
> 
> This also makes me wonder if there are certain messages that we shouldn't
> re-send? But lets cross that bridge when we get to it.

It totally makes sense to resend install on another restart, but for onfetch
just waiting for another call to StartWorker while letting the controllee
documents wait forever feels wrong.  If situation could differ by message types
we shouldn't probably add a generic mechanism here, but the user of this method
should implement that?

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/139923005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version_unittest.cc:192: //
SendMessage should return START_WORKER_FAILED error since it tried to
On 2014/02/04 20:53:24, alecflett wrote:
> I think you should also add a test for letting the worker stop, and then
sending
> it a new message - to make sure stopping a worker doesn't wedge it in some
> permanent way.

Done.

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:28:
current_request_id_ = request_id;
On 2014/02/04 20:53:24, alecflett wrote:
> lets zero this out (or set it to -1) in the handlers, and then DCHECK that it
> isn't set, to make sure we don't accidentally have reentrant handlers or
> something.

Done.  I just set it to -1 in the end of this method.

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.h (right):

https://codereview.chromium.org/139923005/diff/40001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.h:34: void
OnMessageReceived(int request_id, const IPC::Message& message);
On 2014/02/04 20:53:24, alecflett wrote:
> I'm starting to think this should return a bool (for 'handled') and propagate
> that as far as it can go - giving us a backstop to catch bad messages (i.e.
> security checks)

Yup, I had the same thought, that'll be addressed in my next patch

(my local patch once had it but I removed it before uploading since I didn't
want to make this patch bigger)

[alecflett, 2014-02-04 23:16:44]

lgtm

[jsbell, 2014-02-04 23:29:03]

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

https://codereview.chromium.org/139923005/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:176: -1,
ServiceWorkerMsg_FetchEvent(request)) == SERVICE_WORKER_OK;
Sorry for the late drive-by, but -1 is a magic number?

https://codereview.chromium.org/139923005/diff/70001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/139923005/diff/70001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:20:
current_request_id_(-1) {
Ditto, -1 as magic number. Can you use an enum/constant?

[commit-bot: I haz the power, 2014-02-04 23:30:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/139923005/70001

[kinuko, 2014-02-05 00:39:20]

On 2014/02/04 23:29:03, jsbell wrote:
>
https://codereview.chromium.org/139923005/diff/70001/content/browser/service_...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/139923005/diff/70001/content/browser/service_...
> content/browser/service_worker/service_worker_version.cc:176: -1,
> ServiceWorkerMsg_FetchEvent(request)) == SERVICE_WORKER_OK;
> Sorry for the late drive-by, but -1 is a magic number?
> 
>
https://codereview.chromium.org/139923005/diff/70001/content/renderer/service...
> File content/renderer/service_worker/service_worker_script_context.cc (right):
> 
>
https://codereview.chromium.org/139923005/diff/70001/content/renderer/service...
> content/renderer/service_worker/service_worker_script_context.cc:20:
> current_request_id_(-1) {
> Ditto, -1 as magic number. Can you use an enum/constant?

Yup, I'll make a small follow-up patch and land it for this..

[commit-bot: I haz the power, 2014-02-05 01:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/139923005/70001

[kinuko, 2014-02-05 06:57:42]

Committed patchset #3 manually as r248889 (presubmit successful).