Renderers should obtain browser InterfaceProvider by connecting to browser

Before this change, renderers use the incoming InterfaceProvider on the connection that launched them. This is problematic as the interfaces exposed over this connection are not filtered by service manager capability policy.

With this change, renderers connect back to the browser & request interfaces. Currently we expose all interfaces (short term hack). This will be whitelisted shortly.

R=rockot@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5be0b9131772f071eebd3028f1d85d7d0a82705e
Cr-Commit-Position: refs/heads/master@{#409395}

[Ben Goodger (Google), 2016-07-27 19:58:30]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 19:58:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 20:20:30]

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

[commit-bot: I haz the power, 2016-07-27 20:20:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ben Goodger (Google), 2016-07-27 21:05:28]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 21:05:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 21:23:46]

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

[commit-bot: I haz the power, 2016-07-27 21:23:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2016-07-27 23:30:45]

Description was changed from

==========
RPH

BUG=
==========

to

==========
RPH

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ben Goodger (Google), 2016-07-28 23:29:59]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 23:30:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 23:40:14]

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

[commit-bot: I haz the power, 2016-07-28 23:40:15]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Ben Goodger (Google), 2016-07-29 04:20:58]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 04:21:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 04:37:17]

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

[commit-bot: I haz the power, 2016-07-29 04:37:18]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Ben Goodger (Google), 2016-07-29 04:50:15]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 04:50:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 05:05:36]

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

[commit-bot: I haz the power, 2016-07-29 05:05:38]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Ben Goodger (Google), 2016-07-29 05:23:51]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 05:24:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 05:51:23]

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

[commit-bot: I haz the power, 2016-07-29 05:51:23]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ben Goodger (Google), 2016-07-29 05:56:36]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 05:56:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-07-29 06:01:16]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 06:01:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-07-29 06:03:52]

Description was changed from

==========
RPH

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Before this change, renderers use the incoming InterfaceProvider on the
connection that launched them. This is problematic as the interfaces exposed
over this connection are not filtered by service manager capability policy.

With this change, renderers connect back to the browser & request interfaces.
Currently we expose all interfaces (short term hack). This will be whitelisted
shortly.

R=rockot@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ben Goodger (Google), 2016-07-29 06:03:52]

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

[commit-bot: I haz the power, 2016-07-29 07:13:38]

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

[commit-bot: I haz the power, 2016-07-29 07:13:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-07-29 17:09:46]

https://codereview.chromium.org/2183703005/diff/240001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_content_browser_client.cc (right):

https://codereview.chromium.org/2183703005/diff/240001/blimp/engine/app/blimp...
blimp/engine/app/blimp_content_browser_client.cc:52: ui_task_runner);
nit: I'd rather just use base::ThreadTaskRunnerHandle::Get(), but BrowserThread
is certainly more explicit. Your call.

I also suspect callers will be surprised that these callbacks run on the IO
thread by default - i.e. with no TaskRunner provided - given that we're binding
them on the UI thread. We should document this clearly in ContentBrowserClient.

https://codereview.chromium.org/2183703005/diff/240001/content/browser/androi...
File content/browser/android/interface_registry_android_impl.cc (right):

https://codereview.chromium.org/2183703005/diff/240001/content/browser/androi...
content/browser/android/interface_registry_android_impl.cc:91: // All Java
services must be bound on the main thread.
nit: s/services/interfaces/

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:482: if
(child_identity_.name() != remote_identity.name() ||
Could we just add a != operator to Identity? Shouldn't the user_id also be
checked?

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:493:
base::Unretained(this),
This is not safe. The registered callbacks may call into the
ConnectionFilterImpl as long as the Connection (and thus the IOThreadContext of
the MojoChildConnection) is alive. That can live arbitrarily longer than the
RPHI which created it, since it lives on the IO thread.

I think we need to use WeakPtrs here.

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:800:
std::unique_ptr<shell::InterfaceRegistry> registry(
Can't you just put all this junk in RegisterMojoInterfaces() with no argument?

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1120: ui_task_runner);
It's pretty unfortunate that we have to add ui_task_runner arguments to so many
AddInterface calls. I guess I don't have any better ideas for now.

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1167:
registry->AddInterface(base::Bind(&DeviceLightHost::Create), io_task_runner);
io_task_runner isn't necessary now, it just needlessly defers the binding

https://codereview.chromium.org/2183703005/diff/240001/content/child/child_th...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/2183703005/diff/240001/content/child/child_th...
content/child/child_thread_impl.cc:451: "exe:content_browser");
We should really move content/browser/mojo/constants.* to
content/common/mojo/constants.* and you should use kBrowserMojoApplicationName
here.

https://codereview.chromium.org/2183703005/diff/240001/content/child/child_th...
content/child/child_thread_impl.cc:650: if (connect_to_browser_)
Or you could get rid of connect_to_browser_ as a member and just test
browser_connection_ here

https://codereview.chromium.org/2183703005/diff/240001/content/common/mojo/mo...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/2183703005/diff/240001/content/common/mojo/mo...
content/common/mojo/mojo_shell_connection_impl.cc:89: //DCHECK(!started_);
Remove

https://codereview.chromium.org/2183703005/diff/240001/content/common/mojo/mo...
content/common/mojo/mojo_shell_connection_impl.cc:404: if (!provider)
Why would this be null? Tests?

https://codereview.chromium.org/2183703005/diff/240001/content/public/app/moj...
File content/public/app/mojo/content_renderer_manifest.json (right):

https://codereview.chromium.org/2183703005/diff/240001/content/public/app/moj...
content/public/app/mojo/content_renderer_manifest.json:10:
"exe:content_browser": {
nit: Maybe we should file a bug + add a TODO to narrow the scope of the
renderer's required interfaces, to make the intent clear?

[Ben Goodger (Google), 2016-07-29 18:31:31]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 18:32:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 18:48:04]

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

[commit-bot: I haz the power, 2016-07-29 18:48:05]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ben Goodger (Google), 2016-07-29 19:11:19]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 19:12:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-07-29 19:19:41]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 19:20:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-07-29 19:35:02]

Updated PTAL

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:482: if
(child_identity_.name() != remote_identity.name() ||
On 2016/07/29 17:09:45, Ken Rockot wrote:
> Could we just add a != operator to Identity? Shouldn't the user_id also be
> checked?

Can't use user_id since user_id for outbound connections is kInheritUserId

https://codereview.chromium.org/2183703005/diff/240001/content/common/mojo/mo...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/2183703005/diff/240001/content/common/mojo/mo...
content/common/mojo/mojo_shell_connection_impl.cc:404: if (!provider)
On 2016/07/29 17:09:46, Ken Rockot wrote:
> Why would this be null? Tests?

In the case I've just added. We use the IP from the outgoing connection, not the
dummy passed to this method.

[commit-bot: I haz the power, 2016-07-29 19:41:20]

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

[commit-bot: I haz the power, 2016-07-29 19:41:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ben Goodger (Google), 2016-07-29 19:50:33]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 19:51:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 20:12:17]

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

[commit-bot: I haz the power, 2016-07-29 20:12:18]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Ben Goodger (Google), 2016-07-29 20:58:44]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 20:59:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-07-29 21:46:32]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-29 21:46:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-07-29 21:50:42]

Updated again.

On Fri, Jul 29, 2016 at 12:35 PM, <ben@chromium.org> wrote:

> Updated PTAL
>
>
>
>
https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
>
>
>
https://codereview.chromium.org/2183703005/diff/240001/content/browser/render...
> content/browser/renderer_host/render_process_host_impl.cc:482: if
> (child_identity_.name() != remote_identity.name() ||
> On 2016/07/29 17:09:45, Ken Rockot wrote:
> > Could we just add a != operator to Identity? Shouldn't the user_id
> also be
> > checked?
>
> Can't use user_id since user_id for outbound connections is
> kInheritUserId
>
>
>
https://codereview.chromium.org/2183703005/diff/240001/content/common/mojo/mo...
> File content/common/mojo/mojo_shell_connection_impl.cc (right):
>
>
>
https://codereview.chromium.org/2183703005/diff/240001/content/common/mojo/mo...
> content/common/mojo/mojo_shell_connection_impl.cc:404: if (!provider)
> On 2016/07/29 17:09:46, Ken Rockot wrote:
> > Why would this be null? Tests?
>
> In the case I've just added. We use the IP from the outgoing connection,
> not the dummy passed to this method.
>
> https://codereview.chromium.org/2183703005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-07-29 23:14:07]

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

[commit-bot: I haz the power, 2016-07-29 23:14:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-07-29 23:20:47]

lgtm

https://codereview.chromium.org/2183703005/diff/200002/content/common/mojo/mo...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/2183703005/diff/200002/content/common/mojo/mo...
content/common/mojo/mojo_shell_connection_impl.cc:96: for (auto it =
connection_filters_.begin(); it != connection_filters_.end();
This needs to hold lock_ too

[Ben Goodger (Google), 2016-07-29 23:23:28]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-07-29 23:23:28]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2183703005/#ps370001
(title: ".")

[commit-bot: I haz the power, 2016-07-29 23:23:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 00:02:52]

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

[commit-bot: I haz the power, 2016-07-30 00:02:54]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ben Goodger (Google), 2016-08-02 18:17:06]

The CQ bit was checked by ben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 18:17:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-08-02 19:07:57]

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

[Ben Goodger (Google), 2016-08-02 19:08:59]

boliu: android_webview/* changes to browser context as discussed.

[boliu, 2016-08-02 19:22:55]

lgtm % nit

https://codereview.chromium.org/2183703005/diff/390001/android_webview/browse...
File android_webview/browser/aw_browser_main_parts.h (right):

https://codereview.chromium.org/2183703005/diff/390001/android_webview/browse...
android_webview/browser/aw_browser_main_parts.h:39: AwBrowserContext*
browser_context_ = nullptr;
can be removed?

[Ben Goodger (Google), 2016-08-02 20:17:53]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-08-02 20:17:54]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2183703005/#ps410001
(title: ".")

[commit-bot: I haz the power, 2016-08-02 20:18:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ben Goodger (Google), 2016-08-02 22:29:35]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-08-02 22:29:36]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2183703005/#ps430001
(title: ".")

[commit-bot: I haz the power, 2016-08-02 22:30:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 00:17:36]

Description was changed from

==========
Before this change, renderers use the incoming InterfaceProvider on the
connection that launched them. This is problematic as the interfaces exposed
over this connection are not filtered by service manager capability policy.

With this change, renderers connect back to the browser & request interfaces.
Currently we expose all interfaces (short term hack). This will be whitelisted
shortly.

R=rockot@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Before this change, renderers use the incoming InterfaceProvider on the
connection that launched them. This is problematic as the interfaces exposed
over this connection are not filtered by service manager capability policy.

With this change, renderers connect back to the browser & request interfaces.
Currently we expose all interfaces (short term hack). This will be whitelisted
shortly.

R=rockot@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-08-03 00:17:40]

Committed patchset #23 (id:430001)

[commit-bot: I haz the power, 2016-08-03 00:20:38]

Description was changed from

==========
Before this change, renderers use the incoming InterfaceProvider on the
connection that launched them. This is problematic as the interfaces exposed
over this connection are not filtered by service manager capability policy.

With this change, renderers connect back to the browser & request interfaces.
Currently we expose all interfaces (short term hack). This will be whitelisted
shortly.

R=rockot@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Before this change, renderers use the incoming InterfaceProvider on the
connection that launched them. This is problematic as the interfaces exposed
over this connection are not filtered by service manager capability policy.

With this change, renderers connect back to the browser & request interfaces.
Currently we expose all interfaces (short term hack). This will be whitelisted
shortly.

R=rockot@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5be0b9131772f071eebd3028f1d85d7d0a82705e
Cr-Commit-Position: refs/heads/master@{#409395}
==========

[commit-bot: I haz the power, 2016-08-03 00:20:39]

Patchset 23 (id:??) landed as
https://crrev.com/5be0b9131772f071eebd3028f1d85d7d0a82705e
Cr-Commit-Position: refs/heads/master@{#409395}