Created a blocking copy to temporary file function.

services/nacl/content_handler_main.cc

Index: services/nacl/content_handler_main.cc
diff --git a/services/nacl/content_handler_main.cc b/services/nacl/content_handler_main.cc
index 776ebd85ec3c0277ce6583f6406c25ec68f35ea2..238c6bf40da5c3f31ed10ff40d65a82dd59f5dd5 100644
--- a/services/nacl/content_handler_main.cc
+++ b/services/nacl/content_handler_main.cc
@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "build/build_config.h"
@@ -24,24 +23,8 @@ namespace nacl {
 namespace content_handler {
 
 namespace {
-bool URLResponseToTempFile(mojo::URLResponsePtr& response,
-                           base::FilePath* file_path) {
-  if (!base::CreateTemporaryFile(file_path)) {
-    return false;
-  }
-
-  if (!mojo::common::BlockingCopyToFile(response->body.Pass(), *file_path)) {
-    base::DeleteFile(*file_path, false);
-    return false;
-  }
-
-  // TODO(ncbray): can we ensure temp file deletion even if we crash?
-  return true;
-}
-
-bool TempFileForURL(mojo::URLLoaderPtr& url_loader,
-                    const GURL& url,
-                    base::FilePath* path) {
+FILE* TempFileForURL(mojo::URLLoaderPtr& url_loader,
+                    const GURL& url) {

[Mark Seaborn, 2015/09/02 18:46:45]

Nit: Fix indentation.  Fits on previous line?

You could try using "git cl format" for this.

[Sean Klein, 2015/09/02 21:49:11]

Done.

   mojo::URLRequestPtr request(mojo::URLRequest::New());
   request->url = url.spec();
   request->method = "GET";
@@ -61,22 +44,10 @@ bool TempFileForURL(mojo::URLLoaderPtr& url_loader,
     LOG(ERROR) << "could not load " << url.spec() << " ("
                << response->error->code << ") "
                << response->error->description.get().c_str();
-    return false;
+    return NULL;
   }
 
-  return URLResponseToTempFile(response, path);
-}
-
-NaClDesc* NaClDescFromNexePath(base::FilePath& path) {
-  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ |
-                            base::File::FLAG_EXECUTE);
-  if (!file.IsValid()) {
-    LOG(FATAL) << "failed to open " << path.value();
-  }
-  // Note: potentially unsafe, assumes the file is immutable. This should be
-  // OK because we're currently only using temp files.
-  return NaClDescCreateWithFilePathMetadata(file.TakePlatformFile(),
-                                            path.AsUTF8Unsafe().c_str());
+  return mojo::common::BlockingCopyToTempFile(response->body.Pass());
 }
 
 }  // namespace
@@ -109,8 +80,8 @@ class NaClContentHandler : public mojo::ApplicationDelegate,
       LOG(FATAL) << "Cannot resolve IRT URL";
     }
 
-    if (!TempFileForURL(url_loader, irt_url, &irt_path_)) {
-      LOG(FATAL) << "Could not aquire the IRT";
+    if ((irt_fp_ = TempFileForURL(url_loader, irt_url)) == NULL) {

[Mark Seaborn, 2015/09/02 18:46:45]

I'd prefer using two lines for this, to avoid doing assignments inside
conditionals.

(I'm not sure if the style guide takes a position on this.)

[Sean Klein, 2015/09/02 21:49:11]

Done.

+      LOG(FATAL) << "Could not acquire the IRT";
     }
   }
 
@@ -128,33 +99,53 @@ class NaClContentHandler : public mojo::ApplicationDelegate,
     // Needed to use Mojo interfaces on this thread.
     base::MessageLoop loop(mojo::common::MessagePumpMojo::Create());
 
-    // Aquire the nexe.
-    base::FilePath nexe_path;
-    if (!URLResponseToTempFile(response, &nexe_path)) {
+    // Acquire the nexe.
+    FILE* nexe_fp = mojo::common::BlockingCopyToTempFile(
+        response->body.Pass());
+    if (nexe_fp == NULL) {
       LOG(FATAL) << "could not redirect nexe to temp file";
     }
 
-    // Aquire the IRT.
+    // Acquire the IRT.
     mojo::URLLoaderPtr url_loader = url_loader_.Pass();
     LoadIRT(url_loader);
 
     // Get file handles to the IRT and nexe.
-    NaClDesc* irt_desc = NaClDescFromNexePath(irt_path_);
-    NaClDesc* nexe_desc = NaClDescFromNexePath(nexe_path);
+    int irt_fd = fileno(irt_fp_);

[Mark Seaborn, 2015/09/02 18:46:45]

It looks like you could combine:

fileno() + dup() + NaClDescCreateWithFilePathMetadata() + fclose()
(+ maybe BlockingCopyToTempFile())

into one function so that this sequence isn't duplicated between the nexe and
IRT cases.

[Sean Klein, 2015/09/02 21:49:11]

Done.

+    if (irt_fd == -1) {
+      LOG(FATAL) << "Could not open the irt stream pointer's file descriptor";
+    }
+    int nexe_fd = fileno(nexe_fp);
+    if (nexe_fd == -1) {
+      LOG(FATAL) << "Could not open the nexe stream pointer's file descriptor";
+    }
+
+    // These file descriptors must be dup'd, since NaClDesc takes ownership
+    // of the file descriptor. The descriptor is still needed to close
+    // the file stream.
+    if ((irt_fd = dup(irt_fd)) == -1) {
+      LOG(FATAL) << "Could not dup the irt file descriptor";
+    }
+    if ((nexe_fd = dup(nexe_fd)) == -1) {
+      LOG(FATAL) << "Could not dup the nexe file descriptor";
+    }
+
+    NaClDesc* irt_desc = NaClDescCreateWithFilePathMetadata(
+        irt_fd, "");
+    NaClDesc* nexe_desc = NaClDescCreateWithFilePathMetadata(
+        nexe_fd, "");
 
     // Run.
     int exit_code = mojo::LaunchNaCl(
         nexe_desc, irt_desc, 0, NULL,
         application_request.PassMessagePipe().release().value());
 
-    // TODO(ncbray): are the file handles actually closed at this point?
-
     // Clean up.
-    if (!base::DeleteFile(irt_path_, false)) {
-      LOG(FATAL) << "Failed to remove irt temp file " << irt_path_.value();
+    if (fclose(irt_fp_)) {

[Mark Seaborn, 2015/09/02 18:46:45]

Nit: you could do this before LaunchNaCl()

[Sean Klein, 2015/09/02 21:49:11]

Done.

+      LOG(FATAL) << "Failed to close irt temp file";
     }
-    if (!base::DeleteFile(nexe_path, false)) {
-      LOG(FATAL) << "Failed to remove nexe temp file " << nexe_path.value();
+    if (fclose(nexe_fp)) {
+      LOG(FATAL) << "Failed to close nexe temp file";
     }
 
     // Exits the process cleanly, does not return.
@@ -164,7 +155,7 @@ class NaClContentHandler : public mojo::ApplicationDelegate,
 
   mojo::ContentHandlerFactory content_handler_factory_;
   GURL url_;
-  base::FilePath irt_path_;
+  FILE* irt_fp_;
 
   mojo::URLLoaderPtr url_loader_;