SFI NaCl: Batch-open resource files

components/nacl/loader/nacl_ipc_adapter.cc

Index: components/nacl/loader/nacl_ipc_adapter.cc
diff --git a/components/nacl/loader/nacl_ipc_adapter.cc b/components/nacl/loader/nacl_ipc_adapter.cc
index 5df92fbe6adb4ef6e1bcf56b2bba334f19bc1469..25315ce66e95b671fdd4dd279873c9120a7c9ac1 100644
--- a/components/nacl/loader/nacl_ipc_adapter.cc
+++ b/components/nacl/loader/nacl_ipc_adapter.cc
@@ -13,6 +13,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/shared_memory.h"
 #include "base/task_runner_util.h"
+#include "base/tuple.h"
 #include "build/build_config.h"
 #include "ipc/ipc_channel.h"
 #include "ipc/ipc_platform_file.h"
@@ -627,6 +628,15 @@ scoped_ptr<IPC::Message> CreateOpenResourceReply(
 void NaClIPCAdapter::OnFileTokenResolved(const IPC::Message& orig_msg,
                                          IPC::PlatformFileForTransit ipc_fd,
                                          base::FilePath file_path) {
+  base::AutoLock lock(lock_);
+  OnFileTokenResolvedLocked(orig_msg, ipc_fd, file_path);
+}
+
+void NaClIPCAdapter::OnFileTokenResolvedLocked(

[Mark Seaborn, 2015/03/20 00:40:52]

Since this is reused for the new open_resource case when there's no file token
involved, it seems like you should rename this.

[Yusuke Sato, 2015/04/16 02:38:44]

Done.

+    const IPC::Message& orig_msg,
+    IPC::PlatformFileForTransit ipc_fd,
+    base::FilePath file_path) {
+  lock_.AssertAcquired();
   // The path where an invalid ipc_fd is returned isn't currently
   // covered by any tests.
   if (ipc_fd == IPC::InvalidPlatformFileForTransit()) {
@@ -652,11 +662,8 @@ void NaClIPCAdapter::OnFileTokenResolved(const IPC::Message& orig_msg,
 
     scoped_refptr<RewrittenMessage> rewritten_msg(new RewrittenMessage);
     rewritten_msg->AddDescriptor(desc_wrapper.release());
-    {
-      base::AutoLock lock(lock_);
-      SaveMessage(*new_msg, rewritten_msg.get());
-      cond_var_.Signal();
-    }
+    SaveMessage(*new_msg, rewritten_msg.get());

[Yusuke Sato, 2015/04/16 02:38:44]

Reverted this change.

+    cond_var_.Signal();
     return;
   }
 
@@ -673,11 +680,8 @@ void NaClIPCAdapter::OnFileTokenResolved(const IPC::Message& orig_msg,
   struct NaClDesc* desc =
       NaClDescCreateWithFilePathMetadata(handle, file_path_str.c_str());
   rewritten_msg->AddDescriptor(new NaClDescWrapper(desc));
-  {
-    base::AutoLock lock(lock_);
-    SaveMessage(*new_msg, rewritten_msg.get());
-    cond_var_.Signal();
-  }
+  SaveMessage(*new_msg, rewritten_msg.get());

[Yusuke Sato, 2015/04/16 02:38:44]

same. reverted.

+  cond_var_.Signal();
 }
 
 void NaClIPCAdapter::OnChannelConnected(int32 peer_pid) {
@@ -758,6 +762,21 @@ bool NaClIPCAdapter::SendCompleteMessage(const char* buffer,
   if (new_msg)
     msg.reset(new_msg.release());
 
+  // Handle PpapiHostMsg_OpenResource locally without sending an IPC to the

[Yusuke Sato, 2015/04/16 02:38:44]

Moved this to SendMessageOnIOThread so that
NaClIPCAdapter::OnFileTokenResolvedLocked (which is now renamed to
SaveOpenResourceMessage) is called on the IO thread too.

The comment at line 506
 // resolve_file_token_cb_ must be invoked from the main thread
was incorrect, and the slow path actually calls OnFileTokenResolved on the IO
thread. The fast path should do the same.

+  // renderer when possible.
+  PpapiHostMsg_OpenResource::Schema::SendParam send_params;
+  if (!open_resource_cb_.is_null() &&
+      (msg->type() == PpapiHostMsg_OpenResource::ID) &&

[Mark Seaborn, 2015/03/20 00:40:52]

Nit: ()s not needed around this

[Yusuke Sato, 2015/04/16 02:38:44]

Done.

+      PpapiHostMsg_OpenResource::ReadSendParam(msg.get(), &send_params)) {
+    const std::string key = get<0>(send_params);
+    if (open_resource_cb_.Run(
+            *msg.get(), key,
+            base::Bind(&NaClIPCAdapter::OnFileTokenResolvedLocked, this))) {
+      // The callback sent a reply to the untrusted side.
+      return true;
+    }
+  }
+
   // Actual send must be done on the I/O thread.
   task_runner_->PostTask(FROM_HERE,
       base::Bind(&NaClIPCAdapter::SendMessageOnIOThread, this,