Adding slightly clearer separation between SW and EmbeddedWorker (still incomplete)

Adding slightly clearer separation between SW and EmbeddedWorker (still incomplete)

I'm still a bit unsure about bringing separation here is a good thing,
but this patch adds slightly clearer separation between them.

* EmbeddedWorkerInstance now has generic SendMessage() and
  OnMessageReceived() observer method (instead of SendFetchRequest etc)
* ServiceWorker's script context messages are sent and dispatched from/to
  ServiceWorkerVersion to/from ServiceWorkerScriptContext via EW layer.
* EW/SW separation in blink and dispatcher_host is not made yet (and no plan to add it for now)

BUG=313530
TBR=avi

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

[kinuko, 2014-01-16 15:12:32]

This patch removes some SW-specific part from EW code. This patch adds a bit
more layering (but still partial) so that EmbeddedWorkerInstance won't handle
SW-specific messages.

I do have a concern to add more complexity while having no concrete necessity,
so please say no to this patch if this looks adding more complexity than
necessary to you.

I also want to leave remaining hazy part in the layering as is for now (i.e. no
plan to add embedded_worker directory or its own dispatcher_host), I don't feel
we're ready to add it yet.

PTL

[alecflett, 2014-01-16 17:50:08]

I really like this. I think partly-hazy is fine, and this nicely identifies
whats hazy and what is not.

On 2014/01/16 15:12:32, kinuko wrote:

> I do have a concern to add more complexity while having no concrete necessity,
> so please say no to this patch if this looks adding more complexity than
> necessary to you.

This feels like just the right amount, personally - if anything it clarifies the
role of the WebServiceWorkerClientProxy to me. 

> 
> I also want to leave remaining hazy part in the layering as is for now (i.e.
no
> plan to add embedded_worker directory or its own dispatcher_host), I don't
feel
> we're ready to add it yet.
> 

Yeah, I didn't want to push the issue so far that we're adding too many
abstractions/distractions.. But what I like about this is that it allows
layering "violations" (if we're going to consider the SW/EW boundary a layer,
which as you say is hazy.. but that's fine) but it consolidates future
"violations" (i.e. SW-specific messages) to the connection between
EmbeddedWorkerContextClient and ServiceWorkerScriptContext - we can change
SWScriptContext all we want without worrying about making the layering more
hazy.

> PTL

LGTM!

https://codereview.chromium.org/140893002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/140893002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_browsertest.cc:115: virtual void
OnMessageReceived(const IPC::Message& message) OVERRIDE {}
Maybe add a comment indicating that it's safe to ignore any messages, since this
is about startup/shutdown?

(Thought I think personally I'd actually put a NOTREACHED() in here so when in
the future we fire stuff like OnInstallComplete, we can decide if it's important
to check for those specific messages after startup or before shutdown)

[kinuko, 2014-01-17 03:18:54]

Ok, then I'll try implementing install flow in this direction too.

https://codereview.chromium.org/140893002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/140893002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_browsertest.cc:115: virtual void
OnMessageReceived(const IPC::Message& message) OVERRIDE {}
On 2014/01/16 17:50:08, alecflett wrote:
> Maybe add a comment indicating that it's safe to ignore any messages, since
this
> is about startup/shutdown?
> 
> (Thought I think personally I'd actually put a NOTREACHED() in here so when in
> the future we fire stuff like OnInstallComplete, we can decide if it's
important
> to check for those specific messages after startup or before shutdown)
> 

Done. Added NOTREACHED()

[kinuko, 2014-01-17 03:29:13]

tsepez@: could you review changes in content/common/service_worker/*_messages.h?

avi@: could you review content/content_renderer.gypi? (Might just land with TBR)

[Tom Sepez, 2014-01-17 18:13:46]

I didn't see any checks on the kinds of messages that can be tunneled inside
EmbeddedWorkerHostMsg_SendMessageToBrowser. It seems like we shouldn't be
proxying arbitrary messages, just the sets of ones we expect to avoid having the
worker make its host send an unintended browser message.

[alecflett, 2014-01-17 18:42:09]

On 2014/01/17 18:13:46, Tom Sepez wrote:
> I didn't see any checks on the kinds of messages that can be tunneled inside
> EmbeddedWorkerHostMsg_SendMessageToBrowser. It seems like we shouldn't be
> proxying arbitrary messages, just the sets of ones we expect to avoid having
the
> worker make its host send an unintended browser message.

Tom: Is it ok to send IPC::Messages inside other messages as long as we check
them on both sides, or is sending arbitrary IPC::Messages this way too risky?

[Tom Sepez, 2014-01-17 18:50:47]

The question is whether we think there is a security boundary between the worker
and the renderer that it is proxying for it. In other words, if the worker is
compromised, do we want to allow that to spread to the renderer? Given that
there's some attack surface present in the worker that may not be in the
ordinary renderer, it seems like this is a nice-to-have property, especially
since the set of messages we can proxy is so much less than the set of messages
the renderer can send to a browser.

If it's not too much trouble, you could include a switch(msg.type()) to specify
the expected set. If this is infeasible, let me know.

[alecflett, 2014-01-17 22:02:59]

On 2014/01/17 18:50:47, Tom Sepez wrote:
> The question is whether we think there is a security boundary between the
worker
> and the renderer that it is proxying for it. In other words, if the worker is
> compromised, do we want to allow that to spread to the renderer? Given that
> there's some attack surface present in the worker that may not be in the
> ordinary renderer, it seems like this is a nice-to-have property, especially
> since the set of messages we can proxy is so much less than the set of
messages
> the renderer can send to a browser.
> 
> If it's not too much trouble, you could include a switch(msg.type()) to
specify
> the expected set. If this is infeasible, let me know.

Yes, it's quite feasible to switch/filter on the message type.

Though now that I think about it, that's exactly what ServiceWorkerScriptContext
is doing, in dispatching events:

https://codereview.chromium.org/140893002/patch/160001/170015

(see the OnMessageReceived() method)

[kinuko, 2014-01-20 03:48:07]

On 2014/01/17 22:02:59, alecflett wrote:
> On 2014/01/17 18:50:47, Tom Sepez wrote:
> > The question is whether we think there is a security boundary between the
> worker
> > and the renderer that it is proxying for it. In other words, if the worker
is
> > compromised, do we want to allow that to spread to the renderer? Given that
> > there's some attack surface present in the worker that may not be in the
> > ordinary renderer, it seems like this is a nice-to-have property, especially
> > since the set of messages we can proxy is so much less than the set of
> messages
> > the renderer can send to a browser.
> > 
> > If it's not too much trouble, you could include a switch(msg.type()) to
> specify
> > the expected set. If this is infeasible, let me know.
> 
> Yes, it's quite feasible to switch/filter on the message type.
> 
> Though now that I think about it, that's exactly what
ServiceWorkerScriptContext
> is doing, in dispatching events:
> 
> https://codereview.chromium.org/140893002/patch/160001/170015
> 
> (see the OnMessageReceived() method)

Yes, we'll have switch/filter on the message type on both renderer and browser
side, so that we won't have additional security issue in this architecture.

On the renderer side the part Alec pointed (OnMessageReceived() in
service_worker_script_context.cc) would do that, and on the browser side we'll
have a similar filtering code in the embedded_worker_registry.cc.  We don't
define particular message types for renderer->browser msg types that go through
this proxying yet, but I've added a TODO there: 
https://codereview.chromium.org/140893002/diff2/160001:330001/content/browser...

Given that we'll filter messages in the same way as we do in other parts, does
this patch look good/ok to you?

[kinuko, 2014-01-21 01:45:40]

tsepez@: friendly ping, could you give your thoughts?

> Given that we'll filter messages on both ends in the same way as we do in
other parts, does
> this patch look good/ok to you?

[Tom Sepez, 2014-01-21 20:38:53]

On 2014/01/21 01:45:40, kinuko wrote:
> tsepez@: friendly ping, could you give your thoughts?
> 
> > Given that we'll filter messages on both ends in the same way as we do in
> other parts, does
> > this patch look good/ok to you?

LGTM, provided you priomise to fix the TODO() before any of this hits
beta/stable.

[kinuko, 2014-01-22 01:36:24]

On 2014/01/21 20:38:53, Tom Sepez wrote:
> On 2014/01/21 01:45:40, kinuko wrote:
> > tsepez@: friendly ping, could you give your thoughts?
> > 
> > > Given that we'll filter messages on both ends in the same way as we do in
> > other parts, does
> > > this patch look good/ok to you?
> 
> LGTM, provided you priomise to fix the TODO() before any of this hits
> beta/stable.

Thanks! Yes I will.

[commit-bot: I haz the power, 2014-01-22 01:38:52]

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

[commit-bot: I haz the power, 2014-01-22 04:21:26]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-22 06:35:49]

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

[commit-bot: I haz the power, 2014-01-22 10:34:47]

Change committed as 246276