Do PPB_FileIO Query and Read in the plugin process.

ppapi/proxy/file_io_resource.cc

Index: ppapi/proxy/file_io_resource.cc
diff --git a/ppapi/proxy/file_io_resource.cc b/ppapi/proxy/file_io_resource.cc
index 7d98356994abc5765634ee5b997d5f10fbdd402d..2f6dc8a3042d5326660a18f8e8070457b84f9f06 100644
--- a/ppapi/proxy/file_io_resource.cc
+++ b/ppapi/proxy/file_io_resource.cc
@@ -5,11 +5,15 @@
 #include "ppapi/proxy/file_io_resource.h"
 
 #include "base/bind.h"
+#include "base/files/file_util_proxy.h"
 #include "ipc/ipc_message.h"
 #include "ppapi/c/pp_errors.h"
+#include "ppapi/proxy/plugin_globals.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/shared_impl/array_writer.h"
+#include "ppapi/shared_impl/file_type_conversion.h"
 #include "ppapi/shared_impl/ppapi_globals.h"
+#include "ppapi/shared_impl/proxy_lock.h"
 #include "ppapi/shared_impl/resource_tracker.h"
 #include "ppapi/thunk/enter.h"
 #include "ppapi/thunk/ppb_file_ref_api.h"
@@ -18,6 +22,9 @@ using ppapi::thunk::EnterResourceNoLock;
 using ppapi::thunk::PPB_FileIO_API;
 using ppapi::thunk::PPB_FileRef_API;
 
+namespace ppapi {
+namespace proxy {
+
 namespace {
 
 // An adapter to let Read() share the same implementation with ReadToArray().
@@ -25,17 +32,50 @@ void* DummyGetDataBuffer(void* user_data, uint32_t count, uint32_t size) {
   return user_data;
 }
 
-}  // namespace
+// Dummy close callback allows us to call CloseFileHandle in the destructor.
+void DummyCloseCallback(base::PlatformFileError error_code) {
+}
 
-namespace ppapi {
-namespace proxy {
+// We can't directly pass a reference to the resource to our callbacks because
+// when these hold the last reference, the resource destructor will run when we
+// are not holding the proxy lock.
+// Instead, pass a scoped_ptr to a scoped_refptr. This allows us to destroy
+// the resource while we hold the proxy lock.
+typedef scoped_ptr<scoped_refptr<FileIOResource> > PassedRef;

[teravest, 2013/07/18 19:00:36]

This is funky, but I couldn't quickly think of a better way.

[bbudge, 2013/07/18 21:28:16]

If you code search on 'scoped_ptr<scoped_refptr' you will find a few precedents.
It *is* funky though.

+
+void QueryCallback(PassedRef file_io_ref,
+                   scoped_refptr<TrackedCallback> callback,
+                   PP_FileInfo* output_info,
+                   base::PlatformFileError error_code,
+                   const base::PlatformFileInfo& file_info) {
+  ProxyAutoLock lock;
+  scoped_refptr<FileIOResource> file_io;
+  file_io.swap(*file_io_ref.get());
+  file_io->OnQueryComplete(callback, output_info, error_code, file_info);
+}
+
+void ReadCallback(PassedRef file_io_ref,
+                  scoped_refptr<TrackedCallback> callback,
+                  PP_ArrayOutput array_output,
+                  base::PlatformFileError error_code,
+                  const char* data, int bytes_read) {
+  ProxyAutoLock lock;
+  scoped_refptr<FileIOResource> file_io;
+  file_io.swap(*file_io_ref.get());
+  file_io->OnReadComplete(callback, array_output, error_code, data, bytes_read);
+}
+
+}  // namespace
 
 FileIOResource::FileIOResource(Connection connection, PP_Instance instance)
-    : PluginResource(connection, instance) {
+    : PluginResource(connection, instance),
+      file_handle_(PP_kInvalidFileHandle),
+      file_system_type_(PP_FILESYSTEMTYPE_INVALID) {
   SendCreate(RENDERER, PpapiHostMsg_FileIO_Create());
 }
 
 FileIOResource::~FileIOResource() {
+  CloseFileHandle();
 }
 
 PPB_FileIO_API* FileIOResource::AsPPB_FileIO_API() {
@@ -49,6 +89,15 @@ int32_t FileIOResource::Open(PP_Resource file_ref,
   if (enter.failed())
     return PP_ERROR_BADRESOURCE;
 
+  PPB_FileRef_API* file_ref_api = enter.object();
+  PP_FileSystemType type = file_ref_api->GetFileSystemType();
+  if (type != PP_FILESYSTEMTYPE_LOCALPERSISTENT &&
+      type != PP_FILESYSTEMTYPE_LOCALTEMPORARY &&
+      type != PP_FILESYSTEMTYPE_EXTERNAL &&
+      type != PP_FILESYSTEMTYPE_ISOLATED)
+    return PP_ERROR_FAILED;

[dmichael (off chromium), 2013/07/18 20:28:09]

^^^ Seems like you could add NOTREACHED() before the return

[bbudge, 2013/07/18 21:28:16]

Done.

+  file_system_type_ = type;
+
   int32_t rv = state_manager_.CheckOperationState(
       FileIOStateManager::OPERATION_EXCLUSIVE, false);
   if (rv != PP_OK)
@@ -72,11 +121,17 @@ int32_t FileIOResource::Query(PP_FileInfo* info,
   if (rv != PP_OK)
     return rv;
 
-  Call<PpapiPluginMsg_FileIO_QueryReply>(RENDERER,
-      PpapiHostMsg_FileIO_Query(),
-      base::Bind(&FileIOResource::OnPluginMsgQueryComplete, this,
-                 callback, info));
+  if (file_handle_ == base::kInvalidPlatformFileValue)
+    return PP_ERROR_FAILED;
 
+  PassedRef passed_ref(new scoped_refptr<FileIOResource>(this));
+  if (!base::FileUtilProxy::GetFileInfoFromPlatformFile(
+          PpapiGlobals::Get()->GetFileTaskRunner(pp_instance()),
+          file_handle_,
+          base::Bind(&QueryCallback, base::Passed(&passed_ref),

[dmichael (off chromium), 2013/07/18 20:28:09]

Could you just use RunWhileLocked?
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...

Then I don't think you need QueryCallback, and I also think you could make
DoQueryCallback private (same comment for the others).

[bbudge, 2013/07/18 21:28:16]

I tried that but couldn't get it to compile. The problem is that these are
callbacks, and FileUtilProxy adds a few parameters to them, so I can't create a
Closure. Ideas?

If I can't use RunWhileLocked, I could move some definitions into the header and
make the methods private, by declaring these as friend functions. Do you think
it's worth it?

[dmichael (off chromium), 2013/07/18 23:21:48]

Oh, right, sorry. I started poking this on a local branch to come up with a
general solution. In the mean time, maybe you can pass a bound callback to
QueryCallback instead of the PassedRef. That would allow you to make
OnQueryCompleted private, and would eliminate having to use
scoped_ptr<scoped_ref_ptr.

I.e., QueryCallback would look something like this:
void QueryCallback(
    const base::Callback<void(base::PlatformFileError,
                              base::PlatformFileInfo)>& cb,
    base::PlatformFileError error_code,
    const base::PlatformFileInfo& file_info) {
  ProxyAutoLock lock;
  cb->Run(error_code, file_info);
}

And you have two Binds here, something like:
base::Bind(
    &QueryCallback,
    base::Bind(&FileIOResource::OnQueryCompleted,
               this,
               callback,
               info))

Does that make sense?

[bbudge, 2013/07/19 01:16:56]

This does compile, but unfortunately the outermost callback owns the inner one,
which holds a ref to the FileIOResource, so the FileIOResource destructor still
runs outside of the proxy lock when the callback holds the last ref. The problem
is that base::Callback holds onto its arguments for its lifetime, except for
scoped_ptr's. When I run the FileIO tests, the abort tests still fail when they
hit the locked assert.

Am I missing something?

+                     callback, info))) {
+    return PP_ERROR_FAILED;
+  }
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
@@ -110,7 +165,6 @@ int32_t FileIOResource::Read(int64_t offset,
   PP_ArrayOutput output_adapter;
   output_adapter.GetDataBuffer = &DummyGetDataBuffer;
   output_adapter.user_data = buffer;
-  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_READ);
   return ReadValidated(offset, bytes_to_read, output_adapter, callback);
 }
 
@@ -124,7 +178,6 @@ int32_t FileIOResource::ReadToArray(int64_t offset,
   if (rv != PP_OK)
     return rv;
 
-  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_READ);
   return ReadValidated(offset, max_read_length, *array_output, callback);
 }
 
@@ -181,6 +234,7 @@ int32_t FileIOResource::Flush(scoped_refptr<TrackedCallback> callback) {
 }
 
 void FileIOResource::Close() {
+  CloseFileHandle();
   Post(RENDERER, PpapiHostMsg_FileIO_Close());
 }
 
@@ -192,6 +246,23 @@ int32_t FileIOResource::GetOSFileDescriptor() {
   return file_descriptor;
 }
 
+int32_t FileIOResource::RequestOSFileHandle(
+    PP_FileHandle* handle,
+    scoped_refptr<TrackedCallback> callback) {
+  int32_t rv = state_manager_.CheckOperationState(
+      FileIOStateManager::OPERATION_EXCLUSIVE, true);
+  if (rv != PP_OK)
+    return rv;
+
+  Call<PpapiPluginMsg_FileIO_RequestOSFileHandleReply>(RENDERER,
+      PpapiHostMsg_FileIO_RequestOSFileHandle(),
+      base::Bind(&FileIOResource::OnPluginMsgRequestOSFileHandleComplete, this,
+                 callback, handle));
+
+  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
+  return PP_OK_COMPLETIONPENDING;
+}
+
 int32_t FileIOResource::WillWrite(int64_t offset,
                                   int32_t bytes_to_write,
                                   scoped_refptr<TrackedCallback> callback) {
@@ -217,90 +288,116 @@ int32_t FileIOResource::ReadValidated(int64_t offset,
                                       int32_t bytes_to_read,
                                       const PP_ArrayOutput& array_output,
                                       scoped_refptr<TrackedCallback> callback) {
-  Call<PpapiPluginMsg_FileIO_ReadReply>(RENDERER,
-      PpapiHostMsg_FileIO_Read(offset, bytes_to_read),
-      base::Bind(&FileIOResource::OnPluginMsgReadComplete, this,
-                 callback, array_output));
+  if (file_handle_ == base::kInvalidPlatformFileValue)
+    return PP_ERROR_FAILED;
+
+  PassedRef passed_ref(new scoped_refptr<FileIOResource>(this));
+  if (!base::FileUtilProxy::Read(
+          PpapiGlobals::Get()->GetFileTaskRunner(pp_instance()),
+          file_handle_,
+          offset,
+          bytes_to_read,
+          base::Bind(&ReadCallback, base::Passed(&passed_ref),
+                     callback, array_output))) {
+    return PP_ERROR_FAILED;
+  }
+
+  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_READ);
   return PP_OK_COMPLETIONPENDING;
 }
 
-int32_t FileIOResource::RequestOSFileHandle(
-    PP_FileHandle* handle,
-    scoped_refptr<TrackedCallback> callback) {
-  int32_t rv = state_manager_.CheckOperationState(
-      FileIOStateManager::OPERATION_EXCLUSIVE, true);
-  if (rv != PP_OK)
-    return rv;
-
-  Call<PpapiPluginMsg_FileIO_RequestOSFileHandleReply>(RENDERER,
-      PpapiHostMsg_FileIO_RequestOSFileHandle(),
-      base::Bind(&FileIOResource::OnPluginMsgRequestOSFileHandleComplete, this,
-                 callback, handle));
-
-  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
-  return PP_OK_COMPLETIONPENDING;
+void FileIOResource::CloseFileHandle() {
+  if (file_handle_ != base::kInvalidPlatformFileValue) {
+    base::FileUtilProxy::Close(
+        PpapiGlobals::Get()->GetFileTaskRunner(pp_instance()),
+        file_handle_,
+        base::Bind(&DummyCloseCallback));
+    file_handle_ = base::kInvalidPlatformFileValue;
+  }
 }
 
-void FileIOResource::OnPluginMsgGeneralComplete(
+void FileIOResource::OnQueryComplete(
     scoped_refptr<TrackedCallback> callback,
-    const ResourceMessageReplyParams& params) {
+    PP_FileInfo* output_info,
+    base::PlatformFileError error_code,
+    const base::PlatformFileInfo& file_info) {
   DCHECK(state_manager_.get_pending_operation() ==
-         FileIOStateManager::OPERATION_EXCLUSIVE ||
-         state_manager_.get_pending_operation() ==
-         FileIOStateManager::OPERATION_WRITE);
+         FileIOStateManager::OPERATION_EXCLUSIVE);
+
+  if (!TrackedCallback::IsPending(callback)) {
+    state_manager_.SetOperationFinished();
+    return;
+  }
+
+  int32_t result = ::ppapi::PlatformFileErrorToPepperError(error_code);
+  if (result == PP_OK) {
+    ppapi::PlatformFileInfoToPepperFileInfo(file_info, file_system_type_,
+                                            output_info);
+  }
+
   // End the operation now. The callback may perform another file operation.
   state_manager_.SetOperationFinished();
-  callback->Run(params.result());
+  callback->Run(result);
 }
 
-void FileIOResource::OnPluginMsgOpenFileComplete(
+void FileIOResource::OnReadComplete(
     scoped_refptr<TrackedCallback> callback,
-    const ResourceMessageReplyParams& params) {
+    PP_ArrayOutput array_output,
+    base::PlatformFileError error_code,
+    const char* data, int bytes_read) {
   DCHECK(state_manager_.get_pending_operation() ==
-         FileIOStateManager::OPERATION_EXCLUSIVE);
-  if (params.result() == PP_OK)
-    state_manager_.SetOpenSucceed();
+         FileIOStateManager::OPERATION_READ);
+
+  if (!TrackedCallback::IsPending(callback)) {
+    state_manager_.SetOperationFinished();
+    return;
+  }
+
+  int32_t result = ::ppapi::PlatformFileErrorToPepperError(error_code);
+  if (result == PP_OK) {
+    result = std::max(0, bytes_read);
+    ArrayWriter output;
+    output.set_pp_array_output(array_output);
+    if (output.is_valid())
+      output.StoreArray(data, result);
+    else
+      result = PP_ERROR_FAILED;
+  }
+
   // End the operation now. The callback may perform another file operation.

[dmichael (off chromium), 2013/07/18 20:28:09]

This comment and similar ones aren't quite right. state_manager_ actually has a
count of operations. There might still be a read pending, so the pending
operation count is still non-zero, so the state manager still will not allow a
write (for example).

[bbudge, 2013/07/18 21:28:16]

I'm not sure I understand but I modified the comments to try to be a little
clearer.

[dmichael (off chromium), 2013/07/18 23:21:48]

Reads can be done concurrently with other reads, but not with any other kinds of
operations. Similarly, writes can be done concurrently with other writes.

So if the plugin has 3 Reads pending, when the first one returns, the plugin
still is not allowed to do any operations other than Read. It just decrements
the count of pending operations (in this case, the pending operations are reads)
to 2. But the state_manager_ won't allow other kinds of operations unless the
count goes to zero.

Does that help? Your new comment looks fine.

   state_manager_.SetOperationFinished();
-  callback->Run(params.result());
+  callback->Run(result);
 }
 
-void FileIOResource::OnPluginMsgQueryComplete(
+void FileIOResource::OnPluginMsgGeneralComplete(
     scoped_refptr<TrackedCallback> callback,
-    PP_FileInfo* output_info,
-    const ResourceMessageReplyParams& params,
-    const PP_FileInfo& info) {
+    const ResourceMessageReplyParams& params) {
   DCHECK(state_manager_.get_pending_operation() ==
-         FileIOStateManager::OPERATION_EXCLUSIVE);
-  *output_info = info;
+         FileIOStateManager::OPERATION_EXCLUSIVE ||
+         state_manager_.get_pending_operation() ==
+         FileIOStateManager::OPERATION_WRITE);
   // End the operation now. The callback may perform another file operation.
   state_manager_.SetOperationFinished();
   callback->Run(params.result());
 }
 
-void FileIOResource::OnPluginMsgReadComplete(
+void FileIOResource::OnPluginMsgOpenFileComplete(
     scoped_refptr<TrackedCallback> callback,
-    PP_ArrayOutput array_output,
-    const ResourceMessageReplyParams& params,
-    const std::string& data) {
+    const ResourceMessageReplyParams& params) {
   DCHECK(state_manager_.get_pending_operation() ==
-         FileIOStateManager::OPERATION_READ);
+         FileIOStateManager::OPERATION_EXCLUSIVE);
+  if (params.result() == PP_OK)
+    state_manager_.SetOpenSucceed();
 
-  // The result code should contain the data size if it's positive.
   int32_t result = params.result();
-  DCHECK((result < 0 && data.size() == 0) ||
-         result == static_cast<int32_t>(data.size()));
-
-  ArrayWriter output;
-  output.set_pp_array_output(array_output);
-  if (output.is_valid())
-    output.StoreArray(data.data(), std::max(0, result));
-  else
+  IPC::PlatformFileForTransit transit_file;
+  if (result == PP_OK && !params.TakeFileHandleAtIndex(0, &transit_file))
     result = PP_ERROR_FAILED;
+  file_handle_ = IPC::PlatformFileForTransitToPlatformFile(transit_file);
 
   // End the operation now. The callback may perform another file operation.
   state_manager_.SetOperationFinished();
-  callback->Run(result);
+  callback->Run(params.result());
 }
 
 void FileIOResource::OnPluginMsgRequestOSFileHandleComplete(