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 "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"
 
 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().
 void* DummyGetDataBuffer(void* user_data, uint32_t count, uint32_t size) {
   return user_data;
 }
 
+// Dummy close callback allows us to call CloseFileHandle in the destructor.
+void DummyCloseCallback(base::PlatformFileError error_code) {
+}
+
+// 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
 
-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() {
   return this;
 }
 
 int32_t FileIOResource::Open(PP_Resource file_ref,
                              int32_t open_flags,
                              scoped_refptr<TrackedCallback> callback) {
   EnterResourceNoLock<PPB_FileRef_API> enter(file_ref, true);
   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)
     return rv;
 
   Call<PpapiPluginMsg_FileIO_OpenReply>(RENDERER,
       PpapiHostMsg_FileIO_Open(
           enter.resource()->host_resource().host_resource(),
           open_flags),
       base::Bind(&FileIOResource::OnPluginMsgOpenFileComplete, this,
                  callback));
 
   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;
 
-  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;
 }
 
 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)
@@ skipping 13 matching lines @@
                              int32_t bytes_to_read,
                              scoped_refptr<TrackedCallback> callback) {
   int32_t rv = state_manager_.CheckOperationState(
       FileIOStateManager::OPERATION_READ, true);
   if (rv != PP_OK)
     return rv;
 
   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);
 }
 
 int32_t FileIOResource::ReadToArray(int64_t offset,
                                     int32_t max_read_length,
                                     PP_ArrayOutput* array_output,
                                     scoped_refptr<TrackedCallback> callback) {
   DCHECK(array_output);
   int32_t rv = state_manager_.CheckOperationState(
       FileIOStateManager::OPERATION_READ, true);
   if (rv != PP_OK)
     return rv;
 
-  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_READ);
   return ReadValidated(offset, max_read_length, *array_output, callback);
 }
 
 int32_t FileIOResource::Write(int64_t offset,
                               const char* buffer,
                               int32_t bytes_to_write,
                               scoped_refptr<TrackedCallback> callback) {
   int32_t rv = state_manager_.CheckOperationState(
       FileIOStateManager::OPERATION_WRITE, true);
   if (rv != PP_OK)
@@ skipping 36 matching lines @@
   Call<PpapiPluginMsg_FileIO_GeneralReply>(RENDERER,
       PpapiHostMsg_FileIO_Flush(),
       base::Bind(&FileIOResource::OnPluginMsgGeneralComplete, this,
                  callback));
 
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
 
 void FileIOResource::Close() {
+  CloseFileHandle();
   Post(RENDERER, PpapiHostMsg_FileIO_Close());
 }
 
 int32_t FileIOResource::GetOSFileDescriptor() {
   int32_t file_descriptor;
   // Only available when running in process.
   SyncCall<PpapiPluginMsg_FileIO_GetOSFileDescriptorReply>(
       RENDERER, PpapiHostMsg_FileIO_GetOSFileDescriptor(), &file_descriptor);
   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) {
   Call<PpapiPluginMsg_FileIO_GeneralReply>(RENDERER,
       PpapiHostMsg_FileIO_WillWrite(offset, bytes_to_write),
       base::Bind(&FileIOResource::OnPluginMsgGeneralComplete, this,
                  callback));
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t FileIOResource::WillSetLength(int64_t length,
                                       scoped_refptr<TrackedCallback> callback) {
   Call<PpapiPluginMsg_FileIO_GeneralReply>(RENDERER,
       PpapiHostMsg_FileIO_WillSetLength(length),
       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) {
-  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;
+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;
+  }
+}
 
-  Call<PpapiPluginMsg_FileIO_RequestOSFileHandleReply>(RENDERER,
-      PpapiHostMsg_FileIO_RequestOSFileHandle(),
-      base::Bind(&FileIOResource::OnPluginMsgRequestOSFileHandleComplete, this,
-                 callback, handle));
+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);
 
-  state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
-  return PP_OK_COMPLETIONPENDING;
+  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(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 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(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 the operation now. The callback may perform another file operation.
   state_manager_.SetOperationFinished();
   callback->Run(params.result());
 }
 
 void FileIOResource::OnPluginMsgOpenFileComplete(
     scoped_refptr<TrackedCallback> callback,
     const ResourceMessageReplyParams& params) {
   DCHECK(state_manager_.get_pending_operation() ==
          FileIOStateManager::OPERATION_EXCLUSIVE);
   if (params.result() == PP_OK)
     state_manager_.SetOpenSucceed();
+
+  int32_t result = params.result();
+  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(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
-    result = PP_ERROR_FAILED;
-
-  // End the operation now. The callback may perform another file operation.
-  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 the operation now. The callback may perform another file operation.
   state_manager_.SetOperationFinished();
   callback->Run(result);
 }
 
 }  // namespace proxy
 }  // namespace ppapi