NaCl: Detect plugin crashes via EOF on Chromium IPC.

ppapi/native_client/src/trusted/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 "ppapi/native_client/src/trusted/plugin/service_runtime.h"
 
@@ skipping 40 matching lines @@
 
 namespace plugin {
 
 OpenManifestEntryResource::~OpenManifestEntryResource() {
 }
 
 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_);
 }
 
 PluginReverseInterface::~PluginReverseInterface() {
   NaClCondVarDtor(&cv_);
   NaClMutexDtor(&mu_);
 }
 
 void PluginReverseInterface::ShutDown() {
@@ skipping 137 matching lines @@
           4,
           "StreamAsFile_MainThreadContinuation: !PP_OK, setting desc -1\n");
       p->file_info->desc = -1;
     }
     *p->op_complete_ptr = true;
     NaClXCondVarBroadcast(&cv_);
   }
 }
 
 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) {
   // We do nothing here; reporting exit status is handled through a separate
   // embedder interface.
 }
 
 int64_t PluginReverseInterface::RequestQuotaForWrite(
     nacl::string file_id, int64_t offset, int64_t bytes_to_write) {
   return bytes_to_write;
 }
 
 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),
       uses_nonsfi_mode_(uses_nonsfi_mode),
       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),
       bootstrap_channel_(NACL_INVALID_HANDLE) {
   NaClSrpcChannelInitialize(&command_channel_);
   NaClXMutexCtor(&mu_);
   NaClXCondVarCtor(&cond_);
 }
 
 bool ServiceRuntime::SetupCommandChannel() {
@@ skipping 207 matching lines @@
 
 bool ServiceRuntime::StartNexeInternal() {
   if (!SetupCommandChannel())
     return false;
   if (!InitReverseService())
     return false;
   return StartModule();
 }
 
 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) {
   if (main_service_runtime_) {
     plugin_->ReportLoadError(error_info);
   }
 }
 
 SrpcClient* ServiceRuntime::SetupAppChannel() {
   NaClLog(4, "ServiceRuntime::SetupAppChannel (subprocess_=%p)\n",
@@ skipping 58 matching lines @@
     reverse_service_->Unref();
 
   rev_interface_->Unref();
 
   anchor_->Unref();
   NaClCondVarDtor(&cond_);
   NaClMutexDtor(&mu_);
 }
 
 }  // namespace plugin