Convert UtilityThread/Clients to add ConnectionFilters instead of using ChildThread's InterfaceRegi…

Convert UtilityThread/Clients to add ConnectionFilters instead of using ChildThread's InterfaceRegistry.

R=rockot@chromium.org

Review-Url: https://codereview.chromium.org/2810343002
Cr-Commit-Position: refs/heads/master@{#465377}
Committed: https://chromium.googlesource.com/chromium/src/+/7f9189cb86fdc65ca77995d28ee9604a3c5a8312

[Ben Goodger (Google), 2017-04-12 22:29:13]

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

[commit-bot: I haz the power, 2017-04-12 22:29:49]

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-04-12 22:48:27]

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

[commit-bot: I haz the power, 2017-04-12 22:48:28]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ben Goodger (Google), 2017-04-13 03:48:18]

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

[commit-bot: I haz the power, 2017-04-13 03:48:40]

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-04-13 04:03:03]

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

[commit-bot: I haz the power, 2017-04-13 04:03:04]

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...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2017-04-13 04:27:02]

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

[commit-bot: I haz the power, 2017-04-13 04:27: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, 2017-04-13 04:31:58]

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

[commit-bot: I haz the power, 2017-04-13 04:31:59]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ben Goodger (Google), 2017-04-13 04:59:50]

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

[commit-bot: I haz the power, 2017-04-13 05:00:13]

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-04-13 05:11:01]

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

[commit-bot: I haz the power, 2017-04-13 05:11:02]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ben Goodger (Google), 2017-04-13 05:24:07]

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

[commit-bot: I haz the power, 2017-04-13 05:24:27]

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-04-13 05:24:51]

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

[commit-bot: I haz the power, 2017-04-13 05:25: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, 2017-04-13 06:12:38]

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

[commit-bot: I haz the power, 2017-04-13 06:12:39]

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-04-14 20:44:43]

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

[commit-bot: I haz the power, 2017-04-14 20:45:02]

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-04-14 22:40:34]

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

[commit-bot: I haz the power, 2017-04-14 22:41:00]

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-04-14 22:42:11]

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

[commit-bot: I haz the power, 2017-04-14 22:42:30]

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-04-14 22:42:40]

Description was changed from

==========
Convert UtilityThread/Clients to add ConnectionFilters instead of using
ChildThread's InterfaceRegistry.

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

to

==========
Convert UtilityThread/Clients to add ConnectionFilters instead of using
ChildThread's InterfaceRegistry.

R=rockot@chromium.org
==========

[Ben Goodger (Google), 2017-04-14 22:42:40]

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

[commit-bot: I haz the power, 2017-04-15 00:06:27]

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

[commit-bot: I haz the power, 2017-04-15 00:06:28]

Dry run: This issue passed the CQ dry run.

[Ben Goodger (Google), 2017-04-16 04:57:37]

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

[commit-bot: I haz the power, 2017-04-16 04:57:44]

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-04-16 06:03:03]

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

[commit-bot: I haz the power, 2017-04-16 06:03:04]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-04-18 18:05:49]

Yeah, it's racy. The proxying InterfaceRegistry setup is safe because all
incoming interface requests are always forwarded to the main thread by default.

Interface requests may be received on the IO thread as soon as
ChildThreadImpl::StartServiceManagerConnection is run. By the time this happens
today, we've already registered all the binders.

With this CL, ChildThreadImpl::Init will complete, having "started" the SMC, and
then UtilityThreadImpl::Init will run and add the ConnectionFilter. In this
window it's possible (and probably even likely) for interface requests to race
with the ConnectionFilter addition.

The good news is, there's already a ChildThreadOption to disable automatic
StartServiceManagerConnection. You can use it here and then manually call
StartServiceManagerConnection in UtilityThreadImpl::Init, after you've added the
filter.

https://codereview.chromium.org/2810343002/diff/180001/content/public/common/...
File content/public/common/connection_filter_impl.h (right):

https://codereview.chromium.org/2810343002/diff/180001/content/public/common/...
content/public/common/connection_filter_impl.h:20: class CONTENT_EXPORT
ConnectionFilterImpl : public ConnectionFilter {
It seems a little weird and unconventional to have a Foo in content/public along
with a corresponding FooImpl also in content/public, especially when it's just
one kind of Foo and not *the* Foo impl.

I would either change this to something like SimpleConnectionFilter or - since
this doesn't do anything but use a BinderRegistry anyway and doesn't use other
args - just allow BinderRegistry instances to be added to the
ServiceManagerConnection similarly to how ConnectionFilter is used.

I'd probably prefer the first option since it's less new interface

[Ken Rockot(use gerrit already), 2017-04-18 18:06:38]

On 2017/04/18 at 18:05:49, Ken Rockot (OOO til Apr 18) wrote:
> Yeah, it's racy. The proxying InterfaceRegistry setup is safe because all
incoming interface requests are always forwarded to the main thread by default.
> 
> Interface requests may be received on the IO thread as soon as
ChildThreadImpl::StartServiceManagerConnection is run. By the time this happens
today, we've already registered all the binders.

Or rather, by the time this happens today, we've already registered the
forwarding logic, and its tasks won't execute until we've registered all the
binders.

> 
> With this CL, ChildThreadImpl::Init will complete, having "started" the SMC,
and then UtilityThreadImpl::Init will run and add the ConnectionFilter. In this
window it's possible (and probably even likely) for interface requests to race
with the ConnectionFilter addition.
> 
> The good news is, there's already a ChildThreadOption to disable automatic
StartServiceManagerConnection. You can use it here and then manually call
StartServiceManagerConnection in UtilityThreadImpl::Init, after you've added the
filter.
> 
>
https://codereview.chromium.org/2810343002/diff/180001/content/public/common/...
> File content/public/common/connection_filter_impl.h (right):
> 
>
https://codereview.chromium.org/2810343002/diff/180001/content/public/common/...
> content/public/common/connection_filter_impl.h:20: class CONTENT_EXPORT
ConnectionFilterImpl : public ConnectionFilter {
> It seems a little weird and unconventional to have a Foo in content/public
along with a corresponding FooImpl also in content/public, especially when it's
just one kind of Foo and not *the* Foo impl.
> 
> I would either change this to something like SimpleConnectionFilter or - since
this doesn't do anything but use a BinderRegistry anyway and doesn't use other
args - just allow BinderRegistry instances to be added to the
ServiceManagerConnection similarly to how ConnectionFilter is used.
> 
> I'd probably prefer the first option since it's less new interface

[Ben Goodger (Google), 2017-04-18 18:30:35]

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

[commit-bot: I haz the power, 2017-04-18 18:31:12]

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-04-18 19:53:49]

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

[commit-bot: I haz the power, 2017-04-18 19:53:50]

Dry run: This issue passed the CQ dry run.

[Ben Goodger (Google), 2017-04-18 21:04:32]

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

[Ben Goodger (Google), 2017-04-18 21:04:33]

OK how about now

[Ken Rockot(use gerrit already), 2017-04-18 21:14:35]

lgtm

[Ben Goodger (Google), 2017-04-18 21:14:51]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2017-04-18 21:15:22]

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-04-18 21:21:38]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492550091804600,
"parent_rev": "eb2c3bd20976ffebd0b1610bb53e7cf78252796f", "commit_rev":
"7f9189cb86fdc65ca77995d28ee9604a3c5a8312"}

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

Description was changed from

==========
Convert UtilityThread/Clients to add ConnectionFilters instead of using
ChildThread's InterfaceRegistry.

R=rockot@chromium.org
==========

to

==========
Convert UtilityThread/Clients to add ConnectionFilters instead of using
ChildThread's InterfaceRegistry.

R=rockot@chromium.org

Review-Url: https://codereview.chromium.org/2810343002
Cr-Commit-Position: refs/heads/master@{#465377}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f9189cb86fdc65ca77995d28ee9...
==========

[commit-bot: I haz the power, 2017-04-18 21:21:50]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/7f9189cb86fdc65ca77995d28ee9...