Pepper: Remove LOAD_MODULE SRPC call in 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 54 matching lines @@
 
 void Plugin::HistogramTimeSmall(const std::string& name,
                                 int64_t ms) {
   if (ms < 0) return;
   uma_interface_.HistogramCustomTimes(name,
                                       ms,
                                       kTimeSmallMin, kTimeSmallMax,
                                       kTimeSmallBuckets);
 }
 
-bool Plugin::LoadHelperNaClModule(PP_FileHandle file_handle,
-                                  NaClSubprocess* subprocess,
+bool Plugin::LoadHelperNaClModule(NaClSubprocess* subprocess,
                                   const SelLdrStartParams& params) {
   CHECK(!pp::Module::Get()->core()->IsMainThread());
   ServiceRuntime* service_runtime =
       new ServiceRuntime(this,
                          pp_instance(),
                          false,  // No main_service_runtime.
                          false,  // No non-SFI mode (i.e. in SFI-mode).
                          pp::BlockUntilComplete(), pp::BlockUntilComplete());
   subprocess->set_service_runtime(service_runtime);
   PLUGIN_PRINTF(("Plugin::LoadHelperNaClModule "
@@ skipping 14 matching lines @@
   if (!service_runtime->WaitForSelLdrStart()) {
     PLUGIN_PRINTF(("Plugin::LoadHelperNaClModule "
                    "WaitForSelLdrStart timed out!\n"));
     return false;
   }
   PLUGIN_PRINTF(("Plugin::LoadHelperNaClModule (service_runtime_started=%d)\n",
                  service_runtime_started));
   if (!service_runtime_started)
     return false;
 
-  PP_NaClFileInfo info;
-  info.handle = file_handle;
-  info.token_lo = 0;
-  info.token_hi = 0;
-
-  // Now actually load the nexe, which can happen on a background thread.

[Mark Seaborn, 2014/06/30 20:01:04]

What's the reason for removing "which can happen on a background thread"?  (I'm
not exactly what that comment meant.)

[teravest, 2014/06/30 22:04:56]

This is an old, wrong comment. Previously LoadNexeAndStart() was safe to call
from a background thread, but the code was changed so that's no longer the case.
Unfortunately, this stale comment was left behind.

+  // Now actually load the nexe.

[Mark Seaborn, 2014/06/30 20:01:04]

"load" -> "start"

[teravest, 2014/06/30 22:04:56]

Done.

   //
   // We can't use pp::BlockUntilComplete() inside an in-process plugin, so we
   // have to roll our own blocking logic, similar to WaitForSelLdrStart()
   // above, except without timeout logic.
   pp::Module::Get()->core()->CallOnMainThread(
       0,
       callback_factory_.NewCallback(
           &Plugin::LoadNexeAndStart,
-          service_runtime,
-          info));
+          service_runtime));
   return service_runtime->WaitForNexeStart();
 }
 
 void Plugin::StartSelLdrOnMainThread(int32_t pp_error,
                                      ServiceRuntime* service_runtime,
                                      const SelLdrStartParams& params,
                                      pp::CompletionCallback callback) {
   if (pp_error != PP_OK) {
     PLUGIN_PRINTF(("Plugin::StartSelLdrOnMainThread: non-PP_OK arg "
                    "-- SHOULD NOT HAPPEN\n"));
@@ skipping 22 matching lines @@
   // 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 file_info_for_srpc = kInvalidNaClFileInfo;
   PP_NaClFileInfo file_info_for_ipc = kInvalidNaClFileInfo;
   if (uses_nonsfi_mode) {

[Mark Seaborn, 2014/06/30 20:01:04]

This should be going away, shouldn't it?

[teravest, 2014/06/30 22:04:56]

Thanks, removed.

     // In non-SFI mode, LaunchSelLdr is used to pass the nexe file's descriptor
     // to the NaCl loader process.
     file_info_for_ipc = file_info;
   } else {
     // Otherwise (i.e. in SFI-mode), LoadModule SRPC is still being used.

[Mark Seaborn, 2014/06/30 20:01:04]

This is no longer true. :-)

[teravest, 2014/06/30 22:04:56]

Removed.

     file_info_for_srpc = file_info;
   }
 
   SelLdrStartParams params(manifest_base_url_str,
-                           file_info_for_ipc,
+                           file_info,
                            true /* uses_irt */,
                            true /* uses_ppapi */,
                            enable_dyncode_syscalls,
                            enable_exception_handling,
                            enable_crash_throttling);
   ErrorInfo error_info;
   ServiceRuntime* service_runtime = new ServiceRuntime(
       this, pp_instance(), 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;
   }
 
   // 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_for_srpc);
+      &Plugin::LoadNexeAndStart, service_runtime);
   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) {
+                              ServiceRuntime* service_runtime) {
   CHECK(pp::Module::Get()->core()->IsMainThread());
   if (pp_error != PP_OK)
     return;
-  service_runtime->LoadNexeAndStart(file_info);
+  service_runtime->LoadNexeAndStart();
 }
 
 bool Plugin::LoadNaClModuleContinuationIntern() {
   ErrorInfo error_info;
   if (!uses_nonsfi_mode_) {
     if (!main_subprocess_.StartSrpcServices()) {
       // The NaCl process probably crashed. On Linux, a crash causes this
       // error, while on other platforms, the error is detected below, when we
       // attempt to start the proxy. Report a module initialization error here,
       // to make it less confusing for developers.
@@ skipping 18 matching lines @@
                                              PP_FileHandle file_handle,
                                              ErrorInfo* error_info) {
   nacl::scoped_ptr<NaClSubprocess> nacl_subprocess(
       new NaClSubprocess("helper module", NULL, NULL));
   if (NULL == nacl_subprocess.get()) {
     error_info->SetReport(PP_NACL_ERROR_SEL_LDR_INIT,
                           "unable to allocate helper subprocess.");
     return NULL;
   }
 
+  PP_NaClFileInfo info;

[Mark Seaborn, 2014/06/30 20:01:03]

Nit: how about "file_info"?  ("info" is rather generic)

[teravest, 2014/06/30 22:04:56]

Done.

+  info.handle = file_handle;
+  info.token_lo = 0;

[Mark Seaborn, 2014/06/30 20:01:04]

Maybe comment this with: "We never apply validation caching to the PNaCl
translator and linker"

[teravest, 2014/06/30 22:04:56]

Done.

[jvoung (off chromium), 2014/06/30 22:34:43]

Wait -- rebase against 280605 ?

The should be a test "ValidationCacheOfTranslatorNexes" that checks this:
https://codereview.chromium.org/356923004/diff/120001/chrome/test/nacl/nacl_b...

[teravest, 2014/07/01 17:48:34]

Rebased and cleaned this up. Thanks.

+  info.token_hi = 0;
+
   // 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,
+                           info,
                            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)) {
+  if (!LoadHelperNaClModule(nacl_subprocess.get(), params)) {
     return NULL;
   }
   // We need not wait for the init_done callback.  We can block
   // here in StartSrpcServices, since helper NaCl modules
   // are spawned from a private thread.
   //
   // TODO(bsy): if helper module crashes, we should abort.
   // crash_cb is not used here, so we are relying on crashes
   // being detected in StartSrpcServices or later.
   //
@@ skipping 256 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