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

Issue 1675603002: [mojo-edk] Simplify multiprocess pipe bootstrap (Closed)

Created:
4 years, 10 months ago by Ken Rockot(use gerrit already)
Modified:
4 years, 10 months ago
CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[mojo-edk] Simplify multiprocess pipe bootstrap This introduces a new MergePort message at the Ports layer for joining two independent port cycles which each have an unused (i.e. unwritten, unread, unsent) receiving port. MergePort allows us to create a MessagePipeDispatcher which is immediately usable but which will eventually be linked to a MessagePipeDispatcher on another port cycle, potentially in another process. The basic idea is to create a fully functional port pair but only bind one port to an MPD. Do this on each end and merge the dangling ports asynchronously. The simplification here allows a lot of code to be deleted from NodeController, some of which is deleted in this CL. Future work will convert existing bootstrap sites back to using synchronous bootstrap, including the token-based APIs. BUG=584764 TBR=ben@chromium.org for null check in mash shell Committed: https://crrev.com/b3ea203171e07f5c7e476e94d210ec4ad53ce5b0 Cr-Commit-Position: refs/heads/master@{#374322}

Patch Set 1 : #

Total comments: 6

Patch Set 2 : moar testing mostly #

Patch Set 3 : move async deprecation to public API #

Patch Set 4 : #

Total comments: 4

Patch Set 5 : merge in the sync token APIs too #

Patch Set 6 : address comments #

Patch Set 7 : fix some callers to work with sync APIs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+745 lines, -661 lines) Patch
M mash/shell/shell_application_delegate.cc View 1 2 3 4 5 6 1 chunk +6 lines, -2 lines 0 comments Download
M mojo/edk/embedder/embedder.h View 1 2 3 4 4 chunks +27 lines, -7 lines 0 comments Download
M mojo/edk/embedder/embedder.cc View 1 2 3 4 1 chunk +14 lines, -6 lines 0 comments Download
M mojo/edk/system/core.h View 1 2 3 4 5 1 chunk +9 lines, -16 lines 0 comments Download
M mojo/edk/system/core.cc View 1 2 3 4 2 chunks +33 lines, -38 lines 0 comments Download
M mojo/edk/system/multiprocess_message_pipe_unittest.cc View 2 chunks +39 lines, -0 lines 0 comments Download
M mojo/edk/system/node_channel.h View 1 2 3 4 2 chunks +5 lines, -12 lines 0 comments Download
M mojo/edk/system/node_channel.cc View 1 2 3 4 4 chunks +11 lines, -36 lines 0 comments Download
M mojo/edk/system/node_controller.h View 1 2 3 4 5 chunks +16 lines, -88 lines 0 comments Download
M mojo/edk/system/node_controller.cc View 1 2 3 4 9 chunks +30 lines, -174 lines 0 comments Download
M mojo/edk/system/ports/event.h View 2 chunks +6 lines, -0 lines 0 comments Download
M mojo/edk/system/ports/message.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M mojo/edk/system/ports/name.h View 1 2 3 4 5 2 chunks +7 lines, -0 lines 0 comments Download
M mojo/edk/system/ports/node.h View 3 chunks +17 lines, -0 lines 0 comments Download
M mojo/edk/system/ports/node.cc View 1 4 chunks +152 lines, -27 lines 0 comments Download
M mojo/edk/system/ports/ports_unittest.cc View 1 2 chunks +317 lines, -21 lines 0 comments Download
M mojo/edk/system/remote_message_pipe_bootstrap.h View 3 chunks +10 lines, -12 lines 0 comments Download
M mojo/edk/system/remote_message_pipe_bootstrap.cc View 3 chunks +13 lines, -10 lines 0 comments Download
M mojo/shell/runner/child/runner_connection.cc View 1 2 3 4 5 6 2 chunks +3 lines, -13 lines 0 comments Download
M mojo/shell/runner/host/child_process.cc View 1 2 3 4 5 6 4 chunks +14 lines, -39 lines 0 comments Download
M mojo/shell/runner/host/child_process_host.h View 1 2 3 4 5 6 2 chunks +4 lines, -58 lines 0 comments Download
M mojo/shell/runner/host/child_process_host.cc View 1 2 3 4 5 6 5 chunks +9 lines, -102 lines 0 comments Download

Messages

Total messages: 32 (16 generated)
Ken Rockot(use gerrit already)
Darin, mind taking a look at this? Had a crazy idea in the middle of ...
4 years, 10 months ago (2016-02-05 15:45:45 UTC) #4
darin (slow to review)
That is a neat idea. I will try to take a look soon. On Feb ...
4 years, 10 months ago (2016-02-05 15:53:34 UTC) #5
darin (slow to review)
I only got a chance to review the ports/ bit. It looks solid. I like ...
4 years, 10 months ago (2016-02-05 17:46:46 UTC) #7
Ken Rockot(use gerrit already)
Thanks! https://codereview.chromium.org/1675603002/diff/20001/mojo/edk/system/ports/node.cc File mojo/edk/system/ports/node.cc (right): https://codereview.chromium.org/1675603002/diff/20001/mojo/edk/system/ports/node.cc#newcode757 mojo/edk/system/ports/node.cc:757: scoped_refptr<Port> new_port = GetPort_Locked(event.new_port_name); On 2016/02/05 at 17:46:45, ...
4 years, 10 months ago (2016-02-05 19:43:06 UTC) #9
Ken Rockot(use gerrit already)
Anand could you also please take a look?
4 years, 10 months ago (2016-02-05 20:04:36 UTC) #11
Ken Rockot(use gerrit already)
On 2016/02/05 at 20:04:36, Ken Rockot wrote: > Anand could you also please take a ...
4 years, 10 months ago (2016-02-05 20:35:11 UTC) #13
Anand Mistry (off Chromium)
LGTM I don't yet understand the ports implementation nearly as well as I should, but ...
4 years, 10 months ago (2016-02-08 06:23:21 UTC) #14
Ken Rockot(use gerrit already)
https://codereview.chromium.org/1675603002/diff/80001/mojo/edk/system/core.h File mojo/edk/system/core.h (right): https://codereview.chromium.org/1675603002/diff/80001/mojo/edk/system/core.h#newcode55 mojo/edk/system/core.h:55: // Creates a message pipe endpoint connected to an ...
4 years, 10 months ago (2016-02-08 23:20:56 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675603002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675603002/120001
4 years, 10 months ago (2016-02-08 23:29:53 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/178132)
4 years, 10 months ago (2016-02-09 00:52:16 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675603002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675603002/140001
4 years, 10 months ago (2016-02-09 03:38:33 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/144071)
4 years, 10 months ago (2016-02-09 03:55:44 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675603002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675603002/140001
4 years, 10 months ago (2016-02-09 03:59:19 UTC) #28
commit-bot: I haz the power
Committed patchset #7 (id:140001)
4 years, 10 months ago (2016-02-09 06:23:10 UTC) #29
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/b3ea203171e07f5c7e476e94d210ec4ad53ce5b0 Cr-Commit-Position: refs/heads/master@{#374322}
4 years, 10 months ago (2016-02-09 06:24:04 UTC) #31
blundell
4 years, 10 months ago (2016-02-09 10:24:46 UTC) #32
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.chromium.org/1678333003/ by blundell@chromium.org.

The reason for reverting is: This patch broke several browsertests on Linux:
PlatformAppBrowserTest.LoadAndLaunchAppChromeRunning,
PlatformAppBrowserTest.LoadAndLaunchAppWithFile,
PolicyMakeDefaultBrowserTest.MakeDefaultDisabled (e.g.,
https://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests...).
I could repro the failures locally, and reverting this patch fixed them.

Example failure:

PlatformAppBrowserTest.LoadAndLaunchAppChromeRunning (run #1):
[ RUN      ] PlatformAppBrowserTest.LoadAndLaunchAppChromeRunning
<snip>
[26865:26901:0209/004101:FATAL:thread.cc(270)] Check failed:
GetThreadWasQuitProperly().
#0 0x0000f7142d94 base::debug::StackTrace::StackTrace()
#1 0x0000f71a998f logging::LogMessage::~LogMessage()
#2 0x0000f72cc94f base::Thread::ThreadMain()
#3 0x0000f72b487a base::(anonymous namespace)::ThreadFunc()
#4 0x0000e749dd4c start_thread
#5 0x0000e6a7cb8e clone.

Powered by Google App Engine
This is Rietveld 408576698