[PPAPI] It is now possible to pass filesystems from JavaScript to NaCl modules.

ppapi/proxy/raw_var_data.cc

Index: ppapi/proxy/raw_var_data.cc
diff --git a/ppapi/proxy/raw_var_data.cc b/ppapi/proxy/raw_var_data.cc
index 91bcfbfd1f0b0448126f2da92cc6081076cd1f14..4db48050f0b1eed6ffe1011aecacb8e6f83a242e 100644
--- a/ppapi/proxy/raw_var_data.cc
+++ b/ppapi/proxy/raw_var_data.cc
@@ -9,7 +9,12 @@
 #include "base/containers/hash_tables.h"
 #include "base/stl_util.h"
 #include "ipc/ipc_message.h"
+#include "ppapi/proxy/file_system_resource.h"
+#include "ppapi/proxy/plugin_dispatcher.h"
+#include "ppapi/proxy/plugin_globals.h"
+#include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/ppapi_param_traits.h"
+#include "ppapi/proxy/resource_creation_proxy.h"
 #include "ppapi/shared_impl/array_var.h"
 #include "ppapi/shared_impl/dictionary_var.h"
 #include "ppapi/shared_impl/ppapi_globals.h"
@@ -664,6 +669,80 @@ bool DictionaryRawVarData::Read(PP_VarType type,
   return true;
 }
 
+// ResourceDispatcher ----------------------------------------------------------
+// Used by the plugin side to dispatch resource creation messages.
+// This allows messages from the host side with pending resources to be made
+// into actual plugin-side resources.
+// Must only be created on the plugin side.
+class ResourceDispatcher {

[raymes, 2013/10/11 04:02:24]

This class seems like a little overkill for what's needed here and is a little
confusing for me. I think we only need 2 functions:
1) Connection GetConnection(PP_Instance); which returns a connection.
2) void ResourceRawVarData::HandleCreationMessage(const IPC::Message& msg);
which has a switch(msg.type()) {} and unpacks the creation message in a similar
way to content_renderer_pepper_host_factory.cc.

[Matt Giuca, 2013/10/15 00:25:16]

The reason I needed a class (instead of just functions) is because of the way
IPC_BEGIN_MESSAGE_MAP works. It needs to take a class, and calls a method on the
class. It didn't seem to work with a closure; since
OnMsgFileSystem_CreateFromPendingHost requires some outside state, the only way
to pass it is via the members of a class. (Or so it would seem when I wrote
this; it was some time ago.)

[raymes, 2013/10/16 00:45:14]

My suggestion is to not use IPC_BEGIN_MESSAGE_MAP at all and use a switch
statement :) See content_renderer_pepper_host_factory.cc. I think my above
suggestion will simplify this change significantly.

[Matt Giuca, 2013/10/21 05:20:03]

OK DONE! You were right, this is much simpler. Look at the delta between PS 7
and 8. :)

+ public:
+  // The |pp_resource| argument will be written with the real resource ID once
+  // one of the dispatch methods is called.
+  ResourceDispatcher(PP_Instance instance, int pending_renderer_id,

[raymes, 2013/10/11 04:02:24]

nit: one line per argument

[Matt Giuca, 2013/10/15 00:25:16]

Done.

+                     int pending_browser_id, PP_Resource* pp_resource);
+
+  // Execute an IPC message that creates a plugin-side resource.
+  void ExecuteCreationMessage(const IPC::Message& creation_message);
+
+  void OnMsgFileSystem_CreateFromPendingHost(
+      PP_FileSystemType file_system_type);
+
+ private:
+  Connection GetConnection();
+
+  PP_Instance instance_;
+  int pending_renderer_id_;
+  int pending_browser_id_;
+  PP_Resource* pp_resource_;
+  PluginDispatcher* dispatcher_;
+};
+
+ResourceDispatcher::ResourceDispatcher(
+    PP_Instance instance,
+    int pending_renderer_id,
+    int pending_browser_id,
+    PP_Resource* pp_resource)
+    : instance_(instance),
+      pending_renderer_id_(pending_renderer_id),
+      pending_browser_id_(pending_browser_id),
+      pp_resource_(pp_resource),
+      dispatcher_(NULL) {
+  // Check that this is on the plugin side.
+  DCHECK(PpapiGlobals::Get()->IsPluginGlobals());

[yzshen1, 2013/10/11 17:45:45]

Please see my comment on line 779.

+  dispatcher_ = PluginDispatcher::GetForInstance(instance);
+}
+
+void ResourceDispatcher::ExecuteCreationMessage(
+    const IPC::Message& creation_message) {
+  bool handled = true;
+  bool msg_is_good = false;
+  IPC_BEGIN_MESSAGE_MAP_EX(ResourceDispatcher, creation_message, msg_is_good)
+    IPC_MESSAGE_HANDLER(
+        PpapiPluginMsg_FileSystem_CreateFromPendingHost,
+        OnMsgFileSystem_CreateFromPendingHost);
+    IPC_MESSAGE_UNHANDLED_ERROR()
+  IPC_END_MESSAGE_MAP()
+  DCHECK(handled);
+  DCHECK(msg_is_good);
+}
+
+void ResourceDispatcher::OnMsgFileSystem_CreateFromPendingHost(
+    PP_FileSystemType file_system_type) {
+  DCHECK(pending_renderer_id_);
+  DCHECK(pending_browser_id_);
+  // Create a plugin-side resource and attach it to the host resource.
+  ResourceCreationProxy proxy(dispatcher_);

[yzshen1, 2013/10/11 17:45:45]

Can we just EnterResourceCreationNoLock without explicitly handling
|dispatcher_| and ResourceCreationProxy and GetConnection?

[Matt Giuca, 2013/10/17 09:17:58]

It looks as though the ResourceCreationProxy is not used any more. GetConnection
is still required, though, for the FileSystemResource.

I don't know what EnterResourceCreationNoLock is for. Do we need it?

[yzshen1, 2013/10/23 18:29:23]

ResourceCreationProxy is what we use for most resource creation at the plugin
side.
EnterResourceCreationNoLock.functions() points to a ResourceCreationProxy
object.

One way is to add a new CreateFileSystem() function signature to
ResourceCreationAPI/Proxy, and then use EnterResourceCreationNoLock. The
advantage is that you don't need to re-implement a function to get a connection.

(Optional.)

On 2013/10/17 09:17:58, Matt Giuca wrote:

[Matt Giuca, 2013/10/24 03:15:26]

OK, this seems like a complicated change (touching files which I am not
otherwise touching in this CL). Maybe we can save that for post-commit cleanup.

+  *pp_resource_ = (new FileSystemResource(GetConnection(),
+                                          instance_,
+                                          pending_renderer_id_,
+                                          pending_browser_id_,
+                                          file_system_type))->GetReference();
+}
+
+Connection ResourceDispatcher::GetConnection() {
+  return Connection(PluginGlobals::Get()->GetBrowserSender(), dispatcher_);
+}
+
 // ResourceRawVarData ----------------------------------------------------------
 ResourceRawVarData::ResourceRawVarData()
     : pp_resource_(0),
@@ -695,12 +774,20 @@ bool ResourceRawVarData::Init(const PP_Var& var, PP_Instance /*instance*/) {
 }
 
 PP_Var ResourceRawVarData::CreatePPVar(PP_Instance instance) {
-  // If pp_resource_ is NULL, it could be because we are on the plugin side and
-  // there is a pending resource host on the renderer.
-  // TODO(mgiuca): Create a plugin-side resource in this case.
-  // Currently, this should never occur. This will be needed when passing a
-  // resource from the renderer to the plugin (http://crbug.com/177017).
-  DCHECK(pp_resource_);
+  // If the var is a pending resource host, and we are on the plugin side,
+  // create a plugin-side resource.
+  if (PpapiGlobals::Get()->IsPluginGlobals()) {

[yzshen1, 2013/10/11 17:45:45]

If a plugin is run "in-process", the plugin side of logic is also run in the
renderer process, but in this case I think IsPluginGlobals() will return false.

Dispatcher::IsPlugin() may be the right answer.

[Matt Giuca, 2013/10/17 09:17:58]

Hmm, I'm actually really confused about how this will behave in the in-process
case. (I also don't know how to manually test the in-process case.) PpapiGlobals
has to be either PluginGlobals or HostGlobals, right? Which one is it in the
in-process case?

If it's PluginGlobals, then all of our ResourceVars will be PluginResourceVars;
if HostGlobals, then they will all be HostResourceVars. This seems as though it
will be quite broken. I don't know if fixing this will be as simple as calling
Dispatcher::IsPlugin instead of IsPluginGlobals. In fact, I suspect those two
will agree with one another.

Is there a way we can simply disable this functionality in the in-process case?
Otherwise, I may need to fix a lot of bugs for the in-process case before
committing this one.

[dmichael (off chromium), 2013/10/17 17:36:11]

The Var proxies aren't "refactored" to use the in-process router. They're the
old style. The in-process case doesn't use ppapi/proxy code, so I believe this
code will only be used for an out-of-process plugin. So I think
IsPluginGlobals() is safe here.

[Matt Giuca, 2013/10/18 00:00:53]

So (I'm obviously going to have to run the in-process one myself before I'm sure
about this), do you think that the resource vars created in the in-process mode
will be host resource vars or plugin resource vars? I'm not sure what the
implications are of either (since the plugin-side stuff was written assuming
they would be PluginResourceVars, and the host-side stuff was written assuming
they would be HostResourceVars).

[Matt Giuca, 2013/10/18 07:09:30]

OK I just wrote browser tests with the help of Raymes. He suggested that I write
a test so I can make sure it fails in the in-process case.

Currently, it DOES NOT fail. I'll have a closer look at this on Monday.

Sorry this CL is turning in to a bit of a mess :)

[Matt Giuca, 2013/10/21 05:20:03]

Actually, ignore the previous comment. The in-process tests DO fail (if not
disabled), but because the asynchronous nature of them causes the in-process
case to hang forever. (It's the same for Array and Dictionary.) So we cannot
adequately test the in-process case.

Raymes suggested that the ResourceVar will not be used in an in-process case
(since you can't use NaCl modules in-process), and therefore we do not need to
worry about this.

+    if (!pp_resource_) {
+      // If this check fails, then it is a null resource (not pending).
+      DCHECK(creation_message_);
+      ResourceDispatcher dispatcher(instance, pending_renderer_host_id_,
+                                    pending_browser_host_id_, &pp_resource_);
+      dispatcher.ExecuteCreationMessage(*creation_message_);
+      DCHECK(pp_resource_);
+    }
+  } else {
+    DCHECK(pp_resource_);
+  }

[raymes, 2013/10/11 04:02:24]

Have you considered whether all these DCHECKs should be hard failures? Should we
expect to trust the values of all those variables?

[Matt Giuca, 2013/10/17 09:17:58]

I just realised that the recent changes we made to allow pp_resource 0 to be
passed between will mean that we cannot assume pp_resource is nonzero on the
host side, or take pp_resource 0 to mean "pending" on the plugin side. Will have
to change the way this works.

[Matt Giuca, 2013/10/21 05:20:03]

Done.

 
   return PpapiGlobals::Get()->GetVarTracker()->MakeResourcePPVar(pp_resource_);
 }