[PPAPI] Fixed FileSystems from JavaScript not having a context.

content/browser/renderer_host/pepper/pepper_renderer_connection.cc

 // Copyright 2013 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 "content/browser/renderer_host/pepper/pepper_renderer_connection.h"
 
+#include "base/bind.h"
+#include "base/memory/ref_counted.h"
 #include "content/browser/browser_child_process_host_impl.h"
 #include "content/browser/ppapi_plugin_process_host.h"
 #include "content/browser/renderer_host/pepper/browser_ppapi_host_impl.h"
 #include "content/common/pepper_renderer_instance_data.h"
 #include "content/common/view_messages.h"
 #include "content/browser/renderer_host/pepper/pepper_file_ref_host.h"
 #include "content/browser/renderer_host/pepper/pepper_file_system_browser_host.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/content_client.h"
 #include "ipc/ipc_message_macros.h"
 #include "ppapi/host/resource_host.h"
 #include "ppapi/proxy/ppapi_message_utils.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/ppapi_message_utils.h"
 #include "ppapi/proxy/resource_message_params.h"
 
 namespace content {
 
+namespace {
+
+// Responsible for creating the pending resource hosts, holding their IDs until
+// all of them have been created for a single message, and sending the reply to
+// say that the hosts have been created.
+class PendingHostCreator
+    : public base::RefCountedThreadSafe<PendingHostCreator> {

[yzshen1, 2013/11/01 22:27:39]

ThreadSafe is not needed. This code won't be run on multiple thread. (right?)

[Matt Giuca, 2013/11/01 23:19:47]

It will be passed to another thread, then passed back and run on this thread. I
wasn't sure if that required ThreadSafe so I put it in.

Or is it only required if it's run simultaneously on two threads? You're right,
it's probably not necessary so I'll take out.

[yzshen1, 2013/11/02 00:25:07]

If the ref needs to be manipulated by multiple threads, we will need
RefCountedThreadSafe. In this case, I think it is safe to use RefCounted -- we
bind it to a Closure which is eventually run on the same thread.

But I am fine if you want to keep RefCountedThreadSafe.

[Matt Giuca, 2013/11/02 00:51:45]

I agree. RefCounted seems fine.

+ public:
+  PendingHostCreator(int routing_id,
+                     int sequence_id,
+                     BrowserPpapiHostImpl* host,
+                     int nested_msgs_size,

[yzshen1, 2013/11/01 22:27:39]

size_t, please.

nit, optional: maybe it is better to reorder the parameters:
BrowserPpapiHostImpl* host,
BrowserMessageFilter* connection,
int routing_id,
int sequence_id,
size_t nested_msgs_size

[Matt Giuca, 2013/11/01 23:19:47]

Done. I agree.

+                     BrowserMessageFilter* connection);
+
+  // Adds the given resource host as a pending one. The host is remembered as
+  // host number |index|, and will ultimately be sent to the plugin to be
+  // attached to a real resource.
+  void AddPendingResourceHost(
+      size_t index,
+      scoped_ptr<ppapi::host::ResourceHost> resource_host);
+
+ private:
+  friend class base::RefCountedThreadSafe<PendingHostCreator>;
+
+  // When the last reference to this class is released, all of the resource
+  // hosts would have been added. This destructor sends the message to the
+  // plugin to tell it to attach real hosts to all of the pending hosts that
+  // have been added by this object.
+  ~PendingHostCreator();
+
+  int routing_id_;
+  int sequence_id_;
+  BrowserPpapiHostImpl* host_;
+  std::vector<int> pending_resource_host_ids_;
+  BrowserMessageFilter* connection_;
+};
+
+PendingHostCreator::PendingHostCreator(int routing_id,
+                                       int sequence_id,
+                                       BrowserPpapiHostImpl* host,
+                                       int nested_msgs_size,
+                                       BrowserMessageFilter* connection)
+    : routing_id_(routing_id),
+      sequence_id_(sequence_id),
+      host_(host),
+      pending_resource_host_ids_(nested_msgs_size, 0),
+      connection_(connection) {}
+
+void PendingHostCreator::AddPendingResourceHost(
+    size_t index,
+    scoped_ptr<ppapi::host::ResourceHost> resource_host) {
+  pending_resource_host_ids_[index] =
+      host_->GetPpapiHost()->AddPendingResourceHost(resource_host.Pass());
+}
+
+PendingHostCreator::~PendingHostCreator() {
+  connection_->Send(new PpapiHostMsg_CreateResourceHostsFromHostReply(

[yzshen1, 2013/11/01 22:27:39]

Is it possible that something has failed and we have 0 in the array, what would
happen in that case?

[Matt Giuca, 2013/11/01 23:19:47]

No more possible than it already was. (That is, I'm not sure if
PpapiHost::AddPendingResourceHost returns 0, but I haven't introduced any new
possibilities of putting 0 in the array.) The asynchronous function always calls
the callback.

[yzshen1, 2013/11/02 00:25:07]

I think it is possible: The FileSystem resource host may be destroyed before
OpenExisting() completes, in that case, OpenExistingWithContext() won't be
called, and PendingHostCreator::AddPendingResourceHost() won't be called either.
In the destructor, we will send 0 to the renderer.

[Matt Giuca, 2013/11/02 01:05:21]

Had a chat with Yuzhu. I agree, there is a very rare chance that this will
happen if the plugin dies while the host is being created. In my quick testing
of this, passing 0 as the pending host ID may crash the plugin, but if the
plugin is dead this shouldn't matter.

We decided this was not worth holding up the fix. Filed as
http://crbug.com/314289.

+      routing_id_, sequence_id_, pending_resource_host_ids_));
+}
+
+}  // namespace
+
 PepperRendererConnection::PepperRendererConnection(int render_process_id)
     : render_process_id_(render_process_id) {
   // Only give the renderer permission for stable APIs.
   in_process_host_.reset(new BrowserPpapiHostImpl(this,
                                                   ppapi::PpapiPermissions(),
                                                   "",
                                                   base::FilePath(),
                                                   base::FilePath(),
                                                   true /* in_process */,
                                                   false /* external_plugin */));
@@ skipping 51 matching lines @@
   return handled;
 }
 
 void PepperRendererConnection::OnMsgCreateResourceHostsFromHost(
     int routing_id,
     int child_process_id,
     const ppapi::proxy::ResourceMessageCallParams& params,
     PP_Instance instance,
     const std::vector<IPC::Message>& nested_msgs) {
   BrowserPpapiHostImpl* host = GetHostForChildProcess(child_process_id);
+  scoped_refptr<PendingHostCreator> creator =
+      new PendingHostCreator(
+          routing_id, params.sequence(), host, nested_msgs.size(), this);
 
-  std::vector<int> pending_resource_host_ids(nested_msgs.size(), 0);
   if (!host) {
     DLOG(ERROR) << "Invalid plugin process ID.";
   } else {
     for (size_t i = 0; i < nested_msgs.size(); ++i) {
       const IPC::Message& nested_msg = nested_msgs[i];
       scoped_ptr<ppapi::host::ResourceHost> resource_host;
       if (host->IsValidInstance(instance)) {
         if (nested_msg.type() == PpapiHostMsg_FileRef_CreateExternal::ID) {
           // FileRef_CreateExternal is only permitted from the renderer. Because
           // of this, we handle this message here and not in
           // content_browser_pepper_host_factory.cc.
           base::FilePath external_path;
           if (ppapi::UnpackMessage<PpapiHostMsg_FileRef_CreateExternal>(
                   nested_msg, &external_path)) {
             resource_host.reset(new PepperFileRefHost(
                 host, instance, params.pp_resource(), external_path));
           }
         } else if (nested_msg.type() ==
                    PpapiHostMsg_FileSystem_CreateFromRenderer::ID) {
           // Similarly, FileSystem_CreateFromRenderer is only permitted from the
           // renderer.
           std::string root_url;
           PP_FileSystemType file_system_type;
           if (ppapi::UnpackMessage<PpapiHostMsg_FileSystem_CreateFromRenderer>(
                   nested_msg, &root_url, &file_system_type)) {
-            resource_host.reset(
+            PepperFileSystemBrowserHost* browser_host =
                 new PepperFileSystemBrowserHost(host,
                                                 instance,
                                                 params.pp_resource(),
-                                                GURL(root_url),
-                                                file_system_type));
+                                                file_system_type);
+            resource_host.reset(browser_host);
+            // Open the file system resource host. This is an asynchronous
+            // operation, and we must only add the pending resource host and
+            // send the message once it completes.
+            browser_host->OpenExisting(
+                GURL(root_url),
+                base::Bind(
+                    &PendingHostCreator::AddPendingResourceHost,

[dmichael (off chromium), 2013/11/01 17:33:52]

The idea of this being asynchronous is pretty scary to me. We probably have
sequences of operations where we expect that after the creation message has been
sent, the pending resource id is immediately usable. In this case, I think it
won't be if the FileSystem hasn't been opened yet.

I think instead that you need to create the pending resource host immediately,
and make *that host* responsible for queueing up any requests that it gets while
it's not fully initialized. Once it's fully initialized, *then* it can service
those requests.

[teravest, 2013/11/01 19:57:24]

Dave's right-- you should definitely call AddPendingResourceHost immediately
instead of asynchronously.

However, I think you're making this more complicated than it has to be by doing
all this work at resource creation time.

Can you require that FileSystems received by plugins from the renderer also need
a call to Open() to work? Then you can do this asynchronous work there and send
an OpenReply message when you're done.

The alternative will require a lot of work on the host side to make accessors
for GetFileSystemContext asynchronous everywhere. I can help you with this, but
it would be messy.

[yzshen1, 2013/11/01 21:23:12]

(Not saying that it is incorrect, just try to understand.)
Why do we have to call AddPendingResourceHost immediately instead of
asynchronously?
I thought the renderer side won't start using the file system before it gets the
CreateResourceHostsFromHostReply?

And before it is attached, nobody will call the GetFileSystemConext on the
resource host, right?

[Matt Giuca, 2013/11/01 21:41:54]

Hey,

I'm not sure I understand how this is a problem. My understanding was that
OnMsgCreateResourceHostsFromHost is itself called asynchronously -- it is called
in response to a message from the renderer. Therefore, the renderer cannot
assume that anything done in this function is available immediately; it has to
wait until the "Reply" is sent by calling Send.

The refactoring I did in this CL was to ensure that Send is not called until
after all of the callbacks are called.

The (slight) alternative I thought of, which is a bit simpler, was to call
PpapiHost::AddPendingResourceHost synchronously, and add it to the vector, and
just have the callback do nothing, but delay until the final callback is called
before calling Send. But I thought, since the resource host will be unusable
anyway, and its ID will be unavailable before the async operation is completed,
it is probably safer to only add it as a pending resource host once we get a
callback.

Either way, I don't see the problem since nobody can see these resource IDs
before they are added as pending hosts later.

I discussed the option of having the user call Open() briefly with Raymes. I
don't really like it, since it is awkward for the user (conceptually: you have
to "open" the file system in JavaScript, then pass it to NaCl, then "open" it
again). It seems like leaking implementation details to the user if we make the
user pay with extra boilerplate steps just because we can't figure out how to do
it for them before we send it to them.

[dmichael (off chromium), 2013/11/01 21:44:26]

Yeah, I realize now that the sender of CreateFromRenderer doesn't get to see the
pending id, so there's no chance of it messing that up. I think it's okay; sorry
about that.

+                    creator,
+                    i,
+                    base::Passed(&resource_host)));
+            // Do not fall through; the fall-through case adds the pending
+            // resource host to the list. We must do this asynchronously.
+            continue;

[dmichael (off chromium), 2013/11/01 17:33:52]

I think it might be better to structure this differently so you don't need to
continue.

Maybe put "creator->AddPendingResourceHost" up in the FileRef case explicitly,
and take the part that does "host->GetPpapiHost()->CreateResourceHost" below in
an else.

[Matt Giuca, 2013/11/01 23:19:47]

That's messy. The case currently at the bottom is supposed to be run if any of
the if statements fail. Avoiding the continue requires this logic:

if (valid instance) {
  if (message is fileref_createexternal) {
    if (message is valid) {
      special case for resource_host
      default synchronous add
    } else {
      // Not sure why an invalid message continues like this, but this
      // is to preserve the existing behaviour.
      default case for resource_host
      default synchronous add
    }
  } else if (message is filesystem_createfromrenderer) {
      special case for resource_host
      special asynchronous add
  } else {
    default case for resource_host
    default synchronous add
  }
} else {
  default case for resource_host
  default synchronous add
}

There are so many default cases and more will be needed as we add more special
cases here. I think the continue is preferable.

Instead, I tried moving all of the loop body to a separate function. The
continue is now an early return. Do you prefer this? TBH, I think this makes it
harder to read because the logic is separated out. So I've uploaded it as a
separate Patch Set 3. If you don't like that, we can just go with Patch Set 2.

           }
         }
       }
 
       if (!resource_host.get()) {
         resource_host = host->GetPpapiHost()->CreateResourceHost(
             params, instance, nested_msg);
       }
 
-      if (resource_host.get()) {
-        pending_resource_host_ids[i] =
-            host->GetPpapiHost()->AddPendingResourceHost(resource_host.Pass());
-      }
+      if (resource_host.get())
+        creator->AddPendingResourceHost(i, resource_host.Pass());
     }
   }
 
-  Send(new PpapiHostMsg_CreateResourceHostsFromHostReply(
-       routing_id, params.sequence(), pending_resource_host_ids));
+  // Note: All of the pending host IDs that were added as part of this
+  // operation will automatically be send to the plugin when |creator| is

[dmichael (off chromium), 2013/11/01 17:33:52]

nit: s/send/sent

[Matt Giuca, 2013/11/01 23:19:47]

Done.

+  // released. This may happen immediately, or (if there are asynchronous
+  // requests to create resource hosts), once all of them complete.
 }
 
 void PepperRendererConnection::OnMsgDidCreateInProcessInstance(
     PP_Instance instance,
     const PepperRendererInstanceData& instance_data) {
   PepperRendererInstanceData data = instance_data;
   data.render_process_id = render_process_id_;
   in_process_host_->AddInstance(instance, data);
 }
 
 void PepperRendererConnection::OnMsgDidDeleteInProcessInstance(
     PP_Instance instance) {
   in_process_host_->DeleteInstance(instance);
 }
 
 }  // namespace content