Better handling of surfaway/reload in PNaCl translation

On surfaway when the Plugin object gets destroyed, the coordinator joins the
translation thread, which could be waiting for an SRPC to return.
But the compiler or linker could be blocked (or about to block) on a
reverse service call, which could cause a deadlock. So, the reverse service
interface for the translator or linker needs to be shut down as well which
will cause the nexe's RPC to fail and break the deadlock.
Also, join the translation thread in the streaming translation object where
it was created to make sure the objects it needs are live.

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


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

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

3 files changed, 46 insertions, 10 deletions

diff --git a/ppapi/native_client/src/trusted/plugin/pnacl_streaming_translate_thread.cc b/ppapi/native_client/src/trusted/plugin/pnacl_streaming_translate_thread.cc
index 3f4a968..64d6bd4 100644
--- a/ppapi/native_client/src/trusted/plugin/pnacl_streaming_translate_thread.cc
+++ b/ppapi/native_client/src/trusted/plugin/pnacl_streaming_translate_thread.cc
@@ -17,7 +17,12 @@ PnaclStreamingTranslateThread::PnaclStreamingTranslateThread() : done_(false) {
   NaClXCondVarCtor(&buffer_cond_);
 }
 
-PnaclStreamingTranslateThread::~PnaclStreamingTranslateThread() {}
+PnaclStreamingTranslateThread::~PnaclStreamingTranslateThread() {
+  PLUGIN_PRINTF(("~PnaclTranslateThread (translate_thread=%p)\n", this));
+  SetSubprocessesShouldDie();
+  NaClThreadJoin(translate_thread_.get());
+  PLUGIN_PRINTF(("~PnaclTranslateThread joined\n"));
+}
 
 void PnaclStreamingTranslateThread::RunTranslate(
     const pp::CompletionCallback& finish_callback,
@@ -103,6 +108,7 @@ void PnaclStreamingTranslateThread::DoTranslate() {
   PluginReverseInterface* llc_reverse =
       llc_subprocess->service_runtime()->rev_interface();
   llc_reverse->AddTempQuotaManagedFile(obj_file_->identifier());
+  RegisterReverseInterface(llc_reverse);
 
   if (!llc_subprocess->InvokeSrpcMethod("StreamInit",
                                         "h",
@@ -142,6 +148,10 @@ void PnaclStreamingTranslateThread::DoTranslate() {
     } else {
       NaClXMutexUnlock(&cond_mu_);
     }
+    if (SubprocessesShouldDie()) {
+      TranslateFailed("Stopped by coordinator.");
+      return;
+    }
   }
   PLUGIN_PRINTF(("PnaclTranslateThread done with chunks\n"));
   // Finish llc.
@@ -162,6 +172,7 @@ void PnaclStreamingTranslateThread::DoTranslate() {
                  lib_dependencies.c_str()));
 
   // Shut down the llc subprocess.
+  RegisterReverseInterface(NULL);
   llc_subprocess.reset(NULL);
   if (SubprocessesShouldDie()) {
     TranslateFailed("stopped by coordinator.");
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 145a09a..06d31e8 100644
--- a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
+++ b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc
@@ -14,6 +14,7 @@
 namespace plugin {
 
 PnaclTranslateThread::PnaclTranslateThread() : subprocesses_should_die_(false),
+                                               current_rev_interface_(NULL),
                                                manifest_(NULL),
                                                ld_manifest_(NULL),
                                                obj_file_(NULL),
@@ -73,6 +74,7 @@ bool PnaclTranslateThread::RunLdSubprocess(int is_shared_library,
       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",
                                        "hhiss",
                                        &params,
@@ -87,6 +89,7 @@ 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.");
@@ -109,6 +112,19 @@ 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 *interface) {
+  nacl::MutexLocker ml(&subprocess_mu_);
+  current_rev_interface_ = interface;
+}
+
+
 bool PnaclTranslateThread::SubprocessesShouldDie() {
   nacl::MutexLocker ml(&subprocess_mu_);
   return subprocesses_should_die_;
@@ -118,16 +134,13 @@ void PnaclTranslateThread::SetSubprocessesShouldDie() {
   PLUGIN_PRINTF(("PnaclTranslateThread::SetSubprocessesShouldDie\n"));
   nacl::MutexLocker ml(&subprocess_mu_);
   subprocesses_should_die_ = true;
+  if (current_rev_interface_) {
+    current_rev_interface_->ShutDown();
+    current_rev_interface_ = NULL;
+  }
 }
 
 PnaclTranslateThread::~PnaclTranslateThread() {
-  PLUGIN_PRINTF(("~PnaclTranslateThread (translate_thread=%p)\n",
-                 translate_thread_.get()));
-  if (translate_thread_ != NULL) {
-    SetSubprocessesShouldDie();
-    NaClThreadJoin(translate_thread_.get());
-    PLUGIN_PRINTF(("~PnaclTranslateThread joined\n"));
-  }
   NaClMutexDtor(&subprocess_mu_);
 }
 
diff --git a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h
index 0f7f592..82e7d8d 100644
--- a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h
+++ b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h
@@ -11,6 +11,7 @@
 #include "native_client/src/shared/platform/nacl_threads.h"
 #include "native_client/src/shared/platform/nacl_sync_checked.h"
 #include "native_client/src/trusted/plugin/plugin_error.h"
+#include "native_client/src/trusted/plugin/service_runtime.h"
 
 #include "ppapi/cpp/completion_callback.h"
 
@@ -32,8 +33,7 @@ class PnaclTranslateThread {
  public:
   PnaclTranslateThread();
   virtual ~PnaclTranslateThread();
-  // TODO(jvoung/dschuff): handle surfaway issues when coordinator/plugin
-  // goes away. This data may have to be refcounted not touched in that case.
+
   virtual void RunTranslate(const pp::CompletionCallback& finish_callback,
                     const Manifest* manifest,
                     const Manifest* ld_manifest,
@@ -64,6 +64,15 @@ 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 *interface);
+
   // 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.
@@ -73,6 +82,9 @@ class PnaclTranslateThread {
 
   nacl::scoped_ptr<NaClThread> translate_thread_;
 
+  // Reverse interface to shutdown on SetSubprocessesShouldDie
+  PluginReverseInterface* current_rev_interface_;
+
   // Data about the translation files, owned by the coordinator
   const Manifest* manifest_;
   const Manifest* ld_manifest_;