Non-SFI NaCl: Batch-open resource files

components/nacl/renderer/ppb_nacl_private_impl.cc

Index: components/nacl/renderer/ppb_nacl_private_impl.cc
diff --git a/components/nacl/renderer/ppb_nacl_private_impl.cc b/components/nacl/renderer/ppb_nacl_private_impl.cc
index 2df50bbf293e23782a26875353538d74a4b584ac..b2a40e14070591aaf31efcc256727c8ed7781c1f 100644
--- a/components/nacl/renderer/ppb_nacl_private_impl.cc
+++ b/components/nacl/renderer/ppb_nacl_private_impl.cc
@@ -301,6 +301,7 @@ void LaunchSelLdr(PP_Instance instance,
                   PP_Bool main_service_runtime,
                   const char* alleged_url,
                   const PP_NaClFileInfo* nexe_file_info,
+                  const PP_NaClResourceFileHandle* resource_file_handles,
                   PP_Bool uses_nonsfi_mode,
                   PP_Bool enable_ppapi_dev,
                   PP_NaClAppProcessType pp_process_type,
@@ -354,14 +355,30 @@ void LaunchSelLdr(PP_Instance instance,
 
   IPC::PlatformFileForTransit nexe_for_transit =
       IPC::InvalidPlatformFileForTransit();
+  std::vector<IPC::PlatformFileForTransit> resource_files_for_transit;
+  std::vector<std::pair<uint64, uint64> > resource_file_tokens;
+  std::vector<std::string> resource_keys;
 #if defined(OS_POSIX)
   if (nexe_file_info->handle != PP_kInvalidFileHandle)
     nexe_for_transit = base::FileDescriptor(nexe_file_info->handle, true);
+  for (size_t i = 0;
+       resource_file_handles &&
+           (resource_file_handles[i].handle != PP_kInvalidFileHandle);
+       ++i) {
+    resource_files_for_transit.push_back(
+        base::FileDescriptor(resource_file_handles[i].handle, true));
+    resource_file_tokens.push_back(
+        std::make_pair(resource_file_handles[i].token_lo,
+                       resource_file_handles[i].token_hi));
+    resource_keys.push_back(resource_file_handles[i].key);
+  }
 #elif defined(OS_WIN)
   // Duplicate the handle on the browser side instead of the renderer.
   // This is because BrokerGetFileForProcess isn't part of content/public, and
   // it's simpler to do the duplication in the browser anyway.
   nexe_for_transit = nexe_file_info->handle;
+  // TODO(yusukes): Support pre-opening resource files.
+  CHECK(resource_file_handles == NULL);
 #else
 #error Unsupported target platform.
 #endif
@@ -371,6 +388,9 @@ void LaunchSelLdr(PP_Instance instance,
               nexe_for_transit,
               nexe_file_info->token_lo,
               nexe_file_info->token_hi,
+              resource_files_for_transit,
+              resource_file_tokens,
+              resource_keys,
               routing_id,
               perm_bits,
               PP_ToBool(uses_nonsfi_mode),
@@ -681,10 +701,13 @@ void ReportTranslationFinished(PP_Instance instance,
   g_pnacl_resource_host.Get()->ReportTranslationFinished(instance, success);
 }
 
-PP_FileHandle OpenNaClExecutable(PP_Instance instance,
-                                 const char* file_url,
-                                 uint64_t* nonce_lo,
-                                 uint64_t* nonce_hi) {
+PP_FileHandle OpenNaClExecutable(

[teravest, 2014/10/24 16:18:06]

OpenNaClExecutable should probably be renamed to OpenNaClResources throughout.

[Yusuke Sato, 2014/11/04 22:50:22]

Done.

+    PP_Instance instance,
+    const char* file_url,
+    uint64_t* nonce_lo,
+    uint64_t* nonce_hi,
+    const std::vector<std::string>& resource_file_urls,
+    PP_NaClResourceFileHandle** out_resource_file_handles) {
   // Fast path only works for installed file URLs.
   GURL gurl(file_url);
   if (!gurl.SchemeIs("chrome-extension"))
@@ -709,18 +732,58 @@ PP_FileHandle OpenNaClExecutable(PP_Instance instance,
   *nonce_lo = 0;
   *nonce_hi = 0;
   base::FilePath file_path;
-  if (!sender->Send(
-      new NaClHostMsg_OpenNaClExecutable(GetRoutingID(instance),
-                                         GURL(file_url),
-                                         &out_fd,
-                                         nonce_lo,
-                                         nonce_hi))) {
+
+  std::vector<GURL> resource_urls;
+  for (size_t i = 0; i < resource_file_urls.size(); ++i) {
+    GURL gurl(resource_file_urls[i]);
+    if (!gurl.SchemeIs("chrome-extension"))
+      return PP_kInvalidFileHandle;
+    if (!security_origin.canRequest(gurl))
+      return PP_kInvalidFileHandle;
+    resource_urls.push_back(gurl);
+  }
+
+  nacl::NaClOpenExecutableResult open_result;
+  if (!sender->Send(new NaClHostMsg_OpenNaClExecutable(GetRoutingID(instance),
+                                                       GURL(file_url),
+                                                       resource_urls,
+                                                       &open_result))) {
     return PP_kInvalidFileHandle;
   }
 
-  if (out_fd == IPC::InvalidPlatformFileForTransit())
+  if (open_result.file == IPC::InvalidPlatformFileForTransit())
     return PP_kInvalidFileHandle;
 
+  for (size_t i = 0; i < open_result.resource_files.size(); ++i) {
+    if (open_result.resource_files[i] == IPC::InvalidPlatformFileForTransit())
+      return PP_kInvalidFileHandle;

[teravest, 2014/10/24 16:18:06]

There should at least be some debug logging added to note which resource file
failed to open.

[Yusuke Sato, 2014/11/04 22:50:22]

Done.

+  }
+  CHECK(open_result.resource_files.size() ==
+        open_result.resource_file_tokens.size());
+
+  out_fd = open_result.file;
+  *nonce_lo = open_result.file_token_lo;
+  *nonce_lo = open_result.file_token_hi;

[teravest, 2014/10/24 16:18:06]

Shouldn't this be nonce_hi? Is the behavior for file tokens tested?

[Yusuke Sato, 2014/11/04 22:50:22]

Good catch. Fixed. This will be used in the next CL for SFI-NaCl.

+
+  if (out_resource_file_handles && !open_result.resource_files.empty()) {
+    *out_resource_file_handles =
+        new PP_NaClResourceFileHandle[open_result.resource_files.size() + 1];
+    for (size_t i = 0; i < open_result.resource_files.size(); ++i) {
+      PP_NaClResourceFileHandle& entry = (*out_resource_file_handles)[i];
+      entry.handle = IPC::PlatformFileForTransitToPlatformFile(
+          open_result.resource_files[i]);
+      entry.token_lo = open_result.resource_file_tokens[i].first;
+      entry.token_hi = open_result.resource_file_tokens[i].second;
+      entry.key = NULL;  // DownloadNexe fills this later.
+    }
+    PP_NaClResourceFileHandle& sentinel =
+        (*out_resource_file_handles)[open_result.resource_files.size()];

[teravest, 2014/10/24 16:18:06]

So, out_resource_file_handles is always guaranteed to have length one more than
open_result.resource_files?

Why store a sentinel value instead of passing a length around?

[Yusuke Sato, 2014/11/04 22:50:22]

Done.

+    sentinel.handle = PP_kInvalidFileHandle;
+    sentinel.token_lo = 0;
+    sentinel.token_hi = 0;
+    sentinel.key = NULL;
+  }
+
   return IPC::PlatformFileForTransitToPlatformFile(out_fd);
 }
 
@@ -1214,22 +1277,54 @@ void DownloadNexeCompletion(const DownloadNexeRequest& request,
 
 void DownloadNexe(PP_Instance instance,
                   const char* url,
+                  PP_Bool download_resource_files,
                   PP_NaClFileInfo* out_file_info,
+                  PP_NaClResourceFileHandle** out_resource_file_handles,
                   PP_CompletionCallback callback) {
   CHECK(url);
   CHECK(out_file_info);
+  CHECK(!download_resource_files || out_resource_file_handles);
+
   DownloadNexeRequest request;
   request.instance = instance;
   request.url = url;
   request.callback = callback;
   request.start_time = base::Time::Now();
 
+  std::vector<std::string> resource_file_urls;
+  std::vector<std::string> resource_file_keys;
+  if (download_resource_files) {
+    JsonManifest* manifest = GetJsonManifest(instance);
+    if (manifest)
+      manifest->GetFilesUrl(&resource_file_urls, &resource_file_keys);
+    CHECK(resource_file_urls.size() == resource_file_keys.size());
+
+    // TODO(yusukes): Increase kMaxDescriptorsPerMessage in
+    // ipc/file_descriptor_set_posix.h, NACL_ABI_IMC_USER_DESC_MAX in
+    // native_client/src/public/imc_types.h, and NACL_HANDLE_COUNT_MAX in
+    // native_client/src/shared/imc/nacl_imc_c.h, and remove the resize()
+    // calls.
+    if (resource_file_urls.size() > 2) {

[teravest, 2014/10/24 16:18:06]

So what happens if more than two resource file urls are requeted? Only the first
two get fetched?

[Yusuke Sato, 2014/11/04 22:50:22]

Yes.

[Yusuke Sato, 2014/11/05 07:03:23]

It turned out that removing the restriction does not require native_client/
changes. Slightly modified two files in ipc/ and removed the resize(2) calls.

+      resource_file_urls.resize(2);
+      resource_file_keys.resize(2);
+    }
+  }
+
   // Try the fast path for retrieving the file first.
   PP_FileHandle handle = OpenNaClExecutable(instance,
                                             url,
                                             &out_file_info->token_lo,
-                                            &out_file_info->token_hi);
+                                            &out_file_info->token_hi,
+                                            resource_file_urls,
+                                            out_resource_file_handles);
   if (handle != PP_kInvalidFileHandle) {
+    if (!resource_file_urls.empty()) {
+      for (size_t i = 0; i < resource_file_urls.size(); ++i) {
+        PP_NaClResourceFileHandle& entry = (*out_resource_file_handles)[i];
+        CHECK(entry.handle != PP_kInvalidFileHandle) << i;
+        entry.key = strdup(resource_file_keys[i].c_str());
+      }
+    }
     DownloadNexeCompletion(request,
                            out_file_info,
                            FileDownloader::SUCCESS,
@@ -1243,6 +1338,10 @@ void DownloadNexe(PP_Instance instance,
   base::File target_file(CreateTemporaryFile(instance));
   GURL gurl(url);
 
+  // On the slow path, do not retry to pre-open the resource files.
+  if (out_resource_file_handles)
+    *out_resource_file_handles = NULL;
+
   content::PepperPluginInstance* plugin_instance =
       content::PepperPluginInstance::Get(instance);
   if (!plugin_instance) {
@@ -1383,7 +1482,9 @@ void DownloadFile(PP_Instance instance,
   PP_FileHandle file_handle = OpenNaClExecutable(instance,
                                                  url.c_str(),
                                                  &file_token_lo,
-                                                 &file_token_hi);
+                                                 &file_token_hi,
+                                                 std::vector<std::string>(),
+                                                 NULL);
   if (file_handle != PP_kInvalidFileHandle) {
     PP_NaClFileInfo file_info;
     file_info.handle = file_handle;