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..5ea56ca805dcbd8ff762a573ebbcf91b63acdc75 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"
@@ -25,17 +29,57 @@ void* DummyGetDataBuffer(void* user_data, uint32_t count, uint32_t size) {
   return user_data;
 }
 
+// All potentially blocking file tasks should be done on the file thread. In
+// most cases, we can use base::FileUtilProxy to do the task and callback to the
+// original thread. This won't work for blocking PPAPI callbacks, since we
+// block the calling thread, which will cause FileUtilProxy to hang. Instead,

[dmichael (off chromium), 2013/08/02 15:44:29]

how about instead of "which will cause FileUtilProxy to hang"
...
"and so it can not receive the callback from FileUtilProxy."
or something (just to add more detail to why there's a hang)

[bbudge, 2013/08/02 20:56:44]

n/a now that we've eliminated FileUtilProxy.

+// do the same work in these tasks, which correctly wake the original thread.
+// However, we can't use these tasks for non-blocking callbacks because the
+// in-process proxy has no lock, which would lead to unsafe access to the
+// resource by both threads.
+
+void DoQuery(base::PlatformFile file,
+             PP_FileInfo* info,
+             const base::FileUtilProxy::GetFileInfoCallback& callback) {
+  base::PlatformFileError error = base::PLATFORM_FILE_OK;
+  base::PlatformFileInfo file_info;
+  if (!GetPlatformFileInfo(file, &file_info))
+    error = base::PLATFORM_FILE_ERROR_FAILED;
+
+  callback.Run(error, file_info);
+}
+
+void DoRead(base::PlatformFile file,
+            int64 offset,
+            int bytes_to_read,
+            const base::FileUtilProxy::ReadCallback& callback) {
+  scoped_ptr<char[]> buffer(new char[bytes_to_read]);

[dmichael (off chromium), 2013/08/02 15:44:29]

We shouldn't have to make a copy for the "Read" case. Maybe you can pass the
ArrayOutput here and do the read directly in to it?

In fact...  I hadn't realized that FileUtilProxy forces us to make a copy! We
probably shouldn't be using FileUtilProxy at all :-(. I think you can either:
1) Pass is_blocking along to DoRead, and if it's non-blocking, PostTask to the
main thread.
or...
2) Don't bother with is_blocking(). Check in here if ProxyLock is NULL. If so,
you know you need to PostTask to the main thread.

I think I like 2 better than 1. I think it's clearer, and gives you a good place
to stick a "TODO" to get rid of the special case. Maybe more importantly, if we
do (1), for any non-blocking callback that is *not* coming from the main thread,
we'd be bouncing to the main thread only to then bounce it to the real thread.

[bbudge, 2013/08/02 20:56:44]

Looks like we can't avoid the copy due to Abort semantics. Either we hold the
proxy lock for the entire file operation, or we copy.

I did eliminate FileUtilProxy, so will remove it from the IRT build in another
CL.

Note: we're no longer using is_blocking(), so I'll revert it too.

+  int bytes_read =
+      base::ReadPlatformFile(file, offset, buffer.get(), bytes_to_read);
+  base::PlatformFileError error = (bytes_read < 0) ?
+      base::PLATFORM_FILE_ERROR_FAILED : base::PLATFORM_FILE_OK;
+
+  callback.Run(error, buffer.get(), bytes_read);
+}
+
+void DoClose(base::PlatformFile file) {
+  base::ClosePlatformFile(file);
+}
+
 }  // namespace
 
 namespace ppapi {
 namespace proxy {
 
 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 +93,17 @@ 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) {
+    NOTREACHED();
+    return PP_ERROR_FAILED;
+  }
+  file_system_type_ = type;
+
   int32_t rv = state_manager_.CheckOperationState(
       FileIOStateManager::OPERATION_EXCLUSIVE, false);
   if (rv != PP_OK)
@@ -72,10 +127,24 @@ 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;
+
+  base::TaskRunner* file_task_runner =
+      PpapiGlobals::Get()->GetFileTaskRunner(pp_instance());
+  base::FileUtilProxy::GetFileInfoCallback file_task = RunWhileLocked(
+      base::Bind(&FileIOResource::OnQueryComplete, this, callback, info));
+  if (callback->is_blocking()) {
+    file_task_runner->PostTask(
+        FROM_HERE,
+        Bind(&DoQuery, file_handle_, info, file_task));
+  } else {
+    if (!base::FileUtilProxy::GetFileInfoFromPlatformFile(file_task_runner,
+                                                          file_handle_,
+                                                          file_task)) {
+      return PP_ERROR_FAILED;
+    }
+  }
 
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
@@ -110,7 +179,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 +192,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 +248,7 @@ int32_t FileIOResource::Flush(scoped_refptr<TrackedCallback> callback) {
 }
 
 void FileIOResource::Close() {
+  CloseFileHandle();
   Post(RENDERER, PpapiHostMsg_FileIO_Close());
 }
 
@@ -192,6 +260,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) {
@@ -199,6 +284,7 @@ int32_t FileIOResource::WillWrite(int64_t offset,
       PpapiHostMsg_FileIO_WillWrite(offset, bytes_to_write),
       base::Bind(&FileIOResource::OnPluginMsgGeneralComplete, this,
                  callback));
+
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
@@ -209,6 +295,7 @@ int32_t FileIOResource::WillSetLength(int64_t length,
       PpapiHostMsg_FileIO_WillSetLength(length),
       base::Bind(&FileIOResource::OnPluginMsgGeneralComplete, this,
                  callback));
+
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
@@ -217,28 +304,98 @@ 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;
+  if (bytes_to_read < 0)
+    return PP_ERROR_FAILED;
+
+  base::TaskRunner* file_task_runner =
+      PpapiGlobals::Get()->GetFileTaskRunner(pp_instance());
+  base::FileUtilProxy::ReadCallback file_task = RunWhileLocked(
+      base::Bind(&FileIOResource::OnReadComplete, this, callback,
+                 array_output));
+  if (callback->is_blocking()) {
+    file_task_runner->PostTask(
+        FROM_HERE,
+        Bind(&DoRead, file_handle_, offset, bytes_to_read, file_task));
+  } else {
+    if (!base::FileUtilProxy::Read(file_task_runner,
+                                   file_handle_,
+                                   offset,
+                                   bytes_to_read,
+                                   file_task)) {
+      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;
+void FileIOResource::CloseFileHandle() {
+  if (file_handle_ != base::kInvalidPlatformFileValue) {
+    base::TaskRunner* file_task_runner =
+        PpapiGlobals::Get()->GetFileTaskRunner(pp_instance());
+    file_task_runner->PostTask(FROM_HERE,
+                               base::Bind(&DoClose, file_handle_));
 
-  Call<PpapiPluginMsg_FileIO_RequestOSFileHandleReply>(RENDERER,
-      PpapiHostMsg_FileIO_RequestOSFileHandle(),
-      base::Bind(&FileIOResource::OnPluginMsgRequestOSFileHandleComplete, this,
-                 callback, handle));
+    file_handle_ = base::kInvalidPlatformFileValue;
+  }
+}
 
-  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
-  return PP_OK_COMPLETIONPENDING;
+void FileIOResource::OnQueryComplete(
+    scoped_refptr<TrackedCallback> callback,
+    PP_FileInfo* output_info,
+    base::PlatformFileError error_code,
+    const base::PlatformFileInfo& file_info) {
+  DCHECK(state_manager_.get_pending_operation() ==
+         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 this operation now, so the user's callback can execute another FileIO
+  // operation, assuming there are no other pending operations.
+  state_manager_.SetOperationFinished();
+  callback->Run(result);
+}
+
+void FileIOResource::OnReadComplete(
+    scoped_refptr<TrackedCallback> callback,
+    PP_ArrayOutput array_output,
+    base::PlatformFileError error_code,
+    const char* data, int bytes_read) {
+  DCHECK(state_manager_.get_pending_operation() ==
+         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 this operation now, so the user's callback can execute another FileIO
+  // operation, assuming there are no other pending operations.
+  state_manager_.SetOperationFinished();
+  callback->Run(result);
 }
 
 void FileIOResource::OnPluginMsgGeneralComplete(
@@ -248,7 +405,8 @@ void FileIOResource::OnPluginMsgGeneralComplete(
          FileIOStateManager::OPERATION_EXCLUSIVE ||
          state_manager_.get_pending_operation() ==
          FileIOStateManager::OPERATION_WRITE);
-  // End the operation now. The callback may perform another file operation.
+  // End this operation now, so the user's callback can execute another FileIO
+  // operation, assuming there are no other pending operations.
   state_manager_.SetOperationFinished();
   callback->Run(params.result());
 }
@@ -260,47 +418,17 @@ void FileIOResource::OnPluginMsgOpenFileComplete(
          FileIOStateManager::OPERATION_EXCLUSIVE);
   if (params.result() == PP_OK)
     state_manager_.SetOpenSucceed();
-  // End the operation now. The callback may perform another file operation.
-  state_manager_.SetOperationFinished();
-  callback->Run(params.result());
-}
-
-void FileIOResource::OnPluginMsgQueryComplete(
-    scoped_refptr<TrackedCallback> callback,
-    PP_FileInfo* output_info,
-    const ResourceMessageReplyParams& params,
-    const PP_FileInfo& info) {
-  DCHECK(state_manager_.get_pending_operation() ==
-         FileIOStateManager::OPERATION_EXCLUSIVE);
-  *output_info = info;
-  // End the operation now. The callback may perform another file operation.
-  state_manager_.SetOperationFinished();
-  callback->Run(params.result());
-}
-
-void FileIOResource::OnPluginMsgReadComplete(
-    scoped_refptr<TrackedCallback> callback,
-    PP_ArrayOutput array_output,
-    const ResourceMessageReplyParams& params,
-    const std::string& data) {
-  DCHECK(state_manager_.get_pending_operation() ==
-         FileIOStateManager::OPERATION_READ);
 
-  // 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.
+  // End this operation now, so the user's callback can execute another FileIO
+  // operation, assuming there are no other pending operations.
   state_manager_.SetOperationFinished();
-  callback->Run(result);
+  callback->Run(params.result());
 }
 
 void FileIOResource::OnPluginMsgRequestOSFileHandleComplete(
@@ -321,7 +449,8 @@ void FileIOResource::OnPluginMsgRequestOSFileHandleComplete(
     result = PP_ERROR_FAILED;
   *output_handle = IPC::PlatformFileForTransitToPlatformFile(transit_file);
 
-  // End the operation now. The callback may perform another file operation.
+  // End this operation now, so the user's callback can execute another FileIO
+  // operation, assuming there are no other pending operations.
   state_manager_.SetOperationFinished();
   callback->Run(result);
 }