Remove LoadModule SRPC for non-SFI mode.

ppapi/native_client/src/trusted/plugin/plugin.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.
 
 #include "ppapi/native_client/src/trusted/plugin/plugin.h"
 
 #include <sys/stat.h>
 #include <sys/types.h>
 
 #include <string>
@@ skipping 23 matching lines @@
 
 namespace plugin {
 
 namespace {
 
 // Up to 20 seconds
 const int64_t kTimeSmallMin = 1;         // in ms
 const int64_t kTimeSmallMax = 20000;     // in ms
 const uint32_t kTimeSmallBuckets = 100;
 
+const PP_NaClFileInfo kInvalidNaClFileInfo = {
+  PP_kInvalidFileHandle,
+  0,  // token_lo
+  0,  // token_hi
+};
+
 }  // namespace
 
 void Plugin::ShutDownSubprocesses() {
   PLUGIN_PRINTF(("Plugin::ShutDownSubprocesses (this=%p)\n",
                  static_cast<void*>(this)));
   PLUGIN_PRINTF(("Plugin::ShutDownSubprocesses (%s)\n",
                  main_subprocess_.detailed_description().c_str()));
 
   // Shut down service runtime. This must be done before all other calls so
   // they don't block forever when waiting for the upcall thread to exit.
@@ skipping 106 matching lines @@
                             const pp::CompletionCallback& crash_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
   // associated listener threads do not go unjoined because if they
   // outlive the Plugin object, they will not be memory safe.
   ShutDownSubprocesses();
   pp::Var manifest_base_url =
       pp::Var(pp::PASS_REF, nacl_interface_->GetManifestBaseURL(pp_instance()));
   std::string manifest_base_url_str = manifest_base_url.AsString();
+
+  PP_NaClFileInfo launch_sel_ldr_file_info;

[Mark Seaborn, 2014/06/18 19:53:12]

The names "launch_sel_ldr" and "load_module" are a bit generic-sounding.  Could
we perhaps name them something like:

PP_NaClFileInfo file_info_for_srpc; // Old code path using LoadModule SRPC call.
PP_NaClFileInfo file_info_for_ipc; // New code path using Chrome IPC.

Also, could initialise these both with kInvalidNaClFileInfo outside the
conditionals, instead of inside.

[hidehiko, 2014/06/19 07:02:00]

Done.

+  PP_NaClFileInfo load_module_file_info;
+  if (uses_nonsfi_mode) {
+    // In non-SFI mode, LaunchSelLdr is used to pass the nexe file's descriptor
+    // to the plugin.

[Mark Seaborn, 2014/06/18 19:53:12]

"to the NaCl loader process", really.  "Plugin" is very ambiguous (here, the
sender is the trusted plugin).

[hidehiko, 2014/06/19 07:01:59]

Done.

+    launch_sel_ldr_file_info = file_info;
+    load_module_file_info = kInvalidNaClFileInfo;
+  } else {
+    // Otherwise (i.e. in SFI-mode), LoadModule SRPC is still being used.
+    launch_sel_ldr_file_info = kInvalidNaClFileInfo;
+    load_module_file_info = file_info;
+  }
+
   SelLdrStartParams params(manifest_base_url_str,
+                           launch_sel_ldr_file_info,

[Mark Seaborn, 2014/06/18 19:53:12]

This will take ownership of the FD when a message is sent via Chrome IPC...

Does LoadModule() get passed an owning or borrowed reference to the FD?  I think
it's an owning reference, but I think that means nexe_file_info_ will contain a
dangling reference to the FD, which is a bit dodgy.

[hidehiko, 2014/06/19 07:01:59]

Good point. Can I work on it in a separate CL, to focus on SRPC replacement in
this CL? Could you see my comment below, too?

[Mark Seaborn, 2014/06/19 22:36:47]

Yes, feel free to leave that to a separate change.  I'm not expecting you to fix
existing bugs in this changeset, just expecting no regressions to be added. :-)

                            true /* uses_irt */,
                            true /* uses_ppapi */,
                            enable_dyncode_syscalls,
                            enable_exception_handling,
                            enable_crash_throttling);
   ErrorInfo error_info;
   ServiceRuntime* service_runtime = new ServiceRuntime(
       this, true, uses_nonsfi_mode, init_done_cb, crash_cb);
   main_subprocess_.set_service_runtime(service_runtime);
   PLUGIN_PRINTF(("Plugin::LoadNaClModule (service_runtime=%p)\n",
                  static_cast<void*>(service_runtime)));
   if (NULL == service_runtime) {
     error_info.SetReport(
         PP_NACL_ERROR_SEL_LDR_INIT,
         "sel_ldr init failure " + main_subprocess_.description());
     ReportLoadError(error_info);
     return;

[Mark Seaborn, 2014/06/18 19:53:12]

...but would the FD get closed if we return early here?

[hidehiko, 2014/06/19 07:01:59]

Admittedly...

   }
 
   // We don't take any action once nexe loading has completed, so pass an empty
   // callback here for |callback|.
   pp::CompletionCallback callback = callback_factory_.NewCallback(
       &Plugin::LoadNexeAndStart,
-      service_runtime, file_info, pp::CompletionCallback());
+      service_runtime, load_module_file_info, pp::CompletionCallback());

[Mark Seaborn, 2014/06/18 19:53:12]

...and does this existing SRPC code path take ownership of the FD?  (I am not
sure whether SRPC method calls take ownership of FD arguments -- I suspect they
don't, in which case is this a bug in the existing code?)

[hidehiko, 2014/06/19 07:02:00]

It takes.
So, here we have three problems.
1) If creation of the ServiceRuntime fails, the fd is leaked.
2) If module loading is successfully done (regardless of SRPC or Chrome IPC),
nexe_file_info_ would keep the dangling file descriptor, as SRPC/IPC takes the
ownership.
3) If Plugin instance is destroyed during the initialization process, the fd is
leaked, too. This is because CompletionCallback generated by CallbackFactory
does not have enough feature for RAII on cancellation.

Questions:
- does 1) happen actually? Is "new" operator set to return NULL on failure?
- For 3), is there any standard way to handle these case in NaCl? I guess this
is a typical but corner case. An idea is getting rid of CallbackFactory, instead
to handle cancellation by WeakRef etc. manually. If we can use base::Callback,
it can be solved more easily, but pp::CompletionCallback does not have enough
power. Another idea is extend pp::CallbackFactory/CompletionCallback to support
rvalue concept.

Anyway, as this looks not a regression, I'd like to separate it from this CL.
WDYT?

[Mark Seaborn, 2014/06/19 22:36:48]

I don't know the answer.  Justin probably knows this code better than me.
That's OK with me.

   StartSelLdrOnMainThread(
       static_cast<int32_t>(PP_OK), service_runtime, params, callback);
 }
 
 void Plugin::LoadNexeAndStart(int32_t pp_error,
                               ServiceRuntime* service_runtime,
                               PP_NaClFileInfo file_info,
                               const pp::CompletionCallback& callback) {
   CHECK(pp::Module::Get()->core()->IsMainThread());
   if (pp_error != PP_OK)
@@ skipping 34 matching lines @@
   if (NULL == nacl_subprocess.get()) {
     error_info->SetReport(PP_NACL_ERROR_SEL_LDR_INIT,
                           "unable to allocate helper subprocess.");
     return NULL;
   }
 
   // Do not report UMA stats for translator-related nexes.
   // TODO(sehr): define new UMA stats for translator related nexe events.
   // NOTE: The PNaCl translator nexes are not built to use the IRT.  This is
   // done to save on address space and swap space.
+  //
+  // Currently, this works only in SFI-mode. So, LoadModule SRPC is still used.
+  // So, pass kInvalidNaClFileInfo here, and instead |file_handle| is passed
+  // to LoadNaClModuleFromBackgroundThread() below.
+  // TODO(teravest, hidehiko): Pass file_handle to params, so that LaunchSelLdr
+  // will look at the info.
   SelLdrStartParams params(helper_url,
+                           kInvalidNaClFileInfo,
                            false /* uses_irt */,
                            false /* uses_ppapi */,
                            false /* enable_dyncode_syscalls */,
                            false /* enable_exception_handling */,
                            true /* enable_crash_throttling */);
 
   // Helper NaCl modules always use the PNaCl manifest, as there is no
   // corresponding NMF.
   if (!LoadHelperNaClModule(file_handle, nacl_subprocess.get(), params)) {
     return NULL;
@@ skipping 265 matching lines @@
 
 void Plugin::SetExitStatusOnMainThread(int32_t pp_error,
                                        int exit_status) {
   DCHECK(pp::Module::Get()->core()->IsMainThread());
   DCHECK(nacl_interface_);
   nacl_interface_->SetExitStatus(pp_instance(), exit_status);
 }
 
 
 }  // namespace plugin