Pepper: Narrow locking at ld subprocess start.

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

Index: ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
diff --git a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
index 75416a53848d492290c30aa175be5ecbd5df9a1f..821a7a6fa5ae4a2421589489364dbd89b6765636 100644
--- a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
+++ b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
@@ -339,29 +339,37 @@ bool PnaclTranslateThread::RunLdSubprocess() {
 
   nacl::DescWrapper* ld_out_file = nexe_file_->write_wrapper();
   pp::Core* core = pp::Module::Get()->core();
+  int64_t ld_start_time = NaClGetTimeOfDayMicroseconds();
+  PP_FileHandle ld_file_handle = resources_->TakeLdFileHandle();
+  // On success, ownership of ld_file_handle is transferred.
+  NaClSubprocess* ld_subprocess = plugin_->LoadHelperNaClModule(

[bbudge, 2014/06/02 16:58:04]

Why not make ld_subprocess a nacl::scoped_ptr? We'll probably use scoped_ptr
when this code is moved to the renderer anyway.

Then below, you can swap with the ld_subprocess_ field.

+       resources_->GetLlcUrl(), ld_file_handle, &error_info);
+  if (ld_subprocess == NULL) {
+    if (ld_file_handle != PP_kInvalidFileHandle)
+      CloseFileHandle(ld_file_handle);
+    TranslateFailed(PP_NACL_ERROR_PNACL_LD_SETUP,
+                    "Link process could not be created: " +
+                    error_info.message());
+    return false;
+  }
+  core->CallOnMainThread(0,
+                         coordinator_->GetUMATimeCallback(
+                             "NaCl.Perf.PNaClLoadTime.LoadLinker",
+                             NaClGetTimeOfDayMicroseconds() - ld_start_time),
+                         PP_OK);
+
   {
-    // Create LD process
     nacl::MutexLocker ml(&subprocess_mu_);
-    int64_t ld_start_time = NaClGetTimeOfDayMicroseconds();
-    PP_FileHandle ld_file_handle = resources_->TakeLdFileHandle();
-
-    // On success, ownership of ld_file_handle is transferred.
-    ld_subprocess_.reset(plugin_->LoadHelperNaClModule(
-        resources_->GetLdUrl(), ld_file_handle, &error_info));
-    if (ld_subprocess_ == NULL) {
-      if (ld_file_handle != PP_kInvalidFileHandle)
-        CloseFileHandle(ld_file_handle);
-      TranslateFailed(PP_NACL_ERROR_PNACL_LD_SETUP,
-                      "Link process could not be created: " +
-                      error_info.message());
+    // If we received a call to AbortSubprocesses() before we had a chance to
+    // set llc_subprocess_, shut down and clean up the subprocess started here.
+    if (subprocesses_aborted_) {
+      ld_subprocess->service_runtime()->Shutdown();
+      delete ld_subprocess;
       return false;
     }
+    ld_subprocess_.reset(ld_subprocess);
+    ld_subprocess = NULL;

[bbudge, 2014/06/02 16:58:04]

This seems unnecessary, since it's a local and we're exiting its scope.

     ld_subprocess_active_ = true;
-    core->CallOnMainThread(0,
-                           coordinator_->GetUMATimeCallback(
-                               "NaCl.Perf.PNaClLoadTime.LoadLinker",
-                               NaClGetTimeOfDayMicroseconds() - ld_start_time),
-                           PP_OK);
   }
 
   int64_t link_start_time = NaClGetTimeOfDayMicroseconds();