Kill pnacl translation processes immediately on coordinator error and destruction

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 441345aada0a7a0777efd9f5a69f0209c9ee5cdc..f44861e05ca20393af95e49cf0abb0ffd4cc362a 100644
--- a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
+++ b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
@@ -13,9 +13,7 @@
 
 namespace plugin {
 
-PnaclTranslateThread::PnaclTranslateThread() : subprocesses_should_die_(false),
-                                               current_rev_interface_(NULL),
-                                               done_(false),
+PnaclTranslateThread::PnaclTranslateThread() : done_(false),
                                                manifest_(NULL),
                                                ld_manifest_(NULL),
                                                obj_file_(NULL),
@@ -122,28 +120,36 @@ void WINAPI PnaclTranslateThread::DoTranslateThread(void* arg) {
 
 void PnaclTranslateThread::DoTranslate() {
   ErrorInfo error_info;
-  nacl::scoped_ptr<NaClSubprocess> llc_subprocess(
-      StartSubprocess(PnaclUrls::GetLlcUrl(), manifest_, &error_info));
-  if (llc_subprocess == NULL) {
-    TranslateFailed("Compile process could not be created: " +
-                    error_info.message());
-    return;
-  }
-  // Run LLC.
   SrpcParams params;
   nacl::DescWrapper* llc_out_file = obj_file_->get_wrapper();
-  PluginReverseInterface* llc_reverse =
-      llc_subprocess->service_runtime()->rev_interface();
-  llc_reverse->AddTempQuotaManagedFile(obj_file_->identifier());
-  RegisterReverseInterface(llc_reverse);
 
-  if (!llc_subprocess->InvokeSrpcMethod("StreamInit",
-                                        "h",
-                                        &params,
-                                        llc_out_file->desc())) {
-    // StreamInit returns an error message if the RPC fails.
-    TranslateFailed(nacl::string("Stream init failed: ") +
-                    nacl::string(params.outs()[0]->arrays.str));
+  {
+    nacl::MutexLocker ml(&subprocess_mu_);
+    llc_subprocess_.reset(
+      StartSubprocess(PnaclUrls::GetLlcUrl(), manifest_, &error_info));
+    if (llc_subprocess_ == NULL) {
+      TranslateFailed("Compile process could not be created: " +
+                      error_info.message());
+      return;
+    }
+    // Run LLC.
+    PluginReverseInterface* llc_reverse =
+        llc_subprocess_->service_runtime()->rev_interface();
+    llc_reverse->AddTempQuotaManagedFile(obj_file_->identifier());
+  }
+
+  if (!llc_subprocess_->InvokeSrpcMethod("StreamInit",
+                                         "h",
+                                         &params,
+                                         llc_out_file->desc())) {
+    if (llc_subprocess_->srpc_client()->GetLastError() ==
+        NACL_SRPC_RESULT_APP_ERROR) {
+      // The error message is only present if the error was returned from llc
+      TranslateFailed(nacl::string("Stream init failed: ") +
+                      nacl::string(params.outs()[0]->arrays.str));
+    } else {
+      TranslateFailed("Stream init internal error");
+    }
     return;
   }
 
@@ -163,11 +169,11 @@ void PnaclTranslateThread::DoTranslate() {
       data_buffers_.pop_front();
       NaClXMutexUnlock(&cond_mu_);
       PLUGIN_PRINTF(("StreamChunk\n"));
-      if (!llc_subprocess->InvokeSrpcMethod("StreamChunk",
-                                            "C",
-                                            &params,
-                                            &data[0],
-                                            data.size())) {
+      if (!llc_subprocess_->InvokeSrpcMethod("StreamChunk",
+                                             "C",
+                                             &params,
+                                             &data[0],
+                                             data.size())) {
         TranslateFailed("Compile stream chunk failed.");
         return;
       }
@@ -175,18 +181,20 @@ void PnaclTranslateThread::DoTranslate() {
     } else {
       NaClXMutexUnlock(&cond_mu_);
     }
-    if (SubprocessesShouldDie()) {
-      TranslateFailed("Stopped by coordinator.");
-      return;
-    }
   }
   PLUGIN_PRINTF(("PnaclTranslateThread done with chunks\n"));
   // Finish llc.
-  if(!llc_subprocess->InvokeSrpcMethod("StreamEnd",
+  if(!llc_subprocess_->InvokeSrpcMethod("StreamEnd",
                                        "",
                                        &params)) {
     PLUGIN_PRINTF(("PnaclTranslateThread StreamEnd failed\n"));
-    TranslateFailed(params.outs()[3]->arrays.str);
+    if (llc_subprocess_->srpc_client()->GetLastError() ==
+        NACL_SRPC_RESULT_APP_ERROR) {
+      // The error string is only present if the error was sent back from llc
+      TranslateFailed(params.outs()[3]->arrays.str);
+    } else {
+      TranslateFailed("Compile StreamEnd internal error");
+    }
     return;
   }
   // LLC returns values that are used to determine how linking is done.
@@ -199,12 +207,9 @@ void PnaclTranslateThread::DoTranslate() {
                  lib_dependencies.c_str()));
 
   // Shut down the llc subprocess.
-  RegisterReverseInterface(NULL);
-  llc_subprocess.reset(NULL);
-  if (SubprocessesShouldDie()) {
-    TranslateFailed("stopped by coordinator.");
-    return;
-  }
+  NaClXMutexLock(&subprocess_mu_);
+  llc_subprocess_.reset(NULL);
+  NaClXMutexUnlock(&subprocess_mu_);
 
   if(!RunLdSubprocess(is_shared_library, soname, lib_dependencies)) {
     return;
@@ -218,16 +223,7 @@ bool PnaclTranslateThread::RunLdSubprocess(int is_shared_library,
                                            const nacl::string& lib_dependencies
                                            ) {
   ErrorInfo error_info;
-  nacl::scoped_ptr<NaClSubprocess> ld_subprocess(
-      StartSubprocess(PnaclUrls::GetLdUrl(), ld_manifest_, &error_info));
-  if (ld_subprocess == NULL) {
-    TranslateFailed("Link process could not be created: " +
-                    error_info.message());
-    return false;
-  }
-  // Run LD.
   SrpcParams params;
-
   // Reset object file for reading first.
   if (!obj_file_->Reset()) {
     TranslateFailed("Link process could not reset object file");
@@ -235,12 +231,24 @@ bool PnaclTranslateThread::RunLdSubprocess(int is_shared_library,
   }
   nacl::DescWrapper* ld_in_file = obj_file_->get_wrapper();
   nacl::DescWrapper* ld_out_file = nexe_file_->write_wrapper();
-  PluginReverseInterface* ld_reverse =
-      ld_subprocess->service_runtime()->rev_interface();
-  ld_reverse->AddQuotaManagedFile(nexe_file_->identifier(),
-                                  nexe_file_->write_file_io());
-  RegisterReverseInterface(ld_reverse);
-  if (!ld_subprocess->InvokeSrpcMethod("RunWithDefaultCommandLine",
+
+  {
+    // Create LD process
+    nacl::MutexLocker ml(&subprocess_mu_);
+    ld_subprocess_.reset(
+      StartSubprocess(PnaclUrls::GetLdUrl(), ld_manifest_, &error_info));
+    if (ld_subprocess_ == NULL) {
+      TranslateFailed("Link process could not be created: " +
+                      error_info.message());
+      return false;
+    }
+    PluginReverseInterface* ld_reverse =
+        ld_subprocess_->service_runtime()->rev_interface();
+    ld_reverse->AddQuotaManagedFile(nexe_file_->identifier(),
+                                    nexe_file_->write_file_io());
+  }
+  // Run LD.
+  if (!ld_subprocess_->InvokeSrpcMethod("RunWithDefaultCommandLine",
                                        "hhiss",
                                        &params,
                                        ld_in_file->desc(),
@@ -254,12 +262,9 @@ bool PnaclTranslateThread::RunLdSubprocess(int is_shared_library,
   PLUGIN_PRINTF(("PnaclCoordinator: link (translator=%p) succeeded\n",
                  this));
   // Shut down the ld subprocess.
-  RegisterReverseInterface(NULL);
-  ld_subprocess.reset(NULL);
-  if (SubprocessesShouldDie()) {
-    TranslateFailed("stopped by coordinator.");
-    return false;
-  }
+  NaClXMutexLock(&subprocess_mu_);
+  ld_subprocess_.reset(NULL);
+  NaClXMutexUnlock(&subprocess_mu_);
   return true;
 }
 
@@ -277,43 +282,26 @@ void PnaclTranslateThread::TranslateFailed(const nacl::string& error_string) {
   core->CallOnMainThread(0, report_translate_finished_, PP_ERROR_FAILED);
 }
 
-// This synchronization method (using the pointer directly in the
-// translation thread, setting a copy here, and calling shutdown on the
-// main thread) is safe only because only the translation thread sets
-// the copy, and the shutdown method is thread-safe. This method must be
-// called on the translation thread before any RPCs are called, and called
-// again with NULL before the object is destroyed.
-void PnaclTranslateThread::RegisterReverseInterface(
-    PluginReverseInterface *iface) {
-  nacl::MutexLocker ml(&subprocess_mu_);
-  current_rev_interface_ = iface;
-}
-
-
-bool PnaclTranslateThread::SubprocessesShouldDie() {
-  nacl::MutexLocker ml(&subprocess_mu_);
-  return subprocesses_should_die_;
-}
-
-void PnaclTranslateThread::SetSubprocessesShouldDie() {
-  PLUGIN_PRINTF(("PnaclTranslateThread::SetSubprocessesShouldDie\n"));
+void PnaclTranslateThread::AbortSubprocesses() {
+  PLUGIN_PRINTF(("PnaclTranslateThread::AbortSubprocesses\n"));
   NaClXMutexLock(&subprocess_mu_);
-  subprocesses_should_die_ = true;
-  if (current_rev_interface_) {
-    current_rev_interface_->ShutDown();
-    current_rev_interface_ = NULL;
-  }
+  if (llc_subprocess_ != NULL) llc_subprocess_->service_runtime()->Shutdown();

[jvoung (off chromium), 2012/08/02 23:24:10]

Since set llc_subprocess is not set to null after, and AbortSubprocess() is
called from more than one destructor, is it okay to
service_runtime()->Shutdown() more than once?

[Derek Schuff, 2012/08/02 23:46:45]

I *think* so, but it actually goes pretty deep into SRPC destructors and such,
so just to be safe I went ahead and added a guard to avoid calling Shutdown()
more than once.
We do have to wait to destroy the object until after the RPCs are done, however.
On 2012/08/02 23:24:10, jvoung (chromium) wrote:

+  if (ld_subprocess_ != NULL) ld_subprocess_->service_runtime()->Shutdown();
   NaClXMutexUnlock(&subprocess_mu_);
   nacl::MutexLocker ml(&cond_mu_);
   done_ = true;
+  // Free all buffered bitcode chunks
+  data_buffers_.clear();
   NaClXCondVarSignal(&buffer_cond_);
 }
 
 PnaclTranslateThread::~PnaclTranslateThread() {
   PLUGIN_PRINTF(("~PnaclTranslateThread (translate_thread=%p)\n", this));
-  SetSubprocessesShouldDie();
+  AbortSubprocesses();

[jvoung (off chromium), 2012/08/02 23:24:10]

AbortSubprocesses() is called from multiple destructors -- just wanted to check
if that was idempotent.  If it is idempotent, I guess it might be better to nuke
the subprocesses from orbit with all nukes available.  Otherwise,  maybe just
nuke it from this destructor and not the PnaclCoordinator one?

[Derek Schuff, 2012/08/02 23:46:45]

Other than potentially service_runtime Shutdown (addressed above), it's fine.
And I think it's better to keep it idempotent and just use it wherever
necessary.
On 2012/08/02 23:24:10, jvoung (chromium) wrote:

   NaClThreadJoin(translate_thread_.get());
   PLUGIN_PRINTF(("~PnaclTranslateThread joined\n"));
+  NaClCondVarDtor(&buffer_cond_);
+  NaClMutexDtor(&cond_mu_);
   NaClMutexDtor(&subprocess_mu_);
 }