Non-SFI NaCl: Batch-open resource files

components/nacl/browser/nacl_host_message_filter.cc

Index: components/nacl/browser/nacl_host_message_filter.cc
diff --git a/components/nacl/browser/nacl_host_message_filter.cc b/components/nacl/browser/nacl_host_message_filter.cc
index 60093eff3ffdfb2009e767c2c44a82063ca081cf..b7fdaa7034b08d2495bd96068bae6c3e54113568 100644
--- a/components/nacl/browser/nacl_host_message_filter.cc
+++ b/components/nacl/browser/nacl_host_message_filter.cc
@@ -25,6 +25,11 @@ namespace nacl {
 
 namespace {
 
+// The maximum number of resource file handles NaClProcessMsg_Start message
+// can have.
+// TODO(yusukes): Increase the number.
+const size_t kMaxPreOpenResourceFiles = 2;
+
 ppapi::PpapiPermissions GetNaClPermissions(
     uint32 permission_bits,
     content::BrowserContext* browser_context,
@@ -118,6 +123,7 @@ net::HostResolver* NaClHostMessageFilter::GetHostResolver() {
 void NaClHostMessageFilter::OnLaunchNaCl(
     const nacl::NaClLaunchParams& launch_params,
     IPC::Message* reply_msg) {
+  const std::vector<nacl::NaClResourceFileInfo> empty;
   // If we're running llc or ld for the PNaCl translator, we don't need to look
   // up permissions, and we don't have the right browser state to look up some
   // of the whitelisting parameters anyway.
@@ -126,6 +132,7 @@ void NaClHostMessageFilter::OnLaunchNaCl(
     LaunchNaClContinuation(
         launch_params,
         reply_msg,
+        empty,
         ppapi::PpapiPermissions(perms));
     return;
   }
@@ -139,13 +146,113 @@ void NaClHostMessageFilter::OnLaunchNaCl(
       base::Bind(&NaClHostMessageFilter::LaunchNaClContinuation,
                  this,
                  launch_params,
-                 reply_msg));
+                 reply_msg,
+                 empty));
+}
+
+void NaClHostMessageFilter::BatchOpenResourceFiles(
+    const nacl::NaClLaunchParams& launch_params,
+    IPC::Message* reply_msg,
+    ppapi::PpapiPermissions permissions) {
+  content::RenderViewHost* rvh = content::RenderViewHost::FromID(

[Mark Seaborn, 2015/02/25 20:01:23]

You're doing things in this PostBlockingPoolTask() callback that I don't think
are valid to do on this thread.

e.g. How is use of content::RenderViewHost thread-safe?  What keeps this
reference valid?

I think you'd need to do this before PostBlockingPoolTask(), and only call
OpenNaClReadExecImpl() from the blocking-pool thread.

[Yusuke Sato, 2015/03/01 06:59:37]

No, it's not valid. RVH::FromID has to be called in the UI thread (like
nacl_file_host.cc does). Thanks for catching this. Fixed.

+      render_process_id(), launch_params.render_view_id);
+  if (!rvh) {
+    BadMessageReceived();  // Kill the renderer.
+    return;
+  }
+
+  std::vector<nacl::NaClResourceFileInfo> prefetched_resource_files_info;
+  // TODO(yusukes): Support SFI mode.
+  if (launch_params.uses_nonsfi_mode) {

[Yusuke Sato, 2015/03/01 06:59:37]

removed the IF

+    content::SiteInstance* site_instance = rvh->GetSiteInstance();
+    for (size_t i = 0;
+         i < launch_params.prefetched_resource_files.size(); ++i) {
+      nacl::NaClResourceFileInfo prefetched_resource_file;
+      GURL gurl(launch_params.prefetched_resource_files[i].first);
+      // IMPORTANT SECURITY CHECK: Do the same check as OpenNaClExecutable()
+      // in nacl_file_host.cc.
+      if (!content::SiteInstance::IsSameWebSite(
+              site_instance->GetBrowserContext(),
+              site_instance->GetSiteURL(),
+              gurl)) {
+        continue;
+      }
+      if (!nacl::NaClBrowser::GetDelegate()->MapUrlToLocalFilePath(
+              gurl,
+              true,  // blocking

[Mark Seaborn, 2015/02/25 20:01:23]

Nit: "use_blocking_api" (for consistency)

[Yusuke Sato, 2015/03/01 06:59:37]

Done.

+              profile_directory_,
+              &prefetched_resource_file.file_path)) {
+        continue;
+      }
+      base::File file = nacl::OpenNaClReadExecImpl(
+          prefetched_resource_file.file_path, true /* is_executable */);
+      if (!file.IsValid())
+        continue;
+
+      prefetched_resource_file.file =
+          IPC::TakeFileHandleForProcess(file.Pass(), PeerHandle());
+      prefetched_resource_file.file_key =
+          launch_params.prefetched_resource_files[i].second;
+
+      prefetched_resource_files_info.push_back(prefetched_resource_file);
+      if (prefetched_resource_files_info.size() > kMaxPreOpenResourceFiles)
+        break;
+    }
+  }
+
+  nacl::NaClLaunchParams new_launch_params(launch_params);
+  new_launch_params.prefetched_resource_files.clear();
+
+  if (!content::BrowserThread::PostTask(
+          content::BrowserThread::IO,
+          FROM_HERE,
+          base::Bind(&NaClHostMessageFilter::LaunchNaClContinuation,
+                     this,
+                     new_launch_params,
+                     reply_msg,
+                     prefetched_resource_files_info,
+                     permissions))) {
+    for (size_t i = 0; i < prefetched_resource_files_info.size(); ++i) {

[Mark Seaborn, 2015/02/25 20:01:23]

This error handling seems unnecessary.  When would this PostTask() fail?

[Yusuke Sato, 2015/03/01 06:59:37]

Removed. Looks like PostTask always returns true as long as a message loop for
the thread is available.

+      // The base::File destructor will close the file.
+      base::File file(IPC::PlatformFileForTransitToFile(
+          prefetched_resource_files_info[i].file));
+    }
+    NaClHostMsg_LaunchNaCl::WriteReplyParams(
+        reply_msg,
+        NaClLaunchResult(),
+        std::string("Failed to open resource files"));
+    Send(reply_msg);
+  }
 }
 
 void NaClHostMessageFilter::LaunchNaClContinuation(
     const nacl::NaClLaunchParams& launch_params,
     IPC::Message* reply_msg,
+    const std::vector<
+      nacl::NaClResourceFileInfo>& prefetched_resource_files_info,
     ppapi::PpapiPermissions permissions) {
+  if (!launch_params.prefetched_resource_files.empty()) {
+    // Process a list of resource file URLs in
+    // |launch_params.prefetched_resource_files|.
+    // BatchOpenResourceFiles calls this function again with an empty

[Mark Seaborn, 2015/02/25 20:01:23]

It seems kind of hacky for a function to call itself back for another iteration.
 How about splitting this into two functions?

[Yusuke Sato, 2015/03/01 06:59:37]

Done.

+    // |launch_params.prefetched_resource_files|.
+    DCHECK(prefetched_resource_files_info.empty());
+    if (!content::BrowserThread::PostBlockingPoolTask(
+            FROM_HERE,
+            base::Bind(&NaClHostMessageFilter::BatchOpenResourceFiles,
+                       this,
+                       launch_params,
+                       reply_msg,
+                       permissions))) {
+      NaClHostMsg_LaunchNaCl::WriteReplyParams(
+          reply_msg,
+          NaClLaunchResult(),
+          std::string("Failed to open resource files"));
+      Send(reply_msg);
+    }
+    return;
+  }
+
   NaClFileToken nexe_token = {
       launch_params.nexe_token_lo,  // lo
       launch_params.nexe_token_hi   // hi
@@ -180,6 +287,7 @@ void NaClHostMessageFilter::LaunchNaClContinuation(
       GURL(launch_params.manifest_url),
       base::File(nexe_file),
       nexe_token,
+      prefetched_resource_files_info,
       permissions,
       launch_params.render_view_id,
       launch_params.permission_bits,