Set up a NaCl load status callback to start replacing "start_module".

components/nacl/renderer/plugin/service_runtime.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.
  */
 
 #define NACL_LOG_MODULE_NAME "Plugin_ServiceRuntime"
 
 #include "components/nacl/renderer/plugin/service_runtime.h"
 
@@ skipping 70 matching lines @@
 bool ServiceRuntime::StartModule() {
   // start the module.  otherwise we cannot connect for multimedia
   // subsystem since that is handled by user-level code (not secure!)
   // in libsrpc.
   int load_status = -1;
   if (uses_nonsfi_mode_) {
     // In non-SFI mode, we don't need to call start_module SRPC to launch
     // the plugin.
     load_status = LOAD_OK;
   } else {
+    // We invoke start_module to unblock NaClWaitForStartModuleCommand in
+    // sel_main_chrome, but the load_status is obtained by a different hook.

[Mark Seaborn, 2015/04/18 14:07:53]

Nit: How about "sel_main_chrome.c on the NaCl side" to make it clearer where to
look?

[jvoung (off chromium), 2015/04/21 17:11:21]

Done.

+    // Remove this once NaClWaitForStartModuleCommand is no longer needed.
     NaClSrpcResultCodes rpc_result =
         NaClSrpcInvokeBySignature(&command_channel_,
                                   "start_module::i",
                                   &load_status);
 
     if (NACL_SRPC_RESULT_OK != rpc_result) {
       ErrorInfo error_info;
       error_info.SetReport(PP_NACL_ERROR_SEL_LDR_START_MODULE,
                            "ServiceRuntime: could not start nacl module");
       ReportLoadError(error_info);
       return false;
     }
   }
 
   NaClLog(4, "ServiceRuntime::StartModule (load_status=%d)\n", load_status);
-  if (main_service_runtime_) {
-    if (load_status < 0 || load_status > NACL_ERROR_CODE_MAX)
-      load_status = LOAD_STATUS_UNKNOWN;
-    GetNaClInterface()->ReportSelLdrStatus(pp_instance_,
-                                           load_status,
-                                           NACL_ERROR_CODE_MAX);
-  }
-
-  if (LOAD_OK != load_status) {
-    ErrorInfo error_info;
-    error_info.SetReport(
-        PP_NACL_ERROR_SEL_LDR_START_STATUS,
-        NaClErrorString(static_cast<NaClErrorCode>(load_status)));
-    ReportLoadError(error_info);
-    return false;
-  }
-  return true;
+  return LOAD_OK == load_status;
 }
 
 void ServiceRuntime::StartSelLdr(const SelLdrStartParams& params,
                                  pp::CompletionCallback callback) {
   NaClLog(4, "ServiceRuntime::Start\n");
 
   nacl::scoped_ptr<SelLdrLauncherChrome>
       tmp_subprocess(new SelLdrLauncherChrome());
   if (NULL == tmp_subprocess.get()) {
     NaClLog(LOG_ERROR, "ServiceRuntime::Start (subprocess create failed)\n");
@@ skipping 68 matching lines @@
   nacl::MutexLocker take(&mu_);
   start_nexe_done_ = true;
   nexe_started_ok_ = ok;
   NaClXCondVarSignal(&cond_);
 }
 
 void ServiceRuntime::StartNexe() {
   bool ok = StartNexeInternal();
   if (ok) {
     NaClLog(4, "ServiceRuntime::StartNexe (success)\n");
-  } else {
-    ReapLogs();
   }
   // This only matters if a background thread is waiting, but we signal in all
   // cases to simplify the code.
   SignalNexeStarted(ok);
 }
 
 bool ServiceRuntime::StartNexeInternal() {
   if (!SetupCommandChannel())
     return false;
   return StartModule();
 }
 
-void ServiceRuntime::ReapLogs() {
-  // TODO(teravest): We should allow the NaCl process to crash itself when a
-  // module fails to start, and remove the call to RemoteLog() here. The
-  // reverse channel is no longer needed for crash reporting.
-  //
-  // The reasoning behind the current code behavior follows:
-  // On a load failure the NaCl process does not crash itself to
-  // avoid a race where the no-more-senders error on the reverse
-  // channel service thread might cause the crash-detection logic to
-  // kick in before the start_module RPC reply has been received. So
-  // we induce a NaCl process crash here.
-  RemoteLog(LOG_FATAL, "reap logs\n");
-
-  // TODO(teravest): Release subprocess_ here since it's no longer needed. It
-  // was previously kept around to collect crash log output from the bootstrap
-  // channel.
-}
-
 void ServiceRuntime::ReportLoadError(const ErrorInfo& error_info) {
   if (main_service_runtime_) {
     plugin_->ReportLoadError(error_info);
   }
 }
 
 SrpcClient* ServiceRuntime::SetupAppChannel() {
   NaClLog(4, "ServiceRuntime::SetupAppChannel (subprocess_=%p)\n",
           reinterpret_cast<void*>(subprocess_.get()));
   nacl::DescWrapper* connect_desc = subprocess_->socket_addr()->Connect();
   if (NULL == connect_desc) {
     NaClLog(LOG_ERROR, "ServiceRuntime::SetupAppChannel (connect failed)\n");
     return NULL;
   } else {
     NaClLog(4, "ServiceRuntime::SetupAppChannel (conect_desc=%p)\n",
             static_cast<void*>(connect_desc));
     SrpcClient* srpc_client = SrpcClient::New(connect_desc);
     NaClLog(4, "ServiceRuntime::SetupAppChannel (srpc_client=%p)\n",
             static_cast<void*>(srpc_client));
     delete connect_desc;
     return srpc_client;
   }
 }
 
-bool ServiceRuntime::RemoteLog(int severity, const std::string& msg) {
-  NaClSrpcResultCodes rpc_result =
-      NaClSrpcInvokeBySignature(&command_channel_,
-                                "log:is:",
-                                severity,
-                                strdup(msg.c_str()));
-  return (NACL_SRPC_RESULT_OK == rpc_result);
-}
-
 void ServiceRuntime::Shutdown() {
   // Abandon callbacks, tell service threads to quit if they were
   // blocked waiting for main thread operations to finish.  Note that
   // some callbacks must still await their completion event, e.g.,
   // CallOnMainThread must still wait for the time out, or I/O events
   // must finish, so resources associated with pending events cannot
   // be deallocated.
 
   // Note that this does waitpid() to get rid of any zombie subprocess.
   subprocess_.reset(NULL);
 
   NaClSrpcDtor(&command_channel_);
 }
 
 ServiceRuntime::~ServiceRuntime() {
   NaClLog(4, "ServiceRuntime::~ServiceRuntime (this=%p)\n",
           static_cast<void*>(this));
   // We do this just in case Shutdown() was not called.
   subprocess_.reset(NULL);
 
   NaClCondVarDtor(&cond_);
   NaClMutexDtor(&mu_);
 }
 
 }  // namespace plugin