Remove PlatformFile from components/nacl

components/nacl/renderer/ppb_nacl_private_impl.cc

 // Copyright 2013 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 "components/nacl/renderer/ppb_nacl_private_impl.h"
 
 #include <numeric>
 #include <string>
 #include <vector>
 
@@ skipping 535 matching lines @@
 }
 
 PP_FileHandle GetReadonlyPnaclFd(const char* url) {
   std::string filename = PnaclComponentURLToFilename(url);
   IPC::PlatformFileForTransit out_fd = IPC::InvalidPlatformFileForTransit();
   IPC::Sender* sender = content::RenderThread::Get();
   DCHECK(sender);
   if (!sender->Send(new NaClHostMsg_GetReadonlyPnaclFD(
           std::string(filename),
           &out_fd))) {
-    return base::kInvalidPlatformFileValue;
+    return PP_kInvalidFileHandle;
   }
   if (out_fd == IPC::InvalidPlatformFileForTransit()) {
-    return base::kInvalidPlatformFileValue;
+    return PP_kInvalidFileHandle;
   }
-  base::PlatformFile handle =
-      IPC::PlatformFileForTransitToPlatformFile(out_fd);
-  return handle;
+  return IPC::PlatformFileForTransitToPlatformFile(out_fd);
 }
 
 PP_FileHandle CreateTemporaryFile(PP_Instance instance) {
   IPC::PlatformFileForTransit transit_fd = IPC::InvalidPlatformFileForTransit();
   IPC::Sender* sender = content::RenderThread::Get();
   DCHECK(sender);
   if (!sender->Send(new NaClHostMsg_NaClCreateTemporaryFile(
           &transit_fd))) {
-    return base::kInvalidPlatformFileValue;
+    return PP_kInvalidFileHandle;
   }
 
   if (transit_fd == IPC::InvalidPlatformFileForTransit()) {
-    return base::kInvalidPlatformFileValue;
+    return PP_kInvalidFileHandle;
   }
 
-  base::PlatformFile handle = IPC::PlatformFileForTransitToPlatformFile(
-      transit_fd);
-  return handle;
+  return IPC::PlatformFileForTransitToPlatformFile(transit_fd);
 }
 
 int32_t GetNumberOfProcessors() {
   int32_t num_processors;
   IPC::Sender* sender = content::RenderThread::Get();
   DCHECK(sender);
   if(!sender->Send(new NaClHostMsg_NaClGetNumProcessors(&num_processors))) {
     return 1;
   }
   return num_processors;
@@ skipping 101 matching lines @@
   DCHECK(sender);
   *nonce_lo = 0;
   *nonce_hi = 0;
   base::FilePath file_path;
   if (!sender->Send(
       new NaClHostMsg_OpenNaClExecutable(GetRoutingID(instance),
                                          GURL(file_url),
                                          &out_fd,
                                          nonce_lo,
                                          nonce_hi))) {
-    return base::kInvalidPlatformFileValue;
+    return PP_kInvalidFileHandle;
   }
 
-  if (out_fd == IPC::InvalidPlatformFileForTransit()) {
-    return base::kInvalidPlatformFileValue;
-  }
+  if (out_fd == IPC::InvalidPlatformFileForTransit())
+    return PP_kInvalidFileHandle;
 
-  base::PlatformFile handle =
-      IPC::PlatformFileForTransitToPlatformFile(out_fd);
-  return handle;
+  return IPC::PlatformFileForTransitToPlatformFile(out_fd);
 }
 
 void DispatchEvent(PP_Instance instance,
                    PP_NaClEventType event_type,
                    const char *resource_url,
                    PP_Bool length_is_computable,
                    uint64_t loaded_bytes,
                    uint64_t total_bytes) {
   ProgressEvent event(event_type,
                       resource_url,
@@ skipping 397 matching lines @@
 
 PP_Bool GetPNaClResourceInfo(PP_Instance instance,
                              const char* filename,
                              PP_Var* llc_tool_name,
                              PP_Var* ld_tool_name) {
   NexeLoadManager* load_manager = GetNexeLoadManager(instance);
   DCHECK(load_manager);
   if (!load_manager)
     return PP_FALSE;
 
-  base::PlatformFile file = GetReadonlyPnaclFd(filename);
-  if (file == base::kInvalidPlatformFileValue) {
+  // This file was not being closed!

[rvargas (doing something else), 2014/06/03 17:57:23]

Is this OK?

[Mark Seaborn, 2014/06/03 18:23:21]

I don't understand the comment.  Is it referring to what the code used to do? 
If you're fixing something, doesn't this explanation belong in the commit
message instead?

[rvargas (doing something else), 2014/06/03 19:59:49]

In any case, the comment will be removed... it was a line to remind me to point
that out on the review. There was a couple of bugs in the code, but I'm not
familiar with this code to know if there was a reason to leak the file in this
code path.

So, rephrasing... is it OK to close the file here, or was the leak intentional?

[teravest, 2014/06/03 20:04:53]

It should be OK to close the file there.

+  base::File file(GetReadonlyPnaclFd(filename));
+  if (!file.IsValid()) {
     load_manager->ReportLoadError(
         PP_NACL_ERROR_PNACL_RESOURCE_FETCH,
         "The Portable Native Client (pnacl) component is not "
         "installed. Please consult chrome://components for more "
         "information.");
     return PP_FALSE;
   }
 
-  base::PlatformFileInfo file_info;
-  if (!GetPlatformFileInfo(file, &file_info)) {
+  base::File::Info file_info;
+  if (!file.GetInfo(&file_info)) {
     load_manager->ReportLoadError(
         PP_NACL_ERROR_PNACL_RESOURCE_FETCH,
         std::string("GetPNaClResourceInfo, GetFileInfo failed for: ") +
             filename);
     return PP_FALSE;
   }
 
   if (file_info.size > 1 << 20) {
     load_manager->ReportLoadError(
         PP_NACL_ERROR_PNACL_RESOURCE_FETCH,
         std::string("GetPNaClResourceInfo, file too large: ") + filename);
     return PP_FALSE;
   }
 
   scoped_ptr<char[]> buffer(new char[file_info.size + 1]);
   if (buffer.get() == NULL) {
     load_manager->ReportLoadError(
         PP_NACL_ERROR_PNACL_RESOURCE_FETCH,
         std::string("GetPNaClResourceInfo, couldn't allocate for: ") +
             filename);
     return PP_FALSE;
   }
 
-  int rc = base::ReadPlatformFile(file, 0, buffer.get(), file_info.size);
+  int rc = file.Read(0, buffer.get(), file_info.size);
   if (rc < 0) {
     load_manager->ReportLoadError(
         PP_NACL_ERROR_PNACL_RESOURCE_FETCH,
         std::string("GetPNaClResourceInfo, reading failed for: ") + filename);
     return PP_FALSE;
   }
 
   // Null-terminate the bytes we we read from the file.
   buffer.get()[rc] = 0;
 
@@ skipping 132 matching lines @@
       last_event_ = now;
     }
   }
 
  private:
   PP_Instance instance_;
   base::Time last_event_;
 };
 
 void DownloadNexeCompletion(const DownloadNexeRequest& request,
-                            base::PlatformFile target_file,
                             PP_NaClFileInfo* out_file_info,
                             FileDownloader::Status status,
+                            base::File target_file,
                             int http_status);
 
 void DownloadNexe(PP_Instance instance,
                   const char* url,
                   PP_NaClFileInfo* out_file_info,
                   PP_CompletionCallback callback) {
   CHECK(url);
   CHECK(out_file_info);
   DownloadNexeRequest request;
   request.instance = instance;
   request.url = url;
   request.callback = callback;
   request.start_time = base::Time::Now();
 
   // Try the fast path for retrieving the file first.
   PP_FileHandle handle = OpenNaClExecutable(instance,
                                             url,
                                             &out_file_info->token_lo,
                                             &out_file_info->token_hi);
   if (handle != PP_kInvalidFileHandle) {
     DownloadNexeCompletion(request,
-                           handle,
                            out_file_info,
                            FileDownloader::SUCCESS,
+                           base::File(handle),
                            200);
     return;
   }
 
   // The fast path didn't work, we'll fetch the file using URLLoader and write
   // it to local storage.
-  base::PlatformFile target_file = CreateTemporaryFile(instance);
+  base::File target_file(CreateTemporaryFile(instance));
   GURL gurl(url);
 
   content::PepperPluginInstance* plugin_instance =
       content::PepperPluginInstance::Get(instance);
   if (!plugin_instance) {
     ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
         FROM_HERE,
         base::Bind(callback.func, callback.user_data,
                    static_cast<int32_t>(PP_ERROR_FAILED)));
   }
   const blink::WebDocument& document =
       plugin_instance->GetContainer()->element().document();
   scoped_ptr<blink::WebURLLoader> url_loader(
       CreateWebURLLoader(document, gurl));
   blink::WebURLRequest url_request = CreateWebURLRequest(document, gurl);
 
   ProgressEventRateLimiter* tracker = new ProgressEventRateLimiter(instance);
 
   // FileDownloader deletes itself after invoking DownloadNexeCompletion.
   FileDownloader* file_downloader = new FileDownloader(
       url_loader.Pass(),
-      target_file,
-      base::Bind(&DownloadNexeCompletion, request, target_file, out_file_info),
+      target_file.Pass(),
+      base::Bind(&DownloadNexeCompletion, request, out_file_info),
       base::Bind(&ProgressEventRateLimiter::ReportProgress,
                  base::Owned(tracker), url));
   file_downloader->Load(url_request);
 }
 
 void DownloadNexeCompletion(const DownloadNexeRequest& request,
-                            base::PlatformFile target_file,
                             PP_NaClFileInfo* out_file_info,
                             FileDownloader::Status status,
+                            base::File target_file,
                             int http_status) {
   int32_t pp_error = FileDownloaderToPepperError(status);
-  if (pp_error == PP_OK)
-    out_file_info->handle = target_file;
-
   int64_t bytes_read = -1;
-  if (pp_error == PP_OK && target_file != base::kInvalidPlatformFileValue) {
-    base::PlatformFileInfo info;
-    if (GetPlatformFileInfo(target_file, &info))
+  if (pp_error == PP_OK && target_file.IsValid()) {
+    base::File::Info info;
+    if (target_file.GetInfo(&info))
       bytes_read = info.size;
   }
 
   if (bytes_read == -1) {
-    base::ClosePlatformFile(target_file);
+    target_file.Close();
     pp_error = PP_ERROR_FAILED;
   }
 
   base::TimeDelta download_time = base::Time::Now() - request.start_time;
 
   NexeLoadManager* load_manager = GetNexeLoadManager(request.instance);
   if (load_manager) {
     load_manager->NexeFileDidOpen(pp_error,
                                   target_file,
                                   http_status,
                                   bytes_read,
                                   request.url,
                                   download_time);
   }
 
+  if (pp_error == PP_OK && target_file.IsValid())
+    out_file_info->handle = target_file.TakePlatformFile();
+  else
+    out_file_info->handle = PP_kInvalidFileHandle;
+
   request.callback.func(request.callback.user_data, pp_error);
 }
 
-void DownloadFileCompletion(base::PlatformFile file,
-                            PP_NaClFileInfo* file_info,
+void DownloadFileCompletion(PP_NaClFileInfo* file_info,
                             PP_CompletionCallback callback,
                             FileDownloader::Status status,
+                            base::File file,
                             int http_status) {
   int32_t pp_error = FileDownloaderToPepperError(status);
   if (pp_error == PP_OK) {
-    file_info->handle = file;
+    file_info->handle = file.TakePlatformFile();
     file_info->token_lo = 0;
     file_info->token_hi = 0;
   }
   callback.func(callback.user_data, pp_error);
 }
 
 void DownloadFile(PP_Instance instance,
                   const char* url,
                   struct PP_NaClFileInfo* file_info,
                   struct PP_CompletionCallback callback) {
@@ skipping 58 matching lines @@
     file_info->token_hi = file_token_hi;
     ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
         FROM_HERE,
         base::Bind(callback.func, callback.user_data,
                    static_cast<int32_t>(PP_OK)));
     return;
   }
 
   // The fast path didn't work, we'll fetch the file using URLLoader and write
   // it to local storage.
-  base::PlatformFile target_file = CreateTemporaryFile(instance);
+  base::File target_file(CreateTemporaryFile(instance));
   GURL gurl(url);
 
   content::PepperPluginInstance* plugin_instance =
       content::PepperPluginInstance::Get(instance);
   if (!plugin_instance) {
     ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
         FROM_HERE,
         base::Bind(callback.func, callback.user_data,
                    static_cast<int32_t>(PP_ERROR_FAILED)));
   }
   const blink::WebDocument& document =
       plugin_instance->GetContainer()->element().document();
   scoped_ptr<blink::WebURLLoader> url_loader(
       CreateWebURLLoader(document, gurl));
   blink::WebURLRequest url_request = CreateWebURLRequest(document, gurl);
 
   ProgressEventRateLimiter* tracker = new ProgressEventRateLimiter(instance);
 
   // FileDownloader deletes itself after invoking DownloadNexeCompletion.
   FileDownloader* file_downloader = new FileDownloader(
       url_loader.Pass(),
-      target_file,
-      base::Bind(&DownloadFileCompletion, target_file, file_info, callback),
+      target_file.Pass(),
+      base::Bind(&DownloadFileCompletion, file_info, callback),
       base::Bind(&ProgressEventRateLimiter::ReportProgress,
                  base::Owned(tracker), url));
   file_downloader->Load(url_request);
 }
 
 const PPB_NaCl_Private nacl_interface = {
   &LaunchSelLdr,
   &StartPpapiProxy,
   &UrandomFD,
   &Are3DInterfacesDisabled,
@@ skipping 35 matching lines @@
   &DownloadFile
 };
 
 }  // namespace
 
 const PPB_NaCl_Private* GetNaClPrivateInterface() {
   return &nacl_interface;
 }
 
 }  // namespace nacl