NaCl: Detect plugin crashes via EOF on Chromium IPC.

This change uses the OnChannelError() callback in TrustedPluginChannel to
detect that the channel between the renderer and NaCl processes has closed.
Previously, closure of the SRPC channel between those two processes was used to
indicate a plugin crash.

I wasn't sure at first that OnChannelError() would be called, but it appears to
be called reliably in the testing that I've done.

This change removes a call to ReportCrash() in ServiceRuntime::ReapLogs() since
ReportCrash() has become a no-op.

This change reenables the NaClBrowserTest*.Crash tests on Linux to ensure that
there's good coverage-- they seem to pass.

TEST=NaClBrowserTest*.Crash
BUG=366334,401105
R=mseaborn@chromium.org

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

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

11 files changed, 30 insertions, 90 deletions

diff --git a/chrome/test/nacl/nacl_browsertest.cc b/chrome/test/nacl/nacl_browsertest.cc
index 7e26901..1e17317 100644
--- a/chrome/test/nacl/nacl_browsertest.cc
+++ b/chrome/test/nacl/nacl_browsertest.cc
@@ -107,9 +107,6 @@ IN_PROC_BROWSER_TEST_F(NaClBrowserTestNewlib, BadNative) {
 #if defined(OS_WIN)
 // crbug.com/98721
 #  define MAYBE_Crash DISABLED_Crash
-#elif defined(OS_LINUX)
-// crbug.com/366334
-#  define MAYBE_Crash DISABLED_Crash
 #else
 #  define MAYBE_Crash Crash
 #endif
diff --git a/components/nacl/renderer/ppb_nacl_private_impl.cc b/components/nacl/renderer/ppb_nacl_private_impl.cc
index 58faad7..de005df 100644
--- a/components/nacl/renderer/ppb_nacl_private_impl.cc
+++ b/components/nacl/renderer/ppb_nacl_private_impl.cc
@@ -749,12 +749,6 @@ void ReportLoadAbort(PP_Instance instance) {
     load_manager->ReportLoadAbort();
 }
 
-void NexeDidCrash(PP_Instance instance) {
-  NexeLoadManager* load_manager = NexeLoadManager::Get(instance);
-  if (load_manager)
-    load_manager->NexeDidCrash();
-}
-
 void InstanceCreated(PP_Instance instance) {
   NexeLoadManager::Create(instance);
 }
@@ -1666,7 +1660,6 @@ const PPB_NaCl_Private nacl_interface = {
   &ReportLoadSuccess,
   &ReportLoadError,
   &ReportLoadAbort,
-  &NexeDidCrash,
   &InstanceCreated,
   &InstanceDestroyed,
   &NaClDebugEnabledForURL,
diff --git a/components/nacl/renderer/trusted_plugin_channel.cc b/components/nacl/renderer/trusted_plugin_channel.cc
index 0aaf6bc..729644c 100644
--- a/components/nacl/renderer/trusted_plugin_channel.cc
+++ b/components/nacl/renderer/trusted_plugin_channel.cc
@@ -46,6 +46,11 @@ bool TrustedPluginChannel::OnMessageReceived(const IPC::Message& msg) {
   return handled;
 }
 
+void TrustedPluginChannel::OnChannelError() {
+  if (report_exit_status_)
+    nexe_load_manager_->NexeDidCrash();
+}
+
 void TrustedPluginChannel::OnReportExitStatus(int exit_status) {
   if (report_exit_status_)
     nexe_load_manager_->set_exit_status(exit_status);
diff --git a/components/nacl/renderer/trusted_plugin_channel.h b/components/nacl/renderer/trusted_plugin_channel.h
index fd261aa..5f49f12 100644
--- a/components/nacl/renderer/trusted_plugin_channel.h
+++ b/components/nacl/renderer/trusted_plugin_channel.h
@@ -35,6 +35,7 @@ class TrustedPluginChannel : public IPC::Listener {
 
   // Listener implementation.
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  virtual void OnChannelError() OVERRIDE;
 
   void OnReportExitStatus(int exit_status);
 
diff --git a/ppapi/api/private/ppb_nacl_private.idl b/ppapi/api/private/ppb_nacl_private.idl
index e777527..3924ca2 100644
--- a/ppapi/api/private/ppb_nacl_private.idl
+++ b/ppapi/api/private/ppb_nacl_private.idl
@@ -275,9 +275,6 @@ interface PPB_NaCl_Private {
   /* Reports that loading a nexe was aborted. */
   void ReportLoadAbort([in] PP_Instance instance);
 
-  /* Reports that the nexe has crashed. */
-  void NexeDidCrash([in] PP_Instance instance);
-
   /* Performs internal setup when an instance is created. */
   void InstanceCreated([in] PP_Instance instance);
 
diff --git a/ppapi/c/private/ppb_nacl_private.h b/ppapi/c/private/ppb_nacl_private.h
index b28dff2..5ba08d9 100644
--- a/ppapi/c/private/ppb_nacl_private.h
+++ b/ppapi/c/private/ppb_nacl_private.h
@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From private/ppb_nacl_private.idl modified Wed Aug 27 13:55:13 2014. */
+/* From private/ppb_nacl_private.idl modified Thu Sep  4 07:46:02 2014. */
 
 #ifndef PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
 #define PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
@@ -304,8 +304,6 @@ struct PPB_NaCl_Private_1_0 {
                           const char* error_message);
   /* Reports that loading a nexe was aborted. */
   void (*ReportLoadAbort)(PP_Instance instance);
-  /* Reports that the nexe has crashed. */
-  void (*NexeDidCrash)(PP_Instance instance);
   /* Performs internal setup when an instance is created. */
   void (*InstanceCreated)(PP_Instance instance);
   /* Performs internal cleanup when an instance is destroyed. */
diff --git a/ppapi/native_client/src/trusted/plugin/plugin.cc b/ppapi/native_client/src/trusted/plugin/plugin.cc
index ea86558..e7b2568 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.cc
+++ b/ppapi/native_client/src/trusted/plugin/plugin.cc
@@ -72,7 +72,6 @@ bool Plugin::LoadHelperNaClModuleInternal(NaClSubprocess* subprocess,
                          pp_instance(),
                          false,  // No main_service_runtime.
                          false,  // No non-SFI mode (i.e. in SFI-mode).
-                         pp::BlockUntilComplete(),
                          pp::BlockUntilComplete());
   subprocess->set_service_runtime(service_runtime);
 
@@ -128,8 +127,7 @@ void Plugin::LoadNaClModule(PP_NaClFileInfo file_info,
                             bool enable_dyncode_syscalls,
                             bool enable_exception_handling,
                             bool enable_crash_throttling,
-                            const pp::CompletionCallback& init_done_cb,
-                            const pp::CompletionCallback& crash_cb) {
+                            const pp::CompletionCallback& init_done_cb) {
   CHECK(pp::Module::Get()->core()->IsMainThread());
   // Before forking a new sel_ldr process, ensure that we do not leak
   // the ServiceRuntime object for an existing subprocess, and that any
@@ -149,7 +147,7 @@ void Plugin::LoadNaClModule(PP_NaClFileInfo file_info,
                            enable_crash_throttling);
   ErrorInfo error_info;
   ServiceRuntime* service_runtime = new ServiceRuntime(
-      this, pp_instance(), true, uses_nonsfi_mode, init_done_cb, crash_cb);
+      this, pp_instance(), true, uses_nonsfi_mode, init_done_cb);
   main_subprocess_.set_service_runtime(service_runtime);
   if (NULL == service_runtime) {
     error_info.SetReport(
@@ -336,8 +334,7 @@ void Plugin::NexeFileDidOpen(int32_t pp_error) {
       true, /* enable_dyncode_syscalls */
       true, /* enable_exception_handling */
       false, /* enable_crash_throttling */
-      callback_factory_.NewCallback(&Plugin::NexeFileDidOpenContinuation),
-      callback_factory_.NewCallback(&Plugin::NexeDidCrash));
+      callback_factory_.NewCallback(&Plugin::NexeFileDidOpenContinuation));
 }
 
 void Plugin::NexeFileDidOpenContinuation(int32_t pp_error) {
@@ -355,12 +352,6 @@ void Plugin::NexeFileDidOpenContinuation(int32_t pp_error) {
   NaClLog(4, "Leaving NexeFileDidOpenContinuation\n");
 }
 
-void Plugin::NexeDidCrash(int32_t pp_error) {
-  PLUGIN_PRINTF(("Plugin::NexeDidCrash (pp_error=%" NACL_PRId32 ")\n",
-                 pp_error));
-  nacl_interface_->NexeDidCrash(pp_instance());
-}
-
 void Plugin::BitcodeDidTranslate(int32_t pp_error) {
   PLUGIN_PRINTF(("Plugin::BitcodeDidTranslate (pp_error=%" NACL_PRId32 ")\n",
                  pp_error));
@@ -382,8 +373,7 @@ void Plugin::BitcodeDidTranslate(int32_t pp_error) {
       false, /* enable_dyncode_syscalls */
       false, /* enable_exception_handling */
       true, /* enable_crash_throttling */
-      callback_factory_.NewCallback(&Plugin::BitcodeDidTranslateContinuation),
-      callback_factory_.NewCallback(&Plugin::NexeDidCrash));
+      callback_factory_.NewCallback(&Plugin::BitcodeDidTranslateContinuation));
 }
 
 void Plugin::BitcodeDidTranslateContinuation(int32_t pp_error) {
diff --git a/ppapi/native_client/src/trusted/plugin/plugin.h b/ppapi/native_client/src/trusted/plugin/plugin.h
index a55b62d..8b89a88 100644
--- a/ppapi/native_client/src/trusted/plugin/plugin.h
+++ b/ppapi/native_client/src/trusted/plugin/plugin.h
@@ -93,8 +93,7 @@ class Plugin : public pp::Instance {
                       bool enable_dyncode_syscalls,
                       bool enable_exception_handling,
                       bool enable_crash_throttling,
-                      const pp::CompletionCallback& init_done_cb,
-                      const pp::CompletionCallback& crash_cb);
+                      const pp::CompletionCallback& init_done_cb);
 
   // Finish hooking interfaces up, after low-level initialization is
   // complete.
@@ -173,18 +172,6 @@ class Plugin : public pp::Instance {
   void NexeFileDidOpen(int32_t pp_error);
   void NexeFileDidOpenContinuation(int32_t pp_error);
 
-  // Callback used when the reverse channel closes.  This is an
-  // asynchronous event that might turn into a JavaScript error or
-  // crash event -- this is controlled by the two state variables
-  // nacl_ready_state_ and nexe_error_reported_: If an error or crash
-  // had already been reported, no additional crash event is
-  // generated.  If no error has been reported but nacl_ready_state_
-  // is not DONE, then the loadend event has not been reported, and we
-  // enqueue an error event followed by loadend.  If nacl_ready_state_
-  // is DONE, then we are in the post-loadend (we need temporal
-  // predicate symbols), and we enqueue a crash event.
-  void NexeDidCrash(int32_t pp_error);
-
   // Callback used when a .nexe is translated from bitcode.  If the translation
   // is successful, the file descriptor is opened and can be passed to sel_ldr
   // with the sandbox on.
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.cc b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
index b162f62..4787162 100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.cc
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
@@ -58,14 +58,12 @@ PluginReverseInterface::PluginReverseInterface(
     nacl::WeakRefAnchor* anchor,
     PP_Instance pp_instance,
     ServiceRuntime* service_runtime,
-    pp::CompletionCallback init_done_cb,
-    pp::CompletionCallback crash_cb)
+    pp::CompletionCallback init_done_cb)
       : anchor_(anchor),
         pp_instance_(pp_instance),
         service_runtime_(service_runtime),
         shutting_down_(false),
-        init_done_cb_(init_done_cb),
-        crash_cb_(crash_cb) {
+        init_done_cb_(init_done_cb) {
   NaClXMutexCtor(&mu_);
   NaClXCondVarCtor(&cv_);
 }
@@ -222,18 +220,7 @@ void PluginReverseInterface::StreamAsFile_MainThreadContinuation(
 }
 
 void PluginReverseInterface::ReportCrash() {
-  NaClLog(4, "PluginReverseInterface::ReportCrash\n");
-
-  if (crash_cb_.pp_completion_callback().func != NULL) {
-    NaClLog(4, "PluginReverseInterface::ReportCrash: invoking CB\n");
-    pp::Module::Get()->core()->CallOnMainThread(0, crash_cb_, PP_OK);
-    // Clear the callback to avoid it gets invoked twice.
-    crash_cb_ = pp::CompletionCallback();
-  } else {
-    NaClLog(1,
-            "PluginReverseInterface::ReportCrash:"
-            " crash_cb_ not valid, skipping\n");
-  }
+  // This is now handled through Chromium IPC.
 }
 
 void PluginReverseInterface::ReportExitStatus(int exit_status) {
@@ -250,8 +237,7 @@ ServiceRuntime::ServiceRuntime(Plugin* plugin,
                                PP_Instance pp_instance,
                                bool main_service_runtime,
                                bool uses_nonsfi_mode,
-                               pp::CompletionCallback init_done_cb,
-                               pp::CompletionCallback crash_cb)
+                               pp::CompletionCallback init_done_cb)
     : plugin_(plugin),
       pp_instance_(pp_instance),
       main_service_runtime_(main_service_runtime),
@@ -259,7 +245,7 @@ ServiceRuntime::ServiceRuntime(Plugin* plugin,
       reverse_service_(NULL),
       anchor_(new nacl::WeakRefAnchor()),
       rev_interface_(new PluginReverseInterface(anchor_, pp_instance, this,
-                                                init_done_cb, crash_cb)),
+                                                init_done_cb)),
       start_sel_ldr_done_(false),
       start_nexe_done_(false),
       nexe_started_ok_(false),
@@ -487,24 +473,21 @@ bool ServiceRuntime::StartNexeInternal() {
 }
 
 void ServiceRuntime::ReapLogs() {
-  // On a load failure the service runtime does not crash itself to
+  // 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 service runtime crash here. We do not release
-  // subprocess_ since it's needed to collect crash log output after
-  // the error is reported.
+  // we induce a NaCl process crash here.
   RemoteLog(LOG_FATAL, "reap logs\n");
-  if (NULL == reverse_service_) {
-    // No crash detector thread.
-    NaClLog(LOG_ERROR, "scheduling to get crash log\n");
-    // Invoking rev_interface's method is workaround to avoid crash_cb
-    // gets called twice or more. We should clean this up later.
-    rev_interface_->ReportCrash();
-    NaClLog(LOG_ERROR, "should fire soon\n");
-  } else {
-    NaClLog(LOG_ERROR, "Reverse service thread will pick up crash log\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) {
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.h b/ppapi/native_client/src/trusted/plugin/service_runtime.h
index 2b92727..2fccc7e 100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.h
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.h
@@ -96,8 +96,7 @@ class PluginReverseInterface: public nacl::ReverseInterface {
   PluginReverseInterface(nacl::WeakRefAnchor* anchor,
                          PP_Instance pp_instance,
                          ServiceRuntime* service_runtime,
-                         pp::CompletionCallback init_done_cb,
-                         pp::CompletionCallback crash_cb);
+                         pp::CompletionCallback init_done_cb);
 
   virtual ~PluginReverseInterface();
 
@@ -139,20 +138,16 @@ class PluginReverseInterface: public nacl::ReverseInterface {
   bool shutting_down_;
 
   pp::CompletionCallback init_done_cb_;
-  pp::CompletionCallback crash_cb_;
 };
 
 //  ServiceRuntime abstracts a NativeClient sel_ldr instance.
 class ServiceRuntime {
  public:
-  // TODO(sehr): This class should also implement factory methods, using the
-  // Start method below.
   ServiceRuntime(Plugin* plugin,
                  PP_Instance pp_instance,
                  bool main_service_runtime,
                  bool uses_nonsfi_mode,
-                 pp::CompletionCallback init_done_cb,
-                 pp::CompletionCallback crash_cb);
+                 pp::CompletionCallback init_done_cb);
   // The destructor terminates the sel_ldr process.
   ~ServiceRuntime();
 
diff --git a/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c b/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
index 73c18d8..ab1beda 100644
--- a/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
+++ b/ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c
@@ -3492,11 +3492,6 @@ static void Pnacl_M25_PPB_NaCl_Private_ReportLoadAbort(PP_Instance instance) {
   iface->ReportLoadAbort(instance);
 }
 
-static void Pnacl_M25_PPB_NaCl_Private_NexeDidCrash(PP_Instance instance) {
-  const struct PPB_NaCl_Private_1_0 *iface = Pnacl_WrapperInfo_PPB_NaCl_Private_1_0.real_iface;
-  iface->NexeDidCrash(instance);
-}
-
 static void Pnacl_M25_PPB_NaCl_Private_InstanceCreated(PP_Instance instance) {
   const struct PPB_NaCl_Private_1_0 *iface = Pnacl_WrapperInfo_PPB_NaCl_Private_1_0.real_iface;
   iface->InstanceCreated(instance);
@@ -5408,7 +5403,6 @@ static const struct PPB_NaCl_Private_1_0 Pnacl_Wrappers_PPB_NaCl_Private_1_0 = {
     .ReportLoadSuccess = (void (*)(PP_Instance instance, uint64_t loaded_bytes, uint64_t total_bytes))&Pnacl_M25_PPB_NaCl_Private_ReportLoadSuccess,
     .ReportLoadError = (void (*)(PP_Instance instance, PP_NaClError error, const char* error_message))&Pnacl_M25_PPB_NaCl_Private_ReportLoadError,
     .ReportLoadAbort = (void (*)(PP_Instance instance))&Pnacl_M25_PPB_NaCl_Private_ReportLoadAbort,
-    .NexeDidCrash = (void (*)(PP_Instance instance))&Pnacl_M25_PPB_NaCl_Private_NexeDidCrash,
     .InstanceCreated = (void (*)(PP_Instance instance))&Pnacl_M25_PPB_NaCl_Private_InstanceCreated,
     .InstanceDestroyed = (void (*)(PP_Instance instance))&Pnacl_M25_PPB_NaCl_Private_InstanceDestroyed,
     .NaClDebugEnabledForURL = (PP_Bool (*)(const char* alleged_nmf_url))&Pnacl_M25_PPB_NaCl_Private_NaClDebugEnabledForURL,