Support early associated interface binding on ChannelMojo

Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500, 619202
Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Committed: https://crrev.com/0e4de5f9a519c6cd206448a10eccc7a535e3db64
Cr-Original-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Original-Commit-Position: refs/heads/master@{#407050}
Cr-Commit-Position: refs/heads/master@{#407264}

[Ken Rockot(use gerrit already), 2016-07-19 21:00:03]

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

[commit-bot: I haz the power, 2016-07-19 21:00:37]

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-19 21:03:43]

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

[Ken Rockot(use gerrit already), 2016-07-19 21:03:43]

Please take a look

[commit-bot: I haz the power, 2016-07-19 22:16:37]

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

[commit-bot: I haz the power, 2016-07-19 22:16:38]

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

[Ken Rockot(use gerrit already), 2016-07-19 23:57:27]

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

[commit-bot: I haz the power, 2016-07-19 23:58:04]

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-20 00:30:31]

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

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

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-20 04:11:04]

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

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

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-20 06:52:50]

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

[commit-bot: I haz the power, 2016-07-20 06:52:51]

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

[yzshen1, 2016-07-20 17:13:15]

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_channel_mojo.cc
File ipc/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_channel_mojo.cc...
ipc/ipc_channel_mojo.cc:300: associated_interfaces_.clear();
A few questions about lock usage:

- Is it safe to use |lock_| instead of |associated_interface_lock_| to guard
this clear call?

- Is the only reason to put |message_reader_| under |lock_| because it can be
set to null on the IO thread while on another thread someone tries to use it to
Send? If that is the case, can we set message_reader_ to close state without
deleting it so that we can remove the lock?

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:72: base::ThreadTaskRunnerHandle::Get()));
It may not matter, but strictly speaking we probably should use task_runner_
here instead. The two could be different, although they live on the same thread.
(Imagine the user want to do custom scheduling)

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:656: // mojo::MessageReceiverWithResponderStatus:
What is this interface used for ?

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

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

[commit-bot: I haz the power, 2016-07-20 21:03:57]

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-20 21:08:39]

Thanks, I've addressed your comments.

The new patch set also eliminates the deferred MessagePipeReader deleter because
it's no longer necessary and it was leading to racy leaks due to MessageLoop
teardown fighting with channel teardown.

Please take a look!

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_channel_mojo.cc
File ipc/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_channel_mojo.cc...
ipc/ipc_channel_mojo.cc:300: associated_interfaces_.clear();
On 2016/07/20 at 17:13:15, yzshen1 wrote:
> A few questions about lock usage:
> 
> - Is it safe to use |lock_| instead of |associated_interface_lock_| to guard
this clear call?

Good catch :)

> 
> - Is the only reason to put |message_reader_| under |lock_| because it can be
set to null on the IO thread while on another thread someone tries to use it to
Send? If that is the case, can we set message_reader_ to close state without
deleting it so that we can remove the lock?

The only reason was because of thread-safe Send. Since we are not intending to
support that anymore (and ChannelMojo is marked as not having a thread-safe
Send) this is actually unnecessary. |message_reader_| is only accessed on the IO
thread now, so I've removed |lock_|.

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:72: base::ThreadTaskRunnerHandle::Get()));
On 2016/07/20 at 17:13:15, yzshen1 wrote:
> It may not matter, but strictly speaking we probably should use task_runner_
here instead. The two could be different, although they live on the same thread.
(Imagine the user want to do custom scheduling)

Indeed. This was an artifact from a previous iteration of the CL. Fixed

https://codereview.chromium.org/2163633003/diff/40001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:656: // mojo::MessageReceiverWithResponderStatus:
On 2016/07/20 at 17:13:15, yzshen1 wrote:
> What is this interface used for ?

Nothing! It was used when I still had an InterfaceEndpointClient attached to the
master interface, but then I realized we don't even need to do that.

I've removed this interface.

[commit-bot: I haz the power, 2016-07-20 22:47:28]

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

[commit-bot: I haz the power, 2016-07-20 22:47:28]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-07-20 22:54:52]

LGTM with a few nits. Thanks!

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_channel_mojo.cc
File ipc/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_channel_mojo.cc...
ipc/ipc_channel_mojo.cc:297: // avoid deadlock when freeing it below.
nit: how could it lead to deadlock? Is it just out-dated comment?

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:184: scoped_refptr<ChannelAssociatedGroupController>
keepalive(this);
I think this is not necessary because the InterfaceEndpointClient must hold a
ref to this. Besides, it seems no operation in this method will decrease ref
count. Have I missed anything?

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:438: scoped_refptr<ChannelAssociatedGroupController>
keepalive(this);
Why do we need to keep it alive explicitly? I thought the closure will keep a
ref until it is done running?

[yzshen1, 2016-07-20 22:54:52]

LGTM with a few nits. Thanks!

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

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_channel_mojo.cc
File ipc/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_channel_mojo.cc...
ipc/ipc_channel_mojo.cc:297: // avoid deadlock when freeing it below.
On 2016/07/20 at 22:54:51, yzshen1 wrote:
> nit: how could it lead to deadlock? Is it just out-dated comment?

Woops, you're right. Fixed the wording. I think it's still appropriate to warn
about reentry.

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:184: scoped_refptr<ChannelAssociatedGroupController>
keepalive(this);
On 2016/07/20 at 22:54:51, yzshen1 wrote:
> I think this is not necessary because the InterfaceEndpointClient must hold a
ref to this. Besides, it seems no operation in this method will decrease ref
count. Have I missed anything?

NotifyEndpointOfError may synchronously call into the client's NotifyError,
which may invoke an arbitrary user callback that decreases ref count.

However you're right that in this case the InterfaceEndpointClient must already
be holding a ref, so I've removed this keepalive.

https://codereview.chromium.org/2163633003/diff/60001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:438: scoped_refptr<ChannelAssociatedGroupController>
keepalive(this);
On 2016/07/20 at 22:54:51, yzshen1 wrote:
> Why do we need to keep it alive explicitly? I thought the closure will keep a
ref until it is done running?

Ah, you are right :)  Fixed.

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

The CQ bit was checked by rockot@chromium.org

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

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/2163633003/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2016-07-20 23:10:48]

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-21 00:13:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-21 00:16:46]

Description was changed from

==========
Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500,619202
==========

to

==========
Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500,619202

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Cr-Commit-Position: refs/heads/master@{#406720}
==========

[commit-bot: I haz the power, 2016-07-21 00:16:47]

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

[dominickn, 2016-07-21 01:31:05]

On 2016/07/21 00:16:47, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
> Cr-Commit-Position: refs/heads/master@{#406720}

Is this CL causing *all* browser tests to fail on trybots? e.g.
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

The error is: [25407:25505:0720/174311:FATAL:ipc_mojo_bootstrap.cc(371)] Check
failed: proxy_task_runner_ && proxy_task_runner_->BelongsToCurrentThread().

[Ken Rockot(use gerrit already), 2016-07-21 01:32:51]

That's odd. Reverting.

On Wed, Jul 20, 2016 at 6:31 PM, <dominickn@chromium.org> wrote:

> On 2016/07/21 00:16:47, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
> > Cr-Commit-Position: refs/heads/master@{#406720}
>
> Is this CL causing *all* browser tests to fail on trybots? e.g.
>
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
>
> The error is: [25407:25505:0720/174311:FATAL:ipc_mojo_bootstrap.cc(371)]
> Check
> failed: proxy_task_runner_ &&
> proxy_task_runner_->BelongsToCurrentThread().
>
> https://codereview.chromium.org/2163633003/
>

-- 
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), 2016-07-21 01:33:06]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2167973002/ by rockot@chromium.org.

The reason for reverting is: Lots of browser_tests failures.

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium....

[Ken Rockot(use gerrit already), 2016-07-21 01:37:08]

Revert landed. Possibly bad interaction with another channel-related change that
landed at almost the same time. Kicking off another dry run.

[Ken Rockot(use gerrit already), 2016-07-21 01:37:12]

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

[commit-bot: I haz the power, 2016-07-21 01:37:37]

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-21 01:42:03]

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

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

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-07-21 20:26:44]

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

[commit-bot: I haz the power, 2016-07-21 20:26:58]

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

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

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

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-22 00:14:03]

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

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

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-22 01:41:52]

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

[commit-bot: I haz the power, 2016-07-22 01:41:53]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-07-22 01:45:08]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-07-22 01:45:09]

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/2163633003/#ps120001
(title: ".")

[commit-bot: I haz the power, 2016-07-22 01:45:44]

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-22 03:54:19]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-22 03:56:59]

Description was changed from

==========
Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500,619202

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Cr-Commit-Position: refs/heads/master@{#406720}
==========

to

==========
Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500,619202

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Cr-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Commit-Position: refs/heads/master@{#407050}
==========

[commit-bot: I haz the power, 2016-07-22 03:57:00]

Patchset 7 (id:??) landed as
https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Cr-Commit-Position: refs/heads/master@{#407050}

[msramek, 2016-07-22 10:05:34]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2173753002/ by msramek@chromium.org.

The reason for reverting is: Probable cause of failures on Mac ASan. See
crbug.com/630564 for more details..

[Ken Rockot(use gerrit already), 2016-07-22 19:46:47]

Turns out this latest trouble is simply caused by an invalid DCHECK in mojo
internals. I'm turning that off in https://codereview.chromium.org/2178633002
and then I will reland this CL as-is.

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

The CQ bit was checked by rockot@chromium.org

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

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-22 21:18:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-22 21:21:25]

Description was changed from

==========
Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500,619202

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Cr-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Commit-Position: refs/heads/master@{#407050}
==========

to

==========
Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500,619202

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Committed: https://crrev.com/0e4de5f9a519c6cd206448a10eccc7a535e3db64
Cr-Original-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Original-Commit-Position: refs/heads/master@{#407050}
Cr-Commit-Position: refs/heads/master@{#407264}
==========

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

Patchset 7 (id:??) landed as
https://crrev.com/0e4de5f9a519c6cd206448a10eccc7a535e3db64
Cr-Commit-Position: refs/heads/master@{#407264}

[Mark P, 2016-07-23 03:57:30]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2173303002/ by mpearson@chromium.org.

The reason for reverting is: Causes failures on Windows Unit (DrMemory) in
memory test: ipc_tests
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...

---
IPCChannelMojoTest.SendFailWithPendingMessages:
c:\b\build\slave\drm-cr\build\src\ipc\ipc_channel_mojo_unittest.cc(290): error:
Value of: listener.has_error()
Actual: false
Expected: true
---
.