Adds Channel-associated interface support on ChannelProxy's thread

Adds Channel-associated interface support on ChannelProxy's thread

This exposes a way for ChannelProxy users to associate interface
factories with a ChannelProxy, allowing requests to be bound either
on the proxy thread or directly on the IPC thread.

BUG=612500
Committed: https://crrev.com/8d890f6ab069d8e075b3d7aeefc0b9cec7eb2b61
Cr-Commit-Position: refs/heads/master@{#405504}

[Ken Rockot(use gerrit already), 2016-07-13 22:54:13]

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

[commit-bot: I haz the power, 2016-07-13 22:54:56]

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

[Ken Rockot(use gerrit already), 2016-07-13 22:55:39]

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

[Ken Rockot(use gerrit already), 2016-07-13 22:55:43]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-07-13 22:55:55]

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

[Ken Rockot(use gerrit already), 2016-07-13 22:56:24]

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

[Ken Rockot(use gerrit already), 2016-07-13 22:56:25]

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (left):

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:445: CHECK(false);
Forgot to remove this on the last CL. :o

[Ken Rockot(use gerrit already), 2016-07-13 22:56:44]

Description was changed from

==========
Adds Channel-associated interface support on ChannelProxy's thread

This exposes a way for ChanenlProxy users to assocaite interface
factories with a ChannelProxy, allowing requests to be bound either
on the proxy thread or directly on the IPC thread.

BUG=612500
==========

to

==========
Adds Channel-associated interface support on ChannelProxy's thread

This exposes a way for ChannelProxy users to associate interface
factories with a ChannelProxy, allowing requests to be bound either
on the proxy thread or directly on the IPC thread.

BUG=612500
==========

[commit-bot: I haz the power, 2016-07-13 23:19:00]

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

[commit-bot: I haz the power, 2016-07-13 23:19:03]

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

[Ken Rockot(use gerrit already), 2016-07-13 23:21:07]

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

[commit-bot: I haz the power, 2016-07-13 23:22:13]

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

[yzshen1, 2016-07-13 23:25:09]

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_channel_proxy.c...
ipc/ipc_channel_proxy.cc:553: context_->ipc_task_runner()->PostTask(
Posting a task means the associated interface ptr cannot be used immediately
because we cannot start sending messages until |handle| gets into the pipe,
right?

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:480: // The endpoint must have already been inserted
by Accept(), and if the
It seems to be possible that |inserted| is true. Imagine:
1) a task to posted to handle a message on the proxy thread;
2) the corresponding local endpoint is closed;
3) the controller is shutdown on the IO thread, peer_closed is set for all
endpoints so the endpoints already marked as closed will be removed from the
registry.
4) the task of (1) is handled on the proxy thread.

WDYT?

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:486: DCHECK(client);
The local endpoint could already be detached, right? (The associated interface
ptr/binding is already gone)

[commit-bot: I haz the power, 2016-07-14 00:04:25]

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

[commit-bot: I haz the power, 2016-07-14 00:04:26]

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

[Ken Rockot(use gerrit already), 2016-07-14 00:38:35]

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

[Ken Rockot(use gerrit already), 2016-07-14 00:38:38]

Patchset #3 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2016-07-14 00:38:44]

Patchset #2 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2016-07-14 00:39:09]

Comments addressed.

I also fixed a bug where the control_message_proxy_ was being used from the
proxy thread. In order to guarantee that SINGLE_THREAD_SEND is safe on the
Connector, these call sites now post to the io task_runner_ if necessary.

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_channel_proxy.c...
ipc/ipc_channel_proxy.cc:553: context_->ipc_task_runner()->PostTask(
On 2016/07/13 at 23:25:09, yzshen1 wrote:
> Posting a task means the associated interface ptr cannot be used immediately
because we cannot start sending messages until |handle| gets into the pipe,
right?

Discussed offline, the endpoint's SendMessage ultimately posts to the same task
runner, so this is a non issue - sent messages will only be written to the pipe
after this task runs.

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:480: // The endpoint must have already been inserted
by Accept(), and if the
On 2016/07/13 at 23:25:09, yzshen1 wrote:
> It seems to be possible that |inserted| is true. Imagine:
> 1) a task to posted to handle a message on the proxy thread;
> 2) the corresponding local endpoint is closed;
> 3) the controller is shutdown on the IO thread, peer_closed is set for all
endpoints so the endpoints already marked as closed will be removed from the
registry.
> 4) the task of (1) is handled on the proxy thread.
> 
> WDYT?

Ah your right, I had convinced myself that #2 was impossible, but it can happen.
Fixed.

https://codereview.chromium.org/2147493006/diff/20001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:486: DCHECK(client);
On 2016/07/13 at 23:25:09, yzshen1 wrote:
> The local endpoint could already be detached, right? (The associated interface
ptr/binding is already gone)

Indeed. Done.

[commit-bot: I haz the power, 2016-07-14 00:39:33]

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-14 02:28:58]

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

[commit-bot: I haz the power, 2016-07-14 02:28:59]

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

[Ken Rockot(use gerrit already), 2016-07-14 06:05:43]

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

[commit-bot: I haz the power, 2016-07-14 06:06:10]

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

[Ken Rockot(use gerrit already), 2016-07-14 06:12:16]

Patchset #4 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-07-14 06:16:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ken Rockot(use gerrit already), 2016-07-14 06:25:48]

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

[commit-bot: I haz the power, 2016-07-14 06:26:06]

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

[Ken Rockot(use gerrit already), 2016-07-14 07:16:45]

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

[Ken Rockot(use gerrit already), 2016-07-14 07:16:52]

Patchset #4 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-07-14 07:17:00]

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

[Ken Rockot(use gerrit already), 2016-07-14 07:17:06]

Patchset #3 (id:100001) has been deleted

[Ken Rockot(use gerrit already), 2016-07-14 07:31:42]

Patchset #3 (id:160001) has been deleted

[Ken Rockot(use gerrit already), 2016-07-14 07:31:51]

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

[commit-bot: I haz the power, 2016-07-14 07:32: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-14 09:09:39]

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

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

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Rockot(use gerrit already), 2016-07-14 14:29:29]

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

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

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

[yzshen1, 2016-07-14 15:04:04]

LGTM with two nits.

https://codereview.chromium.org/2147493006/diff/180001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2147493006/diff/180001/ipc/ipc_mojo_bootstrap...
ipc/ipc_mojo_bootstrap.cc:439: if (!control_message_handler_.Accept(message))
can we directly return false instead of raising error? The boolean's meaning is
different from that of MultiplexRouter::ProcessIncomingMessage

https://codereview.chromium.org/2147493006/diff/180001/ipc/ipc_mojo_bootstrap...
ipc/ipc_mojo_bootstrap.cc:481: RaiseError();
I think we could return false directly here?

[commit-bot: I haz the power, 2016-07-14 15:24:45]

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

[commit-bot: I haz the power, 2016-07-14 15:24:46]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-07-14 15:26:52]

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

[commit-bot: I haz the power, 2016-07-14 15:27:15]

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

[Ken Rockot(use gerrit already), 2016-07-14 15:27:37]

Thanks!

https://codereview.chromium.org/2147493006/diff/180001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2147493006/diff/180001/ipc/ipc_mojo_bootstrap...
ipc/ipc_mojo_bootstrap.cc:439: if (!control_message_handler_.Accept(message))
On 2016/07/14 at 15:04:04, yzshen1 wrote:
> can we directly return false instead of raising error? The boolean's meaning
is different from that of MultiplexRouter::ProcessIncomingMessage

Good point! Done

https://codereview.chromium.org/2147493006/diff/180001/ipc/ipc_mojo_bootstrap...
ipc/ipc_mojo_bootstrap.cc:481: RaiseError();
On 2016/07/14 at 15:04:04, yzshen1 wrote:
> I think we could return false directly here?

Yep, done

[commit-bot: I haz the power, 2016-07-14 16:30:19]

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

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

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-07-14 16:31:18]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-07-14 16:31:18]

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

[commit-bot: I haz the power, 2016-07-14 16:31: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-14 16:37:32]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2016-07-14 16:39:27]

Description was changed from

==========
Adds Channel-associated interface support on ChannelProxy's thread

This exposes a way for ChannelProxy users to associate interface
factories with a ChannelProxy, allowing requests to be bound either
on the proxy thread or directly on the IPC thread.

BUG=612500
==========

to

==========
Adds Channel-associated interface support on ChannelProxy's thread

This exposes a way for ChannelProxy users to associate interface
factories with a ChannelProxy, allowing requests to be bound either
on the proxy thread or directly on the IPC thread.

BUG=612500

Committed: https://crrev.com/8d890f6ab069d8e075b3d7aeefc0b9cec7eb2b61
Cr-Commit-Position: refs/heads/master@{#405504}
==========

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

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