NaCl: clean up nexe loading logic in trusted plugin.

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 3d7323e2dd2efcae6629ecb6f00436d1125d7c6d..49b66e53d7729a229f3dd770c57911d8ac05c9e7 100644
--- a/ppapi/native_client/src/trusted/plugin/service_runtime.cc
+++ b/ppapi/native_client/src/trusted/plugin/service_runtime.cc
@@ -436,30 +436,6 @@ int64_t PluginReverseInterface::RequestQuotaForWrite(
   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,
@@ -473,56 +449,13 @@ ServiceRuntime::ServiceRuntime(Plugin* plugin,
       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),
@@ -539,39 +472,6 @@ bool ServiceRuntime::SetupCommandChannel() {
   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.
@@ -745,41 +645,93 @@ void ServiceRuntime::SignalStartSelLdrDone() {
   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() {