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

Uses the hook from: https://codereview.chromium.org/1089323006/

The NaClListener will set up a callback, and when the load status
is known at the end of sel_main_chrome's LoadApp, it will use that
to send the load_status to the renderer via Chrome IPC, instead of
getting the load status from a "start_module" SRPC call.

The renderer will use that load_status to report UMA stats
and send progress events. This replaces the load_status coming
from the SRPC "start_module::i" invocation. However, the invocation
is kept for the moment because we currently block the sel_main_chrome
until it is received to avoid a race condition. After this lands,
we can remove the blocking and then remove the invocation.

This also replaces the call to ReapLogs since that will be done
once the load_status is known to be an error on the sel_main_chrome
side instead.

BUG= https://code.google.com/p/chromium/issues/detail?id=459250

Review URL: https://codereview.chromium.org/1090233003

Cr-Commit-Position: refs/heads/master@{#326393}

11 files changed, 60 insertions, 81 deletions

diff --git a/components/nacl/common/DEPS b/components/nacl/common/DEPS
index e13224d..3b9fcea 100644
--- a/components/nacl/common/DEPS
+++ b/components/nacl/common/DEPS
@@ -1,3 +1,4 @@
 include_rules = [
   "+native_client/src/public",
+  "+native_client/src/trusted/service_runtime/nacl_error_code.h",
 ]
diff --git a/components/nacl/common/nacl_renderer_messages.h b/components/nacl/common/nacl_renderer_messages.h
index e07a082..265af86 100644
--- a/components/nacl/common/nacl_renderer_messages.h
+++ b/components/nacl/common/nacl_renderer_messages.h
@@ -6,6 +6,7 @@
 
 // Multiply-included message file, no traditional include guard.
 #include "ipc/ipc_message_macros.h"
+#include "native_client/src/trusted/service_runtime/nacl_error_code.h"
 
 #define IPC_MESSAGE_START NaClHostMsgStart
 
@@ -13,3 +14,10 @@
 // NaCl to the renderer before the NaCl process exits very soon after.
 IPC_SYNC_MESSAGE_CONTROL1_0(NaClRendererMsg_ReportExitStatus,
                             int /* exit_status */)
+
+IPC_ENUM_TRAITS_MAX_VALUE(NaClErrorCode, NACL_ERROR_CODE_MAX)
+
+// This message must be synchronous to ensure that the load status is sent from
+// NaCl to the renderer before the NaCl process exits very soon after.
+IPC_SYNC_MESSAGE_CONTROL1_0(NaClRendererMsg_ReportLoadStatus,
+                            NaClErrorCode /* load_status */)
diff --git a/components/nacl/loader/nacl_listener.cc b/components/nacl/loader/nacl_listener.cc
index 4a58a1c..4cf4a3e 100644
--- a/components/nacl/loader/nacl_listener.cc
+++ b/components/nacl/loader/nacl_listener.cc
@@ -69,6 +69,12 @@ void FatalLogHandler(const char* data, size_t bytes) {
          copy_bytes);
 }
 
+void LoadStatusCallback(int load_status) {
+  g_listener->trusted_listener()->Send(
+      new NaClRendererMsg_ReportLoadStatus(
+          static_cast<NaClErrorCode>(load_status)));
+}
+
 #if defined(OS_MACOSX)
 
 // On Mac OS X, shm_open() works in the sandbox but does not give us
@@ -420,6 +426,7 @@ void NaClListener::OnStart(const nacl::NaClStartParams& params) {
   args->debug_stub_server_port_selected_handler_func =
       DebugStubPortSelectedHandler;
 #endif
+  args->load_status_handler_func = LoadStatusCallback;
 #if defined(OS_LINUX)
   args->prereserved_sandbox_size = prereserved_sandbox_size_;
 #endif
diff --git a/components/nacl/loader/nacl_listener.h b/components/nacl/loader/nacl_listener.h
index 9e8d67b..eccbc19 100644
--- a/components/nacl/loader/nacl_listener.h
+++ b/components/nacl/loader/nacl_listener.h
@@ -48,6 +48,10 @@ class NaClListener : public IPC::Listener {
 
   void* crash_info_shmem_memory() const { return crash_info_shmem_->memory(); }
 
+  NaClTrustedListener* trusted_listener() const {
+    return trusted_listener_.get();
+  }
+
   typedef base::Callback<void(IPC::PlatformFileForTransit, base::FilePath)>
       ResolveFileTokenCallback;
   void ResolveFileToken(uint64_t token_lo,
diff --git a/components/nacl/renderer/DEPS b/components/nacl/renderer/DEPS
index 889a36b..483376b 100644
--- a/components/nacl/renderer/DEPS
+++ b/components/nacl/renderer/DEPS
@@ -1,6 +1,8 @@
 include_rules = [
   "+content/public/renderer",
   "+native_client/src/public/imc_types.h",  # for NaClHandle
+  # for NaClErrorCode
+  "+native_client/src/trusted/service_runtime/nacl_error_code.h",
   "+net",
   "+ppapi/c",
   "+ppapi/proxy",
diff --git a/components/nacl/renderer/plugin/service_runtime.cc b/components/nacl/renderer/plugin/service_runtime.cc
index 485792a..d697c69 100644
--- a/components/nacl/renderer/plugin/service_runtime.cc
+++ b/components/nacl/renderer/plugin/service_runtime.cc
@@ -88,6 +88,10 @@ bool ServiceRuntime::StartModule() {
     // the plugin.
     load_status = LOAD_OK;
   } else {
+    // We invoke start_module to unblock NaClWaitForStartModuleCommand in
+    // sel_main_chrome.c on the NaCl side, but the load_status is obtained by
+    // a different hook. Remove this once NaClWaitForStartModuleCommand is no
+    // longer needed.
     NaClSrpcResultCodes rpc_result =
         NaClSrpcInvokeBySignature(&command_channel_,
                                   "start_module::i",
@@ -103,23 +107,7 @@ bool ServiceRuntime::StartModule() {
   }
 
   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,
@@ -208,8 +196,6 @@ 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.
@@ -222,24 +208,6 @@ bool ServiceRuntime::StartNexeInternal() {
   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);
@@ -264,15 +232,6 @@ SrpcClient* ServiceRuntime::SetupAppChannel() {
   }
 }
 
-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
diff --git a/components/nacl/renderer/plugin/service_runtime.h b/components/nacl/renderer/plugin/service_runtime.h
index 5fe941a..f1991b6 100644
--- a/components/nacl/renderer/plugin/service_runtime.h
+++ b/components/nacl/renderer/plugin/service_runtime.h
@@ -85,7 +85,6 @@ class ServiceRuntime {
   // Starts the application channel to the nexe.
   SrpcClient* SetupAppChannel();
 
-  bool RemoteLog(int severity, const std::string& msg);
   Plugin* plugin() const { return plugin_; }
   void Shutdown();
 
@@ -97,7 +96,6 @@ class ServiceRuntime {
 
   bool SetupCommandChannel();
   bool StartModule();
-  void ReapLogs();
 
   void ReportLoadError(const ErrorInfo& error_info);
 
diff --git a/components/nacl/renderer/ppb_nacl_private.h b/components/nacl/renderer/ppb_nacl_private.h
index 7953461..2683652 100644
--- a/components/nacl/renderer/ppb_nacl_private.h
+++ b/components/nacl/renderer/ppb_nacl_private.h
@@ -326,13 +326,6 @@ struct PPB_NaCl_Private {
                        const char* url,
                        struct PP_NaClFileInfo* file_info,
                        struct PP_CompletionCallback callback);
-  /* Reports the status of sel_ldr for UMA reporting.
-   * |max_status| has to be provided because the implementation of this
-   * interface can't access the NaClErrorCode enum.
-   */
-  void (*ReportSelLdrStatus)(PP_Instance instance,
-                             int32_t load_status,
-                             int32_t max_status);
   /* Logs time taken by an operation to UMA histograms.
    * This function is safe to call on any thread.
    */
@@ -365,4 +358,3 @@ struct PPB_NaCl_Private {
  */
 
 #endif  /* COMPONENTS_NACL_RENDERER_PPB_NACL_PRIVATE_H_ */
-
diff --git a/components/nacl/renderer/ppb_nacl_private_impl.cc b/components/nacl/renderer/ppb_nacl_private_impl.cc
index 8aae9e87..2f3cd58 100644
--- a/components/nacl/renderer/ppb_nacl_private_impl.cc
+++ b/components/nacl/renderer/ppb_nacl_private_impl.cc
@@ -500,13 +500,13 @@ void LaunchSelLdr(PP_Instance instance,
 
   // Create the trusted plugin channel.
   if (IsValidChannelHandle(launch_result.trusted_ipc_channel_handle)) {
-    bool report_exit_status = PP_ToBool(main_service_runtime);
+    bool is_helper_nexe = !PP_ToBool(main_service_runtime);
     scoped_ptr<TrustedPluginChannel> trusted_plugin_channel(
         new TrustedPluginChannel(
             load_manager,
             launch_result.trusted_ipc_channel_handle,
             content::RenderThread::Get()->GetShutdownEvent(),
-            report_exit_status));
+            is_helper_nexe));
     load_manager->set_trusted_plugin_channel(trusted_plugin_channel.Pass());
   } else {
     PostPPCompletionCallback(callback, PP_ERROR_FAILED);
@@ -1484,22 +1484,6 @@ void DownloadFile(PP_Instance instance,
   file_downloader->Load(url_request);
 }
 
-void ReportSelLdrStatus(PP_Instance instance,
-                        int32_t load_status,
-                        int32_t max_status) {
-  HistogramEnumerate("NaCl.LoadStatus.SelLdr", load_status, max_status);
-  NexeLoadManager* load_manager = GetNexeLoadManager(instance);
-  DCHECK(load_manager);
-  if (!load_manager)
-    return;
-
-  // Gather data to see if being installed changes load outcomes.
-  const char* name = load_manager->is_installed() ?
-      "NaCl.LoadStatus.SelLdr.InstalledApp" :
-      "NaCl.LoadStatus.SelLdr.NotInstalledApp";
-  HistogramEnumerate(name, load_status, max_status);
-}
-
 void LogTranslateTime(const char* histogram_name,
                       int64_t time_in_us) {
   ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
@@ -1717,7 +1701,6 @@ const PPB_NaCl_Private nacl_interface = {
   &GetPNaClResourceInfo,
   &GetCpuFeatureAttrs,
   &DownloadNexe,
-  &ReportSelLdrStatus,
   &LogTranslateTime,
   &LogBytesCompiledVsDowloaded,
   &SetPNaClStartTime,
diff --git a/components/nacl/renderer/trusted_plugin_channel.cc b/components/nacl/renderer/trusted_plugin_channel.cc
index 729644c..1ba39fe 100644
--- a/components/nacl/renderer/trusted_plugin_channel.cc
+++ b/components/nacl/renderer/trusted_plugin_channel.cc
@@ -6,6 +6,7 @@
 
 #include "base/callback_helpers.h"
 #include "components/nacl/common/nacl_renderer_messages.h"
+#include "components/nacl/renderer/histogram.h"
 #include "components/nacl/renderer/nexe_load_manager.h"
 #include "content/public/renderer/render_thread.h"
 #include "ipc/ipc_sync_channel.h"
@@ -18,9 +19,9 @@ TrustedPluginChannel::TrustedPluginChannel(
     NexeLoadManager* nexe_load_manager,
     const IPC::ChannelHandle& handle,
     base::WaitableEvent* shutdown_event,
-    bool report_exit_status)
+    bool is_helper_nexe)
     : nexe_load_manager_(nexe_load_manager),
-      report_exit_status_(report_exit_status) {
+      is_helper_nexe_(is_helper_nexe) {
   channel_ = IPC::SyncChannel::Create(
       handle,
       IPC::Channel::MODE_CLIENT,
@@ -41,19 +42,41 @@ bool TrustedPluginChannel::OnMessageReceived(const IPC::Message& msg) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(TrustedPluginChannel, msg)
     IPC_MESSAGE_HANDLER(NaClRendererMsg_ReportExitStatus, OnReportExitStatus);
+    IPC_MESSAGE_HANDLER(NaClRendererMsg_ReportLoadStatus, OnReportLoadStatus);
     IPC_MESSAGE_UNHANDLED(handled = false);
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void TrustedPluginChannel::OnChannelError() {
-  if (report_exit_status_)
+  if (!is_helper_nexe_)
     nexe_load_manager_->NexeDidCrash();
 }
 
 void TrustedPluginChannel::OnReportExitStatus(int exit_status) {
-  if (report_exit_status_)
+  if (!is_helper_nexe_)
     nexe_load_manager_->set_exit_status(exit_status);
 }
 
+void TrustedPluginChannel::OnReportLoadStatus(NaClErrorCode load_status) {
+  if (load_status < 0 || load_status > NACL_ERROR_CODE_MAX) {
+    load_status = LOAD_STATUS_UNKNOWN;
+  }
+  // For now, we only report UMA for non-helper nexes
+  // (don't report for the PNaCl translators nexes).
+  if (!is_helper_nexe_) {
+    HistogramEnumerate("NaCl.LoadStatus.SelLdr", load_status,
+                       NACL_ERROR_CODE_MAX);
+    // Gather data to see if being installed changes load outcomes.
+    const char* name = nexe_load_manager_->is_installed()
+                           ? "NaCl.LoadStatus.SelLdr.InstalledApp"
+                           : "NaCl.LoadStatus.SelLdr.NotInstalledApp";
+    HistogramEnumerate(name, load_status, NACL_ERROR_CODE_MAX);
+  }
+  if (load_status != LOAD_OK) {
+    nexe_load_manager_->ReportLoadError(PP_NACL_ERROR_SEL_LDR_START_STATUS,
+                                        NaClErrorString(load_status));
+  }
+}
+
 }  // namespace nacl
diff --git a/components/nacl/renderer/trusted_plugin_channel.h b/components/nacl/renderer/trusted_plugin_channel.h
index efeb5e8..d657d53 100644
--- a/components/nacl/renderer/trusted_plugin_channel.h
+++ b/components/nacl/renderer/trusted_plugin_channel.h
@@ -8,6 +8,7 @@
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
 #include "ipc/ipc_listener.h"
+#include "native_client/src/trusted/service_runtime/nacl_error_code.h"
 #include "ppapi/c/pp_instance.h"
 
 namespace base {
@@ -28,7 +29,7 @@ class TrustedPluginChannel : public IPC::Listener {
   TrustedPluginChannel(NexeLoadManager* nexe_load_manager,
                        const IPC::ChannelHandle& handle,
                        base::WaitableEvent* shutdown_event,
-                       bool report_exit_status);
+                       bool is_helper_nexe);
   ~TrustedPluginChannel() override;
 
   bool Send(IPC::Message* message);
@@ -38,13 +39,14 @@ class TrustedPluginChannel : public IPC::Listener {
   void OnChannelError() override;
 
   void OnReportExitStatus(int exit_status);
+  void OnReportLoadStatus(NaClErrorCode load_status);
 
  private:
   // Non-owning pointer. This is safe because the TrustedPluginChannel is owned
   // by the NexeLoadManager pointed to here.
   NexeLoadManager* nexe_load_manager_;
   scoped_ptr<IPC::SyncChannel> channel_;
-  bool report_exit_status_;
+  bool is_helper_nexe_;
 
   DISALLOW_COPY_AND_ASSIGN(TrustedPluginChannel);
 };