Refactor NaClProcessHost. Reduce chances to leak the resource.

components/nacl/browser/nacl_process_host.cc

Index: components/nacl/browser/nacl_process_host.cc
diff --git a/components/nacl/browser/nacl_process_host.cc b/components/nacl/browser/nacl_process_host.cc
index 033502986c932dc6c41873e3eb18163df66670dd..ad82e31f8e86ac1f3dde600785989a1f4c8b1a18 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -14,10 +14,12 @@
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/move.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/process/process_iterator.h"
 #include "base/rand_util.h"
+#include "base/scoped_generic.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ -45,6 +47,8 @@
 #include "content/public/common/process_type.h"
 #include "content/public/common/sandboxed_process_launcher_delegate.h"
 #include "ipc/ipc_channel.h"
+#include "ipc/ipc_channel_handle.h"
+#include "ipc/ipc_platform_file.h"
 #include "ipc/ipc_switches.h"
 #include "native_client/src/shared/imc/nacl_imc_c.h"
 #include "net/base/net_util.h"
@@ -149,6 +153,51 @@ bool RunningOnWOW64() {
 
 namespace {
 
+// Usually, NaClProcessHost is working on IO thread, where file operation is

[Mark Seaborn, 2015/04/17 22:13:45]

As an aside: I am sceptical that close() or CloseHandle() really block.  I think
this might be overzealousness on the part of the blocking checks in base/.  But
I don't have any hard data about this.

[hidehiko, 2015/04/30 16:33:01]

According to my coworker who is the expert of Windows, CloseHandle does in most
cases practically (no spec), though close() on, e.g., linux, do not. Though, it
depends on what resource we're releasing.

+// not allowed. So, even just for closing the file, it is necessary to post
+// the task to blocking pool. Note that, it is safer to use this from
+// NaClProcessHost's destructor runnign on IO thread, because BlockingPool

[Mark Seaborn, 2015/04/17 22:13:45]

"running"

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+// is ensured to be alive while the IO thread is alive.
+void CloseFile(base::File file) {

[Mark Seaborn, 2015/04/17 22:13:45]

CloseFile() is a rather generic name.  Maybe CloseFileInBlockingPool() or
CloseFileOnFileThread()?

[hidehiko, 2015/04/30 16:33:01]

Then, probably PostCloseFile etc. would be better.
In chrome tree, OnSomeThread suffix is used for functions which run on the
thread actually, rather than a function to post a task to the target thread, so
maybe it is better to avoid using it here.

Reflected in another CL.

+  if (!file.IsValid()) {
+    return;
+  }
+
+  content::BrowserThread::GetBlockingPool()->PostTask(
+      FROM_HERE,
+      base::Bind(&(ignore_result<base::File>), base::Passed(&file)));

[Mark Seaborn, 2015/04/17 22:13:45]

Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"?  (I'm not
sure.)

[hidehiko, 2015/04/30 16:33:02]

Reflected in another CL. I just misunderstood it must be needed.

+}
+
+// Define "Scoped" handles as utilities to avoid resource leaks.
+struct PlatformFileForTransitTraits {
+  static IPC::PlatformFileForTransit InvalidValue() {
+    return IPC::InvalidPlatformFileForTransit();
+  }
+
+  static void Free(const IPC::PlatformFileForTransit& file) {
+    CloseFile(IPC::PlatformFileForTransitToFile(file).Pass());

[Mark Seaborn, 2015/04/17 22:13:45]

I think this is unsafe on Windows, because this is a handle in another process. 
See my comment below about ScopedSharedMemoryHandle.

[hidehiko, 2015/04/30 16:33:01]

You're right. Fixed.

+  }
+};
+
+typedef base::ScopedGeneric<
+    IPC::PlatformFileForTransit, PlatformFileForTransitTraits>
+    ScopedPlatformFileForTransit;
+
+struct SharedMemoryHandleTraits {
+  static base::SharedMemoryHandle InvalidValue() {
+    return base::SharedMemory::NULLHandle();
+  }
+
+  static void Free(const base::SharedMemoryHandle& handle) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+    base::SharedMemory::CloseHandle(handle);
+  }
+};
+
+typedef base::ScopedGeneric<
+    base::SharedMemoryHandle, SharedMemoryHandleTraits>
+    ScopedSharedMemoryHandle;
+
 // NOTE: changes to this class need to be reviewed by the security team.
 class NaClSandboxedProcessLauncherDelegate
     : public content::SandboxedProcessLauncherDelegate {
@@ -194,46 +243,65 @@ void SetCloseOnExec(NaClHandle fd) {
 #endif
 }
 
-bool ShareHandleToSelLdr(
-    base::ProcessHandle processh,
-    NaClHandle sourceh,
-    bool close_source,
-    std::vector<nacl::FileDescriptor> *handles_for_sel_ldr) {
-#if defined(OS_WIN)
-  HANDLE channel;
-  int flags = DUPLICATE_SAME_ACCESS;
-  if (close_source)
-    flags |= DUPLICATE_CLOSE_SOURCE;
-  if (!DuplicateHandle(GetCurrentProcess(),
-                       reinterpret_cast<HANDLE>(sourceh),
-                       processh,
-                       &channel,
-                       0,  // Unused given DUPLICATE_SAME_ACCESS.
-                       FALSE,
-                       flags)) {
-    LOG(ERROR) << "DuplicateHandle() failed";
-    return false;
-  }
-  handles_for_sel_ldr->push_back(
-      reinterpret_cast<nacl::FileDescriptor>(channel));
-#else
-  nacl::FileDescriptor channel;
-  channel.fd = sourceh;
-  channel.auto_close = close_source;
-  handles_for_sel_ldr->push_back(channel);
-#endif
-  return true;
-}
-
-void CloseFile(base::File file) {
-  // The base::File destructor will close the file for us.
-}
-
 }  // namespace
 
 unsigned NaClProcessHost::keepalive_throttle_interval_milliseconds_ =
     ppapi::kKeepaliveThrottleIntervalDefaultMilliseconds;
 
+// Unfortunately, we cannot use ScopedGeneric directly for IPC::ChannelHandle,
+// because there is neither operator== nor operator!= definition for

[Mark Seaborn, 2015/04/17 22:13:45]

Did you consider defining those operators on IPC::ChannelHandle?  (I'm not sure
what the trade off is here.)

[hidehiko, 2015/04/30 16:33:02]

Defining operator== or operator!= needs to be in the base or IPC namespace,
because of ScopedGeneric implementation, even we have ADL. Our usage is subset
of ChannelHandle now, and our implementation depends on that. So, I prefer not
to use it to avoid making other namespace dirtier.

+// IPC::ChannelHandle. Instead, define a simple wrapper for IPC::ChannelHandle,
+// here.
+class NaClProcessHost::ScopedChannelHandle {
+  MOVE_ONLY_TYPE_FOR_CPP_03(ScopedChannelHandle, RValue);
+ public:
+  ScopedChannelHandle() {
+  }
+  ScopedChannelHandle(const IPC::ChannelHandle& handle) : handle_(handle) {
+  }
+  ScopedChannelHandle(RValue other) : handle_(other.object->handle_) {
+    other.object->handle_ = IPC::ChannelHandle();
+  }
+  ~ScopedChannelHandle() {
+    CloseIfNecessary();
+  }
+
+  const IPC::ChannelHandle& get() const { return handle_; }
+  IPC::ChannelHandle release() WARN_UNUSED_RESULT {
+    IPC::ChannelHandle result = handle_;
+    handle_ = IPC::ChannelHandle();
+    return result;
+  }
+  void reset(const IPC::ChannelHandle& handle = IPC::ChannelHandle()) {
+    // It is not allowed to reset with the same (non-invalid) handle.

[Mark Seaborn, 2015/04/17 22:13:45]

Can you say why?

[hidehiko, 2015/04/30 16:33:02]

Done.

+#if defined(OS_WIN)
+    CHECK(handle.pipe.handle == NULL ||
+          handle.pipe.handle != handle_.pipe.handle);
+#elif defined(OS_POSIX)
+    CHECK(handle.socket.fd == -1 ||
+          !handle.socket.auto_close ||

[Mark Seaborn, 2015/04/17 22:13:45]

Why check auto_close as part of the comparison?

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+          handle.socket.fd != handle_.socket.fd);
+#endif

[Mark Seaborn, 2015/04/17 22:13:45]

"#else
#error Unknown platform"?

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+    CloseIfNecessary();
+    handle_ = handle;
+  }
+
+ private:
+  void CloseIfNecessary() {
+#if defined(OS_WIN)

[Mark Seaborn, 2015/04/17 22:13:45]

How about adding a Close() method to IPC::ChannelHandle so that you don't need
conditionalisation here?

Getting an ipc/ owners review for adding that would actually be quite useful. 
It would be good for an ipc/ owner to look at this change. :-)

[hidehiko, 2015/04/30 16:33:01]

Again, our code depends on some constraint, so having such an API in
ChannelHandle does not make sense.

+    // Defer closing task to the ScopedHandle.
+    base::win::ScopedHandle(handle_.pipe.handle);

[Mark Seaborn, 2015/04/17 22:13:45]

Strangely, this *isn't* going through BlockingPool.

It's kind of odd that you go to the trouble of using BlockingPool in some cases
but not others.  I realise that this is probably an artefact of some Chromium
APIs being (unnecessarily?) stricter than others.

Maybe this warrants a comment to explain the apparent inconsistency?

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+#elif defined(OS_POSIX)
+    if (handle_.socket.auto_close) {
+      // Defer closing task to the ScopedFD.
+      base::ScopedFD(handle_.socket.fd);
+    }
+#endif

[Mark Seaborn, 2015/04/17 22:13:45]

Ditto: add an #else #error?

[hidehiko, 2015/04/30 16:33:01]

Acknowledged.

+  }
+
+  IPC::ChannelHandle handle_;
+};
+
 NaClProcessHost::NaClProcessHost(
     const GURL& manifest_url,
     base::File nexe_file,
@@ -283,6 +351,8 @@ NaClProcessHost::NaClProcessHost(
 }
 
 NaClProcessHost::~NaClProcessHost() {
+  CloseFile(nexe_file_.Pass());
+
   // Report exit status only if the process was successfully started.
   if (process_->GetData().handle != base::kNullProcessHandle) {
     int exit_code = 0;
@@ -301,11 +371,8 @@ NaClProcessHost::~NaClProcessHost() {
   for (size_t i = 0; i < prefetched_resource_files_info_.size(); ++i) {
     // The process failed to launch for some reason. Close resource file
     // handles.
-    base::File file(IPC::PlatformFileForTransitToFile(
-        prefetched_resource_files_info_[i].file));
-    content::BrowserThread::GetBlockingPool()->PostTask(
-        FROM_HERE,
-        base::Bind(&CloseFile, base::Passed(file.Pass())));
+    CloseFile(IPC::PlatformFileForTransitToFile(
+        prefetched_resource_files_info_[i].file).Pass());
   }
 
   if (reply_msg_) {
@@ -682,10 +749,10 @@ void NaClProcessHost::OnResourcesReady() {
   }
 }
 
-bool NaClProcessHost::ReplyToRenderer(
-    const IPC::ChannelHandle& ppapi_channel_handle,
-    const IPC::ChannelHandle& trusted_channel_handle,
-    const IPC::ChannelHandle& manifest_service_channel_handle) {
+void NaClProcessHost::ReplyToRenderer(
+    ScopedChannelHandle ppapi_channel_handle,
+    ScopedChannelHandle trusted_channel_handle,
+    ScopedChannelHandle manifest_service_channel_handle) {
 #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
@@ -695,58 +762,54 @@ bool NaClProcessHost::ReplyToRenderer(
   if (RunningOnWOW64()) {
     if (!content::BrokerAddTargetPeer(process_->GetData().handle)) {
       SendErrorToRenderer("BrokerAddTargetPeer() failed");
-      return false;
+      return;
     }
   }
 #endif
 
-  FileDescriptor imc_handle_for_renderer;
-#if defined(OS_WIN)
-  // Copy the handle into the renderer process.
-  HANDLE handle_in_renderer;
-  if (!DuplicateHandle(base::GetCurrentProcessHandle(),
-                       socket_for_renderer_.TakePlatformFile(),
-                       nacl_host_message_filter_->PeerHandle(),
-                       &handle_in_renderer,
-                       0,  // Unused given DUPLICATE_SAME_ACCESS.
-                       FALSE,
-                       DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
-    SendErrorToRenderer("DuplicateHandle() failed");
-    return false;
+  ScopedPlatformFileForTransit imc_handle_for_renderer(
+      IPC::TakeFileHandleForProcess(socket_for_renderer_.Pass(),
+                                    nacl_host_message_filter_->PeerHandle()));
+  if (!imc_handle_for_renderer.is_valid()) {
+    SendErrorToRenderer("TakeFileHandleForProcess() failed");
+    return;
   }
-  imc_handle_for_renderer = reinterpret_cast<FileDescriptor>(
-      handle_in_renderer);
-#else
-  // No need to dup the imc_handle - we don't pass it anywhere else so
-  // it cannot be closed.
-  FileDescriptor imc_handle;
-  imc_handle.fd = socket_for_renderer_.TakePlatformFile();
-  imc_handle.auto_close = true;
-  imc_handle_for_renderer = imc_handle;
-#endif
 
-  const ChildProcessData& data = process_->GetData();
-  base::SharedMemoryHandle crash_info_shmem_renderer_handle;
-  if (!crash_info_shmem_.ShareToProcess(nacl_host_message_filter_->PeerHandle(),
-                                        &crash_info_shmem_renderer_handle)) {
+  ScopedSharedMemoryHandle crash_info_shmem_renderer_handle;

[Mark Seaborn, 2015/04/17 22:13:45]

I think this is unsafe on Windows.

Remember, on Windows, ShareToProcess() gives you a HANDLE that's an index into
another process's handle table.  If you call CloseHandle() on it, potentially
you're closing an unrelated handle in the current process, which might be
exploitable.

Unfortunately, SharedMemory::CloseHandle() doesn't check the return value of
CloseHandle(), which is rather dangerous.

On Windows, I think your options for error handling are:

 * Let the handle leak in the destination process.
 * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle from the
destination process's handle table.  Does Chromium actually do this anywhere on
an error-handling code path?  This is somewhat risky given that error code paths
are under-tested.

[hidehiko, 2015/04/30 16:33:02]

You're right. Fixed.

+  {
+    base::SharedMemoryHandle raw_handle = base::SharedMemory::NULLHandle();
+    if (crash_info_shmem_.ShareToProcess(
+            nacl_host_message_filter_->PeerHandle(), &raw_handle)) {
+      crash_info_shmem_renderer_handle.reset(raw_handle);
+    }
+  }
+  if (!crash_info_shmem_renderer_handle.is_valid()) {
     SendErrorToRenderer("ShareToProcess() failed");
-    return false;
+    return;
   }
 
-  SendMessageToRenderer(
-      NaClLaunchResult(imc_handle_for_renderer,
-                       ppapi_channel_handle,
-                       trusted_channel_handle,
-                       manifest_service_channel_handle,
-                       base::GetProcId(data.handle),
-                       data.id,
-                       crash_info_shmem_renderer_handle),
-      std::string() /* error_message */);
+  const ChildProcessData& data = process_->GetData();
+  if (SendMessageToRenderer(
+          NaClLaunchResult(imc_handle_for_renderer.get(),
+                           ppapi_channel_handle.get(),
+                           trusted_channel_handle.get(),
+                           manifest_service_channel_handle.get(),
+                           base::GetProcId(data.handle),
+                           data.id,
+                           crash_info_shmem_renderer_handle.get()),
+          std::string() /* error_message */)) {
+    // On success, the NaClLaunchResult is sent to renderer, so that the
+    // handles have been delegated to the message.
+    ignore_result(imc_handle_for_renderer.release());
+    ignore_result(ppapi_channel_handle.release());
+    ignore_result(trusted_channel_handle.release());
+    ignore_result(manifest_service_channel_handle.release());
+    ignore_result(crash_info_shmem_renderer_handle.release());
+  }
 
   // Now that the crash information shmem handles have been shared with the
   // plugin and the renderer, the browser can close its handle.
   crash_info_shmem_.Close();
-  return true;
 }
 
 void NaClProcessHost::SendErrorToRenderer(const std::string& error_message) {
@@ -754,18 +817,19 @@ void NaClProcessHost::SendErrorToRenderer(const std::string& error_message) {
   SendMessageToRenderer(NaClLaunchResult(), error_message);
 }
 
-void NaClProcessHost::SendMessageToRenderer(
+bool NaClProcessHost::SendMessageToRenderer(
     const NaClLaunchResult& result,
     const std::string& error_message) {
   DCHECK(nacl_host_message_filter_.get());
   DCHECK(reply_msg_);
-  if (nacl_host_message_filter_.get() != NULL && reply_msg_ != NULL) {
-    NaClHostMsg_LaunchNaCl::WriteReplyParams(
-        reply_msg_, result, error_message);
-    nacl_host_message_filter_->Send(reply_msg_);
-    nacl_host_message_filter_ = NULL;
-    reply_msg_ = NULL;
+  if (nacl_host_message_filter_.get() == NULL || reply_msg_ == NULL) {
+    return false;
   }
+  NaClHostMsg_LaunchNaCl::WriteReplyParams(reply_msg_, result, error_message);
+  nacl_host_message_filter_->Send(reply_msg_);
+  nacl_host_message_filter_ = NULL;
+  reply_msg_ = NULL;
+  return true;
 }
 
 void NaClProcessHost::SetDebugStubPort(int port) {
@@ -824,6 +888,18 @@ bool NaClProcessHost::StartNaClExecution() {
   params.process_type = process_type_;
   bool enable_nacl_debug = enable_debug_stub_ &&
       NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
+
+  // Because this function may return early on failure, so keep the resources
+  // in local variables, and pass them to params later.
+  ScopedPlatformFileForTransit socket_for_sel_ldr;
+  ScopedPlatformFileForTransit irt_file;
+#if defined(OS_MACOSX)
+  base::ScopedFD memory_fd;
+#endif
+#if defined(OS_POSIX)
+  base::ScopedFD debug_stub_server_bound_socket;
+#endif
+
   if (uses_nonsfi_mode_) {
     // Currently, non-SFI mode is supported only on Linux.
 #if defined(OS_LINUX)
@@ -844,18 +920,20 @@ bool NaClProcessHost::StartNaClExecution() {
         switches::kEnableNaClMojo);
 
     const ChildProcessData& data = process_->GetData();
-    if (!ShareHandleToSelLdr(data.handle,
-                             socket_for_sel_ldr_.TakePlatformFile(),
-                             true,
-                             &params.handles)) {
+    socket_for_sel_ldr.reset(
+        IPC::TakeFileHandleForProcess(socket_for_sel_ldr_.Pass(),
+                                      data.handle));
+    if (!socket_for_sel_ldr.is_valid()) {
       return false;
     }
 
-    const base::File& irt_file = nacl_browser->IrtFile();
-    CHECK(irt_file.IsValid());
+    const base::File& original_irt_file = nacl_browser->IrtFile();
+    CHECK(original_irt_file.IsValid());
     // Send over the IRT file handle.  We don't close our own copy!
-    if (!ShareHandleToSelLdr(data.handle, irt_file.GetPlatformFile(), false,
-                             &params.handles)) {
+    irt_file.reset(IPC::GetFileHandleForProcess(
+        original_irt_file.GetPlatformFile(), data.handle, false));
+    if (!irt_file.is_valid()) {
+      DLOG(ERROR) << "Failed to create irt_file handler for plugin.";
       return false;
     }
 
@@ -872,35 +950,46 @@ bool NaClProcessHost::StartNaClExecution() {
       DLOG(ERROR) << "Failed to allocate memory buffer";
       return false;
     }
-    FileDescriptor memory_fd;
-    memory_fd.fd = dup(memory_buffer.handle().fd);
-    if (memory_fd.fd < 0) {
+    memory_fd.reset(dup(memory_buffer.handle().fd));
+    if (!memory_fd.is_valid()) {
       DLOG(ERROR) << "Failed to dup() a file descriptor";
       return false;
     }
-    memory_fd.auto_close = true;
-    params.handles.push_back(memory_fd);
 #endif
 
 #if defined(OS_POSIX)
     if (params.enable_debug_stub) {
       net::SocketDescriptor server_bound_socket = GetDebugStubSocketHandle();
       if (server_bound_socket != net::kInvalidSocket) {
-        params.debug_stub_server_bound_socket =
-            FileDescriptor(server_bound_socket, true);
+        debug_stub_server_bound_socket.reset(server_bound_socket);
       }
     }
 #endif
   }
 
-  if (!crash_info_shmem_.ShareToProcess(process_->GetData().handle,
-                                        &params.crash_info_shmem_handle)) {
+  ScopedSharedMemoryHandle crash_info_shmem_handle;
+  {
+    base::SharedMemoryHandle raw_handle;
+    if (crash_info_shmem_.ShareToProcess(
+            process_->GetData().handle, &raw_handle)) {
+      crash_info_shmem_handle.reset(raw_handle);
+    }
+  }
+  if (!crash_info_shmem_handle.is_valid()) {
     DLOG(ERROR) << "Failed to ShareToProcess() a shared memory buffer";
     return false;
   }
 
-  base::FilePath file_path;
+  // Hereafter, NaClProcessMsg_Start must be (eventually) sent.
+  // Delegate the resources to NaClStartParams.
+  params.crash_info_shmem_handle = crash_info_shmem_handle.release();
   if (uses_nonsfi_mode_) {
+    DCHECK(!socket_for_sel_ldr.is_valid());
+    DCHECK(!irt_file.is_valid());
+#if defined(OS_MACOSX)
+    DCHECK(!memory_fd.is_valid());
+#endif
+
     // Don't retrieve the file path when using nonsfi mode; there's no
     // validation caching in that case, so it's unnecessary work, and would
     // expose the file path to the plugin.
@@ -915,6 +1004,26 @@ bool NaClProcessHost::StartNaClExecution() {
     }
     prefetched_resource_files_info_.clear();
   } else {
+    // TODO(yusukes): Handle |prefetched_resource_files_info_| for SFI-NaCl.
+    DCHECK(prefetched_resource_files_info_.empty());
+
+    params.handles.push_back(socket_for_sel_ldr.release());
+    params.handles.push_back(irt_file.release());
+    // Note that on OS_POSIX (including OS_MACOSX), IPC::PlatformFileForTransit
+    // is the alias of base::FileDescriptor.
+#if defined(OS_MACOSX)
+    params.handles.push_back(base::FileDescriptor(memory_fd.release(), true));
+#endif
+#if defined(OS_POSIX)
+    if (debug_stub_server_bound_socket.is_valid()) {
+      params.debug_stub_server_bound_socket =
+          base::FileDescriptor(debug_stub_server_bound_socket.release(), true);
+    }
+#endif
+  }
+
+  if (!uses_nonsfi_mode_) {
+    base::FilePath file_path;
     if (NaClBrowser::GetInstance()->GetFilePath(nexe_token_.lo,
                                                 nexe_token_.hi,
                                                 &file_path)) {
@@ -934,8 +1043,6 @@ bool NaClProcessHost::StartNaClExecution() {
         return true;
       }
     }
-    // TODO(yusukes): Handle |prefetched_resource_files_info_| for SFI-NaCl.
-    DCHECK(prefetched_resource_files_info_.empty());
   }
 
   params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
@@ -949,11 +1056,8 @@ void NaClProcessHost::StartNaClFileResolved(
     const base::FilePath& file_path,
     base::File checked_nexe_file) {
   if (checked_nexe_file.IsValid()) {
-    // Release the file received from the renderer. This has to be done on a
-    // thread where IO is permitted, though.
-    content::BrowserThread::GetBlockingPool()->PostTask(
-        FROM_HERE,
-        base::Bind(&CloseFile, base::Passed(nexe_file_.Pass())));
+    // Release the file received from the renderer.
+    CloseFile(nexe_file_.Pass());
     params.nexe_file_path_metadata = file_path;
     params.nexe_file = IPC::TakeFileHandleForProcess(
         checked_nexe_file.Pass(), process_->GetData().handle);
@@ -967,14 +1071,21 @@ void NaClProcessHost::StartNaClFileResolved(
 // This method is called when NaClProcessHostMsg_PpapiChannelCreated is
 // received.
 void NaClProcessHost::OnPpapiChannelsCreated(
-    const IPC::ChannelHandle& browser_channel_handle,
-    const IPC::ChannelHandle& ppapi_renderer_channel_handle,
-    const IPC::ChannelHandle& trusted_renderer_channel_handle,
-    const IPC::ChannelHandle& manifest_service_channel_handle) {
+    const IPC::ChannelHandle& raw_browser_channel_handle,
+    const IPC::ChannelHandle& raw_ppapi_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_trusted_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_manifest_service_channel_handle) {
+  ScopedChannelHandle browser_channel_handle(raw_browser_channel_handle);
+  ScopedChannelHandle ppapi_renderer_channel_handle(
+      raw_ppapi_renderer_channel_handle);
+  ScopedChannelHandle trusted_renderer_channel_handle(
+      raw_trusted_renderer_channel_handle);
+  ScopedChannelHandle manifest_service_channel_handle(
+      raw_manifest_service_channel_handle);
   if (!enable_ppapi_proxy()) {
-    ReplyToRenderer(IPC::ChannelHandle(),
-                    trusted_renderer_channel_handle,
-                    manifest_service_channel_handle);
+    ReplyToRenderer(ScopedChannelHandle(),
+                    trusted_renderer_channel_handle.Pass(),
+                    manifest_service_channel_handle.Pass());
     return;
   }
 
@@ -982,7 +1093,7 @@ void NaClProcessHost::OnPpapiChannelsCreated(
     DCHECK_EQ(PROCESS_TYPE_NACL_LOADER, process_->GetData().process_type);
 
     ipc_proxy_channel_ =
-        IPC::ChannelProxy::Create(browser_channel_handle,
+        IPC::ChannelProxy::Create(browser_channel_handle.release(),
                                   IPC::Channel::MODE_CLIENT,
                                   NULL,
                                   base::MessageLoopProxy::current().get());
@@ -1027,9 +1138,9 @@ void NaClProcessHost::OnPpapiChannelsCreated(
     ipc_proxy_channel_->Send(new PpapiMsg_InitializeNaClDispatcher(args));
 
     // Let the renderer know that the IPC channels are established.
-    ReplyToRenderer(ppapi_renderer_channel_handle,
-                    trusted_renderer_channel_handle,
-                    manifest_service_channel_handle);
+    ReplyToRenderer(ppapi_renderer_channel_handle.Pass(),
+                    trusted_renderer_channel_handle.Pass(),
+                    manifest_service_channel_handle.Pass());
   } else {
     // Attempt to open more than 1 browser channel is not supported.
     // Shut down the NaCl process.