Add a mojo ServiceRegistry to embedded workers.

Add a mojo ServiceRegistry to embedded workers.

This sets up a pair of linked ServiceRegistry instances between the
EmbeddedWorkerInstance in the browser and ServiceWorkerContextClient
in the renderer. These ServiceRegistries can be used to use mojo to
dispatch events to a serviceworker, avoiding any need for
thread-hopping.

This is used in https://codereview.chromium.org/1210643002 to implement
the new navigator.services.onconnect event and in
https://codereview.chromium.org/1220943003 for the background sync event.

BUG=426458
Committed: https://crrev.com/27c9d745b80924002ac4c053fe77f6d9be0f1089
Cr-Commit-Position: refs/heads/master@{#339082}

[Marijn Kruisselbrink, 2015-07-07 15:09:03]

mek@chromium.org changed reviewers:
+ michaeln@chromium.org, rockot@chromium.org

[Marijn Kruisselbrink, 2015-07-07 15:09:03]

+rockot: does this look reasonable from a mojo point of view (or is there
somebody else I should be asking)
+michaeln as service worker owner

[Ken Rockot(use gerrit already), 2015-07-07 16:59:57]

Yes this looks great. It's kind of unfortunate that we keep duplicating the
"ExchangeServiceProviders" setup pattern, but the alternative is probably not
worth pursuing yet. In this case it would mean making service worker a proper
Mojo application, and that's a large problem whose solution has little practical
benefit.

So anyway, LGTM

[michaeln, 2015-07-07 22:15:09]

https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:98:
setup->ExchangeServiceProviders(thread_id, services.Pass(),
I'm not familiar enough to know what happens at runtime but am curious.

How expensive is a connection to a remote service, what happens under the covers
here to establish the connection? Are these super liteweight? Is there a round
trip handshake that has to happen prior to remote service calls being sent?

And then once setup, when code on the IO thread invokes a method of a remote
service, what happens with regard to thread context switching? I'm wondering if
tasks get bounced to the UI thread first, then back to the IO thread for real
sending since the RPH's mojo registry service is tied to the UI thread? I have
the same questions about what happens on the remote side under the convers.

[Ken Rockot(use gerrit already), 2015-07-07 22:36:52]

On 2015/07/07 22:15:09, michaeln wrote:
>
https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
> File content/browser/service_worker/embedded_worker_instance.cc (right):
> 
>
https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
> content/browser/service_worker/embedded_worker_instance.cc:98:
> setup->ExchangeServiceProviders(thread_id, services.Pass(),
> I'm not familiar enough to know what happens at runtime but am curious.
> 
> How expensive is a connection to a remote service, what happens under the
covers
> here to establish the connection? Are these super liteweight? Is there a round
> trip handshake that has to happen prior to remote service calls being sent?

They're super lightweight. A connection is a Mojo message pipe, which is just a
pair of entangled endpoint handles (integers). An endpoint handle is used for
message routing and can be passed across processes. Mojo EDK in either process
coordinates message routing using these entangled handles.

There's no round-trip handshake implied.

> 
> And then once setup, when code on the IO thread invokes a method of a remote
> service, what happens with regard to thread context switching? I'm wondering
if
> tasks get bounced to the UI thread first, then back to the IO thread for real
> sending since the RPH's mojo registry service is tied to the UI thread? I have
> the same questions about what happens on the remote side under the convers.

I'm not sure what you're asking. The RPH's service registry does live on the UI
thread, but it's only used (via the ServiceProvider::ConnectToService interface)
to get connections to other services. Once you're connected to some service, the
service registry is no longer involved: you have a pipe handle on your end which
speaks the service's language, and the other end is connected directly to some
bound service implementation on the other end (which itself may live on any
thread).

So any given message will hop at most twice: once (from the calling thread to
the IO thread) in the sender process, and once (from the IO thread to the
binding thread) in the receiver process.

[michaeln, 2015-07-08 01:03:53]

message: Good answers :)

Establishing a service registry pair per running instance makes sense to me.
We'll have to be careful about how we use it since the core ServiceWorker
abstractions like Version and EmbeddedWorkerInstance outlive running instances
(i think the pattern being used in the downstream patches get tripped up on
that, easily error prone). I see that there are service registry pairs per
process (MojoApplication) and per RenderFrame as well, both of which funtion on
UI threads. It'd be nice to be able to setup the registry for the embedded
worker w/o touching UI threads in either processes. Would a long lived service
registry pair per process that functioned on the IO threads make that possible?
If these things are as cheap as advertised, we should feel free to set peer
pairs up liberally.

Deep down, all of the messaging happens on the same channel for all services
registered directly or indirectly with the MojoApplication/Host pair, is that
right?

>
https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
> > File content/browser/service_worker/embedded_worker_instance.cc (right):
> > 
> >
>
https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
> > content/browser/service_worker/embedded_worker_instance.cc:98:
> > setup->ExchangeServiceProviders(thread_id, services.Pass(),
> > I'm not familiar enough to know what happens at runtime but am curious.
> > 
> > How expensive is a connection to a remote service, what happens under the
> covers
> > here to establish the connection? Are these super liteweight? Is there a
round
> > trip handshake that has to happen prior to remote service calls being sent?
> 
> They're super lightweight. A connection is a Mojo message pipe, which is just
a
> pair of entangled endpoint handles (integers). An endpoint handle is used for
> message routing and can be passed across processes. Mojo EDK in either process
> coordinates message routing using these entangled handles.
> 
> There's no round-trip handshake implied.
> 
> > 
> > And then once setup, when code on the IO thread invokes a method of a remote
> > service, what happens with regard to thread context switching? I'm wondering
> if
> > tasks get bounced to the UI thread first, then back to the IO thread for
real
> > sending since the RPH's mojo registry service is tied to the UI thread? I
have
> > the same questions about what happens on the remote side under the convers.
> 
> I'm not sure what you're asking. The RPH's service registry does live on the
UI
> thread, but it's only used (via the ServiceProvider::ConnectToService
interface)
> to get connections to other services. Once you're connected to some service,
the
> service registry is no longer involved: you have a pipe handle on your end
which
> speaks the service's language, and the other end is connected directly to some
> bound service implementation on the other end (which itself may live on any
> thread).
> 
> So any given message will hop at most twice: once (from the calling thread to
> the IO thread) in the sender process, and once (from the IO thread to the
> binding thread) in the receiver process.

[kinuko, 2015-07-08 01:41:29]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[Ken Rockot(use gerrit already), 2015-07-08 01:52:36]

On 2015/07/08 01:03:53, michaeln wrote:
> message: Good answers :)
> 
> Establishing a service registry pair per running instance makes sense to me.
> We'll have to be careful about how we use it since the core ServiceWorker
> abstractions like Version and EmbeddedWorkerInstance outlive running instances
> (i think the pattern being used in the downstream patches get tripped up on
> that, easily error prone). I see that there are service registry pairs per
> process (MojoApplication) and per RenderFrame as well, both of which funtion
on
> UI threads. It'd be nice to be able to setup the registry for the embedded
> worker w/o touching UI threads in either processes. Would a long lived service
> registry pair per process that functioned on the IO threads make that
possible?

Really the whole business of adding service registry pairs, and in fact the
existence of ServiceRegistry itself, is an interim solution anyway. In the limit
all service providers and consumers should belong to one Mojo app or another,
and the Shell interface (which every app gets a pipe to) already provides a
unified way for any two apps to exchange services with each other.

Any new work we do outside of that framework should at least aim toward
emulating it and preparing for the future state of things. In that spirit, the
RenderFrame's registry exposes a Shell interface to the frame which behaves as
if the frame itself were a mojo app (i.e. the frame can call
Shell::ConnectToApplication to get services from a Mojo app who will see the
requestor identified by the frame's web origin). Ideally some day soon, any
ServiceRegistry in existence will only be used to expose a Shell interface for
similar emulation, at which point ServiceRegistry can be replaced with something
simpler.

I think we should probably (soon) scrap the process-wide registry (i.e.
MojoApplication/Host) altogether since - at least before process-per-site is
done - processes alone don't provide any meaningful identity for service
exposure logic to use. That is, if RPHI were to do the same thing as RFHI
by servicing Shell interface requests, it wouldn't have any meaningful (albeit
fake) app identity to use in outgoing requests.

It then occurs to me that I'd probably like workers to end up in the same place
as frames, but looking into it now it seems they have the same problem as
processes themselves: they don't appear to have any authoritative web origin
associated with them in the browser. Hmm.

I don't think I answered your question as such, but hopefully this background is
helpful.

> If these things are as cheap as advertised, we should feel free to set peer
> pairs up liberally.
> 
> Deep down, all of the messaging happens on the same channel for all services
> registered directly or indirectly with the MojoApplication/Host pair, is that
> right?
> 

Correct. Any given process pair has a single IPC channel between them to carry
all Mojo messages between those processes.

> >
>
https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
> > > File content/browser/service_worker/embedded_worker_instance.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1221503003/diff/60001/content/browser/service...
> > > content/browser/service_worker/embedded_worker_instance.cc:98:
> > > setup->ExchangeServiceProviders(thread_id, services.Pass(),
> > > I'm not familiar enough to know what happens at runtime but am curious.
> > > 
> > > How expensive is a connection to a remote service, what happens under the
> > covers
> > > here to establish the connection? Are these super liteweight? Is there a
> round
> > > trip handshake that has to happen prior to remote service calls being
sent?
> > 
> > They're super lightweight. A connection is a Mojo message pipe, which is
just
> a
> > pair of entangled endpoint handles (integers). An endpoint handle is used
for
> > message routing and can be passed across processes. Mojo EDK in either
process
> > coordinates message routing using these entangled handles.
> > 
> > There's no round-trip handshake implied.
> > 
> > > 
> > > And then once setup, when code on the IO thread invokes a method of a
remote
> > > service, what happens with regard to thread context switching? I'm
wondering
> > if
> > > tasks get bounced to the UI thread first, then back to the IO thread for
> real
> > > sending since the RPH's mojo registry service is tied to the UI thread? I
> have
> > > the same questions about what happens on the remote side under the
convers.
> > 
> > I'm not sure what you're asking. The RPH's service registry does live on the
> UI
> > thread, but it's only used (via the ServiceProvider::ConnectToService
> interface)
> > to get connections to other services. Once you're connected to some service,
> the
> > service registry is no longer involved: you have a pipe handle on your end
> which
> > speaks the service's language, and the other end is connected directly to
some
> > bound service implementation on the other end (which itself may live on any
> > thread).
> > 
> > So any given message will hop at most twice: once (from the calling thread
to
> > the IO thread) in the sender process, and once (from the IO thread to the
> > binding thread) in the receiver process.

[kinuko, 2015-07-08 08:32:41]

(Drive-by, this one just caught my attention)

On 2015/07/08 01:52:36, Ken Rockot wrote:
> On 2015/07/08 01:03:53, michaeln wrote:
> > message: Good answers :)
> > 
> > Establishing a service registry pair per running instance makes sense to me.
> > We'll have to be careful about how we use it since the core ServiceWorker
> > abstractions like Version and EmbeddedWorkerInstance outlive running
instances
> > (i think the pattern being used in the downstream patches get tripped up on
> > that, easily error prone).

I found this design tends to introduce confusion, I think we could possibly
change this part if it helps us in future.
 
> Really the whole business of adding service registry pairs, and in fact the
> existence of ServiceRegistry itself, is an interim solution anyway. In the
limit
> all service providers and consumers should belong to one Mojo app or another,
> and the Shell interface (which every app gets a pipe to) already provides a
> unified way for any two apps to exchange services with each other.
> 
> Any new work we do outside of that framework should at least aim toward
> emulating it and preparing for the future state of things. In that spirit, the
> RenderFrame's registry exposes a Shell interface to the frame which behaves as
> if the frame itself were a mojo app (i.e. the frame can call
> Shell::ConnectToApplication to get services from a Mojo app who will see the
> requestor identified by the frame's web origin). Ideally some day soon, any
> ServiceRegistry in existence will only be used to expose a Shell interface for
> similar emulation, at which point ServiceRegistry can be replaced with
something
> simpler.
> 
> I think we should probably (soon) scrap the process-wide registry (i.e.
> MojoApplication/Host) altogether since - at least before process-per-site is
> done - processes alone don't provide any meaningful identity for service
> exposure logic to use. That is, if RPHI were to do the same thing as RFHI
> by servicing Shell interface requests, it wouldn't have any meaningful (albeit
> fake) app identity to use in outgoing requests.

I'm not familiar with the current mojo architecture so I'm not sure if I
understand what you meant here--
Do you mean we should basically consider setting up an 'application' unit per
worker (or per some meaningful unit in each module) rather than assuming that we
have single per-process mojo setup? And because of that we will probably need
the UI/main-thread hopping at most once in each browser/renderer process for the
mojo setup (as is currently coded)?

> It then occurs to me that I'd probably like workers to end up in the same
place
> as frames, but looking into it now it seems they have the same problem as
> processes themselves: they don't appear to have any authoritative web origin
> associated with them in the browser. Hmm.

Could you elaborate a bit more on what you mean by 'authoritative web origin'?

https://codereview.chromium.org/1221503003/diff/60001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1221503003/diff/60001/content/renderer/render...
content/renderer/render_thread_impl.cc:387: DCHECK(client);
It feels it's possible to end up with null client here if the worker gets killed
while all these thread hopping is happening on both processes?

[Marijn Kruisselbrink, 2015-07-08 16:46:17]

On 2015/07/08 at 08:32:41, kinuko wrote:
> (Drive-by, this one just caught my attention)
> 
> On 2015/07/08 01:52:36, Ken Rockot wrote:
> > On 2015/07/08 01:03:53, michaeln wrote:
> > > message: Good answers :)
> > > 
> > > Establishing a service registry pair per running instance makes sense to
me.
> > > We'll have to be careful about how we use it since the core ServiceWorker
> > > abstractions like Version and EmbeddedWorkerInstance outlive running
instances
> > > (i think the pattern being used in the downstream patches get tripped up
on
> > > that, easily error prone).
> 
> I found this design tends to introduce confusion, I think we could possibly
change this part if it helps us in future.

One problem here that makes it a bit harder to deal with this cleanly is that
there doesn't really seem to be any generic way to store a list of connections
to services in a worker such that all those can be closed automatically when the
worker is stopped. Ideally ServiceWorkerVersion or EmbeddedWorkerInstance would
somehow be able to keep track of all the InterfacePtr<> instances it has
connected to, and then close all those when the worker is stopped, but without a
common base class for InterfacePtr<Foo> and InterfacePtr<Bar> doing that becomes
really hard. But then ServiceWorkerVersion already has many of these cases where
all the various request types, callbacks etc are all explicitly enumerated
multiple times, so having one more thing like that might not be too bad (but
certainly unfortunate).
  
>  
> > Really the whole business of adding service registry pairs, and in fact the
> > existence of ServiceRegistry itself, is an interim solution anyway. In the
limit
> > all service providers and consumers should belong to one Mojo app or
another,
> > and the Shell interface (which every app gets a pipe to) already provides a
> > unified way for any two apps to exchange services with each other.
> > 
> > Any new work we do outside of that framework should at least aim toward
> > emulating it and preparing for the future state of things. In that spirit,
the
> > RenderFrame's registry exposes a Shell interface to the frame which behaves
as
> > if the frame itself were a mojo app (i.e. the frame can call
> > Shell::ConnectToApplication to get services from a Mojo app who will see the
> > requestor identified by the frame's web origin). Ideally some day soon, any
> > ServiceRegistry in existence will only be used to expose a Shell interface
for
> > similar emulation, at which point ServiceRegistry can be replaced with
something
> > simpler.
> > 
> > I think we should probably (soon) scrap the process-wide registry (i.e.
> > MojoApplication/Host) altogether since - at least before process-per-site is
> > done - processes alone don't provide any meaningful identity for service
> > exposure logic to use. That is, if RPHI were to do the same thing as RFHI
> > by servicing Shell interface requests, it wouldn't have any meaningful
(albeit
> > fake) app identity to use in outgoing requests.
> 
> I'm not familiar with the current mojo architecture so I'm not sure if I
understand what you meant here--
> Do you mean we should basically consider setting up an 'application' unit per
worker (or per some meaningful unit in each module) rather than assuming that we
have single per-process mojo setup? And because of that we will probably need
the UI/main-thread hopping at most once in each browser/renderer process for the
mojo setup (as is currently coded)?

I'm not entirely sure what Ken is proposing here either, but one relatively easy
thing that I think would be a good step towards mojofication of embedded workers
would be to replace all the EmbeddedWorker(Host) messages with a mojo service;
to start a new embedded worker you'd connect to a new EmbeddedWorker service in
whichever process you want to connect to, StopWorker would just be closing that
connection, and the other browser->renderer messages would be calls on that
interface. To replace EmbeddedWorkerContextMsg_MessageToWorker (without
replacing all browser->worker IPC with mojo) there could be a mojo message pipe
backing an IPC::Channel for these messages. It would still be nice if somehow
closing one mojo message pipe (in this case the one backing the connection to
the EmbeddedWorker instance) could automatically close every other channel that
was set up to some service within that worker as well, but I don't think mojo
currently really has the concept of dependent channels?

> 
> > It then occurs to me that I'd probably like workers to end up in the same
place
> > as frames, but looking into it now it seems they have the same problem as
> > processes themselves: they don't appear to have any authoritative web origin
> > associated with them in the browser. Hmm.
> 
> Could you elaborate a bit more on what you mean by 'authoritative web origin'?
> 
>
https://codereview.chromium.org/1221503003/diff/60001/content/renderer/render...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/1221503003/diff/60001/content/renderer/render...
> content/renderer/render_thread_impl.cc:387: DCHECK(client);
> It feels it's possible to end up with null client here if the worker gets
killed while all these thread hopping is happening on both processes?

Yes, I think you're right. Instead of DCHECK-ing, this should probably just
close the connection if the thread doesn't have a worker anymore by the time
this point is reached.

[Ken Rockot(use gerrit already), 2015-07-08 17:39:26]

On 2015/07/08 16:46:17, Marijn Kruisselbrink wrote:
> On 2015/07/08 at 08:32:41, kinuko wrote:
> > (Drive-by, this one just caught my attention)
> > 
> > On 2015/07/08 01:52:36, Ken Rockot wrote:
> > > On 2015/07/08 01:03:53, michaeln wrote:
> > > > message: Good answers :)
> > > > 
> > > > Establishing a service registry pair per running instance makes sense to
> me.
> > > > We'll have to be careful about how we use it since the core
ServiceWorker
> > > > abstractions like Version and EmbeddedWorkerInstance outlive running
> instances
> > > > (i think the pattern being used in the downstream patches get tripped up
> on
> > > > that, easily error prone).
> > 
> > I found this design tends to introduce confusion, I think we could possibly
> change this part if it helps us in future.
> 
> One problem here that makes it a bit harder to deal with this cleanly is that
> there doesn't really seem to be any generic way to store a list of connections
> to services in a worker such that all those can be closed automatically when
the
> worker is stopped. Ideally ServiceWorkerVersion or EmbeddedWorkerInstance
would
> somehow be able to keep track of all the InterfacePtr<> instances it has
> connected to, and then close all those when the worker is stopped, but without
a
> common base class for InterfacePtr<Foo> and InterfacePtr<Bar> doing that
becomes
> really hard. But then ServiceWorkerVersion already has many of these cases
where
> all the various request types, callbacks etc are all explicitly enumerated
> multiple times, so having one more thing like that might not be too bad (but
> certainly unfortunate).

I don't understand why this is desirable. If you really wanted, you could retain
weak handles to every InterfacePtr you hand off to someone and close them
manually whenever you want -- at the end of the day InterfacePtr<Foo> it's just
wrapping a MojoHandle (an uint32) which you can monitor efficiently with
MojoWait and close with MojoClose, but why would you do that? What's wrong with
letting the InterfacePtr's owner control pipe lifetime, and if you need to force
close a client's pipe, add some way to notify each client.

>   
> >  
> > > Really the whole business of adding service registry pairs, and in fact
the
> > > existence of ServiceRegistry itself, is an interim solution anyway. In the
> limit
> > > all service providers and consumers should belong to one Mojo app or
> another,
> > > and the Shell interface (which every app gets a pipe to) already provides
a
> > > unified way for any two apps to exchange services with each other.
> > > 
> > > Any new work we do outside of that framework should at least aim toward
> > > emulating it and preparing for the future state of things. In that spirit,
> the
> > > RenderFrame's registry exposes a Shell interface to the frame which
behaves
> as
> > > if the frame itself were a mojo app (i.e. the frame can call
> > > Shell::ConnectToApplication to get services from a Mojo app who will see
the
> > > requestor identified by the frame's web origin). Ideally some day soon,
any
> > > ServiceRegistry in existence will only be used to expose a Shell interface
> for
> > > similar emulation, at which point ServiceRegistry can be replaced with
> something
> > > simpler.
> > > 
> > > I think we should probably (soon) scrap the process-wide registry (i.e.
> > > MojoApplication/Host) altogether since - at least before process-per-site
is
> > > done - processes alone don't provide any meaningful identity for service
> > > exposure logic to use. That is, if RPHI were to do the same thing as RFHI
> > > by servicing Shell interface requests, it wouldn't have any meaningful
> (albeit
> > > fake) app identity to use in outgoing requests.
> > 
> > I'm not familiar with the current mojo architecture so I'm not sure if I
> understand what you meant here--
> > Do you mean we should basically consider setting up an 'application' unit
per
> worker (or per some meaningful unit in each module) rather than assuming that
we
> have single per-process mojo setup? And because of that we will probably need
> the UI/main-thread hopping at most once in each browser/renderer process for
the
> mojo setup (as is currently coded)?
> 
> I'm not entirely sure what Ken is proposing here either, but one relatively
easy

I'm not entirely sure what I'm proposing either, mostly because I don't have a
lucid understanding of the meaning of "worker". What I can say for sure is that
we will continue to have exactly one Mojo IPC channel between the browser and
each child process, operating on each process's IO thread only; and that the
abstractions of "Mojo application" and "process" are not a 1:1 correspondence.
Any given process may be running N >= 1 Mojo applications, each on their own
thread.

We're eventually going to refactor anything in a child process to be part of one
or more Mojo applications, which consume services from other applications via
the Mojo shell interface (as opposed to using some one-off service registry).
Anything we build now should at least mimic that environment, so even if workers
aren't part of a real Mojo application now they should access browser services
via a Shell interface in a way that pretends to be a Mojo application. This is
what render frames do now. The issue I was raising is that unlike render frames,
it's unclear how a worker's virtual application would identify itself in a
meaningful way, assuming workers can access services with origin-based
restrictions (if not, the problem doesn't exist).

I don't know where the Mojo application boundaries should be drawn among
concepts like render process, render frame, worker instance, etc., but I think
we can wing it for now since the cost of rearranging the bits later seems
relatively low. So I guess I'm proposing that for now, yes, we treat each worker
instance as its own virtual application. I don't think this has to change any
details about what process or thread it lives on. [on that note, do workers do
IPC on their own thread rather than the child's IO thread? that could explain
some of my confusion here...]

In any case I only brought up all this stuff in response to questions about the
right way to arrange all our service registries, because I think the real answer
is that we should get rid of service registries ASAP, and that goal should carry
significant weight in any related design discussions.

> thing that I think would be a good step towards mojofication of embedded
workers
> would be to replace all the EmbeddedWorker(Host) messages with a mojo service;
> to start a new embedded worker you'd connect to a new EmbeddedWorker service
in
> whichever process you want to connect to, StopWorker would just be closing
that
> connection, and the other browser->renderer messages would be calls on that
> interface. To replace EmbeddedWorkerContextMsg_MessageToWorker (without
> replacing all browser->worker IPC with mojo) there could be a mojo message
pipe
> backing an IPC::Channel for these messages. It would still be nice if somehow

I agree that making EWH a Mojo service is a good idea. We do have an
implementation of IPC::Channel which uses a Mojo pipe underneath; unfortunately
we have it turned off in production because of performance issues. Why would
this be necessary to support MessageToWorker?

> closing one mojo message pipe (in this case the one backing the connection to
> the EmbeddedWorker instance) could automatically close every other channel
that
> was set up to some service within that worker as well, but I don't think mojo
> currently really has the concept of dependent channels?

I'm not sure I understand what benefit we'd get out of dependent pipes. Why
would it be better for mojo to provide this mechanism rather than letting
application code manage dependent lifetimes as needed?

> 
> > 
> > > It then occurs to me that I'd probably like workers to end up in the same
> place
> > > as frames, but looking into it now it seems they have the same problem as
> > > processes themselves: they don't appear to have any authoritative web
origin
> > > associated with them in the browser. Hmm.
> > 
> > Could you elaborate a bit more on what you mean by 'authoritative web
origin'?
> > 

Probably a poor choice of words. I just mean the URL of some web content as
known by the browser -- it's "authoritative" insofar as this URL can be trusted,
unlike any information that would come from the sandboxed content itself. RFHI
can say that requests from its corresponding render frame are associated with a
SiteInstance URL, and browser services can trust this information and make
security decisions accordingly. As far as I can tell, this isn't the case with
workers, but maybe it doesn't make sense to associate a worker with a single URL
or origin?

> >
>
https://codereview.chromium.org/1221503003/diff/60001/content/renderer/render...
> > File content/renderer/render_thread_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1221503003/diff/60001/content/renderer/render...
> > content/renderer/render_thread_impl.cc:387: DCHECK(client);
> > It feels it's possible to end up with null client here if the worker gets
> killed while all these thread hopping is happening on both processes?
> 
> Yes, I think you're right. Instead of DCHECK-ing, this should probably just
> close the connection if the thread doesn't have a worker anymore by the time
> this point is reached.

[michaeln, 2015-07-08 21:07:53]

The terms and distinctions between mojo application, shell, service, etc don't
mean much to me so I'm pretty sure I don't fully follow. Also, we've strayed
from the specific of this CL and probably should refocus on the here and now.
Should we proceed with this CL or scrap it and do something completely
different? If the answer to that is proceed...

How can we arrange for EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl to run on
the IPC thread in the renderer?

> I don't understand why this is desirable. If you really wanted, you could
retain
> weak handles to every InterfacePtr you hand off to someone and close them
> manually whenever you want -- at the end of the day InterfacePtr<Foo> it's
just
> wrapping a MojoHandle (an uint32) which you can monitor efficiently with
> MojoWait and close with MojoClose, but why would you do that? What's wrong
with
> letting the InterfacePtr's owner control pipe lifetime, and if you need to
force
> close a client's pipe, add some way to notify each client.

MojoWait is a blocking call so probably not very useful in chrome. Without
waiting, how can you be told when the process housing a remote service, you have
an intfptr to, has crashed.

> > > > Really the whole business of adding service registry pairs, and in fact
> the
> > > > existence of ServiceRegistry itself, is an interim solution anyway. In
the
> > limit
> > > > all service providers and consumers should belong to one Mojo app or
> > another,
> > > > and the Shell interface (which every app gets a pipe to) already
provides
> a
> > > > unified way for any two apps to exchange services with each other.
> > > > 
> > > > Any new work we do outside of that framework should at least aim toward
> > > > emulating it and preparing for the future state of things. In that
spirit,
> > the
> > > > RenderFrame's registry exposes a Shell interface to the frame which
> behaves
> > as
> > > > if the frame itself were a mojo app (i.e. the frame can call
> > > > Shell::ConnectToApplication to get services from a Mojo app who will see
> the
> > > > requestor identified by the frame's web origin). Ideally some day soon,
> any
> > > > ServiceRegistry in existence will only be used to expose a Shell
interface
> > for
> > > > similar emulation, at which point ServiceRegistry can be replaced with
> > something
> > > > simpler.
> > > > 
> > > > I think we should probably (soon) scrap the process-wide registry (i.e.
> > > > MojoApplication/Host) altogether since - at least before
process-per-site
> is
> > > > done - processes alone don't provide any meaningful identity for service
> > > > exposure logic to use. That is, if RPHI were to do the same thing as
RFHI
> > > > by servicing Shell interface requests, it wouldn't have any meaningful
> > (albeit
> > > > fake) app identity to use in outgoing requests.
> > > 
> > > I'm not familiar with the current mojo architecture so I'm not sure if I
> > understand what you meant here--
> > > Do you mean we should basically consider setting up an 'application' unit
> per
> > worker (or per some meaningful unit in each module) rather than assuming
that
> we
> > have single per-process mojo setup? And because of that we will probably
need
> > the UI/main-thread hopping at most once in each browser/renderer process for
> the
> > mojo setup (as is currently coded)?

Meta question: What benefit do we hope to gain by using mojo in the context of
chrome?

> > I'm not entirely sure what Ken is proposing here either, but one relatively
> easy
> 
> I'm not entirely sure what I'm proposing either, mostly because I don't have a
> lucid understanding of the meaning of "worker". What I can say for sure is
that
> we will continue to have exactly one Mojo IPC channel between the browser and
> each child process, operating on each process's IO thread only; and that the
> abstractions of "Mojo application" and "process" are not a 1:1 correspondence.
> Any given process may be running N >= 1 Mojo applications, each on their own
> thread.

exactly one Mojo IPC channel, good to know

> We're eventually going to refactor anything in a child process to be part of
one
> or more Mojo applications, which consume services from other applications via
> the Mojo shell interface (as opposed to using some one-off service registry).
> Anything we build now should at least mimic that environment, so even if
workers
> aren't part of a real Mojo application now they should access browser services
> via a Shell interface in a way that pretends to be a Mojo application. This is
> what render frames do now. The issue I was raising is that unlike render
frames,
> it's unclear how a worker's virtual application would identify itself in a
> meaningful way, assuming workers can access services with origin-based
> restrictions (if not, the problem doesn't exist).

Some of that sounds more like mandoline than chrome. The existing
content::serviceregistry seems like a very rough approximation of a shell for
now. Is that a fair view of it?

> I don't know where the Mojo application boundaries should be drawn among
> concepts like render process, render frame, worker instance, etc., but I think
> we can wing it for now since the cost of rearranging the bits later seems
> relatively low. So I guess I'm proposing that for now, yes, we treat each
worker
> instance as its own virtual application. I don't think this has to change any
> details about what process or thread it lives on. [on that note, do workers do
> IPC on their own thread rather than the child's IO thread? that could explain
> some of my confusion here...]

aside: Rearranging will probably be more expensive then you think.

Workers use the main browser-to-child IPC channel.

> In any case I only brought up all this stuff in response to questions about
the
> right way to arrange all our service registries, because I think the real
answer
> is that we should get rid of service registries ASAP, and that goal should
carry
> significant weight in any related design discussions.

I'm interested in how to eliminate the potential for races and deadlocks by
avoiding cross-thread-task posting as much as possible. Downstream CLs are
creating mojo<meaningfulunits> at a fairly fine grain (in some cases per event
type and per function call). What's the best way to setup those
mojo<meaningfulunits> without relying on the main thread making progress?

Specifically, in the context of this CL, how can we arrange for
EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl to run on the IPC thread in the
renderer?

> > thing that I think would be a good step towards mojofication of embedded
> workers
> > would be to replace all the EmbeddedWorker(Host) messages with a mojo
service;
> > to start a new embedded worker you'd connect to a new EmbeddedWorker service
> in
> > whichever process you want to connect to, StopWorker would just be closing
> that
> > connection, and the other browser->renderer messages would be calls on that
> > interface. To replace EmbeddedWorkerContextMsg_MessageToWorker (without
> > replacing all browser->worker IPC with mojo) there could be a mojo message
> pipe
> > backing an IPC::Channel for these messages. It would still be nice if
somehow
> 
> I agree that making EWH a Mojo service is a good idea. We do have an
> implementation of IPC::Channel which uses a Mojo pipe underneath;
unfortunately
> we have it turned off in production because of performance issues. Why would
> this be necessary to support MessageToWorker?
> 
> > closing one mojo message pipe (in this case the one backing the connection
to
> > the EmbeddedWorker instance) could automatically close every other channel
> that
> > was set up to some service within that worker as well, but I don't think
mojo
> > currently really has the concept of dependent channels?
> 
> I'm not sure I understand what benefit we'd get out of dependent pipes. Why
> would it be better for mojo to provide this mechanism rather than letting
> application code manage dependent lifetimes as needed?
> 
> > 
> > > 
> > > > It then occurs to me that I'd probably like workers to end up in the
same
> > place
> > > > as frames, but looking into it now it seems they have the same problem
as
> > > > processes themselves: they don't appear to have any authoritative web
> origin
> > > > associated with them in the browser. Hmm.
> > > 
> > > Could you elaborate a bit more on what you mean by 'authoritative web
> origin'?
> > > 
> 
> Probably a poor choice of words. I just mean the URL of some web content as
> known by the browser -- it's "authoritative" insofar as this URL can be
trusted,
> unlike any information that would come from the sandboxed content itself. RFHI
> can say that requests from its corresponding render frame are associated with
a
> SiteInstance URL, and browser services can trust this information and make
> security decisions accordingly. As far as I can tell, this isn't the case with
> workers, but maybe it doesn't make sense to associate a worker with a single
URL
> or origin?

Odd, the opposite seems more true to me? Frames navigate to different urls that
can be from different origins. Workers on the other hand never navigate and are
always associated with a single url. You can think of a Worker kind of like a
Document without a visual representation.

[Ken Rockot(use gerrit already), 2015-07-08 21:47:08]

On 2015/07/08 21:07:53, michaeln wrote:
> The terms and distinctions between mojo application, shell, service, etc don't
> mean much to me so I'm pretty sure I don't fully follow. Also, we've strayed
> from the specific of this CL and probably should refocus on the here and now.
> Should we proceed with this CL or scrap it and do something completely
> different? If the answer to that is proceed...

You're right, this has gotten off track (my fault). But given this discussion
I'm starting to think we should reevaluate the approach. It would be nice to
treat a worker context as a mojo application from the start, since we seem to
have that opportunity. I'm still trying to figure out what that looks like
exactly.

> 
> How can we arrange for EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl to run on
> the IPC thread in the renderer?

FWIW even if we went through with this, I don't understand how this part is
significant. Is it really so bad that this one-time worker initialization step
has to thread hop? Keep in mind that it's not necessary for the main thread to
make progress before other threads start requesting services and issuing calls
to them - everything will be queued and deferred by the EDK until the pipes are
actually ready.

> MojoWait is a blocking call so probably not very useful in chrome. Without
> waiting, how can you be told when the process housing a remote service, you
have
> an intfptr to, has crashed.

Right, it would need to be used on its own monitoring thread or something (a la
mojo::HandleWatcher), but I guess the point is just that we -could- implement a
monolithic pipe manager if we wanted to. I still don't think we want to.

> Meta question: What benefit do we hope to gain by using mojo in the context of
> chrome?

Ideally chrome can provide a close approximation of the mandoline shell
environment (content/browser is already hosting the same shell implementation as
mandoline, for example) so we can easily share components between the two. As
components are factored into mojo apps for mandoline use, they can be used as
such within chrome without needing a separate integration path, i.e. without
registering various services at various service registry boundaries.

> Some of that sounds more like mandoline than chrome. The existing
> content::serviceregistry seems like a very rough approximation of a shell for
> now. Is that a fair view of it?

Yes that's right, but it's an approximation I think we're prepared to phase out
for reasons stated above

> 
> > I don't know where the Mojo application boundaries should be drawn among
> > concepts like render process, render frame, worker instance, etc., but I
think
> > we can wing it for now since the cost of rearranging the bits later seems
> > relatively low. So I guess I'm proposing that for now, yes, we treat each
> worker
> > instance as its own virtual application. I don't think this has to change
any
> > details about what process or thread it lives on. [on that note, do workers
do
> > IPC on their own thread rather than the child's IO thread? that could
explain
> > some of my confusion here...]
> 
> aside: Rearranging will probably be more expensive then you think.

Heh. Yeah, you're probably right.

> 
> Workers use the main browser-to-child IPC channel.
> 
> > In any case I only brought up all this stuff in response to questions about
> the
> > right way to arrange all our service registries, because I think the real
> answer
> > is that we should get rid of service registries ASAP, and that goal should
> carry
> > significant weight in any related design discussions.
> 
> I'm interested in how to eliminate the potential for races and deadlocks by
> avoiding cross-thread-task posting as much as possible. Downstream CLs are
> creating mojo<meaningfulunits> at a fairly fine grain (in some cases per event
> type and per function call). What's the best way to setup those
> mojo<meaningfulunits> without relying on the main thread making progress?
> 
> Specifically, in the context of this CL, how can we arrange for
> EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl to run on the IPC thread in the
> renderer?
> 
> > > thing that I think would be a good step towards mojofication of embedded
> > workers
> > > would be to replace all the EmbeddedWorker(Host) messages with a mojo
> service;
> > > to start a new embedded worker you'd connect to a new EmbeddedWorker
service
> > in
> > > whichever process you want to connect to, StopWorker would just be closing
> > that
> > > connection, and the other browser->renderer messages would be calls on
that
> > > interface. To replace EmbeddedWorkerContextMsg_MessageToWorker (without
> > > replacing all browser->worker IPC with mojo) there could be a mojo message
> > pipe
> > > backing an IPC::Channel for these messages. It would still be nice if
> somehow
> > 
> > I agree that making EWH a Mojo service is a good idea. We do have an
> > implementation of IPC::Channel which uses a Mojo pipe underneath;
> unfortunately
> > we have it turned off in production because of performance issues. Why would
> > this be necessary to support MessageToWorker?
> > 
> > > closing one mojo message pipe (in this case the one backing the connection
> to
> > > the EmbeddedWorker instance) could automatically close every other channel
> > that
> > > was set up to some service within that worker as well, but I don't think
> mojo
> > > currently really has the concept of dependent channels?
> > 
> > I'm not sure I understand what benefit we'd get out of dependent pipes. Why
> > would it be better for mojo to provide this mechanism rather than letting
> > application code manage dependent lifetimes as needed?
> > 
> > > 
> > > > 
> > > > > It then occurs to me that I'd probably like workers to end up in the
> same
> > > place
> > > > > as frames, but looking into it now it seems they have the same problem
> as
> > > > > processes themselves: they don't appear to have any authoritative web
> > origin
> > > > > associated with them in the browser. Hmm.
> > > > 
> > > > Could you elaborate a bit more on what you mean by 'authoritative web
> > origin'?
> > > > 
> > 
> > Probably a poor choice of words. I just mean the URL of some web content as
> > known by the browser -- it's "authoritative" insofar as this URL can be
> trusted,
> > unlike any information that would come from the sandboxed content itself.
RFHI
> > can say that requests from its corresponding render frame are associated
with
> a
> > SiteInstance URL, and browser services can trust this information and make
> > security decisions accordingly. As far as I can tell, this isn't the case
with
> > workers, but maybe it doesn't make sense to associate a worker with a single
> URL
> > or origin?
> 
> Odd, the opposite seems more true to me? Frames navigate to different urls
that
> can be from different origins. Workers on the other hand never navigate and
are
> always associated with a single url. You can think of a Worker kind of like a
> Document without a visual representation.

Good to know about worker having a URL association, so what you say sounds
right. But what I mean is that in practice today, the browser-side abstraction
for a frame (RFHI) is aware of the child's URL at any given time. Unless I'm
missing something, this does not appear to be the case for the browser side of a
worker.

[michaeln, 2015-07-08 22:32:14]

On 2015/07/08 21:47:08, Ken Rockot wrote:
> On 2015/07/08 21:07:53, michaeln wrote:
> > The terms and distinctions between mojo application, shell, service, etc
don't
> > mean much to me so I'm pretty sure I don't fully follow. Also, we've strayed
> > from the specific of this CL and probably should refocus on the here and
now.
> > Should we proceed with this CL or scrap it and do something completely
> > different? If the answer to that is proceed...
> 
> You're right, this has gotten off track (my fault). But given this discussion
> I'm starting to think we should reevaluate the approach. It would be nice to
> treat a worker context as a mojo application from the start, since we seem to
> have that opportunity. I'm still trying to figure out what that looks like
> exactly.
> 
> > 
> > How can we arrange for EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl to run
on
> > the IPC thread in the renderer?
> 
> FWIW even if we went through with this, I don't understand how this part is
> significant. Is it really so bad that this one-time worker initialization step
> has to thread hop? Keep in mind that it's not necessary for the main thread to
> make progress before other threads start requesting services and issuing calls
> to them - everything will be queued and deferred by the EDK until the pipes
are
> actually ready.

In the renderer, the main thread can block on worker threads and i think
sometimes runs nested message loops instead of outright blocking. Worker threads
sometimes run nested message loops waiting for a particular task from the main
thread.

By looping the main thread into the connection establishment sequence, I suspect
new and unexpected and pathological code paths become possible. Debugging them
is very difficult. I'd rather avoid the possibility if its within reason to do
so.

Would it make sense for the child process to initiate the mojo connection
establishment instead of the other way around? That way it could be wedged into
the worker startup sequence (part of which occurs on the main thread) w/o any
risk of unexpected code paths. We could wedge this in prior to sending the
EmbeddedWorkerHostMsg_WorkerScriptLoaded msg. The browser-side is already knows
you to wait for worker startup to do things.

> > MojoWait is a blocking call so probably not very useful in chrome. Without
> > waiting, how can you be told when the process housing a remote service, you
> have
> > an intfptr to, has crashed.
> 
> Right, it would need to be used on its own monitoring thread or something (a
la
> mojo::HandleWatcher), but I guess the point is just that we -could- implement
a
> monolithic pipe manager if we wanted to. I still don't think we want to.
> 
> > Meta question: What benefit do we hope to gain by using mojo in the context
of
> > chrome?
> 
> Ideally chrome can provide a close approximation of the mandoline shell
> environment (content/browser is already hosting the same shell implementation
as
> mandoline, for example) so we can easily share components between the two. As
> components are factored into mojo apps for mandoline use, they can be used as
> such within chrome without needing a separate integration path, i.e. without
> registering various services at various service registry boundaries.
> 
> > Some of that sounds more like mandoline than chrome. The existing
> > content::serviceregistry seems like a very rough approximation of a shell
for
> > now. Is that a fair view of it?
> 
> Yes that's right, but it's an approximation I think we're prepared to phase
out
> for reasons stated above
> 
> > 
> > > I don't know where the Mojo application boundaries should be drawn among
> > > concepts like render process, render frame, worker instance, etc., but I
> think
> > > we can wing it for now since the cost of rearranging the bits later seems
> > > relatively low. So I guess I'm proposing that for now, yes, we treat each
> > worker
> > > instance as its own virtual application. I don't think this has to change
> any
> > > details about what process or thread it lives on. [on that note, do
workers
> do
> > > IPC on their own thread rather than the child's IO thread? that could
> explain
> > > some of my confusion here...]
> > 
> > aside: Rearranging will probably be more expensive then you think.
> 
> Heh. Yeah, you're probably right.
> 
> > 
> > Workers use the main browser-to-child IPC channel.
> > 
> > > In any case I only brought up all this stuff in response to questions
about
> > the
> > > right way to arrange all our service registries, because I think the real
> > answer
> > > is that we should get rid of service registries ASAP, and that goal should
> > carry
> > > significant weight in any related design discussions.
> > 
> > I'm interested in how to eliminate the potential for races and deadlocks by
> > avoiding cross-thread-task posting as much as possible. Downstream CLs are
> > creating mojo<meaningfulunits> at a fairly fine grain (in some cases per
event
> > type and per function call). What's the best way to setup those
> > mojo<meaningfulunits> without relying on the main thread making progress?
> > 
> > Specifically, in the context of this CL, how can we arrange for
> > EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl to run on the IPC thread in the
> > renderer?
> > 
> > > > thing that I think would be a good step towards mojofication of embedded
> > > workers
> > > > would be to replace all the EmbeddedWorker(Host) messages with a mojo
> > service;
> > > > to start a new embedded worker you'd connect to a new EmbeddedWorker
> service
> > > in
> > > > whichever process you want to connect to, StopWorker would just be
closing
> > > that
> > > > connection, and the other browser->renderer messages would be calls on
> that
> > > > interface. To replace EmbeddedWorkerContextMsg_MessageToWorker (without
> > > > replacing all browser->worker IPC with mojo) there could be a mojo
message
> > > pipe
> > > > backing an IPC::Channel for these messages. It would still be nice if
> > somehow
> > > 
> > > I agree that making EWH a Mojo service is a good idea. We do have an
> > > implementation of IPC::Channel which uses a Mojo pipe underneath;
> > unfortunately
> > > we have it turned off in production because of performance issues. Why
would
> > > this be necessary to support MessageToWorker?
> > > 
> > > > closing one mojo message pipe (in this case the one backing the
connection
> > to
> > > > the EmbeddedWorker instance) could automatically close every other
channel
> > > that
> > > > was set up to some service within that worker as well, but I don't think
> > mojo
> > > > currently really has the concept of dependent channels?
> > > 
> > > I'm not sure I understand what benefit we'd get out of dependent pipes.
Why
> > > would it be better for mojo to provide this mechanism rather than letting
> > > application code manage dependent lifetimes as needed?
> > > 
> > > > 
> > > > > 
> > > > > > It then occurs to me that I'd probably like workers to end up in the
> > same
> > > > place
> > > > > > as frames, but looking into it now it seems they have the same
problem
> > as
> > > > > > processes themselves: they don't appear to have any authoritative
web
> > > origin
> > > > > > associated with them in the browser. Hmm.
> > > > > 
> > > > > Could you elaborate a bit more on what you mean by 'authoritative web
> > > origin'?
> > > > > 
> > > 
> > > Probably a poor choice of words. I just mean the URL of some web content
as
> > > known by the browser -- it's "authoritative" insofar as this URL can be
> > trusted,
> > > unlike any information that would come from the sandboxed content itself.
> RFHI
> > > can say that requests from its corresponding render frame are associated
> with
> > a
> > > SiteInstance URL, and browser services can trust this information and make
> > > security decisions accordingly. As far as I can tell, this isn't the case
> with
> > > workers, but maybe it doesn't make sense to associate a worker with a
single
> > URL
> > > or origin?
> > 
> > Odd, the opposite seems more true to me? Frames navigate to different urls
> that
> > can be from different origins. Workers on the other hand never navigate and
> are
> > always associated with a single url. You can think of a Worker kind of like
a
> > Document without a visual representation.
> 
> Good to know about worker having a URL association, so what you say sounds
> right. But what I mean is that in practice today, the browser-side abstraction
> for a frame (RFHI) is aware of the child's URL at any given time. Unless I'm
> missing something, this does not appear to be the case for the browser side of
a
> worker.

That always makes me a little queasy. There's a period of time where a new
document for a different url is being loaded into the frame. Ipc messages
related to that resource load are going back and forth in the interim. Raciness,
schedule work to be done across threads and then upon completion probably don't
always check to see of the frame's url has changed.

void EmbeddedWorkerInstance::Start(int64 service_worker_version_id,
                                   const GURL& scope,
                                   const GURL& script_url,  <----- url is right
here
                                   bool pause_after_download,
                                   const StatusCallback& callback) {

[Ken Rockot(use gerrit already), 2015-07-09 00:13:38]

message: On 2015/07/08 22:32:14, michaeln wrote:
> In the renderer, the main thread can block on worker threads and i think
> sometimes runs nested message loops instead of outright blocking. Worker
threads
> sometimes run nested message loops waiting for a particular task from the main
> thread.
> 
> By looping the main thread into the connection establishment sequence, I
suspect
> new and unexpected and pathological code paths become possible. Debugging them
> is very difficult. I'd rather avoid the possibility if its within reason to do
> so.
> 
> Would it make sense for the child process to initiate the mojo connection
> establishment instead of the other way around? That way it could be wedged
into
> the worker startup sequence (part of which occurs on the main thread) w/o any
> risk of unexpected code paths. We could wedge this in prior to sending the
> EmbeddedWorkerHostMsg_WorkerScriptLoaded msg. The browser-side is already
knows
> you to wait for worker startup to do things.
> 

Hmm. I think the master process has to initiate the mojo channel connection, but
could we instead just defer worker initialization until after the channel is set
up? This could be done immediately in response MojoMsg_Activate, which is always
the first IPC sent from browser->child immediately on process start.

> > 
> > Good to know about worker having a URL association, so what you say sounds
> > right. But what I mean is that in practice today, the browser-side
abstraction
> > for a frame (RFHI) is aware of the child's URL at any given time. Unless I'm
> > missing something, this does not appear to be the case for the browser side
of
> a
> > worker.
> 
> That always makes me a little queasy. There's a period of time where a new
> document for a different url is being loaded into the frame. Ipc messages
> related to that resource load are going back and forth in the interim.
Raciness,
> schedule work to be done across threads and then upon completion probably
don't
> always check to see of the frame's url has changed.

If a frame navigates to a different origin though, doesn't it end up with a
different RFHI instance on the browser side? It doesn't look to me like an RHFI
can change its SiteInstance. If the old RHFI can receive IPC messages intended
for new url resource loads, that would be a big problem already wouldn't it?
Since the RHFI is responsible for connecting to services on behalf of its frame
I would think this should still be safe and not racy. I'm probably missing
something.

> 
> void EmbeddedWorkerInstance::Start(int64 service_worker_version_id,
>                                    const GURL& scope,
>                                    const GURL& script_url,  <----- url is
right
> here
>                                    bool pause_after_download,
>                                    const StatusCallback& callback) {

Oh, great! Thanks, I obviously missed this.

[Marijn Kruisselbrink, 2015-07-09 00:13:48]

On 2015/07/08 at 22:32:14, michaeln wrote:
> On 2015/07/08 21:47:08, Ken Rockot wrote:
> > On 2015/07/08 21:07:53, michaeln wrote:
> > > The terms and distinctions between mojo application, shell, service, etc
don't
> > > mean much to me so I'm pretty sure I don't fully follow. Also, we've
strayed
> > > from the specific of this CL and probably should refocus on the here and
now.
> > > Should we proceed with this CL or scrap it and do something completely
> > > different? If the answer to that is proceed...
> > 
> > You're right, this has gotten off track (my fault). But given this
discussion
> > I'm starting to think we should reevaluate the approach. It would be nice to
> > treat a worker context as a mojo application from the start, since we seem
to
> > have that opportunity. I'm still trying to figure out what that looks like
> > exactly.

What exactly do we gain by somehow treating a worker context as a mojo
application (and what exactly does it mean to treat anything as a mojo
application)? At some point the browser will need to start a worker in a
renderer and get some kind of handle to that worker. Currently that handle is a
process-id + thread-id, to be able to use mojo at a minimum we'd need to add  a
ServiceProvider as another handle to the same worker instance (note again that
the reverse, connecting from the worker to other services is explicitly not part
of the scope of anything we're currently trying to address). If it makes it
easier to move away from ServiceRegistry in the future I'd be happy to not use
that for now and instead only pass a ServiceProvider one way, in whatever shape
that will take (separate Setup, new EmbeddedWorker service that is connected to,
or somehow make this a mojo application, whatever that means). But I would like
to find a way forward sooner rather than later. At the moment we basically have
these options:
1) don't use mojo at all, and instead add new regular IPCs for the various APIs
we're implementing, and rewrite it all as mojo services after we figure out what
that would look like
2) block implementing these new APIs till we figure out what exactly it means to
have this be a mojo application, do whatever refactoring that would be required
for that, and only then resume work on these APIs
3) implement something like this CL now so new APIs can start to expose features
as mojo services from the worker to the browser process, and in parallel figure
out what a more mojofied version would look like; moving over to a more mojofied
version then should be much easier than after 1), since after all the
functionality is already exposed as mojo services, we'd just change how to
connect to those services

What I'm suggesting is to do 3. What rockot is suggesting seems to be either 1
or 2 (suggesting 2, but I think it would degenerate into 1 since it is unclear
how long it would actually take to figure out 2, and moving ahead with other
feature work is important too).

> > 
> > > 
> > > How can we arrange for EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl to run
on
> > > the IPC thread in the renderer?
> > 
> > FWIW even if we went through with this, I don't understand how this part is
> > significant. Is it really so bad that this one-time worker initialization
step
> > has to thread hop? Keep in mind that it's not necessary for the main thread
to
> > make progress before other threads start requesting services and issuing
calls
> > to them - everything will be queued and deferred by the EDK until the pipes
are
> > actually ready.
> 
> In the renderer, the main thread can block on worker threads and i think
sometimes runs nested message loops instead of outright blocking. Worker threads
sometimes run nested message loops waiting for a particular task from the main
thread.
> 
> By looping the main thread into the connection establishment sequence, I
suspect new and unexpected and pathological code paths become possible.
Debugging them is very difficult. I'd rather avoid the possibility if its within
reason to do so.
> 
> Would it make sense for the child process to initiate the mojo connection
establishment instead of the other way around? That way it could be wedged into
the worker startup sequence (part of which occurs on the main thread) w/o any
risk of unexpected code paths. We could wedge this in prior to sending the
EmbeddedWorkerHostMsg_WorkerScriptLoaded msg. The browser-side is already knows
you to wait for worker startup to do things.

I think the main problem with avoiding the main thread in any kind of mojo
connection establishment is that currently the way to connect to services from
browser to renderer or vice versa has to go through the ServiceRegistry, which
for some reason lives on the main and UI threads instead of on the IO thread
(where it seems like it would make much more sense). It would be fairly easy to
fold it all into actually starting the embedded worker though by moving just the
browser->renderer EmbeddedWorkerMsg messages to use mojo instead, eliminating
the extra setup step. Due to how connections to renderers currently seem to only
be able to be made on the main thread (since the per process ServiceRegistry in
the renderer lives on the main thread) it would still involve unneeded thread
hopping through the main thread though. I'm not sure why the existing
ServiceRegistry instances don't live on the IO thread anyway, it seems that
would make a lot more sense, since eventually everything has to go through there
anyway.

I'd be happy to come up with some CL as to what it would look like to use more
mojo in the setup of embedded workers. I'm not entirely sure if it makes sense
to make this an actual mojo application though, as it is unclear to me what if
any the benefits of that would be, and how exactly that would work/look. Mojo
documentation/designs seem to be rather hard to find and often seem to be very
outdated...

[michaeln, 2015-07-09 02:28:56]

>>  It doesn't look to me like an RHFI can change its SiteInstance.

Hmmm, it sure doesn't? I'm not sure what the relationship between a RF and a
WebFrame is then, and what happens when one page navigates its frame to another
origin? I think this has been changing in support of SiteIsolation?

>> Hmm. I think the master process has to initiate the mojo channel connection,

Maybe the privileged side has to create the pipe to start with?

> What I'm suggesting is to do 3. What rockot is suggesting seems to be either 1

My vote is #3, failing that #1.

> and what exactly does it mean to treat anything as a mojo application

Glad you asked, idk either?

> I think the main problem with avoiding the main thread in any kind of mojo
> connection establishment is that currently the way to connect to services from
> browser to renderer or vice versa has to go through the ServiceRegistry, 
which
> for some reason lives on the main and UI threads instead of on the IO thread
> (where it seems like it would make much more sense).

Maybe we should fix that or build on what's there?

Your current CL establishes a pair of connected ServiceRegistryImpl's with one
side having IO thread affinity and the other having Worker thread affinity. I'm
assuming code that looks like EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl could
be run on the Worker thread. If so, for #3, maybe we could setup two
ServiceRegistryImpl pairs at child process startup instead of one. The first
would facilitate UI to main thread mojo-connection-establishment and general
chit-chat (UIServiceRegistry), the second would facilitate the same between IO
and IPC threads (IOServiceRegistry).

Then we'd use the IOServiceRegistry to setup of the IOtoWorkerThread registry
pair this current CL is trying to setup.

Proper and prescribed usage of mojo abstractions aside, technically, can that be
composed?

[kinuko, 2015-07-09 02:36:22]

On 2015/07/09 00:13:48, Marijn Kruisselbrink wrote:
> What exactly do we gain by somehow treating a worker context as a mojo
> application (and what exactly does it mean to treat anything as a mojo
> application)? At some point the browser will need to start a worker in a
> renderer and get some kind of handle to that worker. Currently that handle is
a
> process-id + thread-id, to be able to use mojo at a minimum we'd need to add 
a
> ServiceProvider as another handle to the same worker instance (note again that
> the reverse, connecting from the worker to other services is explicitly not
part
> of the scope of anything we're currently trying to address). If it makes it
> easier to move away from ServiceRegistry in the future I'd be happy to not use
> that for now and instead only pass a ServiceProvider one way, in whatever
shape
> that will take (separate Setup, new EmbeddedWorker service that is connected
to,
> or somehow make this a mojo application, whatever that means). But I would
like
> to find a way forward sooner rather than later. At the moment we basically
have
> these options:
> 1) don't use mojo at all, and instead add new regular IPCs for the various
APIs
> we're implementing, and rewrite it all as mojo services after we figure out
what
> that would look like
> 2) block implementing these new APIs till we figure out what exactly it means
to
> have this be a mojo application, do whatever refactoring that would be
required
> for that, and only then resume work on these APIs
> 3) implement something like this CL now so new APIs can start to expose
features
> as mojo services from the worker to the browser process, and in parallel
figure
> out what a more mojofied version would look like; moving over to a more
mojofied
> version then should be much easier than after 1), since after all the
> functionality is already exposed as mojo services, we'd just change how to
> connect to those services
> 
> What I'm suggesting is to do 3. What rockot is suggesting seems to be either 1
> or 2 (suggesting 2, but I think it would degenerate into 1 since it is unclear
> how long it would actually take to figure out 2, and moving ahead with other
> feature work is important too).

I usually like incremental approaches like 2 (in most cases that's the only way
to move things forward), but for this one I'm afraid having both half-mojofied
code and traditional IPC code make things harder to maintain, especially if
doing 2 needs to rely on additional thread hopping in the middle of worker
initialization, and also if the component we'll rely on (i.e. service registry)
would be an interim solution.

> I think the main problem with avoiding the main thread in any kind of mojo
> connection establishment is that currently the way to connect to services from
> browser to renderer or vice versa has to go through the ServiceRegistry, which
> for some reason lives on the main and UI threads instead of on the IO thread
> (where it seems like it would make much more sense). It would be fairly easy
to
> fold it all into actually starting the embedded worker though by moving just
the
> browser->renderer EmbeddedWorkerMsg messages to use mojo instead, eliminating
> the extra setup step. Due to how connections to renderers currently seem to
only
> be able to be made on the main thread (since the per process ServiceRegistry
in
> the renderer lives on the main thread) it would still involve unneeded thread
> hopping through the main thread though. I'm not sure why the existing
> ServiceRegistry instances don't live on the IO thread anyway, it seems that
> would make a lot more sense, since eventually everything has to go through
there
> anyway.

Is there a possibility that we could answer this question (i.e. why the existing
ServiceRegistry instances don't live on the IO thread) now rather than later?

> I'd be happy to come up with some CL as to what it would look like to use more
> mojo in the setup of embedded workers. I'm not entirely sure if it makes sense
> to make this an actual mojo application though, as it is unclear to me what if
> any the benefits of that would be, and how exactly that would work/look. Mojo
> documentation/designs seem to be rather hard to find and often seem to be very
> outdated...

Yup, I share this feeling.

(Btw does it make sense to have a short VC or something to chat about this?)

[Marijn Kruisselbrink, 2015-07-09 16:00:56]

On 2015/07/09 at 02:36:22, kinuko wrote:
> On 2015/07/09 00:13:48, Marijn Kruisselbrink wrote:
> > What exactly do we gain by somehow treating a worker context as a mojo
> > application (and what exactly does it mean to treat anything as a mojo
> > application)? At some point the browser will need to start a worker in a
> > renderer and get some kind of handle to that worker. Currently that handle
is a
> > process-id + thread-id, to be able to use mojo at a minimum we'd need to add
 a
> > ServiceProvider as another handle to the same worker instance (note again
that
> > the reverse, connecting from the worker to other services is explicitly not
part
> > of the scope of anything we're currently trying to address). If it makes it
> > easier to move away from ServiceRegistry in the future I'd be happy to not
use
> > that for now and instead only pass a ServiceProvider one way, in whatever
shape
> > that will take (separate Setup, new EmbeddedWorker service that is connected
to,
> > or somehow make this a mojo application, whatever that means). But I would
like
> > to find a way forward sooner rather than later. At the moment we basically
have
> > these options:
> > 1) don't use mojo at all, and instead add new regular IPCs for the various
APIs
> > we're implementing, and rewrite it all as mojo services after we figure out
what
> > that would look like
> > 2) block implementing these new APIs till we figure out what exactly it
means to
> > have this be a mojo application, do whatever refactoring that would be
required
> > for that, and only then resume work on these APIs
> > 3) implement something like this CL now so new APIs can start to expose
features
> > as mojo services from the worker to the browser process, and in parallel
figure
> > out what a more mojofied version would look like; moving over to a more
mojofied
> > version then should be much easier than after 1), since after all the
> > functionality is already exposed as mojo services, we'd just change how to
> > connect to those services
> > 
> > What I'm suggesting is to do 3. What rockot is suggesting seems to be either
1
> > or 2 (suggesting 2, but I think it would degenerate into 1 since it is
unclear
> > how long it would actually take to figure out 2, and moving ahead with other
> > feature work is important too).
> 
> I usually like incremental approaches like 2 (in most cases that's the only
way to move things forward), but for this one I'm afraid having both
half-mojofied code and traditional IPC code make things harder to maintain,
especially if doing 2 needs to rely on additional thread hopping in the middle
of worker initialization, and also if the component we'll rely on (i.e. service
registry) would be an interim solution.

More concretely I think what I'm proposing is to replace the messages that are
handled by EmbeddedWorkerDispatcher by a mojo interface/service/application
(with probably just one method, "Start"). That would improve this CL in that it
would eliminate the need of the seperate EmbeddedWorkerSetup step, instead it
would all just be part of the StartWorker call. It also wouldn't introduce any
new thread hopping in the middle of worker initialization (at least not on the
renderer side). The StartWorker IPC currently already is handled on the main
thread in the browser, so this new EmbeddedWorkerImpl::Start would be perfectly
fine to execute on the main thread as well. Similarly on the browser side
actually starting a worker already involves thread hops through the UI thread,
so actually connecting to this new EmbeddedWorker service would not add any new
thread hopping either. And we don't need to use ServiceRegistry in any of this
(other than as way to actually connect to a new EmbeddedWorker instance in a
renderer). We can just use ServiceProviderImpl directly, or something closer to
that. Unless I'm missing something, no matter what the future will look like,
there will be some ServiceProvider implementation that is used to expose
services from this EmbeddedWorker service/application to the browser process.

> 
> > I think the main problem with avoiding the main thread in any kind of mojo
> > connection establishment is that currently the way to connect to services
from
> > browser to renderer or vice versa has to go through the ServiceRegistry,
which
> > for some reason lives on the main and UI threads instead of on the IO thread
> > (where it seems like it would make much more sense). It would be fairly easy
to
> > fold it all into actually starting the embedded worker though by moving just
the
> > browser->renderer EmbeddedWorkerMsg messages to use mojo instead,
eliminating
> > the extra setup step. Due to how connections to renderers currently seem to
only
> > be able to be made on the main thread (since the per process ServiceRegistry
in
> > the renderer lives on the main thread) it would still involve unneeded
thread
> > hopping through the main thread though. I'm not sure why the existing
> > ServiceRegistry instances don't live on the IO thread anyway, it seems that
> > would make a lot more sense, since eventually everything has to go through
there
> > anyway.
> 
> Is there a possibility that we could answer this question (i.e. why the
existing ServiceRegistry instances don't live on the IO thread) now rather than
later?
> 
> > I'd be happy to come up with some CL as to what it would look like to use
more
> > mojo in the setup of embedded workers. I'm not entirely sure if it makes
sense
> > to make this an actual mojo application though, as it is unclear to me what
if
> > any the benefits of that would be, and how exactly that would work/look.
Mojo
> > documentation/designs seem to be rather hard to find and often seem to be
very
> > outdated...
> 
> Yup, I share this feeling.
> 
> (Btw does it make sense to have a short VC or something to chat about this?)

I think it might make sense to do so, yes. Unfortunately I don't think I'll be
able to make it this week (not sure the wifi at the place I'm at this week can
support VC), but something monday california time might still make sense?

[Ken Rockot(use gerrit already), 2015-07-09 17:07:52]

On 2015/07/09 16:00:56, Marijn Kruisselbrink wrote:
> On 2015/07/09 at 02:36:22, kinuko wrote:
> > On 2015/07/09 00:13:48, Marijn Kruisselbrink wrote:
> > > What exactly do we gain by somehow treating a worker context as a mojo
> > > application (and what exactly does it mean to treat anything as a mojo
> > > application)? At some point the browser will need to start a worker in a
> > > renderer and get some kind of handle to that worker. Currently that handle
> is a
> > > process-id + thread-id, to be able to use mojo at a minimum we'd need to
add
>  a
> > > ServiceProvider as another handle to the same worker instance (note again
> that
> > > the reverse, connecting from the worker to other services is explicitly
not
> part
> > > of the scope of anything we're currently trying to address). If it makes
it
> > > easier to move away from ServiceRegistry in the future I'd be happy to not
> use
> > > that for now and instead only pass a ServiceProvider one way, in whatever
> shape
> > > that will take (separate Setup, new EmbeddedWorker service that is
connected
> to,
> > > or somehow make this a mojo application, whatever that means). But I would
> like
> > > to find a way forward sooner rather than later. At the moment we basically
> have
> > > these options:
> > > 1) don't use mojo at all, and instead add new regular IPCs for the various
> APIs
> > > we're implementing, and rewrite it all as mojo services after we figure
out
> what
> > > that would look like
> > > 2) block implementing these new APIs till we figure out what exactly it
> means to
> > > have this be a mojo application, do whatever refactoring that would be
> required
> > > for that, and only then resume work on these APIs
> > > 3) implement something like this CL now so new APIs can start to expose
> features
> > > as mojo services from the worker to the browser process, and in parallel
> figure
> > > out what a more mojofied version would look like; moving over to a more
> mojofied
> > > version then should be much easier than after 1), since after all the
> > > functionality is already exposed as mojo services, we'd just change how to
> > > connect to those services
> > > 
> > > What I'm suggesting is to do 3. What rockot is suggesting seems to be
either
> 1
> > > or 2 (suggesting 2, but I think it would degenerate into 1 since it is
> unclear
> > > how long it would actually take to figure out 2, and moving ahead with
other
> > > feature work is important too).
> > 
> > I usually like incremental approaches like 2 (in most cases that's the only
> way to move things forward), but for this one I'm afraid having both
> half-mojofied code and traditional IPC code make things harder to maintain,
> especially if doing 2 needs to rely on additional thread hopping in the middle
> of worker initialization, and also if the component we'll rely on (i.e.
service
> registry) would be an interim solution.
> 
> More concretely I think what I'm proposing is to replace the messages that are
> handled by EmbeddedWorkerDispatcher by a mojo interface/service/application
> (with probably just one method, "Start"). That would improve this CL in that
it
> would eliminate the need of the seperate EmbeddedWorkerSetup step, instead it
> would all just be part of the StartWorker call. It also wouldn't introduce any
> new thread hopping in the middle of worker initialization (at least not on the
> renderer side). The StartWorker IPC currently already is handled on the main
> thread in the browser, so this new EmbeddedWorkerImpl::Start would be
perfectly
> fine to execute on the main thread as well. Similarly on the browser side
> actually starting a worker already involves thread hops through the UI thread,
> so actually connecting to this new EmbeddedWorker service would not add any
new
> thread hopping either. And we don't need to use ServiceRegistry in any of this
> (other than as way to actually connect to a new EmbeddedWorker instance in a
> renderer). We can just use ServiceProviderImpl directly, or something closer
to
> that. Unless I'm missing something, no matter what the future will look like,
> there will be some ServiceProvider implementation that is used to expose
> services from this EmbeddedWorker service/application to the browser process.
> 
> > 
> > > I think the main problem with avoiding the main thread in any kind of mojo
> > > connection establishment is that currently the way to connect to services
> from
> > > browser to renderer or vice versa has to go through the ServiceRegistry,
> which
> > > for some reason lives on the main and UI threads instead of on the IO
thread
> > > (where it seems like it would make much more sense). It would be fairly
easy
> to
> > > fold it all into actually starting the embedded worker though by moving
just
> the
> > > browser->renderer EmbeddedWorkerMsg messages to use mojo instead,
> eliminating
> > > the extra setup step. Due to how connections to renderers currently seem
to
> only
> > > be able to be made on the main thread (since the per process
ServiceRegistry
> in
> > > the renderer lives on the main thread) it would still involve unneeded
> thread
> > > hopping through the main thread though. I'm not sure why the existing
> > > ServiceRegistry instances don't live on the IO thread anyway, it seems
that
> > > would make a lot more sense, since eventually everything has to go through
> there
> > > anyway.
> > 
> > Is there a possibility that we could answer this question (i.e. why the
> existing ServiceRegistry instances don't live on the IO thread) now rather
than
> later?
> > 
> > > I'd be happy to come up with some CL as to what it would look like to use
> more
> > > mojo in the setup of embedded workers. I'm not entirely sure if it makes
> sense
> > > to make this an actual mojo application though, as it is unclear to me
what
> if
> > > any the benefits of that would be, and how exactly that would work/look.
> Mojo
> > > documentation/designs seem to be rather hard to find and often seem to be
> very
> > > outdated...
> > 
> > Yup, I share this feeling.
> > 
> > (Btw does it make sense to have a short VC or something to chat about this?)
> 
> I think it might make sense to do so, yes. Unfortunately I don't think I'll be
> able to make it this week (not sure the wifi at the place I'm at this week can
> support VC), but something monday california time might still make sense?

Yeah let's meet up next week. I'll prepare some long overdue documentation about
mojo apps in chrome and mock up a design for #2 (which i don't think will be a
real blocker).

[Ken Rockot(use gerrit already), 2015-07-09 19:43:42]

On 2015/07/09 02:28:56, michaeln wrote:
> >>  It doesn't look to me like an RHFI can change its SiteInstance.
> 
> Hmmm, it sure doesn't? I'm not sure what the relationship between a RF and a
> WebFrame is then, and what happens when one page navigates its frame to
another
> origin? I think this has been changing in support of SiteIsolation?
> 
> >> Hmm. I think the master process has to initiate the mojo channel
connection,
> 
> Maybe the privileged side has to create the pipe to start with?
> 
> > What I'm suggesting is to do 3. What rockot is suggesting seems to be either
1
> 
> My vote is #3, failing that #1.
> 
> > and what exactly does it mean to treat anything as a mojo application
> 
> Glad you asked, idk either?
> 
> > I think the main problem with avoiding the main thread in any kind of mojo
> > connection establishment is that currently the way to connect to services
from
> > browser to renderer or vice versa has to go through the ServiceRegistry, 
> which
> > for some reason lives on the main and UI threads instead of on the IO thread
> > (where it seems like it would make much more sense).
> 
> Maybe we should fix that or build on what's there?
> 
> Your current CL establishes a pair of connected ServiceRegistryImpl's with one
> side having IO thread affinity and the other having Worker thread affinity.
I'm
> assuming code that looks like EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl
could
> be run on the Worker thread. If so, for #3, maybe we could setup two
> ServiceRegistryImpl pairs at child process startup instead of one. The first
> would facilitate UI to main thread mojo-connection-establishment and general
> chit-chat (UIServiceRegistry), the second would facilitate the same between IO
> and IPC threads (IOServiceRegistry).
> 
> Then we'd use the IOServiceRegistry to setup of the IOtoWorkerThread registry
> pair this current CL is trying to setup.
> 
> Proper and prescribed usage of mojo abstractions aside, technically, can that
be
> composed?

In general moving forward with this CL as-is (essentially, choosing #3) still
SGTM if we can satisfy the original concerns over races. Is that what this
two-SRI-pair proposal addresses? If so, how?

[Marijn Kruisselbrink, 2015-07-09 19:59:30]

On 2015/07/09 at 19:43:42, rockot wrote:
> On 2015/07/09 02:28:56, michaeln wrote:
> > >>  It doesn't look to me like an RHFI can change its SiteInstance.
> > 
> > Hmmm, it sure doesn't? I'm not sure what the relationship between a RF and a
> > WebFrame is then, and what happens when one page navigates its frame to
another
> > origin? I think this has been changing in support of SiteIsolation?
> > 
> > >> Hmm. I think the master process has to initiate the mojo channel
connection,
> > 
> > Maybe the privileged side has to create the pipe to start with?
> > 
> > > What I'm suggesting is to do 3. What rockot is suggesting seems to be
either 1
> > 
> > My vote is #3, failing that #1.
> > 
> > > and what exactly does it mean to treat anything as a mojo application
> > 
> > Glad you asked, idk either?
> > 
> > > I think the main problem with avoiding the main thread in any kind of mojo
> > > connection establishment is that currently the way to connect to services
from
> > > browser to renderer or vice versa has to go through the ServiceRegistry, 
> > which
> > > for some reason lives on the main and UI threads instead of on the IO
thread
> > > (where it seems like it would make much more sense).
> > 
> > Maybe we should fix that or build on what's there?
> > 
> > Your current CL establishes a pair of connected ServiceRegistryImpl's with
one
> > side having IO thread affinity and the other having Worker thread affinity.
I'm
> > assuming code that looks like EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl
could
> > be run on the Worker thread. If so, for #3, maybe we could setup two
> > ServiceRegistryImpl pairs at child process startup instead of one. The first
> > would facilitate UI to main thread mojo-connection-establishment and general
> > chit-chat (UIServiceRegistry), the second would facilitate the same between
IO
> > and IPC threads (IOServiceRegistry).
> > 
> > Then we'd use the IOServiceRegistry to setup of the IOtoWorkerThread
registry
> > pair this current CL is trying to setup.
> > 
> > Proper and prescribed usage of mojo abstractions aside, technically, can
that be
> > composed?
> 
> In general moving forward with this CL as-is (essentially, choosing #3) still
SGTM if we can satisfy the original concerns over races. Is that what this
two-SRI-pair proposal addresses? If so, how?

I think the race condition concerns can be addressed without too much problems.
I'm not sure if having two SRI pairs is the way to go (and if that would really
get us anything) though. Certainly having two completely separate SRI pairs
between processes seems like it would just cause even more confusion as some
services would only be available in one and other services would only be
available in the other. Maybe it makes sense to move the "real"
ServiceRegistryImpl to always live on the IO thread, and have some kind of
wrapper with thread hopping around it for the few cases where a ServiceRegistry
is actually needed on the main/UI thread. It seems eventually every connection
attempt has to pass through the IO thread anyway, so this should cause much
slowdown in existing code (but maybe I'm wrong there. I don't know what the
reasoning was to have ServiceRegistry exist on the main thread in the first
place).

[michaeln, 2015-07-09 21:42:58]

On 2015/07/09 19:59:30, Marijn Kruisselbrink wrote:
> On 2015/07/09 at 19:43:42, rockot wrote:
> > On 2015/07/09 02:28:56, michaeln wrote:
> > > >>  It doesn't look to me like an RHFI can change its SiteInstance.
> > > 
> > > Hmmm, it sure doesn't? I'm not sure what the relationship between a RF and
a
> > > WebFrame is then, and what happens when one page navigates its frame to
> another
> > > origin? I think this has been changing in support of SiteIsolation?
> > > 
> > > >> Hmm. I think the master process has to initiate the mojo channel
> connection,
> > > 
> > > Maybe the privileged side has to create the pipe to start with?
> > > 
> > > > What I'm suggesting is to do 3. What rockot is suggesting seems to be
> either 1
> > > 
> > > My vote is #3, failing that #1.
> > > 
> > > > and what exactly does it mean to treat anything as a mojo application
> > > 
> > > Glad you asked, idk either?
> > > 
> > > > I think the main problem with avoiding the main thread in any kind of
mojo
> > > > connection establishment is that currently the way to connect to
services
> from
> > > > browser to renderer or vice versa has to go through the ServiceRegistry,

> > > which
> > > > for some reason lives on the main and UI threads instead of on the IO
> thread
> > > > (where it seems like it would make much more sense).
> > > 
> > > Maybe we should fix that or build on what's there?
> > > 
> > > Your current CL establishes a pair of connected ServiceRegistryImpl's with
> one
> > > side having IO thread affinity and the other having Worker thread
affinity.
> I'm
> > > assuming code that looks like EmbeddedWorkerSetup::EmbeddedWorkerSetupImpl
> could
> > > be run on the Worker thread. If so, for #3, maybe we could setup two
> > > ServiceRegistryImpl pairs at child process startup instead of one. The
first
> > > would facilitate UI to main thread mojo-connection-establishment and
general
> > > chit-chat (UIServiceRegistry), the second would facilitate the same
between
> IO
> > > and IPC threads (IOServiceRegistry).
> > > 
> > > Then we'd use the IOServiceRegistry to setup of the IOtoWorkerThread
> registry
> > > pair this current CL is trying to setup.
> > > 
> > > Proper and prescribed usage of mojo abstractions aside, technically, can
> that be
> > > composed?
> > 
> > In general moving forward with this CL as-is (essentially, choosing #3)
still
> SGTM if we can satisfy the original concerns over races. Is that what this
> two-SRI-pair proposal addresses? If so, how?
> 
> I think the race condition concerns can be addressed without too much
problems.
> I'm not sure if having two SRI pairs is the way to go (and if that would
really
> get us anything) though. Certainly having two completely separate SRI pairs
> between processes seems like it would just cause even more confusion as some
> services would only be available in one and other services would only be
> available in the other. Maybe it makes sense to move the "real"
> ServiceRegistryImpl to always live on the IO thread, and have some kind of
> wrapper with thread hopping around it for the few cases where a
ServiceRegistry
> is actually needed on the main/UI thread. It seems eventually every connection
> attempt has to pass through the IO thread anyway, so this should cause much
> slowdown in existing code (but maybe I'm wrong there. I don't know what the
> reasoning was to have ServiceRegistry exist on the main thread in the first
> place).

Meeting sgtm2, looking forward to it!

I'm confused about some of the comments about multiple SRI pairs? This CL
creates an SRI pair which I believe allows IOthread to WorkerThread messaging
w/o hopping to the main thread on either side and which will have a set of
services available on it distinct from the set of services available on main
thread SRI pairs. Existing code in the project sets up multiple SRI pairs
(one-per-process and one-per-frame) also with a distinct set of services
registered.

I'm curious, how is setting up an IOthread to IOthread pair any different?

[Marijn Kruisselbrink, 2015-07-14 17:38:36]

On 2015/07/09 at 21:42:58, michaeln wrote:
> Meeting sgtm2, looking forward to it!

I think the outcome of the meeting was to go ahead with this, so unless there is
something else that needs addressing I'd like to do so. I don't think the extra
UI/main thread hopping when starting a new worker is going to cause any problems
(several steps of starting a worker on both the browser and renderer side
already depend on these threads making progress), and the extra thread hop in
setting up the SRI pair doesn't actually slow down any of the existing starting
of a worker code paths, as it's all done asynchronously.

[michaeln, 2015-07-15 02:34:22]

https://codereview.chromium.org/1221503003/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1221503003/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:221: ServiceRegistry*
EmbeddedWorkerInstance::GetServiceRegistry() {
Maybe DCHECK its either STARTING or RUNNING and service_registry_ is not null.

But as coded, I think that DCHECK would trip? The EmbeddedWorkerInstance goes
into the STARTING state in the Start() method, a whole lot happens prior to
OnScriptLoaded() running which is where the ServiceRegistry is being created.

Is it possible to create it and BindRemoteServiceProvider() here, and defer
SetupMojoOnUIThread until after the script has loaded? Or maybe just say "please
only call GetServiceRegistry() when its RUNNING."?

https://codereview.chromium.org/1221503003/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:418:
ReleaseProcess();
probably should reset the registry here too, looks like ReleaseProcess() is the
probably the best place to do this

https://codereview.chromium.org/1221503003/diff/80001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1221503003/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:278:
service_registry_.BindRemoteServiceProvider(exposed_services.Pass());
Script will be running before this happens. Does that have an impact on whether
this registry pair can be used as an ipc replacement for worker to browser
messaging. Suppose during eval, script calls a method back by c++. Can it use
the registry prior to the Bind happening?

[Marijn Kruisselbrink, 2015-07-15 18:13:28]

https://codereview.chromium.org/1221503003/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1221503003/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:221: ServiceRegistry*
EmbeddedWorkerInstance::GetServiceRegistry() {
On 2015/07/15 at 02:34:22, michaeln wrote:
> Maybe DCHECK its either STARTING or RUNNING and service_registry_ is not null.
> 
> But as coded, I think that DCHECK would trip? The EmbeddedWorkerInstance goes
into the STARTING state in the Start() method, a whole lot happens prior to
OnScriptLoaded() running which is where the ServiceRegistry is being created.
> 
> Is it possible to create it and BindRemoteServiceProvider() here, and defer
SetupMojoOnUIThread until after the script has loaded? Or maybe just say "please
only call GetServiceRegistry() when its RUNNING."?

I added the DCHECK and moved the creating of the service_registry_ to the
Start() method. ServiceRegistry will just queue up any connection attempts until
it is bound to a remote service provider, so trying to use it before
SetupMojoOnUIThread is called works in that calls will eventually resolve
(assuming the worker actually starts). Creating the ServiceRegistry on demand
here in the getter would of course also work, but since it's always going to be
created anyway (and is fairly lightweight) I don't think that really gains us
much.

https://codereview.chromium.org/1221503003/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:418:
ReleaseProcess();
On 2015/07/15 at 02:34:22, michaeln wrote:
> probably should reset the registry here too, looks like ReleaseProcess() is
the probably the best place to do this

Yes, moved the resetting of the registry to ReleaseProcess (and the one other
spot where Status can get set to STOPPED if starting it fails).

https://codereview.chromium.org/1221503003/diff/80001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1221503003/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:278:
service_registry_.BindRemoteServiceProvider(exposed_services.Pass());
On 2015/07/15 at 02:34:22, michaeln wrote:
> Script will be running before this happens. Does that have an impact on
whether this registry pair can be used as an ipc replacement for worker to
browser messaging. Suppose during eval, script calls a method back by c++. Can
it use the registry prior to the Bind happening?

The service registry is usable before either Bind happens. Attempts to connect
to remote services are just queued up until BindRemoteServiceProvider is
eventually called (or until the registry is destroyed if
BindRemoteServiceProvider was never called).

[Marijn Kruisselbrink, 2015-07-15 18:24:27]

Patchset #6 (id:100001) has been deleted

[michaeln, 2015-07-15 21:18:52]

https://codereview.chromium.org/1221503003/diff/80001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1221503003/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:278:
service_registry_.BindRemoteServiceProvider(exposed_services.Pass());
On 2015/07/15 18:13:28, Marijn Kruisselbrink wrote:
> On 2015/07/15 at 02:34:22, michaeln wrote:
> > Script will be running before this happens. Does that have an impact on
> whether this registry pair can be used as an ipc replacement for worker to
> browser messaging. Suppose during eval, script calls a method back by c++. Can
> it use the registry prior to the Bind happening?
> 
> The service registry is usable before either Bind happens. Attempts to connect
> to remote services are just queued up until BindRemoteServiceProvider is
> eventually called (or until the registry is destroyed if
> BindRemoteServiceProvider was never called).

thats neat

https://codereview.chromium.org/1221503003/diff/120001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1221503003/diff/120001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:282:
service_registry_.reset();
nice catch

https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:227:
ServiceRegistryImpl service_registry_;
The ServiceWorkerContextClient is used on multiple threads. It's created deleted
on the main thread, but this registry data member will be bound and used on the
worker thread. Is it OK to delete this member on the main thread? I would expect
that it is not but don't really know.

Ah, ServiceRegistryImpl class hasa WeakPtrFactory, that all on its own has
thread affinity and shouldn't be deleted on the wrong thread.

The ServiceWorkerDispatcher class is generally a safer place to store objects
with strong thread affinity like this. The ServiceWorkerDispatcher class is
strictly single threaded (created, used, deleted exclusively on one thread) and
intentionally difficult to reach from the wrong thread.

[Marijn Kruisselbrink, 2015-07-15 21:45:58]

https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:227:
ServiceRegistryImpl service_registry_;
On 2015/07/15 at 21:18:52, michaeln wrote:
> The ServiceWorkerContextClient is used on multiple threads. It's created
deleted on the main thread, but this registry data member will be bound and used
on the worker thread. Is it OK to delete this member on the main thread? I would
expect that it is not but don't really know.
> 
> Ah, ServiceRegistryImpl class hasa WeakPtrFactory, that all on its own has
thread affinity and shouldn't be deleted on the wrong thread.
> 
> The ServiceWorkerDispatcher class is generally a safer place to store objects
with strong thread affinity like this. The ServiceWorkerDispatcher class is
strictly single threaded (created, used, deleted exclusively on one thread) and
intentionally difficult to reach from the wrong thread.

Hmm, yeah, that's a good point. I'm not sure if ServiceWorkerDispatcher is
necessarily a better fit in this case though. SWDispatcher currently seems to be
used just for the navigator.serviceWorker API; that is, generally the "client
side" service worker API, not for the service worker side of the API. So it
feels a bit wrong to plump through the bits necessary to actually dispatch
events (would it still be ServiceWorkerContextClient::workerContextStarted that
would actually register services with the ServiceRegistry that would then live
in SWDispatcher?).

So I'm not sure what would be more desirable; move the ServiceRegistry instance
to be part of SWDispatcher (while still initializing parts of it from
SWCC::workerContextStarted, since that's where the blink bits that are needed
are available), or keep it as owned by SWDispatcher, just making sure
ServiceRegistry isn't created until workerContextStarted is called and is
deleted by willDestroyWorkerContext.
Or maybe as a third option add a new thread-local EmbeddedWorkerSomething class,
that for now just owns this ServiceRegistry. But that just seems extra
boilerplate for no good reason.

[michaeln, 2015-07-15 22:06:47]

On 2015/07/15 21:45:58, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
> File content/renderer/service_worker/service_worker_context_client.h (right):
> 
>
https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
> content/renderer/service_worker/service_worker_context_client.h:227:
> ServiceRegistryImpl service_registry_;
> On 2015/07/15 at 21:18:52, michaeln wrote:
> > The ServiceWorkerContextClient is used on multiple threads. It's created
> deleted on the main thread, but this registry data member will be bound and
used
> on the worker thread. Is it OK to delete this member on the main thread? I
would
> expect that it is not but don't really know.
> > 
> > Ah, ServiceRegistryImpl class hasa WeakPtrFactory, that all on its own has
> thread affinity and shouldn't be deleted on the wrong thread.
> > 
> > The ServiceWorkerDispatcher class is generally a safer place to store
objects
> with strong thread affinity like this. The ServiceWorkerDispatcher class is
> strictly single threaded (created, used, deleted exclusively on one thread)
and
> intentionally difficult to reach from the wrong thread.
> 
> Hmm, yeah, that's a good point. I'm not sure if ServiceWorkerDispatcher is
> necessarily a better fit in this case though. SWDispatcher currently seems to
be
> used just for the navigator.serviceWorker API; that is, generally the "client
> side" service worker API, not for the service worker side of the API. So it
> feels a bit wrong to plump through the bits necessary to actually dispatch
> events (would it still be ServiceWorkerContextClient::workerContextStarted
that
> would actually register services with the ServiceRegistry that would then live
> in SWDispatcher?).
> 
> So I'm not sure what would be more desirable; move the ServiceRegistry
instance
> to be part of SWDispatcher (while still initializing parts of it from
> SWCC::workerContextStarted, since that's where the blink bits that are needed
> are available), or keep it as owned by SWDispatcher, just making sure
> ServiceRegistry isn't created until workerContextStarted is called and is
> deleted by willDestroyWorkerContext.
> Or maybe as a third option add a new thread-local EmbeddedWorkerSomething
class,
> that for now just owns this ServiceRegistry. But that just seems extra
> boilerplate for no good reason.

Right, the WorkerContextData class looks like it might be the best fit for this
bit of thread-specific data.

> I'm not sure if ServiceWorkerDispatcher is a better fit...
> ServiceWorkerDispatcher just for the navigator.serviceWorker API

That api is availble from within workers too, but your right that it's not
exclusively for use within workers.

[Marijn Kruisselbrink, 2015-07-15 22:34:58]

On 2015/07/15 at 22:06:47, michaeln wrote:
> On 2015/07/15 21:45:58, Marijn Kruisselbrink wrote:
> >
https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
> > File content/renderer/service_worker/service_worker_context_client.h
(right):
> > 
> >
https://codereview.chromium.org/1221503003/diff/120001/content/renderer/servi...
> > content/renderer/service_worker/service_worker_context_client.h:227:
> > ServiceRegistryImpl service_registry_;
> > On 2015/07/15 at 21:18:52, michaeln wrote:
> > > The ServiceWorkerContextClient is used on multiple threads. It's created
> > deleted on the main thread, but this registry data member will be bound and
used
> > on the worker thread. Is it OK to delete this member on the main thread? I
would
> > expect that it is not but don't really know.
> > > 
> > > Ah, ServiceRegistryImpl class hasa WeakPtrFactory, that all on its own has
> > thread affinity and shouldn't be deleted on the wrong thread.
> > > 
> > > The ServiceWorkerDispatcher class is generally a safer place to store
objects
> > with strong thread affinity like this. The ServiceWorkerDispatcher class is
> > strictly single threaded (created, used, deleted exclusively on one thread)
and
> > intentionally difficult to reach from the wrong thread.
> > 
> > Hmm, yeah, that's a good point. I'm not sure if ServiceWorkerDispatcher is
> > necessarily a better fit in this case though. SWDispatcher currently seems
to be
> > used just for the navigator.serviceWorker API; that is, generally the
"client
> > side" service worker API, not for the service worker side of the API. So it
> > feels a bit wrong to plump through the bits necessary to actually dispatch
> > events (would it still be ServiceWorkerContextClient::workerContextStarted
that
> > would actually register services with the ServiceRegistry that would then
live
> > in SWDispatcher?).
> > 
> > So I'm not sure what would be more desirable; move the ServiceRegistry
instance
> > to be part of SWDispatcher (while still initializing parts of it from
> > SWCC::workerContextStarted, since that's where the blink bits that are
needed
> > are available), or keep it as owned by SWDispatcher, just making sure
> > ServiceRegistry isn't created until workerContextStarted is called and is
> > deleted by willDestroyWorkerContext.
> > Or maybe as a third option add a new thread-local EmbeddedWorkerSomething
class,
> > that for now just owns this ServiceRegistry. But that just seems extra
> > boilerplate for no good reason.
> 
> Right, the WorkerContextData class looks like it might be the best fit for
this bit of thread-specific data.

Ah yes, agree that that's where this should live. Done.

[michaeln, 2015-07-15 23:35:00]

lgtm, thanx for being patient with my inane questions

This looks like a reasonable way to setup a per-worker mojo registry pair. How
exactly to best utilize that is a different question? I think you should give
kinuko a chance to chime-in on this before committing.

Not trying to suggest anything more for this cl, just thinking out loud. It'd be
nice to have a browser tests that invasively injects a service and verifies mojo
connectivity, but i'm not sure how that would work? Hmmm, mechanisms that might
make that that were easy to setup in a browsertest could make it easy to setup
connections in the product too? Something like...

ContentBrowserClient::RegisterServiceWorkerMojoServices(ServiceRegistry*
registry) {}
ContentRendererClient::RegisterServiceWorkerMojoServices(ServiceRegistry*
registry) {}

[kinuko, 2015-07-16 15:29:41]

Looks like the discussion between michaeln covered most of questions. lgtm/2
given that we basically agreed on this direction

https://codereview.chromium.org/1221503003/diff/140001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/1221503003/diff/140001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:281:
context_->service_registry.BindRemoteServiceProvider(exposed_services.Pass());
nit: could we have a short helpful comment to note for readers that are not
familiar with mojo that ServiceRegistry queues up connection attempts until this
is called so timing to call this is not that sensitive?

[Marijn Kruisselbrink, 2015-07-16 17:22:02]

mek@chromium.org changed reviewers:
+ tsepez@chromium.org

[Marijn Kruisselbrink, 2015-07-16 17:22:03]

+tsepez for *.mojom OWNERS

[Tom Sepez, 2015-07-16 18:14:51]

lgtm

[Marijn Kruisselbrink, 2015-07-16 18:16:32]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-07-16 18:16:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
kinuko@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1221503003/#ps160001
(title: "add comment to BindServiceRegistry")

[commit-bot: I haz the power, 2015-07-16 18:19:57]

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

[commit-bot: I haz the power, 2015-07-16 18:30:32]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-07-16 18:31:30]

Patchset 8 (id:??) landed as
https://crrev.com/27c9d745b80924002ac4c053fe77f6d9be0f1089
Cr-Commit-Position: refs/heads/master@{#339082}