Perform InterfaceProviderSpec intersection in the ServiceManager

Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext, this just calls through to OnConnect still since updating callsites is a pretty large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy way to avoid this without adding an unsafe mode to InterfaceRegistry that skips the check that I'd be concerned might be misused. And IR should probably just die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2610853013
Cr-Original-Commit-Position: refs/heads/master@{#443119}
Committed: https://chromium.googlesource.com/chromium/src/+/e70e635ab325eede70d564b995130583bb3404ce
Review-Url: https://codereview.chromium.org/2610853013
Cr-Commit-Position: refs/heads/master@{#444496}
Committed: https://chromium.googlesource.com/chromium/src/+/5079d8944bb80c049f8ecab6128cb71b04c245a7

[Ben Goodger (Google), 2017-01-09 21:38:24]

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

[commit-bot: I haz the power, 2017-01-09 21:38:53]

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), 2017-01-09 21:50:16]

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

[commit-bot: I haz the power, 2017-01-09 21:50: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), 2017-01-09 21:54:23]

Description was changed from

==========
Perform InterfaceProviderSpec intersection in the ServiceManager

R=
==========

to

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 
. Adds a mode to InterfaceRegistry that forces it to bind all incoming interface
requests. Because we're still relying on client lib's OnConnect, clients are
still using InterfaceRegistry & were relying on local policy checking to work.
This needs to be explicitly disabled in this case since it's done in the service
manager.

R=
==========

[Ben Goodger (Google), 2017-01-09 22:02:12]

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

[Ben Goodger (Google), 2017-01-09 22:03:28]

Description was changed from

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 
. Adds a mode to InterfaceRegistry that forces it to bind all incoming interface
requests. Because we're still relying on client lib's OnConnect, clients are
still using InterfaceRegistry & were relying on local policy checking to work.
This needs to be explicitly disabled in this case since it's done in the service
manager.

R=
==========

to

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy
way to avoid this without adding an unsafe mode to InterfaceRegistry that skips
the check that I'd be concerned might be misused. And IR should probably just
die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org
==========

[Ben Goodger (Google), 2017-01-09 22:03:28]

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

[commit-bot: I haz the power, 2017-01-09 22:03:45]

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), 2017-01-09 22:04:33]

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

[commit-bot: I haz the power, 2017-01-09 22:05:02]

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, 2017-01-09 23:55:07]

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

[commit-bot: I haz the power, 2017-01-09 23:55:08]

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), 2017-01-10 00:49:31]

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

[commit-bot: I haz the power, 2017-01-10 00:50:20]

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), 2017-01-10 00:55:44]

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

[commit-bot: I haz the power, 2017-01-10 00:56:39]

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, 2017-01-10 02:58:33]

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

[commit-bot: I haz the power, 2017-01-10 02:58:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Ben Goodger (Google), 2017-01-10 20:20:35]

poke

On Mon, Jan 9, 2017 at 2:06 PM, <ben@chromium.org> wrote:

> Reviewers: Ken Rockot, Tom Sepez
> CL: https://codereview.chromium.org/2610853013/
>
> Description:
> Perform InterfaceProviderSpec intersection in the ServiceManager.
>
> . Introduces a parallel flow in ServiceManager to Connect called
> BindInterface
> which carries (via ConnectParams) interface name and pipe.
> . Introduces an OnBindInterface() method to mojom::Service. In
> ServiceContext,
> this just calls through to OnConnect still since updating callsites is a
> pretty
> large task for a subsequent change.
>
> Note that policy checking is still happening in the client - there was no
> easy
> way to avoid this without adding an unsafe mode to InterfaceRegistry that
> skips
> the check that I'd be concerned might be misused. And IR should probably
> just
> die once this work is complete anyway.
>
> R=rockot@chromium.org,tsepez@chromium.org
>
>
> Affected files (+261, -60 lines):
> M services/service_manager/connect_params.h
> M services/service_manager/public/cpp/lib/service_context.cc
> M services/service_manager/public/cpp/service_context.h
> M services/service_manager/public/interfaces/service.mojom
> M services/service_manager/service_manager.cc
>
>
>

-- 
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.

[Ken Rockot(use gerrit already), 2017-01-10 21:12:30]

lgtm. It's pretty confusing that ConnectParams is used for StartService and
BindInterface. Not necessary here but I think I'd prefer we just flatten the
params into the various method arguments and get rid of ConnectParams
altogether.

[Tom Sepez, 2017-01-10 21:30:45]

lgtm

[Ben Goodger (Google), 2017-01-11 00:31:09]

On 2017/01/10 21:12:30, Ken Rockot wrote:
> lgtm. It's pretty confusing that ConnectParams is used for StartService and
> BindInterface. Not necessary here but I think I'd prefer we just flatten the
> params into the various method arguments and get rid of ConnectParams
> altogether.

I agree. Once I kill the OnConnect variant this will be easier.

[Ben Goodger (Google), 2017-01-11 00:31:18]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2017-01-11 00:32:30]

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, 2017-01-11 01:28:44]

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

[commit-bot: I haz the power, 2017-01-11 01:28:45]

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), 2017-01-11 20:40:40]

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

[commit-bot: I haz the power, 2017-01-11 20:41:02]

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, 2017-01-11 21:58:41]

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

[commit-bot: I haz the power, 2017-01-11 21:58:42]

Dry run: This issue passed the CQ dry run.

[Ben Goodger (Google), 2017-01-12 00:16:36]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2017-01-12 00:16:36]

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

[commit-bot: I haz the power, 2017-01-12 00:17:19]

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, 2017-01-12 02:29:38]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484180196003980,
"parent_rev": "e62b61b8ae8d5679c3433e616ba0c88f01d0592c", "commit_rev":
"e70e635ab325eede70d564b995130583bb3404ce"}

[commit-bot: I haz the power, 2017-01-12 02:30:05]

Description was changed from

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy
way to avoid this without adding an unsafe mode to InterfaceRegistry that skips
the check that I'd be concerned might be misused. And IR should probably just
die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org
==========

to

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy
way to avoid this without adding an unsafe mode to InterfaceRegistry that skips
the check that I'd be concerned might be misused. And IR should probably just
die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2610853013
Cr-Commit-Position: refs/heads/master@{#443119}
Committed:
https://chromium.googlesource.com/chromium/src/+/e70e635ab325eede70d564b99513...
==========

[commit-bot: I haz the power, 2017-01-12 02:30:06]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/e70e635ab325eede70d564b99513...

[Ken Rockot(use gerrit already), 2017-01-12 23:05:00]

https://codereview.chromium.org/2610853013/diff/220001/services/service_manag...
File services/service_manager/service_manager.cc (right):

https://codereview.chromium.org/2610853013/diff/220001/services/service_manag...
services/service_manager/service_manager.cc:168: if (!service_.is_bound())
We need to run the response callback in this case. I suppose we just might never
(or rarely) hit this branch in OnConnect and hence that hasn't been triggering a
DCHECK similar to http://crbug.com/680700).

[James Cook, 2017-01-13 02:22:48]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2634493002/ by jamescook@chromium.org.

The reason for reverting is: This CL is causing chrome --mash to fail to start
up on Chromebook hardware. See
https://bugs.chromium.org/p/chromium/issues/detail?id=680700

I'm not sure why our existing test coverage doesn't catch this. Let me know if I
can help manually test a future patch.
.

[Ben Goodger (Google), 2017-01-13 17:59:35]

On 2017/01/12 23:05:00, Ken Rockot wrote:
>
https://codereview.chromium.org/2610853013/diff/220001/services/service_manag...
> File services/service_manager/service_manager.cc (right):
> 
>
https://codereview.chromium.org/2610853013/diff/220001/services/service_manag...
> services/service_manager/service_manager.cc:168: if (!service_.is_bound())
> We need to run the response callback in this case. I suppose we just might
never
> (or rarely) hit this branch in OnConnect and hence that hasn't been triggering
a
> DCHECK similar to http://crbug.com/680700).

Ah yes I wondered about this. I figured since it wasn't called during the
OnConnect flow I didn't understand under what conditions it'd be needed :-)

[Ben Goodger (Google), 2017-01-13 18:00:09]

On 2017/01/13 17:59:35, Ben Goodger (Google) wrote:
> On 2017/01/12 23:05:00, Ken Rockot wrote:
> >
>
https://codereview.chromium.org/2610853013/diff/220001/services/service_manag...
> > File services/service_manager/service_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/2610853013/diff/220001/services/service_manag...
> > services/service_manager/service_manager.cc:168: if (!service_.is_bound())
> > We need to run the response callback in this case. I suppose we just might
> never
> > (or rarely) hit this branch in OnConnect and hence that hasn't been
triggering
> a
> > DCHECK similar to http://crbug.com/680700).
> 
> Ah yes I wondered about this. I figured since it wasn't called during the
> OnConnect flow I didn't understand under what conditions it'd be needed :-)

Do you run tests with chrome --mash? or just mash_unittests? I guess there's
mash browsertests?

[Ben Goodger (Google), 2017-01-18 20:27:18]

Description was changed from

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy
way to avoid this without adding an unsafe mode to InterfaceRegistry that skips
the check that I'd be concerned might be misused. And IR should probably just
die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2610853013
Cr-Commit-Position: refs/heads/master@{#443119}
Committed:
https://chromium.googlesource.com/chromium/src/+/e70e635ab325eede70d564b99513...
==========

to

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy
way to avoid this without adding an unsafe mode to InterfaceRegistry that skips
the check that I'd be concerned might be misused. And IR should probably just
die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2610853013
Cr-Commit-Position: refs/heads/master@{#443119}
Committed:
https://chromium.googlesource.com/chromium/src/+/e70e635ab325eede70d564b99513...
==========

[Ben Goodger (Google), 2017-01-18 20:38:35]

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

[commit-bot: I haz the power, 2017-01-18 20:39:09]

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), 2017-01-18 20:48:05]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2017-01-18 20:48:05]

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

[commit-bot: I haz the power, 2017-01-18 20:48:42]

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, 2017-01-18 21:49:00]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1484772485122760,
"parent_rev": "e6bef6ebd4103140f0e052dd4c3bd3f86e3717a1", "commit_rev":
"5079d8944bb80c049f8ecab6128cb71b04c245a7"}

[commit-bot: I haz the power, 2017-01-18 21:49:32]

Description was changed from

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy
way to avoid this without adding an unsafe mode to InterfaceRegistry that skips
the check that I'd be concerned might be misused. And IR should probably just
die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2610853013
Cr-Commit-Position: refs/heads/master@{#443119}
Committed:
https://chromium.googlesource.com/chromium/src/+/e70e635ab325eede70d564b99513...
==========

to

==========
Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface
which carries (via ConnectParams) interface name and pipe. 
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext,
this just calls through to OnConnect still since updating callsites is a pretty
large task for a subsequent change. 

Note that policy checking is still happening in the client - there was no easy
way to avoid this without adding an unsafe mode to InterfaceRegistry that skips
the check that I'd be concerned might be misused. And IR should probably just
die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2610853013
Cr-Original-Commit-Position: refs/heads/master@{#443119}
Committed:
https://chromium.googlesource.com/chromium/src/+/e70e635ab325eede70d564b99513...
Review-Url: https://codereview.chromium.org/2610853013
Cr-Commit-Position: refs/heads/master@{#444496}
Committed:
https://chromium.googlesource.com/chromium/src/+/5079d8944bb80c049f8ecab6128c...
==========

[commit-bot: I haz the power, 2017-01-18 21:49:33]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/5079d8944bb80c049f8ecab6128c...