NaCl: Detect plugin crashes via EOF on Chromium IPC.

ppapi/native_client/src/trusted/plugin/service_runtime.cc

Index: ppapi/native_client/src/trusted/plugin/service_runtime.cc
diff --git a/ppapi/native_client/src/trusted/plugin/service_runtime.cc b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
index ffce56320dedd85df0dc9ae4d7f82fa6291d2142..5c463e0815ce50939a2b5199346ffb658d70cb34 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_);
 }
@@ -223,18 +221,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) {
@@ -251,8 +238,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),
@@ -260,7 +246,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),
@@ -488,24 +474,22 @@ bool ServiceRuntime::StartNexeInternal() {
 }
 
 void ServiceRuntime::ReapLogs() {
+  // TODO(teravest): Now that crashes are reported over Chrome IPC instead of

[Mark Seaborn, 2014/08/28 20:58:39]

Nit: "reported over" -> "detected via"

since the NaCl process doesn't send any messages when it crashes, it just
generates an EOF.

[teravest, 2014/09/03 15:18:22]

I've reworked this whole block since it was confusing. 

+  // through the reverse service, allow the service runtime to crash itself

[Mark Seaborn, 2014/08/28 20:58:39]

Nit: "service runtime" -> "NaCl process" or "service_runtime in the NaCl
process" to be a little clearer?

[teravest, 2014/09/03 15:18:22]

Done.

+  // when a module fails to start.

[Mark Seaborn, 2014/08/28 20:58:39]

I find this paragraph a bit confusing.  It says "Now that X, do Y".  But the
code was doing Y (i.e. telling the NaCl process to crash itself) before this
change.

Oh, I noticed that it's a TODO.  Do you mean "TODO: We *should* allow the
service_runtime to crash itself..."?

[teravest, 2014/09/03 15:18:22]

Yes, that's what I meant by the TODO. Is it clearer now?

+  //
+  // The reasoning behind the current code behavior (dependent on the service
+  // runtime and reverse service) follows:
   // On a load failure the service runtime 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 service runtime 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();

[Mark Seaborn, 2014/08/28 20:58:39]

It took me a while to understand this change.

Basically: removing this ReportCrash() call is, by itself, a simple cleanup that
doesn't change behaviour, because you also changed ReportCrash() (above) to be a
no-op.

Could you state that explicitly in the commit message?

[teravest, 2014/09/03 15:18:22]

Done.

-    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) {