Pepper: TempFile cleanup.

This cleans up some usage of TempFile and some logic inside PnaclCoordinator to
make it easier to follow. I made these changes while doing some other cleanup
for DescWrapper use in the trusted plugin.

BUG=333950
R=jvoung@chromium.org

Review URL: https://codereview.chromium.org/299143004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273024 0039d316-1c4b-4281-b951-d872f2087c98

ppapi/native_client/src/trusted/plugin/plugin.cc

@@ -485,10 +485,11 @@ void Plugin::BitcodeDidTranslate(int32_t pp_error) {
   }
 
   // Inform JavaScript that we successfully translated the bitcode to a nexe.
-  nacl::scoped_ptr<nacl::DescWrapper>
-      wrapper(pnacl_coordinator_.get()->ReleaseTranslatedFD());
+  PP_FileHandle handle = pnacl_coordinator_->TakeTranslatedFileHandle();
+  int32_t fd = ConvertFileDescriptor(handle, true);
+  nacl::DescWrapper* wrapper = wrapper_factory()->MakeFileDesc(fd, O_RDONLY);
   LoadNaClModule(
-      wrapper.release(),
+      wrapper,
       false, /* uses_nonsfi_mode */
       false, /* enable_dyncode_syscalls */
       false, /* enable_exception_handling */

ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc

@@ -195,9 +195,9 @@ PnaclCoordinator::~PnaclCoordinator() {
   }
 }
 
-nacl::DescWrapper* PnaclCoordinator::ReleaseTranslatedFD() {
+PP_FileHandle PnaclCoordinator::TakeTranslatedFileHandle() {
   DCHECK(temp_nexe_file_ != NULL);
-  return temp_nexe_file_->release_read_wrapper();
+  return temp_nexe_file_->TakeFileHandle();
 }
 
 void PnaclCoordinator::ReportNonPpapiError(PP_NaClError err_code,
@@ -426,7 +426,7 @@ void PnaclCoordinator::ResourcesDidLoad(int32_t pp_error) {
           headers.c_str(),
           architecture_attributes_.c_str(), // Extra compile flags.
           &is_cache_hit_,
-          temp_nexe_file_->existing_handle(),
+          temp_nexe_file_->internal_handle(),
           cb.pp_completion_callback());
   if (nexe_fd_err < PP_OK_COMPLETIONPENDING) {
     ReportPpapiError(PP_NACL_ERROR_PNACL_CREATE_TEMP, nexe_fd_err,
@@ -436,16 +436,15 @@ void PnaclCoordinator::ResourcesDidLoad(int32_t pp_error) {
 
 void PnaclCoordinator::NexeFdDidOpen(int32_t pp_error) {
   PLUGIN_PRINTF(("PnaclCoordinator::NexeFdDidOpen (pp_error=%"
-                 NACL_PRId32 ", hit=%d, handle=%d)\n", pp_error,
-                 is_cache_hit_ == PP_TRUE,
-                 *temp_nexe_file_->existing_handle()));
+                 NACL_PRId32 ", hit=%d)\n", pp_error,
+                 is_cache_hit_ == PP_TRUE));
   if (pp_error < PP_OK) {
     ReportPpapiError(PP_NACL_ERROR_PNACL_CREATE_TEMP, pp_error,
                      nacl::string("GetNexeFd failed"));
     return;
   }
 
-  if (*temp_nexe_file_->existing_handle() == PP_kInvalidFileHandle) {
+  if (*temp_nexe_file_->internal_handle() == PP_kInvalidFileHandle) {
     ReportNonPpapiError(
         PP_NACL_ERROR_PNACL_CREATE_TEMP,
         nacl::string(
@@ -458,18 +457,20 @@ void PnaclCoordinator::NexeFdDidOpen(int32_t pp_error) {
     // Cache hit -- no need to stream the rest of the file.
     streaming_downloader_.reset(NULL);
     // Open it for reading as the cached nexe file.
-    pp::CompletionCallback cb =
-        callback_factory_.NewCallback(&PnaclCoordinator::NexeReadDidOpen);
-    temp_nexe_file_->Open(cb, false);
+    NexeReadDidOpen(temp_nexe_file_->Open(false));
   } else {
     // Open an object file first so the translator can start writing to it
     // during streaming translation.
     for (int i = 0; i < split_module_count_; i++) {
       obj_files_.push_back(new TempFile(plugin_));
-
-      pp::CompletionCallback obj_cb =
-          callback_factory_.NewCallback(&PnaclCoordinator::ObjectFileDidOpen);
-      obj_files_[i]->Open(obj_cb, true);
+      int32_t pp_error = obj_files_[i]->Open(true);
+      if (pp_error != PP_OK) {
+        ReportPpapiError(PP_NACL_ERROR_PNACL_CREATE_TEMP,
+                         pp_error,
+                         "Failed to open scratch object file.");
+      } else {
+        num_object_files_opened_++;
+      }
     }
     invalid_desc_wrapper_.reset(plugin_->wrapper_factory()->MakeInvalid());
 
@@ -480,6 +481,12 @@ void PnaclCoordinator::NexeFdDidOpen(int32_t pp_error) {
     pp::CompletionCallback finish_cb = callback_factory_.NewCallback(
         &PnaclCoordinator::BitcodeStreamDidFinish);
     streaming_downloader_->FinishStreaming(finish_cb);
+
+    if (num_object_files_opened_ == split_module_count_) {
+      // Open the nexe file for connecting ld and sel_ldr.
+      // Start translation when done with this last step of setup!
+      RunTranslate(temp_nexe_file_->Open(true));
+    }
   }
 }
 
@@ -586,25 +593,6 @@ void PnaclCoordinator::GetCurrentProgress(int64_t* bytes_loaded,
   *bytes_total = expected_pexe_size_;
 }
 
-void PnaclCoordinator::ObjectFileDidOpen(int32_t pp_error) {
-  PLUGIN_PRINTF(("PnaclCoordinator::ObjectFileDidOpen (pp_error=%"
-                 NACL_PRId32 ")\n", pp_error));
-  if (pp_error != PP_OK) {
-    ReportPpapiError(PP_NACL_ERROR_PNACL_CREATE_TEMP,
-                     pp_error,
-                     "Failed to open scratch object file.");
-    return;
-  }
-  num_object_files_opened_++;
-  if (num_object_files_opened_ == split_module_count_) {
-    // Open the nexe file for connecting ld and sel_ldr.
-    // Start translation when done with this last step of setup!
-    pp::CompletionCallback cb =
-        callback_factory_.NewCallback(&PnaclCoordinator::RunTranslate);
-    temp_nexe_file_->Open(cb, true);
-  }
-}
-
 void PnaclCoordinator::RunTranslate(int32_t pp_error) {
   PLUGIN_PRINTF(("PnaclCoordinator::RunTranslate (pp_error=%"
                  NACL_PRId32 ")\n", pp_error));

ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h

@@ -51,29 +51,6 @@ class TempFile;
 // Translation proceeds in two steps:
 // (1) llc translates the bitcode in pexe_url_ to an object in obj_file_.
 // (2) ld links the object code in obj_file_ and produces a nexe in nexe_file_.
-//
-// The coordinator proceeds through several states.  They are
-// OPEN_BITCODE_STREAM
-//       Complete when BitcodeStreamDidOpen is invoked
-// LOAD_TRANSLATOR_BINARIES
-//     Complete when ResourcesDidLoad is invoked.
-// GET_NEXE_FD
-//       Get an FD which contains the cached nexe, or is writeable for
-//       translation output. Complete when NexeFdDidOpen is called.
-//
-// If there was a cache hit, go to OPEN_NEXE_FOR_SEL_LDR, otherwise,
-// continue streaming the bitcode, and:
-// OPEN_TMP_FOR_LLC_TO_LD_COMMUNICATION
-//     Complete when ObjectFileDidOpen is invoked.
-// OPEN_NEXE_FD_FOR_WRITING
-//     Complete when RunTranslate is invoked.
-// START_LD_AND_LLC_SUBPROCESS_AND_INITIATE_TRANSLATION
-//     Complete when RunTranslate returns.
-// TRANSLATION_COMPLETE
-//     Complete when TranslateFinished is invoked.
-//
-// OPEN_NEXE_FOR_SEL_LDR
-//   Complete when NexeReadDidOpen is invoked.
 class PnaclCoordinator: public CallbackSource<FileStreamData> {
  public:
   // Maximum number of object files passable to the translator. Cannot be
@@ -90,7 +67,7 @@ class PnaclCoordinator: public CallbackSource<FileStreamData> {
 
   // Call this to take ownership of the FD of the translated nexe after
   // BitcodeToNative has completed (and the finish_callback called).
-  nacl::DescWrapper* ReleaseTranslatedFD();
+  PP_FileHandle TakeTranslatedFileHandle();
 
   // Run |translate_notify_callback_| with an error condition that is not
   // PPAPI specific.  Also set ErrorInfo report.
@@ -166,8 +143,6 @@ class PnaclCoordinator: public CallbackSource<FileStreamData> {
   void BitcodeGotCompiled(int32_t pp_error, int64_t bytes_compiled);
   // Invoked when the pexe download finishes (using streaming translation)
   void BitcodeStreamDidFinish(int32_t pp_error);
-  // Invoked when the write descriptor for obj_file_ is created.
-  void ObjectFileDidOpen(int32_t pp_error);
   // Once llc and ld nexes have been loaded and the two temporary files have
   // been created, this starts the translation.  Translation starts two
   // subprocesses, one for llc and one for ld.

ppapi/native_client/src/trusted/plugin/temporary_file.cc

@@ -19,32 +19,26 @@
 namespace plugin {
 
 TempFile::TempFile(Plugin* plugin) : plugin_(plugin),
-                                     existing_handle_(PP_kInvalidFileHandle) {
-  PLUGIN_PRINTF(("TempFile::TempFile\n"));
+                                     internal_handle_(PP_kInvalidFileHandle) {
 }
 
-TempFile::~TempFile() {
-  PLUGIN_PRINTF(("TempFile::~TempFile\n"));
-}
+TempFile::~TempFile() { }
 
-void TempFile::Open(const pp::CompletionCallback& cb, bool writeable) {
-  PLUGIN_PRINTF(("TempFile::Open\n"));
-  PP_FileHandle file_handle;
-  if (existing_handle_ == PP_kInvalidFileHandle) {
-    file_handle =
+int32_t TempFile::Open(bool writeable) {
+  // TODO(teravest): Clean up this Open() behavior; this is really confusing as
+  // written.
+  if (internal_handle_ == PP_kInvalidFileHandle) {
+    internal_handle_ =
         plugin_->nacl_interface()->CreateTemporaryFile(plugin_->pp_instance());
-  } else {
-    file_handle = existing_handle_;
   }
 
-  pp::Core* core = pp::Module::Get()->core();
-  if (file_handle == PP_kInvalidFileHandle) {
+  if (internal_handle_ == PP_kInvalidFileHandle) {
     PLUGIN_PRINTF(("TempFile::Open failed w/ PP_kInvalidFileHandle\n"));
-    core->CallOnMainThread(0, cb, PP_ERROR_FAILED);
+    return PP_ERROR_FAILED;
   }
 
 #if NACL_WINDOWS
-  HANDLE handle = file_handle;
+  HANDLE handle = internal_handle_;
 
   //////// Now try the posix view.
   int rdwr_flag = writeable ? _O_RDWR : _O_RDONLY;
@@ -58,21 +52,19 @@ void TempFile::Open(const pp::CompletionCallback& cb, bool writeable) {
   }
   int32_t fd = posix_desc;
 #else
-  int32_t fd = file_handle;
+  int32_t fd = internal_handle_;
 #endif
 
   if (fd < 0) {
     PLUGIN_PRINTF(("TempFile::Open failed\n"));
-    core->CallOnMainThread(0, cb, PP_ERROR_FAILED);
-    return;
+    return PP_ERROR_FAILED;
   }
 
   // dup the fd to make allow making separate read and write wrappers.
   int32_t read_fd = DUP(fd);
   if (read_fd == NACL_NO_FILE_DESC) {
     PLUGIN_PRINTF(("TempFile::Open DUP failed\n"));
-    core->CallOnMainThread(0, cb, PP_ERROR_FAILED);
-    return;
+    return PP_ERROR_FAILED;
   }
 
   if (writeable) {
@@ -82,7 +74,7 @@ void TempFile::Open(const pp::CompletionCallback& cb, bool writeable) {
 
   read_wrapper_.reset(
       plugin_->wrapper_factory()->MakeFileDesc(read_fd, O_RDONLY));
-  core->CallOnMainThread(0, cb, PP_OK);
+  return PP_OK;
 }
 
 bool TempFile::Reset() {
@@ -94,4 +86,12 @@ bool TempFile::Reset() {
   return newpos == 0;
 }
 
+PP_FileHandle TempFile::TakeFileHandle() {
+  PP_FileHandle to_return = internal_handle_;
+  internal_handle_ = PP_kInvalidFileHandle;
+  read_wrapper_.release();
+  write_wrapper_.release();
+  return to_return;
+}
+
 }  // namespace plugin

ppapi/native_client/src/trusted/plugin/temporary_file.h

@@ -9,7 +9,6 @@
 #include "native_client/src/trusted/desc/nacl_desc_wrapper.h"
 
 #include "ppapi/c/private/pp_file_handle.h"
-#include "ppapi/cpp/completion_callback.h"
 
 namespace plugin {
 
@@ -43,7 +42,7 @@ class TempFile {
   // Opens a temporary file object and descriptor wrapper referring to the file.
   // If |writeable| is true, the descriptor will be opened for writing, and
   // write_wrapper will return a valid pointer, otherwise it will return NULL.
-  void Open(const pp::CompletionCallback& cb, bool writeable);
+  int32_t Open(bool writeable);
   // Resets file position of the handle, for reuse.
   bool Reset();
 
@@ -51,11 +50,13 @@ class TempFile {
   // The nacl::DescWrapper* for the writeable version of the file.
   nacl::DescWrapper* write_wrapper() { return write_wrapper_.get(); }
   nacl::DescWrapper* read_wrapper() { return read_wrapper_.get(); }
-  nacl::DescWrapper* release_read_wrapper() {
-    return read_wrapper_.release();
-  }
 
-  PP_FileHandle* existing_handle() { return &existing_handle_; }
+  // Returns the handle to the file repesented and resets the internal handle
+  // and all wrappers.
+  PP_FileHandle TakeFileHandle();
+
+  // Used by GetNexeFd() to set the underlying internal handle.
+  PP_FileHandle* internal_handle() { return &internal_handle_; }
 
  private:
   NACL_DISALLOW_COPY_AND_ASSIGN(TempFile);
@@ -63,7 +64,7 @@ class TempFile {
   Plugin* plugin_;
   nacl::scoped_ptr<nacl::DescWrapper> read_wrapper_;
   nacl::scoped_ptr<nacl::DescWrapper> write_wrapper_;
-  PP_FileHandle existing_handle_;
+  PP_FileHandle internal_handle_;
 };
 
 }  // namespace plugin