Pepper: Narrow locking at llc subprocess start.

Currently, subprocess_mu_ is held the entire time the llc subprocess is
starting. This blocks a larger refactor that I'd like to make as part of nexe
loading.

I'd like to change ServiceRuntime::LoadModule() to be asynchronous to simplify
threading behavior on the plugin side when loading nexe modules. However, this
requires that many methods are made asynchronous, including
Plugin::LoadHelperNaClModule().

PnaclTranslateThread will need to use a pattern similar to WaitForSelLdrStart()
to resume execution on a background thread. However, I don't like the idea of
introducing another mutex and condvar to deal with while holding
subprocess_mu_.

This change narrows the time that subprocess_mu_ is held to make that refactor
possible. I'll make a similar change for the ld subprocess, but I wanted to get
this reviewed before proceeding further.

BUG=333950
R=bbudge@chromium.org

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

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

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

@@ -54,6 +54,7 @@ void GetLlcCommandLine(Plugin* plugin,
 
 PnaclTranslateThread::PnaclTranslateThread() : llc_subprocess_active_(false),
                                                ld_subprocess_active_(false),
+                                               subprocesses_aborted_(false),
                                                done_(false),
                                                compile_time_(0),
                                                obj_files_(NULL),
@@ -160,15 +161,12 @@ void PnaclTranslateThread::DoTranslate() {
     llc_out_files.push_back(invalid_desc_wrapper_);
 
   pp::Core* core = pp::Module::Get()->core();
-  {
-    nacl::MutexLocker ml(&subprocess_mu_);
   int64_t llc_start_time = NaClGetTimeOfDayMicroseconds();
   PP_FileHandle llc_file_handle = resources_->TakeLlcFileHandle();
-
   // On success, ownership of llc_file_handle is transferred.
-    llc_subprocess_.reset(plugin_->LoadHelperNaClModule(
-        resources_->GetLlcUrl(), llc_file_handle, &error_info));
-    if (llc_subprocess_.get() == NULL) {
+  NaClSubprocess* llc_subprocess = plugin_->LoadHelperNaClModule(
+      resources_->GetLlcUrl(), llc_file_handle, &error_info);
+  if (llc_subprocess == NULL) {
     if (llc_file_handle != PP_kInvalidFileHandle)
       CloseFileHandle(llc_file_handle);
     TranslateFailed(PP_NACL_ERROR_PNACL_LLC_SETUP,
@@ -176,13 +174,24 @@ void PnaclTranslateThread::DoTranslate() {
                     error_info.message());
     return;
   }
-
-    llc_subprocess_active_ = true;
   core->CallOnMainThread(0,
                          coordinator_->GetUMATimeCallback(
                              "NaCl.Perf.PNaClLoadTime.LoadCompiler",
                              NaClGetTimeOfDayMicroseconds() - llc_start_time),
                          PP_OK);
+
+  {
+    nacl::MutexLocker ml(&subprocess_mu_);
+    // 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_) {
+      llc_subprocess->service_runtime()->Shutdown();
+      delete llc_subprocess;
+      return;
+    }
+    llc_subprocess_.reset(llc_subprocess);
+    llc_subprocess = NULL;
+    llc_subprocess_active_ = true;
   }
 
   int64_t compile_start_time = NaClGetTimeOfDayMicroseconds();
@@ -426,6 +435,7 @@ void PnaclTranslateThread::AbortSubprocesses() {
     ld_subprocess_->service_runtime()->Shutdown();
     ld_subprocess_active_ = false;
   }
+  subprocesses_aborted_ = true;
   NaClXMutexUnlock(&subprocess_mu_);
   nacl::MutexLocker ml(&cond_mu_);
   done_ = true;

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

@@ -92,6 +92,8 @@ class PnaclTranslateThread {
   bool llc_subprocess_active_;
   bool ld_subprocess_active_;
 
+  bool subprocesses_aborted_;
+
   // Condition variable to synchronize communication with the SRPC thread.
   // SRPC thread waits on this condvar if data_buffers_ is empty (meaning
   // there is no bitcode to send to the translator), and the main thread