Mojo Channel: Fix deferred proxy dispatch; support paused channels

Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

RPHI must not perform its own queueing once the Channel exists:
interface requests and mojom messages must also be queued, but RPHI
only hooks into the IPC::Sender interface. Queueing outside of the
Channel breaks FIFO between early IPC::Messages and early Channel-
associated Mojo IPC.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

BUG=612500
Committed: https://crrev.com/401fb2ccad8315b60f9f6dcbf892e628975cee54
Cr-Commit-Position: refs/heads/master@{#416678}

[Ken Rockot(use gerrit already), 2016-09-02 18:25:19]

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

[commit-bot: I haz the power, 2016-09-02 18:25:47]

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-09-02 18:28:44]

Description was changed from

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

Both of these changes are a prerequisite for converting more IPCs
to mojo.

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

to

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

Both of these changes are a prerequisite for converting more IPCs
to mojo.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

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

[Ken Rockot(use gerrit already), 2016-09-02 18:30:38]

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

[Ken Rockot(use gerrit already), 2016-09-02 18:30:39]

John, could you please look at the Channel interface changes regarding pause,
unpause, flush behavior?

Yuzhu, could you please look at the changes to ChannelAssociatedGroup controller
regarding proxied dispatch to yet-unbound interfaces?

Thanks!

[Ken Rockot(use gerrit already), 2016-09-02 20:00:57]

Description was changed from

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

Both of these changes are a prerequisite for converting more IPCs
to mojo.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

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

to

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

Both of these changes are a prerequisite for converting more IPCs
to mojo.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

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

[Ken Rockot(use gerrit already), 2016-09-02 20:01:19]

Description was changed from

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

Both of these changes are a prerequisite for converting more IPCs
to mojo.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

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

to

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

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

[commit-bot: I haz the power, 2016-09-02 21:46:50]

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

[commit-bot: I haz the power, 2016-09-02 21:46:51]

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

[yzshen1, 2016-09-02 22:03:36]

ChannelAssociatedGroup controller change LGTM

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel.h#newcode230
ipc/ipc_channel.h:230: // force some early messages to be transmitted before
ones which were sent
nit: force -> to force?

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel_proxy.h#new...
ipc/ipc_channel_proxy.h:116: // thread. If |connect_now| is true, pipe
connection is initiated immediately.
this comment seems outdated.

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_message_pipe_reader.h
File ipc/ipc_message_pipe_reader.h (right):

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_message_pipe_reader...
ipc/ipc_message_pipe_reader.h:113: // Raw message pipe handle and interface ID
we use to send legacy IPC messages
nit: please update the comment accordingly.

[jam, 2016-09-03 03:37:53]

On 2016/09/02 18:30:39, Ken Rockot wrote:
> John, could you please look at the Channel interface changes regarding pause,
> unpause, flush behavior?
> 
> Yuzhu, could you please look at the changes to ChannelAssociatedGroup
controller
> regarding proxied dispatch to yet-unbound interfaces?
> 
> Thanks!

From the description, I don't understand why we need 2. Is that the only reason
you're adding the new methods on IPC::Channel? I presume not (since that code is
trivial and seems simpler to keep than changing IPC::Channel).

[Ken Rockot(use gerrit already), 2016-09-03 15:41:33]

On Sep 2, 2016 8:37 PM, <jam@chromium.org> wrote:
>
> On 2016/09/02 18:30:39, Ken Rockot wrote:
> > John, could you please look at the Channel interface changes regarding
pause,
> > unpause, flush behavior?
> >
> > Yuzhu, could you please look at the changes to ChannelAssociatedGroup
> controller
> > regarding proxied dispatch to yet-unbound interfaces?
> >
> > Thanks!
>
> From the description, I don't understand why we need 2. Is that the only
reason
> you're adding the new methods on IPC::Channel? I presume not (since that
code is
> trivial and seems simpler to keep than changing IPC::Channel).
>

The reason is that RPHI is only intercepting and queuing Send calls. It has
no way of hooking into arbitrary interface requests or messages on those
interfaces, and this means that FIFO is broken if any channel associated
interfaces are used before OnProcessLaunched.

> https://codereview.chromium.org/2301123004/

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

Description was changed from

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

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

to

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

RPHI must not perform its own queueing once the Channel exists:
interface requests and mojom messages must also be queued, but RPHI
only hooks into the IPC::Sender interface. Queueing outside of the
Channel breaks FIFO between early IPC::Messages and early Channel-
associated Mojo IPC.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

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

[Ken Rockot(use gerrit already), 2016-09-06 16:48:48]

Updated the CL description to include rationale for Channel changes

[jam, 2016-09-06 16:49:35]

On 2016/09/02 18:30:39, Ken Rockot wrote:
> John, could you please look at the Channel interface changes regarding pause,
> unpause, flush behavior?

fine with me since it's all temporary

[Ken Rockot(use gerrit already), 2016-09-06 17:21:28]

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel.h#newcode230
ipc/ipc_channel.h:230: // force some early messages to be transmitted before
ones which were sent
On 2016/09/02 at 22:03:36, yzshen1 wrote:
> nit: force -> to force?

Done

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_channel_proxy.h#new...
ipc/ipc_channel_proxy.h:116: // thread. If |connect_now| is true, pipe
connection is initiated immediately.
On 2016/09/02 at 22:03:36, yzshen1 wrote:
> this comment seems outdated.

Oops, yes, updated

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_message_pipe_reader.h
File ipc/ipc_message_pipe_reader.h (right):

https://codereview.chromium.org/2301123004/diff/1/ipc/ipc_message_pipe_reader...
ipc/ipc_message_pipe_reader.h:113: // Raw message pipe handle and interface ID
we use to send legacy IPC messages
On 2016/09/02 at 22:03:36, yzshen1 wrote:
> nit: please update the comment accordingly.

Done

[Ken Rockot(use gerrit already), 2016-09-06 17:21:33]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-09-06 17:21:33]

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

[commit-bot: I haz the power, 2016-09-06 17:22:00]

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-09-06 18:36:16]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-06 18:39:38]

Description was changed from

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

RPHI must not perform its own queueing once the Channel exists:
interface requests and mojom messages must also be queued, but RPHI
only hooks into the IPC::Sender interface. Queueing outside of the
Channel breaks FIFO between early IPC::Messages and early Channel-
associated Mojo IPC.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

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

to

==========
Mojo Channel: Fix deferred proxy dispatch; support paused channels

This is a twofold change because of some inherent overlap:

 1. Fixes deferred proxy dispatch. This means if a message comes in
    for an unknown associated interface endpoint, we still dispatch
    to the proxy thread, as was the intended behavior. Adds a test
    to verify that this actually works now.

 2. Adds the ability to create a "paused" Channel, and corresponding
    Unpause and Flush operations (which only ChannelMojo supports).
    This is used by a follow-up CL to avoid having
    RenderProcessHostImpl perform its own message queueing, which it
    does to allow certain initialization messages to preempt other
    early Sends.

RPHI must not perform its own queueing once the Channel exists:
interface requests and mojom messages must also be queued, but RPHI
only hooks into the IPC::Sender interface. Queueing outside of the
Channel breaks FIFO between early IPC::Messages and early Channel-
associated Mojo IPC.

In order to support both of these changes, the manual serialization
gunk has also been removed from MessagePipeReader (it was only there
to support thread-safe Send, which we've since abandoned.) This is
necessary to ensure that all send logic goes through the same single
queueing mechanism in the ChannelAssociatedGroupController.

Part a series of CLs to enable and demonstrate WebContents associated
interfaces:

  1. This CL
  2. https://codereview.chromium.org/2309513002
  3. https://codereview.chromium.org/2310563002
  4. https://codereview.chromium.org/2310583002

BUG=612500

Committed: https://crrev.com/401fb2ccad8315b60f9f6dcbf892e628975cee54
Cr-Commit-Position: refs/heads/master@{#416678}
==========

[commit-bot: I haz the power, 2016-09-06 18:39:39]

Patchset 2 (id:??) landed as
https://crrev.com/401fb2ccad8315b60f9f6dcbf892e628975cee54
Cr-Commit-Position: refs/heads/master@{#416678}