Implement the ServiceWorker "activate" event during activation

This dispatches the "activate" event to service workers after they're installed, and will indicate that the service worker failed if it rejects a promise.

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

[dominicc (has gone to gerrit), 2014-03-27 09:24:01]

WIP but if anyone has cycles for a sanity check, much appreciated.

This dispatches the "activate" event on the service worker and little else. The
Blink side patch is <https://codereview.chromium.org/212593010>. I was going to
see it work end-to-end before I worried about breaking it into pieces.

[dominicc (has gone to gerrit), 2014-03-27 09:25:10]

Oops, I mean the Blink side is <https://codereview.chromium.org/214413004>.

[dominicc (has gone to gerrit), 2014-03-28 10:34:08]

PTAL

This depends on the Blink side 216603003 and 214413004 going first.

[dominicc (has gone to gerrit), 2014-03-28 10:35:54]

On 2014/03/28 10:34:08, dominicc wrote:
> PTAL
> 
> This depends on the Blink side 216603003 and 214413004 going first.

Oops, there's this thing I've seen you Chromium people do, lemme try:

jschuh: content/common/service_worker/service_worker_messages.h
michaeln: everything else

kthxbye

[jschuh, 2014-03-28 21:27:29]

My questions are more with the surrounding code than the additions. Could you
provide a bit more context?

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

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:72: void
EmbeddedWorkerRegistry::OnSendMessageToBrowser(
This seems badly named. Where exactly are we running at this point?

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:81: if
(message.type() == ServiceWorkerHostMsg_ActivateEventFinished::ID ||
Why are we filtering here? Shouldn't found->second->OnMessageReceived() have an
explicit message map?

[dominicc (has gone to gerrit), 2014-03-31 03:59:33]

Thanks jschuh, comments inline.

Hmm, no response from michaeln; kinuko, could you take a look?

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

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:72: void
EmbeddedWorkerRegistry::OnSendMessageToBrowser(
On 2014/03/28 21:27:29, Justin Schuh wrote:
> This seems badly named. Where exactly are we running at this point?

This method is named OnSendMessageToBrowser because it's the receiving side of a
method on the ServiceWorker side called SendMessageToBrowser. I agree it is
mildly confusing since it turns around and sends a message (but not to browser)
therefore one could misinterpret this as "OnBeforeSend..." But it is not that.

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:81: if
(message.type() == ServiceWorkerHostMsg_ActivateEventFinished::ID ||
On 2014/03/28 21:27:29, Justin Schuh wrote:
> Why are we filtering here? Shouldn't found->second->OnMessageReceived() have
an
> explicit message map?

This class acts as a proxy routing messages to a worker; the point of checking
here is to limit the kinds of messages sent via this  payload in the event the
renderer is compromised. The kinds of methods that should go via this route may
be fewer than all the kinds of messages the worker responds to.

This is based on advice from Tom Sepez here:
<https://codereview.chromium.org/140893002#msg7>.

[kinuko, 2014-03-31 04:31:02]

lgtm + one style nit

(As for method naming I don't expect we fix it in this CL)

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

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:72: void
EmbeddedWorkerRegistry::OnSendMessageToBrowser(
On 2014/03/31 03:59:33, dominicc wrote:
> On 2014/03/28 21:27:29, Justin Schuh wrote:
> > This seems badly named. Where exactly are we running at this point?
> 
> This method is named OnSendMessageToBrowser because it's the receiving side of
a
> method on the ServiceWorker side called SendMessageToBrowser. I agree it is
> mildly confusing since it turns around and sends a message (but not to
browser)
> therefore one could misinterpret this as "OnBeforeSend..." But it is not that.

I agree it could be confusing, but so far we don't seem to have a better
consensus for method naming for receiving posted / sent messages other than this
style (e.g. I could find some precedence of 'OnPostMessage' etc for receiving
posted messages)

Maybe just removing 'Send' works in our case?

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

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:248:
ServiceWorkerStatusCode expected_status=SERVICE_WORKER_OK) {
nit: we don't use default args in chromium (unlink in blink)

[jschuh, 2014-03-31 13:36:15]

lgtm for ipc security, but lets see if we can come up with something to make it
clearer for any future security reviewer.

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

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:72: void
EmbeddedWorkerRegistry::OnSendMessageToBrowser(
On 2014/03/31 04:31:03, kinuko wrote:
> On 2014/03/31 03:59:33, dominicc wrote:
> > On 2014/03/28 21:27:29, Justin Schuh wrote:
> > > This seems badly named. Where exactly are we running at this point?
> > 
> > This method is named OnSendMessageToBrowser because it's the receiving side
of
> a
> > method on the ServiceWorker side called SendMessageToBrowser. I agree it is
> > mildly confusing since it turns around and sends a message (but not to
> browser)
> > therefore one could misinterpret this as "OnBeforeSend..." But it is not
that.
> 
> I agree it could be confusing, but so far we don't seem to have a better
> consensus for method naming for receiving posted / sent messages other than
this
> style (e.g. I could find some precedence of 'OnPostMessage' etc for receiving
> posted messages)
> 
> Maybe just removing 'Send' works in our case?

Yeah, I'm not worried now that I understand what this does. But it's very
confusing to have a message handler with this name under content/browser but
actually running in a sandboxed process. Maybe the name is fine, but we need
some sort of comment to explain what's going on here

https://codereview.chromium.org/212593010/diff/20001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:81: if
(message.type() == ServiceWorkerHostMsg_ActivateEventFinished::ID ||
On 2014/03/31 03:59:33, dominicc wrote:
> On 2014/03/28 21:27:29, Justin Schuh wrote:
> > Why are we filtering here? Shouldn't found->second->OnMessageReceived() have
> an
> > explicit message map?
> 
> This class acts as a proxy routing messages to a worker; the point of checking
> here is to limit the kinds of messages sent via this  payload in the event the
> renderer is compromised. The kinds of methods that should go via this route
may
> be fewer than all the kinds of messages the worker responds to.
> 
> This is based on advice from Tom Sepez here:
> <https://codereview.chromium.org/140893002#msg7>.

Okay, I guess @tsepez and I disagree on the value f this, but it certainly
doesn't hurt anything.

[dominicc (has gone to gerrit), 2014-04-01 04:17:52]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2014-04-01 04:18:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominicc@chromium.org/212593010/80001

[commit-bot: I haz the power, 2014-04-01 09:39:29]

Change committed as 260820