Better handling of surfaway/reload in PNaCl translation

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),
                                                manifest_(NULL),
                                                ld_manifest_(NULL),
                                                obj_file_(NULL),
                                                nexe_file_(NULL),
                                                coordinator_error_info_(NULL),
                                                resources_(NULL),
                                                plugin_(NULL) {
   NaClXMutexCtor(&subprocess_mu_);
 }
 
@@ skipping 39 matching lines @@
   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",
                                        "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;
   }
   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 *interface) {
+  nacl::MutexLocker ml(&subprocess_mu_);
+  current_rev_interface_ = interface;
+}
+
+
 bool PnaclTranslateThread::SubprocessesShouldDie() {
   nacl::MutexLocker ml(&subprocess_mu_);
   return subprocesses_should_die_;
 }
 
 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();

[jvoung (off chromium), 2012/07/31 18:51:45]

I assume that this works better than shutting down the llc_subprocess or
ld_subprocess objects.

[Derek Schuff, 2012/07/31 18:57:50]

Well, this was simpler than trying to own those objects outside their current
local scopes (in this case the only thing we expose externally is the bit we
need). 
On 2012/07/31 18:51:45, jvoung (chromium) wrote:

+    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_);
 }
 
 } // namespace plugin