Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(67)

Issue 2301123004: Mojo Channel: Fix deferred proxy dispatch; support paused channels (Closed)

Created:
4 years, 3 months ago by Ken Rockot(use gerrit already)
Modified:
4 years, 3 months ago
Reviewers:
jam, yzshen1
CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 6

Patch Set 2 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+348 lines, -80 lines) Patch
M ipc/ipc_channel.h View 1 1 chunk +28 lines, -0 lines 0 comments Download
M ipc/ipc_channel_common.cc View 1 chunk +9 lines, -0 lines 0 comments Download
M ipc/ipc_channel_mojo.h View 1 chunk +3 lines, -0 lines 0 comments Download
M ipc/ipc_channel_mojo.cc View 1 3 chunks +24 lines, -1 line 0 comments Download
M ipc/ipc_channel_mojo_unittest.cc View 6 chunks +178 lines, -4 lines 0 comments Download
M ipc/ipc_channel_proxy.h View 1 2 chunks +22 lines, -5 lines 0 comments Download
M ipc/ipc_channel_proxy.cc View 5 chunks +32 lines, -6 lines 0 comments Download
M ipc/ipc_message_pipe_reader.h View 1 2 chunks +2 lines, -6 lines 0 comments Download
M ipc/ipc_message_pipe_reader.cc View 1 3 chunks +5 lines, -44 lines 0 comments Download
M ipc/ipc_mojo_bootstrap.h View 1 chunk +6 lines, -0 lines 0 comments Download
M ipc/ipc_mojo_bootstrap.cc View 8 chunks +28 lines, -11 lines 0 comments Download
M ipc/ipc_sync_channel.h View 1 chunk +1 line, -1 line 0 comments Download
M ipc/ipc_sync_channel.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M ipc/ipc_test.mojom View 1 chunk +8 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 22 (12 generated)
Ken Rockot(use gerrit already)
John, could you please look at the Channel interface changes regarding pause, unpause, flush behavior? ...
4 years, 3 months ago (2016-09-02 18:30:39 UTC) #5
yzshen1
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 ...
4 years, 3 months ago (2016-09-02 22:03:36 UTC) #10
jam
On 2016/09/02 18:30:39, Ken Rockot wrote: > John, could you please look at the Channel ...
4 years, 3 months ago (2016-09-03 03:37:53 UTC) #11
Ken Rockot(use gerrit already)
On Sep 2, 2016 8:37 PM, <jam@chromium.org> wrote: > > On 2016/09/02 18:30:39, Ken Rockot ...
4 years, 3 months ago (2016-09-03 15:41:33 UTC) #12
Ken Rockot(use gerrit already)
Updated the CL description to include rationale for Channel changes
4 years, 3 months ago (2016-09-06 16:48:48 UTC) #14
jam
On 2016/09/02 18:30:39, Ken Rockot wrote: > John, could you please look at the Channel ...
4 years, 3 months ago (2016-09-06 16:49:35 UTC) #15
Ken Rockot(use gerrit already)
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 ...
4 years, 3 months ago (2016-09-06 17:21:28 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2301123004/20001
4 years, 3 months ago (2016-09-06 17:22:00 UTC) #19
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 3 months ago (2016-09-06 18:36:16 UTC) #20
commit-bot: I haz the power
4 years, 3 months ago (2016-09-06 18:39:39 UTC) #22
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/401fb2ccad8315b60f9f6dcbf892e628975cee54
Cr-Commit-Position: refs/heads/master@{#416678}

Powered by Google App Engine
This is Rietveld 408576698