Kill pnacl translation processes immediately on coordinator error and destruction

On errors and destruction, use the service_runtime object in the subprocess
to kill the translator processes immediately rather than just signaling the
translation thread, which may be blocked on an RPC.
Any pending or new RPC calls will fail immediately, and the translation thread
can simply bail.
This makes destruction/surfaway much faster and more responsive and simplifies
error handling and object cleanup. The only caveat is that now the translation
thread and NaClSubprocess must be careful not to race service runtime operations
(which are protected by subprocess_mu_) with RPCs (which are called
without a mutex because they block). This is currently already ensured because
srpc_client is a separate object from service_runtime in NaClSubprocess.

R=sehr@chromium.org,jvoung@chromium.org,robertm@chromium.org
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2195
TEST=nacl_integration (especially pnacl_bad_browser_test)


Review URL: https://chromiumcodereview.appspot.com/10830149

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

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

@@ -251,7 +251,7 @@ PnaclCoordinator::~PnaclCoordinator() {
   // will have been destroyed.  This will result in the cancellation of
   // translation_complete_callback_, so no notification will be delivered.
   if (translate_thread_.get() != NULL) {
-    translate_thread_->SetSubprocessesShouldDie();
+    translate_thread_->AbortSubprocesses();
   }
 }
 
@@ -444,10 +444,8 @@ void PnaclCoordinator::CachedFileDidOpen(int32_t pp_error) {
     ReportNonPpapiError("could not allocate translation thread.");
     return;
   }
-  // In the streaming case we also want to open the object file now so the
+  // We also want to open the object file now so the
   // translator can start writing to it during streaming translation.
-  // In the non-streaming case this can wait until the bitcode download is
-  // finished.
   obj_file_.reset(new TempFile(plugin_));
   pp::CompletionCallback obj_cb =
     callback_factory_.NewCallback(&PnaclCoordinator::ObjectFileDidOpen);
@@ -475,7 +473,7 @@ void PnaclCoordinator::BitcodeStreamDidFinish(int32_t pp_error) {
     translate_finish_error_ = pp_error;
     error_info_.SetReport(ERROR_UNKNOWN,
                           nacl::string("PnaclCoordinator: pexe load failed."));
-    translate_thread_->SetSubprocessesShouldDie();
+    translate_thread_->AbortSubprocesses();
   }
 }
 

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

@@ -13,8 +13,8 @@
 
 namespace plugin {
 
-PnaclTranslateThread::PnaclTranslateThread() : subprocesses_should_die_(false),
-                                               current_rev_interface_(NULL),
+PnaclTranslateThread::PnaclTranslateThread() : llc_subprocess_active_(false),
+                                               ld_subprocess_active_(false),
                                                done_(false),
                                                manifest_(NULL),
                                                ld_manifest_(NULL),
@@ -122,28 +122,37 @@ 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;
+    }
+    llc_subprocess_active_ = true;
+    // 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 +172,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 +184,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 +210,10 @@ 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_active_ = false;
+  llc_subprocess_.reset(NULL);
+  NaClXMutexUnlock(&subprocess_mu_);
 
   if(!RunLdSubprocess(is_shared_library, soname, lib_dependencies)) {
     return;
@@ -218,16 +227,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 +235,25 @@ 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;
+    }
+    ld_subprocess_active_ = true;
+    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 +267,10 @@ 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_active_ = false;
+  ld_subprocess_.reset(NULL);
+  NaClXMutexUnlock(&subprocess_mu_);
   return true;
 }
 
@@ -277,43 +288,32 @@ 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_active_) {
+    llc_subprocess_->service_runtime()->Shutdown();
+    llc_subprocess_active_ = false;
+  }
+  if (ld_subprocess_ != NULL && ld_subprocess_active_) {
+    ld_subprocess_->service_runtime()->Shutdown();
+    ld_subprocess_active_ = false;
   }
   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();
   NaClThreadJoin(translate_thread_.get());
   PLUGIN_PRINTF(("~PnaclTranslateThread joined\n"));
+  NaClCondVarDtor(&buffer_cond_);
+  NaClMutexDtor(&cond_mu_);
   NaClMutexDtor(&subprocess_mu_);
 }
 

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

@@ -48,16 +48,18 @@ class PnaclTranslateThread {
                     PnaclResources* resources,
                     Plugin* plugin);
 
-  // Signal the translate thread and subprocesses that they should stop.
-  void SetSubprocessesShouldDie();
+  // Kill the llc and/or ld subprocesses. This happens by closing the command
+  // channel on the plugin side, which causes the trusted code in the nexe to
+  // exit, which will cause any pending SRPCs to error. Because this is called
+  // on the main thread, the translation thread must not use the subprocess
+  // objects without the lock, other than InvokeSrpcMethod, which does not
+  // race with service runtime shutdown.
+  void AbortSubprocesses();
 
   // Send bitcode bytes to the translator. Called from the main thread.
   void PutBytes(std::vector<char>* data, int count);
 
  private:
-  // Returns true if the translate thread and subprocesses should stop.
-  bool SubprocessesShouldDie();
-
   // Starts an individual llc or ld subprocess used for translation.
   NaClSubprocess* StartSubprocess(const nacl::string& url,
                                   const Manifest* manifest,
@@ -74,28 +76,19 @@ class PnaclTranslateThread {
                        const nacl::string& soname,
                        const nacl::string& lib_dependencies);
 
-  // Register a reverse service interface which will be shutdown if the
-  // plugin is shutdown. The reverse service pointer must be available on the
-  // main thread because the translation thread could be blocked on SRPC
-  // waiting for the translator, which could be waiting on a reverse
-  // service call.
-  // (see also the comments in Plugin::~Plugin about ShutdownSubprocesses,
-  // but that only handles the main nexe and not the translator nexes.)
-  void RegisterReverseInterface(PluginReverseInterface *iface);
 
   // Callback to run when tasks are completed or an error has occurred.
   pp::CompletionCallback report_translate_finished_;
-  // True if the translation thread and related subprocesses should exit.
-  bool subprocesses_should_die_;
-  // Used to guard and publish subprocesses_should_die_.
-  struct NaClMutex subprocess_mu_;
 
   nacl::scoped_ptr<NaClThread> translate_thread_;
 
-  // Reverse interface to shutdown on SetSubprocessesShouldDie
-  PluginReverseInterface* current_rev_interface_;
-
-
+  // Used to guard llc_subprocess and ld_subprocess
+  struct NaClMutex subprocess_mu_;
+  nacl::scoped_ptr<NaClSubprocess> llc_subprocess_;
+  nacl::scoped_ptr<NaClSubprocess> ld_subprocess_;
+  // Used to ensure the subprocesses don't get shutdown more than once.
+  bool llc_subprocess_active_;
+  bool ld_subprocess_active_;
 
   // Condition variable to synchronize communication with the SRPC thread.
   // SRPC thread waits on this condvar if data_buffers_ is empty (meaning

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

@@ -182,14 +182,14 @@ bool SrpcClient::Invoke(const nacl::string& method_name, SrpcParams* params) {
 
   PLUGIN_PRINTF(("SrpcClient::Invoke (sending the rpc)\n"));
   // Call the method
-  NaClSrpcError err = NaClSrpcInvokeV(&srpc_channel_,
-                                      methods_[method_name]->index(),
-                                      params->ins(),
-                                      params->outs());
-  PLUGIN_PRINTF(("SrpcClient::Invoke (response=%d)\n", err));
-  if (NACL_SRPC_RESULT_OK != err) {
+  last_error_ = NaClSrpcInvokeV(&srpc_channel_,
+                                methods_[method_name]->index(),
+                                params->ins(),
+                                params->outs());
+  PLUGIN_PRINTF(("SrpcClient::Invoke (response=%d)\n", last_error_));
+  if (NACL_SRPC_RESULT_OK != last_error_) {
     PLUGIN_PRINTF(("SrpcClient::Invoke (err='%s', return 0)\n",
-                   NaClSrpcErrorString(err)));
+                   NaClSrpcErrorString(last_error_)));
     return false;
   }
 

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

@@ -45,6 +45,8 @@ class SrpcClient {
   bool HasMethod(const nacl::string& method_name);
   //  Invoke an SRPC method.
   bool Invoke(const nacl::string& method_name, SrpcParams* params);
+  // Get the error status from that last method invocation
+  NaClSrpcError GetLastError() { return last_error_; }
   bool InitParams(const nacl::string& method_name, SrpcParams* params);
 
   // Attach a service for reverse-direction (from .nexe) RPCs.
@@ -58,6 +60,7 @@ class SrpcClient {
   Methods methods_;
   NaClSrpcChannel srpc_channel_;
   bool srpc_channel_initialised_;
+  NaClSrpcError last_error_;
 };
 
 }  // namespace plugin