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 1df67ccba1be125a9b47b902a1a9e08564ed86a1..8350a871bd00b7a1377455fa5a231a434a2ddb90 100644
--- a/ppapi/proxy/file_io_resource.cc
+++ b/ppapi/proxy/file_io_resource.cc
@@ -22,6 +22,11 @@ using ppapi::thunk::PPB_FileRef_API;
 
 namespace {
 
+// We must allocate a buffer sized according to the request of the plugin. To
+// reduce the chance of out-of-memory errors, we cap the read size to 32MB.
+// This is OK since the API specifies that it may perform a partial read.
+static const int32_t kMaxReadSize = 32 * 1024 * 1024;  // 32MB
+
 // An adapter to let Read() share the same implementation with ReadToArray().
 void* DummyGetDataBuffer(void* user_data, uint32_t count, uint32_t size) {
   return user_data;
@@ -37,10 +42,19 @@ void DoClose(base::PlatformFile file) {
 namespace ppapi {
 namespace proxy {
 
+FileIOResource::ReadData::ReadData(int32_t bytes_to_read)
+  : buffer_(new char[bytes_to_read]),
+    bytes_read_(0) {
+}
+
+FileIOResource::ReadData::~ReadData() {
+}
+
 FileIOResource::FileIOResource(Connection connection, PP_Instance instance)
     : PluginResource(connection, instance),
       file_handle_(base::kInvalidPlatformFileValue),
-      file_system_type_(PP_FILESYSTEMTYPE_INVALID) {
+      file_system_type_(PP_FILESYSTEMTYPE_INVALID),
+      query_result_(0) {
   SendCreate(RENDERER, PpapiHostMsg_FileIO_Create());
 }
 
@@ -92,33 +106,32 @@ int32_t FileIOResource::Query(PP_FileInfo* info,
       FileIOStateManager::OPERATION_EXCLUSIVE, true);
   if (rv != PP_OK)
     return rv;
+  if (!info)
+    return PP_ERROR_BADARGUMENT;
+  if (file_handle_ == base::kInvalidPlatformFileValue)
+    return PP_ERROR_FAILED;
+
+  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
 
-  if (callback->is_blocking() &&
-      file_handle_ != base::kInvalidPlatformFileValue) {
-    state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
-    int32_t result = PP_ERROR_FAILED;
-    base::PlatformFileInfo file_info;
-    // Release the proxy lock while making a potentially blocking file call.
+  // If the callback is blocking, perform the task on the calling thread.
+  if (callback->is_blocking()) {
     {
+      // Release the proxy lock while making a potentially slow file call.
       ProxyAutoUnlock unlock;
-      result = base::GetPlatformFileInfo(file_handle_, &file_info) ?
-          PP_OK : PP_ERROR_FAILED;
+      DoQuery();
     }
-    if ((result == PP_OK) && TrackedCallback::IsPending(callback)) {
-      ppapi::PlatformFileInfoToPepperFileInfo(file_info, file_system_type_,
-                                              info);
-    }
-
-    state_manager_.SetOperationFinished();
-    return result;
+    return OnQueryComplete(info, PP_OK);

[dmichael (off chromium), 2013/08/09 17:37:42]

I think you could pass the result of the query to OnQueryComplete? Especially if
you can make DoQuery return the result. (see comments below about the Read
stuff...  this would be structured in an analogous way)

[bbudge, 2013/08/09 21:43:24]

Done.

   }
 
-  Call<PpapiPluginMsg_FileIO_QueryReply>(RENDERER,
-      PpapiHostMsg_FileIO_Query(),
-      base::Bind(&FileIOResource::OnPluginMsgQueryComplete, this,
-                 callback, info));
+  // For the non-blocking case, post a task to the file thread.
+  PpapiGlobals::Get()->GetFileTaskRunner(pp_instance())->PostTaskAndReply(
+      FROM_HERE,
+      Bind(&FileIOResource::DoQuery, this),
+      RunWhileLocked(
+          Bind(&FileIOResource::OnFileTaskComplete, this, callback)));
+  callback->set_completion_task(
+      Bind(&FileIOResource::OnQueryComplete, this, info));
 
-  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
 
@@ -274,41 +287,33 @@ int32_t FileIOResource::ReadValidated(int64_t offset,
                                       int32_t bytes_to_read,
                                       const PP_ArrayOutput& array_output,
                                       scoped_refptr<TrackedCallback> callback) {
+  if (bytes_to_read < 0)
+    return PP_ERROR_FAILED;
+  if (file_handle_ == base::kInvalidPlatformFileValue)
+    return PP_ERROR_FAILED;
+
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_READ);
 
-  if (callback->is_blocking() &&
-      file_handle_ != base::kInvalidPlatformFileValue) {
-    int32_t result = PP_ERROR_FAILED;
-    if (bytes_to_read >= 0) {
-      // We need a buffer (and therefore we must copy) since we don't know until
-      // reacquire the lock whether to write data to the plugin's buffer.
-      scoped_ptr<char[]> buffer;
-      // Release the proxy lock while making a potentially blocking file call.
-      {
-        ProxyAutoUnlock unlock;
-        buffer.reset(new char[bytes_to_read]);
-        int32_t bytes_read = base::ReadPlatformFile(
-            file_handle_, offset, buffer.get(), bytes_to_read);
-        result = (bytes_read < 0) ? PP_ERROR_FAILED : bytes_read;
-      }
-      if ((result >= 0) && TrackedCallback::IsPending(callback)) {
-        ArrayWriter output;
-        output.set_pp_array_output(array_output);
-        if (output.is_valid())
-          output.StoreArray(buffer.get(), result);
-        else
-          result = PP_ERROR_FAILED;
-      }
+  bytes_to_read = std::min(bytes_to_read, kMaxReadSize);
+  scoped_refptr<ReadData> read_data(new ReadData(bytes_to_read));

[dmichael (off chromium), 2013/08/09 17:37:42]

It would be better to create an empty ReadData here while the lock is held, and
let DoRead tell it to allocate the read buffer while the lock is not held. Or
something. Just since allocations are relatively slow, and we can fairly easily
do it without the lock. I don't care too much how it's organized to accomplish
that.

[bbudge, 2013/08/09 21:43:24]

It would be messy, as the allocation has to happen while the lock is held in the
non-blocking case.
WDYT?

[bbudge, 2013/08/09 21:58:14]

Turns out I was needlessly worried about allocating and deleting on different
threads. I now defer allocating the buffer until the read, which always happens
outside the lock.

+  if (callback->is_blocking()) {
+    // Release the proxy lock while making a potentially slow file call.
+    {
+      ProxyAutoUnlock unlock;
+      DoRead(offset, bytes_to_read, read_data);

[dmichael (off chromium), 2013/08/09 17:37:42]

I'm thinking maybe DoRead can return the result (instead of it being in
ReadData, or perhaps in addition, if that's easier).

(You can probably even get rid of ReadData, if you want...  details below...)

[bbudge, 2013/08/09 21:43:24]

Done.

     }
-
-    state_manager_.SetOperationFinished();
-    return result;
+    return OnReadComplete(read_data, array_output, PP_OK);

[dmichael (off chromium), 2013/08/09 17:37:42]

would it work to pass read_data->bytes_read() instead of PP_OK? I think it might
make OnReadComplete less complex...

[bbudge, 2013/08/09 21:43:24]

Done.

   }
 
-  Call<PpapiPluginMsg_FileIO_ReadReply>(RENDERER,
-      PpapiHostMsg_FileIO_Read(offset, bytes_to_read),
-      base::Bind(&FileIOResource::OnPluginMsgReadComplete, this,
-                 callback, array_output));
+  // For the non-blocking case, post a task to the file thread.
+  PpapiGlobals::Get()->GetFileTaskRunner(pp_instance())->PostTaskAndReply(
+      FROM_HERE,
+      Bind(&FileIOResource::DoRead, this, offset, bytes_to_read, read_data),
+      RunWhileLocked(
+          Bind(&FileIOResource::OnFileTaskComplete, this, callback)));

[dmichael (off chromium), 2013/08/09 17:37:42]

If you make OnFileTaskComplete take a result parameter, and DoRead returns the
result, you can use PostTaskAndReplyWithResult.

[bbudge, 2013/08/09 21:43:24]

Done.

+  callback->set_completion_task(
+      Bind(&FileIOResource::OnReadComplete, this, read_data, array_output));

[dmichael (off chromium), 2013/08/09 17:37:42]

If you are able to remove the "result" part of ReadData, you could use a
heap-allocated std::vector<char> instead of having ReadData.

I.e., before posting DoRead, you have:
std::vector<char> buffer* = new std::vector<char>();

pass it as base::Unretained() to DoRead.
pass it as base::Owned to OnReadComplete.

(This may be more complex than just having scoped_refptr<ReadData>, so take it
as an optional suggestion)

[bbudge, 2013/08/09 21:43:24]

I couldn't get this to work, since in the abort case it's possible for the file
thread task to have a dangling pointer. (Thread race).

+
   return PP_OK_COMPLETIONPENDING;
 }
 
@@ -324,6 +329,58 @@ void FileIOResource::CloseFileHandle() {
   }
 }
 
+void FileIOResource::DoQuery() {
+  query_result_ = base::GetPlatformFileInfo(file_handle_, &file_info_) ?
+      PP_OK : PP_ERROR_FAILED;
+}
+
+void FileIOResource::DoRead(int64_t offset,
+                            int32_t bytes_to_read,
+                            scoped_refptr<ReadData> read_data) {
+  read_data->set_bytes_read(base::ReadPlatformFile(
+      file_handle_, offset, read_data->buffer(), bytes_to_read));
+}
+
+void FileIOResource::OnFileTaskComplete(
+    scoped_refptr<TrackedCallback> callback) {
+  callback->Run(PP_OK);

[dmichael (off chromium), 2013/08/09 17:37:42]

I think OnFileTaskComplete should take a result parameter that you pass here.
Then, the "completion_task" will only ever override the result if it fails to do
the array allocation step for array output.

[bbudge, 2013/08/09 21:43:24]

Done.

+}
+
+int32_t FileIOResource::OnQueryComplete(PP_FileInfo* info, int32_t result) {
+  DCHECK(state_manager_.get_pending_operation() ==
+         FileIOStateManager::OPERATION_EXCLUSIVE);
+
+  if (result == PP_OK && query_result_ == PP_OK) {
+    ppapi::PlatformFileInfoToPepperFileInfo(file_info_, file_system_type_,
+                                            info);
+  }
+  state_manager_.SetOperationFinished();
+  return result;

[dmichael (off chromium), 2013/08/09 17:37:42]

Given the way this is all structured currently, I think you should be returning
query_result? It kind of looks like result is always PP_OK. You always pass
PP_OK in the blocking case, and OnFileTaskComplete always passes PP_OK to Run().
But query_result_ may indicate there was a failure.

[bbudge, 2013/08/09 21:43:24]

Done.

+}
+
+int32_t FileIOResource::OnReadComplete(scoped_refptr<ReadData> read_data,
+                                       PP_ArrayOutput array_output,
+                                       int32_t result) {
+  DCHECK(state_manager_.get_pending_operation() ==
+         FileIOStateManager::OPERATION_READ);
+  if (result == PP_OK) {
+    result = read_data->bytes_read();
+    if (result >= 0) {
+      ArrayWriter output;
+      output.set_pp_array_output(array_output);
+      if (output.is_valid())
+        output.StoreArray(read_data->buffer(), result);
+      else
+        result = PP_ERROR_FAILED;
+    } else {
+      // The read operation failed.
+      result = PP_ERROR_FAILED;
+    }
+  }
+  state_manager_.SetOperationFinished();
+  return result;
+}
+
 void FileIOResource::OnPluginMsgGeneralComplete(
     scoped_refptr<TrackedCallback> callback,
     const ResourceMessageReplyParams& params) {
@@ -355,46 +412,6 @@ void FileIOResource::OnPluginMsgOpenFileComplete(
   callback->Run(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 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());
-}
-
-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
-    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::OnPluginMsgRequestOSFileHandleComplete(
     scoped_refptr<TrackedCallback> callback,
     PP_FileHandle* output_handle,