Kill pnacl translation processes immediately on coordinator error and destruction

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "native_client/src/trusted/plugin/pnacl_translate_thread.h"
 
 #include "native_client/src/trusted/desc/nacl_desc_wrapper.h"
 #include "native_client/src/trusted/plugin/plugin.h"
 #include "native_client/src/trusted/plugin/pnacl_resources.h"
 #include "native_client/src/trusted/plugin/srpc_params.h"
 #include "native_client/src/trusted/plugin/temporary_file.h"
 #include "native_client/src/trusted/plugin/utility.h"
 
 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),
                                                nexe_file_(NULL),
                                                coordinator_error_info_(NULL),
                                                resources_(NULL),
                                                plugin_(NULL) {
   NaClXMutexCtor(&subprocess_mu_);
   NaClXMutexCtor(&cond_mu_);
   NaClXCondVarCtor(&buffer_cond_);
@@ skipping 86 matching lines @@
 }
 
 void WINAPI PnaclTranslateThread::DoTranslateThread(void* arg) {
   PnaclTranslateThread* translator =
       reinterpret_cast<PnaclTranslateThread*>(arg);
   translator->DoTranslate();
 }
 
 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;
   }
 
   PLUGIN_PRINTF(("PnaclCoordinator: StreamInit successful\n"));
 
   // llc process is started.
   while(!done_ || data_buffers_.size() > 0) {
     NaClXMutexLock(&cond_mu_);
     while(!done_ && data_buffers_.size() == 0) {
       NaClXCondVarWait(&buffer_cond_, &cond_mu_);
     }
     PLUGIN_PRINTF(("PnaclTranslateThread awake, done %d, size %d\n",
                    done_, data_buffers_.size()));
     if (data_buffers_.size() > 0) {
       std::vector<char> data;
       data.swap(data_buffers_.front());
       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;
       }
       PLUGIN_PRINTF(("StreamChunk Successful\n"));
     } 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.
   int is_shared_library = (params.outs()[0]->u.ival != 0);
   nacl::string soname = params.outs()[1]->arrays.str;
   nacl::string lib_dependencies = params.outs()[2]->arrays.str;
   PLUGIN_PRINTF(("PnaclCoordinator: compile (translator=%p) succeeded"
                  " is_shared_library=%d, soname='%s', lib_dependencies='%s')\n",
                  this, is_shared_library, soname.c_str(),
                  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;
   }
   pp::Core* core = pp::Module::Get()->core();
   core->CallOnMainThread(0, report_translate_finished_, PP_OK);
 }
 
 bool PnaclTranslateThread::RunLdSubprocess(int is_shared_library,
                                            const nacl::string& soname,
                                            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");
     return false;
   }
   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(),
                                        ld_out_file->desc(),
                                        is_shared_library,
                                        soname.c_str(),
                                        lib_dependencies.c_str())) {
     TranslateFailed("link failed.");
     return false;
   }
   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;
 }
 
 void PnaclTranslateThread::TranslateFailed(const nacl::string& error_string) {
   PLUGIN_PRINTF(("PnaclTranslateThread::TranslateFailed (error_string='%s')\n",
                  error_string.c_str()));
   pp::Core* core = pp::Module::Get()->core();
   if (coordinator_error_info_->message().empty()) {
     // Only use our message if one hasn't already been set by the coordinator
     // (e.g. pexe load failed).
     coordinator_error_info_->SetReport(ERROR_UNKNOWN,
                                        nacl::string("PnaclCoordinator: ") +
                                        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_);
 }
 
 } // namespace plugin