Hook embedded shell up to MojoShellConnection

Hook embedded shell up to MojoShellConnection

BUG=586314, 589242
Committed: https://crrev.com/19c030e8689cd94f5bc43fcce349e86e4d1ea405
Cr-Commit-Position: refs/heads/master@{#377823}

[Ken Rockot(use gerrit already), 2016-02-25 02:32:06]

rockot@chromium.org changed reviewers:
+ ben@chromium.org

[Ken Rockot(use gerrit already), 2016-02-25 02:32:07]

I still need to add gyp support for mojo runner host. I think it's worth doing.
This is otherwise ready for review.

Note that MOJO_SHELL_CLIENT is changing meaning slightly here; it should be
renamed to MASH_SHELL_CLIENT or MUS_CLIENT or something, because it now only
guards mus-related stuff.

This CL makes it so that content processes always have a MojoShellConnection.
For now native apps are run in-process.

[Ken Rockot(use gerrit already), 2016-02-25 02:50:07]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2016-02-25 02:50:12]

Patchset #1 (id:20001) has been deleted

[Ben Goodger (Google), 2016-02-25 03:02:58]

https://codereview.chromium.org/1738663002/diff/40001/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1738663002/diff/40001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:266: new mojo::shell::ConnectParams);
you must always set a source.

https://codereview.chromium.org/1738663002/diff/40001/mojo/shell/application_...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1738663002/diff/40001/mojo/shell/application_...
mojo/shell/application_manager.h:86: // for the instance.
I'm wondering if we can make this method sound less general purpose and more
specific to this specific use case. i.e.

you are getting a ShellClientRequest for an initialization case where the params
describes the embedder.

InitInstanceForEmbedder()?

you should also ensure this only gets called once.

[Ken Rockot(use gerrit already), 2016-02-25 04:42:05]

https://codereview.chromium.org/1738663002/diff/40001/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1738663002/diff/40001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:266: new mojo::shell::ConnectParams);
On 2016/02/25 at 03:02:58, Ben Goodger (Google) wrote:
> you must always set a source.

Ah right, so it doesn't even really make sense to treat this like a connection.
I've just removed use of ConnectParams.

https://codereview.chromium.org/1738663002/diff/40001/mojo/shell/application_...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1738663002/diff/40001/mojo/shell/application_...
mojo/shell/application_manager.h:86: // for the instance.
On 2016/02/25 at 03:02:58, Ben Goodger (Google) wrote:
> I'm wondering if we can make this method sound less general purpose and more
specific to this specific use case. i.e.
> 
> you are getting a ShellClientRequest for an initialization case where the
params describes the embedder.
> 
> InitInstanceForEmbedder()?
> 
> you should also ensure this only gets called once.

sgtm and done

[Ben Goodger (Google), 2016-02-25 04:53:37]

lgtm

https://codereview.chromium.org/1738663002/diff/100001/mojo/shell/application...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1738663002/diff/100001/mojo/shell/application...
mojo/shell/application_manager.h:85: // |url| is never resolves, there mmust not
have an existing instance for the
please re-read this sentence and fix all the typos.

[Ken Rockot(use gerrit already), 2016-02-25 05:08:24]

https://codereview.chromium.org/1738663002/diff/100001/mojo/shell/application...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1738663002/diff/100001/mojo/shell/application...
mojo/shell/application_manager.h:85: // |url| is never resolves, there mmust not
have an existing instance for the
On 2016/02/25 at 04:53:37, Ben Goodger (Google) wrote:
> please re-read this sentence and fix all the typos.

yikes - fixed

[Ken Rockot(use gerrit already), 2016-02-25 05:16:40]

rockot@chromium.org changed reviewers:
+ dcheng@chromium.org

[Ken Rockot(use gerrit already), 2016-02-25 05:16:41]

+dcheng for trivial renderer_capability_filter.cc change as well as the
registration of some new URI schemes. The mojo shell expects mojo: and exe: URIs
to be interpreted with no authority component. Do you see any risk in
registering these for internal use?

[dcheng, 2016-02-25 18:39:25]

dcheng@chromium.org changed reviewers:
+ creis@chromium.org

[dcheng, 2016-02-25 18:39:26]

Deferring this review to creis@ for the url scheme registration question.

[Ken Rockot(use gerrit already), 2016-02-25 21:19:24]

On 2016/02/25 at 18:39:26, dcheng wrote:
> Deferring this review to creis@ for the url scheme registration question.

Just as a TL;DR for creis@ - We'd like to register "mojo" and "exe" schemes in
content to ensure proper parsing. Any problems with this?

[Charlie Reis, 2016-02-25 22:11:06]

On 2016/02/25 21:19:24, Ken Rockot wrote:
> On 2016/02/25 at 18:39:26, dcheng wrote:
> > Deferring this review to creis@ for the url scheme registration question.
> 
> Just as a TL;DR for creis@ - We'd like to register "mojo" and "exe" schemes in
> content to ensure proper parsing. Any problems with this?

Adding a new scheme is always risky security-wise.  We really need a set of
guidelines for how to do it safely, since it comes up from time to time.  Are
you intending for web pages to be able to link to these schemes?  Or load
resources from them?

Hopefully not, and you could add a test that the new ones return false for
ChildProcessSecurityPolicy::IsWebSafeScheme.

I'm not seeing other immediate concerns with these two schemes.

[Ken Rockot(use gerrit already), 2016-02-25 22:17:48]

Thanks for looking

On 2016/02/25 at 22:11:06, creis wrote:
> On 2016/02/25 21:19:24, Ken Rockot wrote:
> > On 2016/02/25 at 18:39:26, dcheng wrote:
> > > Deferring this review to creis@ for the url scheme registration question.
> > 
> > Just as a TL;DR for creis@ - We'd like to register "mojo" and "exe" schemes
in
> > content to ensure proper parsing. Any problems with this?
> 
> Adding a new scheme is always risky security-wise.  We really need a set of
guidelines for how to do it safely, since it comes up from time to time.  Are
you intending for web pages to be able to link to these schemes?  Or load
resources from them?

Definitely not. Usage of these schemes is strictly internal to Chrome.

> 
> Hopefully not, and you could add a test that the new ones return false for
ChildProcessSecurityPolicy::IsWebSafeScheme.

Done.

> 
> I'm not seeing other immediate concerns with these two schemes.

[Ben Goodger (Google), 2016-02-25 22:54:14]

On 2016/02/25 22:11:06, Charlie Reis (slow til Mar 7) wrote:
> On 2016/02/25 21:19:24, Ken Rockot wrote:
> > On 2016/02/25 at 18:39:26, dcheng wrote:
> > > Deferring this review to creis@ for the url scheme registration question.
> > 
> > Just as a TL;DR for creis@ - We'd like to register "mojo" and "exe" schemes
in
> > content to ensure proper parsing. Any problems with this?
> 
> Adding a new scheme is always risky security-wise.  We really need a set of
> guidelines for how to do it safely, since it comes up from time to time.  Are
> you intending for web pages to be able to link to these schemes?  Or load
> resources from them?
> 
> Hopefully not, and you could add a test that the new ones return false for
> ChildProcessSecurityPolicy::IsWebSafeScheme.
> 
> I'm not seeing other immediate concerns with these two schemes.

BTW, these are not really "URLs". We've been using URLs as identifiers to Mojo
services, but now we never expect to pass anything other than mojo: or exe: to
Connect(). As such, I want to change the shell to treat these as structured
strings (type:name) instead of URLs. This simplifies the service side quite a
bit, and means we'll be able to get rid of this registration step.

[Charlie Reis, 2016-02-25 22:59:49]

On 2016/02/25 22:54:14, Ben Goodger (Google) wrote:
> On 2016/02/25 22:11:06, Charlie Reis (slow til Mar 7) wrote:
> > On 2016/02/25 21:19:24, Ken Rockot wrote:
> > > On 2016/02/25 at 18:39:26, dcheng wrote:
> > > > Deferring this review to creis@ for the url scheme registration
question.
> > > 
> > > Just as a TL;DR for creis@ - We'd like to register "mojo" and "exe"
schemes
> in
> > > content to ensure proper parsing. Any problems with this?
> > 
> > Adding a new scheme is always risky security-wise.  We really need a set of
> > guidelines for how to do it safely, since it comes up from time to time. 
Are
> > you intending for web pages to be able to link to these schemes?  Or load
> > resources from them?
> > 
> > Hopefully not, and you could add a test that the new ones return false for
> > ChildProcessSecurityPolicy::IsWebSafeScheme.
> > 
> > I'm not seeing other immediate concerns with these two schemes.
> 
> BTW, these are not really "URLs". We've been using URLs as identifiers to Mojo
> services, but now we never expect to pass anything other than mojo: or exe: to
> Connect(). As such, I want to change the shell to treat these as structured
> strings (type:name) instead of URLs. This simplifies the service side quite a
> bit, and means we'll be able to get rid of this registration step.

So then you wouldn't need the new scheme registrations?  That would be great. 
Can we add a TODO to remove these when that happens (unless it's easy to do that
first and skip this entirely)?

To be clear, I don't see any concrete problems with adding them, but there's
lots of places where the code checks which schemes are allowed or not (in a
variety of ways), so adding new ones can have some unexpected implications.

[Ken Rockot(use gerrit already), 2016-02-25 23:03:19]

On 2016/02/25 at 22:59:49, creis wrote:
> On 2016/02/25 22:54:14, Ben Goodger (Google) wrote:
> > On 2016/02/25 22:11:06, Charlie Reis (slow til Mar 7) wrote:
> > > On 2016/02/25 21:19:24, Ken Rockot wrote:
> > > > On 2016/02/25 at 18:39:26, dcheng wrote:
> > > > > Deferring this review to creis@ for the url scheme registration
question.
> > > > 
> > > > Just as a TL;DR for creis@ - We'd like to register "mojo" and "exe"
schemes
> > in
> > > > content to ensure proper parsing. Any problems with this?
> > > 
> > > Adding a new scheme is always risky security-wise.  We really need a set
of
> > > guidelines for how to do it safely, since it comes up from time to time. 
Are
> > > you intending for web pages to be able to link to these schemes?  Or load
> > > resources from them?
> > > 
> > > Hopefully not, and you could add a test that the new ones return false for
> > > ChildProcessSecurityPolicy::IsWebSafeScheme.
> > > 
> > > I'm not seeing other immediate concerns with these two schemes.
> > 
> > BTW, these are not really "URLs". We've been using URLs as identifiers to
Mojo
> > services, but now we never expect to pass anything other than mojo: or exe:
to
> > Connect(). As such, I want to change the shell to treat these as structured
> > strings (type:name) instead of URLs. This simplifies the service side quite
a
> > bit, and means we'll be able to get rid of this registration step.
> 
> So then you wouldn't need the new scheme registrations?  That would be great. 
Can we add a TODO to remove these when that happens (unless it's easy to do that
first and skip this entirely)?
> 

TODO added

> To be clear, I don't see any concrete problems with adding them, but there's
lots of places where the code checks which schemes are allowed or not (in a
variety of ways), so adding new ones can have some unexpected implications.

ack

[Charlie Reis, 2016-02-25 23:12:47]

On 2016/02/25 23:03:19, Ken Rockot wrote:
> On 2016/02/25 at 22:59:49, creis wrote:
> > On 2016/02/25 22:54:14, Ben Goodger (Google) wrote:
> > > On 2016/02/25 22:11:06, Charlie Reis (slow til Mar 7) wrote:
> > > > On 2016/02/25 21:19:24, Ken Rockot wrote:
> > > > > On 2016/02/25 at 18:39:26, dcheng wrote:
> > > > > > Deferring this review to creis@ for the url scheme registration
> question.
> > > > > 
> > > > > Just as a TL;DR for creis@ - We'd like to register "mojo" and "exe"
> schemes
> > > in
> > > > > content to ensure proper parsing. Any problems with this?
> > > > 
> > > > Adding a new scheme is always risky security-wise.  We really need a set
> of
> > > > guidelines for how to do it safely, since it comes up from time to time.

> Are
> > > > you intending for web pages to be able to link to these schemes?  Or
load
> > > > resources from them?
> > > > 
> > > > Hopefully not, and you could add a test that the new ones return false
for
> > > > ChildProcessSecurityPolicy::IsWebSafeScheme.
> > > > 
> > > > I'm not seeing other immediate concerns with these two schemes.
> > > 
> > > BTW, these are not really "URLs". We've been using URLs as identifiers to
> Mojo
> > > services, but now we never expect to pass anything other than mojo: or
exe:
> to
> > > Connect(). As such, I want to change the shell to treat these as
structured
> > > strings (type:name) instead of URLs. This simplifies the service side
quite
> a
> > > bit, and means we'll be able to get rid of this registration step.
> > 
> > So then you wouldn't need the new scheme registrations?  That would be
great. 
> Can we add a TODO to remove these when that happens (unless it's easy to do
that
> first and skip this entirely)?
> > 
> 
> TODO added
> 
> > To be clear, I don't see any concrete problems with adding them, but there's
> lots of places where the code checks which schemes are allowed or not (in a
> variety of ways), so adding new ones can have some unexpected implications.
> 
> ack

Thanks.  New temporary schemes LGTM.  Back to dcheng@ for
renderer_capability_filter.cc.

[dcheng, 2016-02-25 23:13:53]

lgtm

[Ken Rockot(use gerrit already), 2016-02-26 03:29:44]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-02-26 03:29:45]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org,
creis@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1738663002/#ps320001
(title: "fix stupid gyp typo")

[commit-bot: I haz the power, 2016-02-26 03:30:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738663002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738663002/320001

[commit-bot: I haz the power, 2016-02-26 04:09:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-26 04:09:23]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Ken Rockot(use gerrit already), 2016-02-26 04:31:06]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-02-26 04:31:07]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
ben@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1738663002/#ps340001
(title: " ")

[commit-bot: I haz the power, 2016-02-26 04:31:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738663002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738663002/340001

[commit-bot: I haz the power, 2016-02-26 05:47:08]

Committed patchset #16 (id:340001)

[commit-bot: I haz the power, 2016-02-26 05:49:26]

Description was changed from

==========
Hook embedded shell up to MojoShellConnection

BUG=586314,589242
==========

to

==========
Hook embedded shell up to MojoShellConnection

BUG=586314,589242

Committed: https://crrev.com/19c030e8689cd94f5bc43fcce349e86e4d1ea405
Cr-Commit-Position: refs/heads/master@{#377823}
==========

[commit-bot: I haz the power, 2016-02-26 05:49:28]

Patchset 16 (id:??) landed as
https://crrev.com/19c030e8689cd94f5bc43fcce349e86e4d1ea405
Cr-Commit-Position: refs/heads/master@{#377823}