Add an IPC channel between the NaCl loader process and the renderer.

chrome/browser/nacl_host/nacl_process_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/nacl_host/nacl_process_host.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 92 matching lines @@
   handles_for_sel_ldr->push_back(channel);
 #endif
   return true;
 }
 
 }  // namespace
 
 struct NaClProcessHost::NaClInternal {
   std::vector<nacl::Handle> sockets_for_renderer;
   std::vector<nacl::Handle> sockets_for_sel_ldr;
+  std::vector<nacl::FileDescriptor> handles_for_renderer;
 };
 
 // -----------------------------------------------------------------------------
 
 NaClProcessHost::NaClProcessHost(const GURL& manifest_url, bool off_the_record)
     : manifest_url_(manifest_url),
 #if defined(OS_WIN)
       process_launched_by_broker_(false),
 #elif defined(OS_LINUX)
       wait_for_nacl_gdb_(false),
@@ skipping 407 matching lines @@
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
     IPC_MESSAGE_HANDLER(NaClProcessMsg_QueryKnownToValidate,
                         OnQueryKnownToValidate)
     IPC_MESSAGE_HANDLER(NaClProcessMsg_SetKnownToValidate,
                         OnSetKnownToValidate)
 #if defined(OS_WIN)
     IPC_MESSAGE_HANDLER_DELAY_REPLY(NaClProcessMsg_AttachDebugExceptionHandler,
                                     OnAttachDebugExceptionHandler)
 #endif
+    IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelCreated,
+                        OnPpapiChannelCreated)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void NaClProcessHost::OnProcessLaunched() {
   if (!StartWithLaunchedProcess())
     delete this;
 }
 
 // Called when the NaClBrowser singleton has been fully initialized.
 void NaClProcessHost::OnResourcesReady() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   if (!nacl_browser->IsReady() || !SendStart()) {
     DLOG(ERROR) << "Cannot launch NaCl process";
     delete this;
   }
 }
 
-bool NaClProcessHost::ReplyToRenderer() {

[Mark Seaborn, 2012/06/20 19:24:40]

Why do you need to merge ReplyToRenderer() and StartNaClExecution()?  When
enable_ipc_proxy is true, you can just defer the call to ReplyToRenderer() until
OnPpapiChannelCreated().  That would avoid a handle lifetime/cleanup issue
below...

[bbudge, 2012/06/21 00:00:46]

I merged these because we now have to wait for the asynchronous reply from
sel_ldr before replying to the renderer. This was the cleanest way I could see
to do this. The non-IPC case looks a little strange but I favored the IPC proxy
case because we'll remove the old path in the near future. See my comment about
handle cleanup below.

[Mark Seaborn, 2012/06/21 15:30:03]

I understand why you've done it, but I don't think you need to move all this
code around.  I think you can do something like this:

bool NaClProcessHost::SendStart() {
  if (!enable_ipc_proxy_) {
    if (!ReplyToRenderer(IPC::ChannelHandle()))
      return false;
    }
  }
  return StartNaClExecution();
}

void NaClProcessHost::OnPpapiChannelCreated(
    const IPC::ChannelHandle& channel_handle) {
  DCHECK(enable_ipc_proxy_);
  ReplyToRenderer(channel_handle);
}

+ add a ChannelHandle arg to ReplyToRenderer()

[bbudge, 2012/06/21 18:16:53]

Done. Good idea, it's a much smaller change this way.

-  std::vector<nacl::FileDescriptor> handles_for_renderer;
+bool NaClProcessHost::SendStart() {
   for (size_t i = 0; i < internal_->sockets_for_renderer.size(); i++) {
 #if defined(OS_WIN)
     // Copy the handle into the renderer process.
     HANDLE handle_in_renderer;
     if (!DuplicateHandle(base::GetCurrentProcessHandle(),
                          reinterpret_cast<HANDLE>(
                              internal_->sockets_for_renderer[i]),
                          chrome_render_message_filter_->peer_handle(),
                          &handle_in_renderer,
                          0,  // Unused given DUPLICATE_SAME_ACCESS.
                          FALSE,
                          DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
       DLOG(ERROR) << "DuplicateHandle() failed";
       return false;
     }
-    handles_for_renderer.push_back(
+    internal_->handles_for_renderer.push_back(
         reinterpret_cast<nacl::FileDescriptor>(handle_in_renderer));
 #else
     // No need to dup the imc_handle - we don't pass it anywhere else so
     // it cannot be closed.
     nacl::FileDescriptor imc_handle;
     imc_handle.fd = internal_->sockets_for_renderer[i];
     imc_handle.auto_close = true;
-    handles_for_renderer.push_back(imc_handle);
+    internal_->handles_for_renderer.push_back(imc_handle);
 #endif
   }
 
+  const ChildProcessData& data = process_->GetData();
 #if defined(OS_WIN)
   // If we are on 64-bit Windows, the NaCl process's sandbox is
   // managed by a different process from the renderer's sandbox.  We
   // need to inform the renderer's sandbox about the NaCl process so
   // that the renderer can send handles to the NaCl process using
   // BrokerDuplicateHandle().
   if (RunningOnWOW64()) {
-    if (!content::BrokerAddTargetPeer(process_->GetData().handle)) {
+    if (!content::BrokerAddTargetPeer(data.handle)) {
       DLOG(ERROR) << "Failed to add NaCl process PID";
       return false;
     }
   }
 #endif
 
-  ChromeViewHostMsg_LaunchNaCl::WriteReplyParams(
-      reply_msg_, handles_for_renderer);
-  chrome_render_message_filter_->Send(reply_msg_);
-  chrome_render_message_filter_ = NULL;
-  reply_msg_ = NULL;
-  internal_->sockets_for_renderer.clear();

[Mark Seaborn, 2012/06/20 19:24:40]

Removing this call isn't quite right.  If NaClProcessHost() gets destroyed, it
will try to close handles in sockets_for_renderer that have already been closed
(via DUPLICATE_CLOSE_SOURCE), which is unsafe.  The code isn't right before this
change, but the change increases the window in which things can go wrong.

[bbudge, 2012/06/21 00:00:46]

I'm adding a TODO to fix this. I think we should clean up here at the point of
failure.

-  return true;
-}
-
-bool NaClProcessHost::StartNaClExecution() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   nacl::NaClStartParams params;
   params.validation_cache_enabled = nacl_browser->ValidationCacheIsEnabled();
   params.validation_cache_key = nacl_browser->GetValidationCacheKey();
   params.version = chrome::VersionInfo().CreateVersionString();
   params.enable_exception_handling = enable_exception_handling_;
   params.enable_debug_stub =
       CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableNaClDebug);
+  params.enable_ipc_proxy = CommandLine::ForCurrentProcess()->HasSwitch(
+                                switches::kEnableNaClIPCProxy);
 
   base::PlatformFile irt_file = nacl_browser->IrtFile();
   CHECK_NE(irt_file, base::kInvalidPlatformFileValue);
 
-  const ChildProcessData& data = process_->GetData();
   for (size_t i = 0; i < internal_->sockets_for_sel_ldr.size(); i++) {
     if (!ShareHandleToSelLdr(data.handle,
                              internal_->sockets_for_sel_ldr[i], true,
                              &params.handles)) {
       return false;
     }
   }
 
   // Send over the IRT file handle.  We don't close our own copy!
   if (!ShareHandleToSelLdr(data.handle, irt_file, false, &params.handles))
@@ skipping 15 matching lines @@
   nacl::FileDescriptor memory_fd;
   memory_fd.fd = dup(memory_buffer.handle().fd);
   if (memory_fd.fd < 0) {
     DLOG(ERROR) << "Failed to dup() a file descriptor";
     return false;
   }
   memory_fd.auto_close = true;
   params.handles.push_back(memory_fd);
 #endif
 
+  // The start message should only be sent once we are sure we won't delete
+  // ourselves.
   process_->Send(new NaClProcessMsg_Start(params));
 
   internal_->sockets_for_sel_ldr.clear();
+
+  // If we aren't creating the IPC channel, send the reply message without
+  // waiting for the NaCl process to signal that it's ready.
+  // TODO(bbudge) remove this after we switch to the IPC proxy.
+  if (!params.enable_ipc_proxy) {
+    OnPpapiChannelCreated(IPC::ChannelHandle());
+  }
+
   return true;
 }
 
-bool NaClProcessHost::SendStart() {
-  return ReplyToRenderer() && StartNaClExecution();
+void NaClProcessHost::OnPpapiChannelCreated(
+    const IPC::ChannelHandle& channel_handle) {
+  // Now that the server end of the channel has been created, send the reply to
+  // the renderer.
+  base::ProcessId nacl_process_id = base::GetProcId(process_->GetData().handle);
+  ChromeViewHostMsg_LaunchNaCl::WriteReplyParams(
+      reply_msg_, internal_->handles_for_renderer, channel_handle,
+      nacl_process_id);
+  chrome_render_message_filter_->Send(reply_msg_);
+  chrome_render_message_filter_ = NULL;
+  reply_msg_ = NULL;
+
+  internal_->sockets_for_renderer.clear();
+  internal_->handles_for_renderer.clear();
 }
 
 bool NaClProcessHost::StartWithLaunchedProcess() {
 #if defined(OS_LINUX)
   if (wait_for_nacl_gdb_) {
     if (LaunchNaClGdb(base::GetProcId(process_->GetData().handle))) {
       // We will be called with wait_for_nacl_gdb_ = false once debugger is
       // attached to the program.
       return true;
     }
@@ skipping 86 matching lines @@
   } else {
     NaClStartDebugExceptionHandlerThread(
         process_handle.Take(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif