NaCl: clean up nexe loading logic in trusted plugin.

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 418 matching lines @@
 
 void PluginReverseInterface::ReportExitStatus(int exit_status) {
   service_runtime_->set_exit_status(exit_status);
 }
 
 int64_t PluginReverseInterface::RequestQuotaForWrite(
     nacl::string file_id, int64_t offset, int64_t bytes_to_write) {
   return bytes_to_write;
 }
 
-// Thin wrapper for the arguments of LoadNexeAndStart(), as WeakRefNewCallback
-// can take only one argument. Also, this dtor has the responsibility to invoke
-// callbacks on destruction.
-struct ServiceRuntime::LoadNexeAndStartData {
-  explicit LoadNexeAndStartData(const pp::CompletionCallback& callback)
-      : callback(callback) {
-  }
-
-  ~LoadNexeAndStartData() {
-    // We must call the callbacks here if they are not yet called, otherwise
-    // the resource would be leaked.
-    if (callback.pp_completion_callback().func)
-      callback.RunAndClear(PP_ERROR_ABORTED);
-  }
-
-  // On success path, this must be invoked manually. Otherwise the dtor would
-  // invoke callbacks with error code unexpectedly.
-  void Clear() {
-    callback = pp::CompletionCallback();
-  }
-
-  pp::CompletionCallback callback;
-};
-
 ServiceRuntime::ServiceRuntime(Plugin* plugin,
                                bool main_service_runtime,
                                bool uses_nonsfi_mode,
                                pp::CompletionCallback init_done_cb,
                                pp::CompletionCallback crash_cb)
     : plugin_(plugin),
       main_service_runtime_(main_service_runtime),
       uses_nonsfi_mode_(uses_nonsfi_mode),
       reverse_service_(NULL),
       anchor_(new nacl::WeakRefAnchor()),
       rev_interface_(new PluginReverseInterface(anchor_, plugin, this,
                                                 init_done_cb, crash_cb)),
       start_sel_ldr_done_(false),
-      nexe_started_(false) {
+      nexe_started_(false),
+      nexe_started_ok_(false) {
   NaClSrpcChannelInitialize(&command_channel_);
   NaClXMutexCtor(&mu_);
   NaClXCondVarCtor(&cond_);
 }
 
-void ServiceRuntime::LoadNexeAndStartAfterLoadModule(
-    LoadNexeAndStartData* data, int32_t pp_error) {
-  if (pp_error != PP_OK) {
-    DidLoadNexeAndStart(data, pp_error);
-    return;
-  }
-
-  // Here, LoadModule is successfully done. So the remaining task is just
-  // calling StartModule(), here.
-  DidLoadNexeAndStart(data, StartModule() ? PP_OK : PP_ERROR_FAILED);
-}
-
-void ServiceRuntime::DidLoadNexeAndStart(
-    LoadNexeAndStartData* data, int32_t pp_error) {
-  if (pp_error == PP_OK) {
-    NaClLog(4, "ServiceRuntime::LoadNexeAndStart (success)\n");

[hidehiko, 2014/06/18 05:07:04]

Probably we want to keep this lo, too.

-  } else {
-    // 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 esrvice 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.
-    Log(LOG_FATAL, "reap logs");
-    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");
-    }
-  }
-
-  pp::Module::Get()->core()->CallOnMainThread(0, data->callback, pp_error);
-
-  // Because the ownership of data is taken by caller, we must clear it
-  // manually here. Otherwise, its dtor invokes callbacks again.
-  data->Clear();
-}
-
 bool ServiceRuntime::SetupCommandChannel() {
   NaClLog(4, "ServiceRuntime::SetupCommand (this=%p, subprocess=%p)\n",
           static_cast<void*>(this),
           static_cast<void*>(subprocess_.get()));
   if (!subprocess_->SetupCommand(&command_channel_)) {
     if (main_service_runtime_) {
       ErrorInfo error_info;
       error_info.SetReport(PP_NACL_ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
                            "ServiceRuntime: command channel creation failed");
       plugin_->ReportLoadError(error_info);
     }
     return false;
   }
   return true;
 }
 
-void ServiceRuntime::LoadModule(PP_NaClFileInfo file_info,
-                                pp::CompletionCallback callback) {
-  NaClFileInfo nacl_file_info;
-  nacl_file_info.desc = ConvertFileDescriptor(file_info.handle, true);
-  nacl_file_info.file_token.lo = file_info.token_lo;
-  nacl_file_info.file_token.hi = file_info.token_hi;
-  NaClDesc* desc = NaClDescIoFromFileInfo(nacl_file_info, O_RDONLY);
-  if (desc == NULL) {
-    DidLoadModule(callback, PP_ERROR_FAILED);
-    return;
-  }
-
-  // We don't use a scoped_ptr here since we would immediately release the
-  // DescWrapper to LoadModule().
-  nacl::DescWrapper* wrapper =
-      plugin_->wrapper_factory()->MakeGenericCleanup(desc);
-
-  // TODO(teravest, hidehiko): Replace this by Chrome IPC.
-  bool result = subprocess_->LoadModule(&command_channel_, wrapper);
-  DidLoadModule(callback, result ? PP_OK : PP_ERROR_FAILED);
-}
-
-void ServiceRuntime::DidLoadModule(pp::CompletionCallback callback,
-                                   int32_t pp_error) {
-  if (pp_error != PP_OK) {
-    ErrorInfo error_info;
-    error_info.SetReport(PP_NACL_ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
-                         "ServiceRuntime: load module failed");
-    plugin_->ReportLoadError(error_info);
-  }
-  callback.Run(pp_error);
-}
-
 bool ServiceRuntime::InitReverseService() {
   if (uses_nonsfi_mode_) {
     // In non-SFI mode, no reverse service is set up. Just returns success.
     return true;
   }
 
   // Hook up the reverse service channel.  We are the IMC client, but
   // provide SRPC service.
   NaClDesc* out_conn_cap;
   NaClSrpcResultCodes rpc_result =
@@ skipping 153 matching lines @@
   }
   return start_sel_ldr_done_;
 }
 
 void ServiceRuntime::SignalStartSelLdrDone() {
   nacl::MutexLocker take(&mu_);
   start_sel_ldr_done_ = true;
   NaClXCondVarSignal(&cond_);
 }
 
-void ServiceRuntime::WaitForNexeStart() {
+bool ServiceRuntime::WaitForNexeStart() {
   nacl::MutexLocker take(&mu_);
   while (!nexe_started_)
     NaClXCondVarWait(&cond_, &mu_);
-  // Reset nexe_started_ here in case we run again.
-  nexe_started_ = false;
+  return nexe_started_ok_;
 }
 
-void ServiceRuntime::SignalNexeStarted() {
+void ServiceRuntime::SignalNexeStarted(bool ok) {
   nacl::MutexLocker take(&mu_);
   nexe_started_ = true;

[hidehiko, 2014/06/18 05:07:04]

Probably we should rename this variable (and the method name, too?).
If LoadNexeAndStartInternal fails, actually nexe is not started, but
nexe_started_ is expected to be set to true even such a case. Sounds confusing.
Also, nexe_started_ and nexe_started_ok_ sounds confusing, because those "name"s
look same meaning.

[Nick Bray (chromium), 2014/06/18 18:29:44]

Done, but please provide suggestions when names are bad.  I find it hard to come
up with good ones on my own, and a second brain is always welcome.

+  nexe_started_ok_ = ok;
   NaClXCondVarSignal(&cond_);
 }
 
-void ServiceRuntime::LoadNexeAndStart(PP_NaClFileInfo file_info,
-                                      const pp::CompletionCallback& callback) {
+void ServiceRuntime::LoadNexeAndStart(PP_NaClFileInfo file_info) {
   NaClLog(4, "ServiceRuntime::LoadNexeAndStart (handle_valid=%d "
              "token_lo=%" NACL_PRIu64 " token_hi=%" NACL_PRIu64 ")\n",
       file_info.handle != PP_kInvalidFileHandle,
       file_info.token_lo,
       file_info.token_hi);
 
-  nacl::scoped_ptr<LoadNexeAndStartData> data(
-      new LoadNexeAndStartData(callback));
-  if (!SetupCommandChannel() || !InitReverseService()) {
-    DidLoadNexeAndStart(data.get(), PP_ERROR_FAILED);
-    return;
+  bool ok = LoadNexeAndStartInternal(file_info);
+  if (!ok) {
+    ReapLogs();
   }
+  // This only matters if a background thread is waiting, but we signal in all
+  // cases to simplify the code.
+  SignalNexeStarted(ok);
+}
 
-  LoadModule(
-      file_info,
-      WeakRefNewCallback(anchor_,
-                         this,
-                         &ServiceRuntime::LoadNexeAndStartAfterLoadModule,
-                         data.release()));  // Delegate the ownership.
+bool ServiceRuntime::LoadNexeAndStartInternal(PP_NaClFileInfo file_info) {
+  if(!SetupCommandChannel()) {
+    return false;
+  }
+  if (!InitReverseService()) {
+    return false;
+  }
+  NaClFileInfo nacl_file_info;
+  nacl_file_info.desc = ConvertFileDescriptor(file_info.handle, true);
+  nacl_file_info.file_token.lo = file_info.token_lo;
+  nacl_file_info.file_token.hi = file_info.token_hi;
+  NaClDesc* desc = NaClDescIoFromFileInfo(nacl_file_info, O_RDONLY);
+  if (desc == NULL) {
+    // TODO(ncbray): better error reporting?
+    ErrorInfo error_info;

[hidehiko, 2014/06/18 05:07:04]

Can we avoid dup?

Probably, we should refactor LoadModule() out, similar to
InitReverseService/StartModule etc.

[Nick Bray (chromium), 2014/06/18 18:29:44]

Done.  Good call.  I am not 100% happy with reporting the error in this method
once the functionality is factored out because it breaks the pattern of the
callee doing error reporting, but... I have no better idea.

+    error_info.SetReport(PP_NACL_ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
+                         "ServiceRuntime: load module failed");
+    plugin_->ReportLoadError(error_info);
+    return false;
+  }
+  // We don't use a scoped_ptr here since we would immediately release the
+  // DescWrapper to LoadModule().
+  nacl::DescWrapper* wrapper =
+    plugin_->wrapper_factory()->MakeGenericCleanup(desc);
+  // TODO(teravest, hidehiko): Replace this by Chrome IPC.
+  if (!subprocess_->LoadModule(&command_channel_, wrapper)) {
+    ErrorInfo error_info;
+    error_info.SetReport(PP_NACL_ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
+                         "ServiceRuntime: load module failed");
+    plugin_->ReportLoadError(error_info);
+    return false;
+  }
+  if (!StartModule()) {

[teravest, 2014/06/18 01:37:52]

We had initially planned on LoadModule() being asynchronous and sending messages
over Chrome IPC, though that changed to no longer be necessary.

However, I'm not sure yet whether or not StartModule will happen over Chrome IPC
(or whether it'll just happen as part of StartSelLdr). I suppose this change is
fine for now, though StartModule and the methods that call it might have to be
made asynchronous again.

[Nick Bray (chromium), 2014/06/18 02:02:38]

I think the rest of the cleanup is still good (?), even if I put the callback
back in (the callback would only happen in one place, now... previously the
callback can either be invoked synchonously or asynchronously.  Joy.)  Tell me
what you want.  The only real trick would be sorting out the weakref/lifetime
issues.

+    return false;
+  }
+  return true;
+}
+
+void ServiceRuntime::ReapLogs() {
+  // 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 esrvice 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.
+  NaClLog(LOG_ERROR, "reap logs\n");

[hidehiko, 2014/06/18 05:07:04]

Why not fatal?

[Nick Bray (chromium), 2014/06/18 18:29:44]

Oh.  This is even more subtle than I realized.  It originally called "Log" 
which I though was digging around with the internals of NaClLog... but it looks
like it was calling a method on the class which makes an SRPC request which
indirectly kills the sel_ldr process.  (Which I believe causes flakiness for
some of the "bad" tests because the plugin does not immediately detect the
sel_ldr process has died... :/)  I need to look at this again.

+  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");
+  }
 }
 
 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 {
@@ skipping 66 matching lines @@
 
 nacl::string ServiceRuntime::GetCrashLogOutput() {
   if (NULL != subprocess_.get()) {
     return subprocess_->GetCrashLogOutput();
   } else {
     return std::string();
   }
 }
 
 }  // namespace plugin