Do PPB_FileIO Query and Read in the plugin process.

ppapi/proxy/file_io_resource.cc

 // Copyright (c) 2012 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 "ppapi/proxy/file_io_resource.h"
 
 #include "base/bind.h"
 #include "ipc/ipc_message.h"
 #include "ppapi/c/pp_errors.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"
 
 using ppapi::thunk::EnterResourceNoLock;
 using ppapi::thunk::PPB_FileIO_API;
 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;
 }
 
 // File thread task to close the file handle.
 void DoClose(base::PlatformFile file) {
   base::ClosePlatformFile(file);
 }
 
 }  // namespace
 
 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());
 }
 
 FileIOResource::~FileIOResource() {
   CloseFileHandle();
 }
 
 PPB_FileIO_API* FileIOResource::AsPPB_FileIO_API() {
   return this;
 }
@@ skipping 31 matching lines @@
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t FileIOResource::Query(PP_FileInfo* info,
                               scoped_refptr<TrackedCallback> callback) {
   int32_t rv = state_manager_.CheckOperationState(
       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;
 
-  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.
+  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
+
+  // 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;
 }
 
 int32_t FileIOResource::Touch(PP_Time last_access_time,
                               PP_Time last_modified_time,
                               scoped_refptr<TrackedCallback> callback) {
   int32_t rv = state_manager_.CheckOperationState(
       FileIOStateManager::OPERATION_EXCLUSIVE, true);
   if (rv != PP_OK)
     return rv;
@@ skipping 135 matching lines @@
       base::Bind(&FileIOResource::OnPluginMsgGeneralComplete, this, callback));
 
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
 
 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;
 }
 
 void FileIOResource::CloseFileHandle() {
   if (file_handle_ != base::kInvalidPlatformFileValue) {
     // Close our local fd on the file thread.
     base::TaskRunner* file_task_runner =
         PpapiGlobals::Get()->GetFileTaskRunner(pp_instance());
     file_task_runner->PostTask(FROM_HERE,
                                base::Bind(&DoClose, file_handle_));
 
     file_handle_ = base::kInvalidPlatformFileValue;
   }
 }
 
+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) {
   DCHECK(state_manager_.get_pending_operation() ==
          FileIOStateManager::OPERATION_EXCLUSIVE ||
          state_manager_.get_pending_operation() ==
          FileIOStateManager::OPERATION_WRITE);
   // End this operation now, so the user's callback can execute another FileIO
   // operation, assuming there are no other pending operations.
   state_manager_.SetOperationFinished();
@@ skipping 11 matching lines @@
   int32_t result = params.result();
   IPC::PlatformFileForTransit transit_file;
   if ((result == PP_OK) && params.TakeFileHandleAtIndex(0, &transit_file))
     file_handle_ = IPC::PlatformFileForTransitToPlatformFile(transit_file);
   // 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::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,
     const ResourceMessageReplyParams& params) {
   DCHECK(state_manager_.get_pending_operation() ==
          FileIOStateManager::OPERATION_EXCLUSIVE);
 
   if (!TrackedCallback::IsPending(callback)) {
     state_manager_.SetOperationFinished();
     return;
   }
 
   int32_t result = params.result();
   IPC::PlatformFileForTransit transit_file;
   if (!params.TakeFileHandleAtIndex(0, &transit_file))
     result = PP_ERROR_FAILED;
   *output_handle = IPC::PlatformFileForTransitToPlatformFile(transit_file);
 
   // 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);
 }
 
 }  // namespace proxy
 }  // namespace ppapi