Use ChannelMojo from the browser to NaCl loader process.

components/nacl/broker/nacl_broker_listener.cc

Index: components/nacl/broker/nacl_broker_listener.cc
diff --git a/components/nacl/broker/nacl_broker_listener.cc b/components/nacl/broker/nacl_broker_listener.cc
index f21e32f48db331f1539954d9c83e5185bb1ca1c5..0adbfe2fa9a642f71fb715d39600e68347eea91e 100644
--- a/components/nacl/broker/nacl_broker_listener.cc
+++ b/components/nacl/broker/nacl_broker_listener.cc
@@ -12,16 +12,23 @@
 #include "base/process/launch.h"
 #include "base/process/process.h"
 #include "base/process/process_handle.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/win/win_util.h"
 #include "components/nacl/common/nacl_cmd_line.h"
 #include "components/nacl/common/nacl_debug_exception_handler_win.h"
 #include "components/nacl/common/nacl_messages.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/mojo_channel_switches.h"
 #include "content/public/common/sandbox_init.h"
 #include "ipc/attachment_broker_unprivileged.h"
 #include "ipc/ipc_channel.h"
 #include "ipc/ipc_switches.h"
+#include "mojo/edk/embedder/embedder.h"
+#include "mojo/edk/embedder/platform_channel_pair.h"
+#include "mojo/edk/embedder/scoped_ipc_support.h"
+#include "mojo/public/cpp/system/message_pipe.h"
 #include "sandbox/win/src/sandbox_policy.h"
 
 namespace {
@@ -34,6 +41,14 @@ void SendReply(IPC::Channel* channel, int32_t pid, bool result) {
 
 NaClBrokerListener::NaClBrokerListener() {
   IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
+  mojo::edk::Init();

[Mark Seaborn, 2016/07/08 20:26:19]

How about putting this global-init stuff in NaClBrokerMain(), in
components/nacl/loader/nacl_helper_win_64.cc, rather than in this constructor?

[Anand Mistry (off Chromium), 2016/07/12 07:26:23]

Done.

+  mojo_ipc_support_.reset(new mojo::edk::ScopedIPCSupport(

[Mark Seaborn, 2016/07/08 20:26:19]

mojo_ipc_support_ is never read, so I don't think it needs to be a member, does
it?

[Anand Mistry (off Chromium), 2016/07/12 07:26:23]

It needs to exist for the life of this object. But moving it out with the rest
of the init code mean it no longer needs to be here.

+      base::MessageLoop::current()->task_runner()));
+  mojo::edk::ScopedPlatformHandle platform_channel(
+      mojo::edk::PlatformChannelPair::PassClientHandleFromParentProcess(
+          *base::CommandLine::ForCurrentProcess()));
+  DCHECK(platform_channel.is_valid());
+  mojo::edk::SetParentPipeHandle(std::move(platform_channel));
 }
 
 NaClBrokerListener::~NaClBrokerListener() {
@@ -43,10 +58,14 @@ NaClBrokerListener::~NaClBrokerListener() {
 }
 
 void NaClBrokerListener::Listen() {
-  std::string channel_name =
-      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-          switches::kProcessChannelID);
-  channel_ = IPC::Channel::CreateClient(channel_name, this);
+  mojo::ScopedMessagePipeHandle handle(
+      mojo::edk::CreateChildMessagePipe(
+          base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+              switches::kMojoChannelToken)));
+  DCHECK(handle.is_valid());
+  IPC::ChannelHandle channel_handle(handle.release());
+
+  channel_ = IPC::Channel::CreateClient(channel_handle, this);
   IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
   if (broker && !broker->IsPrivilegedBroker())
     broker->RegisterBrokerCommunicationChannel(channel_.get());
@@ -95,6 +114,10 @@ void NaClBrokerListener::OnLaunchLoaderThroughBroker(
     const std::string& loader_channel_id) {

[Mark Seaborn, 2016/07/08 20:26:19]

Should this be called "token" rather than "id" now?

[Anand Mistry (off Chromium), 2016/07/12 07:26:23]

Done.

   base::ProcessHandle loader_handle_in_browser = 0;
 
+  mojo::ScopedMessagePipeHandle loader_handle(

[Mark Seaborn, 2016/07/08 20:26:19]

Nit: With this addition, there's "loader_handle_in_browser" (a process handle)
and "loader_handle" (a pipe handle).  That naming is a bit confusing.

[Anand Mistry (off Chromium), 2016/07/12 07:26:23]

s/handle/message_pipe

+      mojo::edk::CreateChildMessagePipe(loader_channel_id));
+  DCHECK(loader_handle.is_valid());
+
   // Create the path to the nacl broker/loader executable - it's the executable
   // this code is running in.
   base::FilePath exe_path;
@@ -106,14 +129,35 @@ void NaClBrokerListener::OnLaunchLoaderThroughBroker(
     cmd_line->AppendSwitchASCII(switches::kProcessType,
                                 switches::kNaClLoaderProcess);
 
-    cmd_line->AppendSwitchASCII(switches::kProcessChannelID,
-                                loader_channel_id);
+    // Mojo IPC setup.

[Mark Seaborn, 2016/07/08 20:26:19]

This is a largish chunk of plumbing for creating a process.  How and where does
this plumbing happen for normal Chrome child processes (such as renderers)?  Is
it spread across a larger area of code?

[Anand Mistry (off Chromium), 2016/07/12 07:26:23]

Most Chrome code uses content::ChildProcessLauncher to create child processes.
That class does the browser-side setup. On the child side, ChildThreadImpl is a
common base class, so it has the child setup code.

There are a small number of special cases which for whatever reason don't use
ChildProcessLauncher/ChildThreadImpl. In those, this dance is pretty much
repeated.

+    mojo::edk::PlatformChannelPair channel_pair;
+    mojo::edk::ScopedPlatformHandle parent_handle =
+        channel_pair.PassServerHandle();
+    mojo::edk::ScopedPlatformHandle client_handle =
+        channel_pair.PassClientHandle();
+    base::HandlesToInheritVector handles;
+    handles.push_back(client_handle.get().handle);
+    cmd_line->AppendSwitchASCII(
+        mojo::edk::PlatformChannelPair::kMojoPlatformChannelHandleSwitch,
+        base::UintToString(base::win::HandleToUint32(handles[0])));
+    const std::string mojo_child_token = mojo::edk::GenerateRandomToken();
+    const std::string mojo_channel_token = mojo::edk::GenerateRandomToken();
+    mojo::ScopedMessagePipeHandle host_handle =
+        mojo::edk::CreateParentMessagePipe(mojo_channel_token,
+                                           mojo_child_token);
+    cmd_line->AppendSwitchASCII(switches::kMojoChannelToken,
+                                mojo_channel_token);
+    CHECK_EQ(MOJO_RESULT_OK, mojo::FuseMessagePipes(std::move(loader_handle),

[Mark Seaborn, 2016/07/08 20:26:19]

Does this cause messages to be forwarded by the NaCl-broker process between the
browser and NaCl-loader processes, or does Mojo implement "path shortening" so
that the messages go directly between the browser and NaCl-loader (as I believe
they currently do)?

[Anand Mistry (off Chromium), 2016/07/12 07:26:23]

Yes, Mojo does "path shortening". It may take a few round-trips and so the first
few messages might go via the broker, but Mojo does actively try to create
direct connections between the two endpoints.

+                                                    std::move(host_handle)));
 
     base::Process loader_process;
     sandbox::ResultCode result = content::StartSandboxedProcess(
-        this, cmd_line, base::HandlesToInheritVector(), &loader_process);
+        this, cmd_line, handles, &loader_process);
 
     if (result == sandbox::SBOX_ALL_OK) {
+      mojo::edk::ChildProcessLaunched(loader_process.Handle(),
+                                      std::move(parent_handle),
+                                      mojo_child_token);
       // Note: PROCESS_DUP_HANDLE is necessary here, because:
       // 1) The current process is the broker, which is the loader's parent.
       // 2) The browser is not the loader's parent, and so only gets the
@@ -127,6 +171,8 @@ void NaClBrokerListener::OnLaunchLoaderThroughBroker(
           browser_process_.Handle(), &loader_handle_in_browser,
           PROCESS_DUP_HANDLE | PROCESS_QUERY_INFORMATION | PROCESS_TERMINATE,
           FALSE, 0);
+    } else {
+      mojo::edk::ChildProcessLaunchFailed(mojo_child_token);
     }
   }
   channel_->Send(new NaClProcessMsg_LoaderLaunched(loader_channel_id,

[Mark Seaborn, 2016/07/08 20:26:19]

Is loader_channel_id "consumed" by the call to
mojo::edk::CreateChildMessagePipe() above?

I notice that it still being passed along here.  I think it is being used
secondarily as an ID for associating this reply with the request.  That might be
worth a comment here to avoid potential confusion.

[Anand Mistry (off Chromium), 2016/07/12 07:26:23]

Yes. Just like before.
Added comment.