Provide a safer FileIO Read API

An testing util class TestCompletionCallbackWithOutput is introduced for the callback with output.

BUG=155395

Review URL: https://chromiumcodereview.appspot.com/11361117

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@168156 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 176 insertions, 40 deletions

diff --git a/ppapi/cpp/array_output.h b/ppapi/cpp/array_output.h
index 57fa790..fe0bbc3 100644
--- a/ppapi/cpp/array_output.h
+++ b/ppapi/cpp/array_output.h
@@ -176,7 +176,7 @@ template<typename T>
 class ArrayOutputAdapterWithStorage : public ArrayOutputAdapter<T> {
  public:
   ArrayOutputAdapterWithStorage() {
-    set_output(&output_storage_);
+    this->set_output(&output_storage_);
   }
 
   std::vector<T>& output() { return output_storage_; }
diff --git a/ppapi/cpp/file_io.cc b/ppapi/cpp/file_io.cc
index e596577..faaf284 100644
--- a/ppapi/cpp/file_io.cc
+++ b/ppapi/cpp/file_io.cc
@@ -4,9 +4,12 @@
 
 #include "ppapi/cpp/file_io.h"
 
+#include <string.h>  // memcpy
+
 #include "ppapi/c/ppb_file_io.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/cpp/completion_callback.h"
+#include "ppapi/cpp/dev/resource_array_dev.h"
 #include "ppapi/cpp/file_ref.h"
 #include "ppapi/cpp/instance_handle.h"
 #include "ppapi/cpp/module.h"
@@ -20,16 +23,23 @@ template <> const char* interface_name<PPB_FileIO_1_0>() {
   return PPB_FILEIO_INTERFACE_1_0;
 }
 
+template <> const char* interface_name<PPB_FileIO_1_1>() {
+  return PPB_FILEIO_INTERFACE_1_1;
+}
+
 }  // namespace
 
 FileIO::FileIO() {
 }
 
 FileIO::FileIO(const InstanceHandle& instance) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return;
-  PassRefFromConstructor(get_interface<PPB_FileIO_1_0>()->Create(
-      instance.pp_instance()));
+  if (has_interface<PPB_FileIO_1_1>()) {
+    PassRefFromConstructor(get_interface<PPB_FileIO_1_1>()->Create(
+        instance.pp_instance()));
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    PassRefFromConstructor(get_interface<PPB_FileIO_1_0>()->Create(
+        instance.pp_instance()));
+  }
 }
 
 FileIO::FileIO(const FileIO& other)
@@ -39,71 +49,147 @@ FileIO::FileIO(const FileIO& other)
 int32_t FileIO::Open(const FileRef& file_ref,
                      int32_t open_flags,
                      const CompletionCallback& cc) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return cc.MayForce(PP_ERROR_NOINTERFACE);
-  return get_interface<PPB_FileIO_1_0>()->Open(
-      pp_resource(), file_ref.pp_resource(), open_flags,
-      cc.pp_completion_callback());
+  if (has_interface<PPB_FileIO_1_1>()) {
+    return get_interface<PPB_FileIO_1_1>()->Open(
+        pp_resource(), file_ref.pp_resource(), open_flags,
+        cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    return get_interface<PPB_FileIO_1_0>()->Open(
+        pp_resource(), file_ref.pp_resource(), open_flags,
+        cc.pp_completion_callback());
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
 }
 
 int32_t FileIO::Query(PP_FileInfo* result_buf,
                       const CompletionCallback& cc) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return cc.MayForce(PP_ERROR_NOINTERFACE);
-  return get_interface<PPB_FileIO_1_0>()->Query(
-      pp_resource(), result_buf, cc.pp_completion_callback());
+  if (has_interface<PPB_FileIO_1_1>()) {
+    return get_interface<PPB_FileIO_1_1>()->Query(
+        pp_resource(), result_buf, cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    return get_interface<PPB_FileIO_1_0>()->Query(
+        pp_resource(), result_buf, cc.pp_completion_callback());
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
 }
 
 int32_t FileIO::Touch(PP_Time last_access_time,
                       PP_Time last_modified_time,
                       const CompletionCallback& cc) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return cc.MayForce(PP_ERROR_NOINTERFACE);
-  return get_interface<PPB_FileIO_1_0>()->Touch(
-      pp_resource(), last_access_time, last_modified_time,
-      cc.pp_completion_callback());
+  if (has_interface<PPB_FileIO_1_1>()) {
+    return get_interface<PPB_FileIO_1_1>()->Touch(
+        pp_resource(), last_access_time, last_modified_time,
+        cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    return get_interface<PPB_FileIO_1_0>()->Touch(
+        pp_resource(), last_access_time, last_modified_time,
+        cc.pp_completion_callback());
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
 }
 
 int32_t FileIO::Read(int64_t offset,
                      char* buffer,
                      int32_t bytes_to_read,
                      const CompletionCallback& cc) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return cc.MayForce(PP_ERROR_NOINTERFACE);
-  return get_interface<PPB_FileIO_1_0>()->Read(pp_resource(),
-      offset, buffer, bytes_to_read, cc.pp_completion_callback());
+  if (has_interface<PPB_FileIO_1_1>()) {
+    return get_interface<PPB_FileIO_1_1>()->Read(pp_resource(),
+        offset, buffer, bytes_to_read, cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    return get_interface<PPB_FileIO_1_0>()->Read(pp_resource(),
+        offset, buffer, bytes_to_read, cc.pp_completion_callback());
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
+}
+
+int32_t FileIO::Read(
+    int32_t offset,
+    int32_t max_read_length,
+    const CompletionCallbackWithOutput< std::vector<char> >& cc) {
+  if (has_interface<PPB_FileIO_1_1>()) {
+    PP_ArrayOutput array_output = cc.output();
+    return get_interface<PPB_FileIO_1_1>()->ReadToArray(pp_resource(),
+        offset, max_read_length, &array_output,
+        cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    // Data for our callback wrapper. The callback handler will delete it and
+    // temp_buffer.
+    CallbackData1_0* data = new CallbackData1_0;
+    data->output = cc.output();
+    data->temp_buffer = max_read_length >= 0 ? new char[max_read_length] : NULL;
+    data->original_callback = cc.pp_completion_callback();
+
+    // Actual returned bytes might not equals to max_read_length.  We need to
+    // read to a temporary buffer first and copy later to make sure the array
+    // buffer has correct size.
+    return get_interface<PPB_FileIO_1_0>()->Read(
+        pp_resource(), offset, data->temp_buffer, max_read_length,
+        PP_MakeCompletionCallback(&CallbackConverter, data));
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
 }
 
 int32_t FileIO::Write(int64_t offset,
                       const char* buffer,
                       int32_t bytes_to_write,
                       const CompletionCallback& cc) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return cc.MayForce(PP_ERROR_NOINTERFACE);
-  return get_interface<PPB_FileIO_1_0>()->Write(
-      pp_resource(), offset, buffer, bytes_to_write,
-      cc.pp_completion_callback());
+  if (has_interface<PPB_FileIO_1_1>()) {
+    return get_interface<PPB_FileIO_1_1>()->Write(
+        pp_resource(), offset, buffer, bytes_to_write,
+        cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    return get_interface<PPB_FileIO_1_0>()->Write(
+        pp_resource(), offset, buffer, bytes_to_write,
+        cc.pp_completion_callback());
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
 }
 
 int32_t FileIO::SetLength(int64_t length,
                           const CompletionCallback& cc) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return cc.MayForce(PP_ERROR_NOINTERFACE);
-  return get_interface<PPB_FileIO_1_0>()->SetLength(
-      pp_resource(), length, cc.pp_completion_callback());
+  if (has_interface<PPB_FileIO_1_1>()) {
+    return get_interface<PPB_FileIO_1_1>()->SetLength(
+        pp_resource(), length, cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    return get_interface<PPB_FileIO_1_0>()->SetLength(
+        pp_resource(), length, cc.pp_completion_callback());
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
 }
 
 int32_t FileIO::Flush(const CompletionCallback& cc) {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return cc.MayForce(PP_ERROR_NOINTERFACE);
-  return get_interface<PPB_FileIO_1_0>()->Flush(
-      pp_resource(), cc.pp_completion_callback());
+  if (has_interface<PPB_FileIO_1_1>()) {
+    return get_interface<PPB_FileIO_1_1>()->Flush(
+        pp_resource(), cc.pp_completion_callback());
+  } else if (has_interface<PPB_FileIO_1_0>()) {
+    return get_interface<PPB_FileIO_1_0>()->Flush(
+        pp_resource(), cc.pp_completion_callback());
+  }
+  return cc.MayForce(PP_ERROR_NOINTERFACE);
 }
 
 void FileIO::Close() {
-  if (!has_interface<PPB_FileIO_1_0>())
-    return;
-  get_interface<PPB_FileIO_1_0>()->Close(pp_resource());
+  if (has_interface<PPB_FileIO_1_1>())
+    get_interface<PPB_FileIO_1_1>()->Close(pp_resource());
+  else if (has_interface<PPB_FileIO_1_0>())
+    get_interface<PPB_FileIO_1_0>()->Close(pp_resource());
+}
+
+// static
+void FileIO::CallbackConverter(void* user_data, int32_t result) {
+  CallbackData1_0* data = static_cast<CallbackData1_0*>(user_data);
+
+  if (result >= 0) {
+    // Copy to the destination buffer owned by the callback.
+    char* buffer = static_cast<char*>(data->output.GetDataBuffer(
+        data->output.user_data, result, sizeof(char)));
+    memcpy(buffer, data->temp_buffer, result);
+    delete[] data->temp_buffer;
+  }
+
+  // Now execute the original callback.
+  PP_RunCompletionCallback(&data->original_callback, result);
+  delete data;
 }
 
 }  // namespace pp
diff --git a/ppapi/cpp/file_io.h b/ppapi/cpp/file_io.h
index 0d8dcc9..3b93bfe 100644
--- a/ppapi/cpp/file_io.h
+++ b/ppapi/cpp/file_io.h
@@ -6,6 +6,7 @@
 #define PPAPI_CPP_FILE_IO_H_
 
 #include "ppapi/c/pp_time.h"
+#include "ppapi/cpp/completion_callback.h"
 #include "ppapi/cpp/resource.h"
 
 /// @file
@@ -15,7 +16,6 @@ struct PP_FileInfo;
 
 namespace pp {
 
-class CompletionCallback;
 class FileRef;
 class InstanceHandle;
 
@@ -87,6 +87,19 @@ class FileIO : public Resource {
   /// large enough to hold the specified number of bytes to read.  This
   /// function might perform a partial read.
   ///
+  /// <strong>Caveat:</strong> This Read() is potentially unsafe if you're using
+  /// a CompletionCallbackFactory to scope callbacks to the lifetime of your
+  /// class.  When your class goes out of scope, the callback factory will not
+  /// actually cancel the callback, but will rather just skip issuing the
+  /// callback on your class.  This means that if the FileIO object outlives
+  /// your class (if you made a copy saved somewhere else, for example), then
+  /// the browser will still try to write into your buffer when the
+  /// asynchronous read completes, potentially causing a crash.
+  ///
+  /// See the other version of Read() which avoids this problem by writing into
+  /// CompletionCallbackWithOutput, where the output buffer is automatically
+  /// managed by the callback.
+  ///
   /// @param[in] offset The offset into the file.
   /// @param[in] buffer The buffer to hold the specified number of bytes read.
   /// @param[in] bytes_to_read The number of bytes to read from
@@ -103,6 +116,28 @@ class FileIO : public Resource {
                int32_t bytes_to_read,
                const CompletionCallback& cc);
 
+  /// Read() reads from an offset in the file.  A PP_ArrayOutput must be
+  /// provided so that output will be stored in its allocated buffer.  This
+  /// function might perform a partial read.
+  ///
+  /// @param[in] file_io A <code>PP_Resource</code> corresponding to a file
+  /// FileIO.
+  /// @param[in] offset The offset into the file.
+  /// @param[in] max_read_length The maximum number of bytes to read from
+  /// <code>offset</code>.
+  /// @param[in] output A <code>PP_ArrayOutput</code> to hold the output data.
+  /// @param[in] callback A <code>PP_CompletionCallback</code> to be called upon
+  /// completion of Read().
+  ///
+  /// @return The number of bytes read or an error code from
+  /// <code>pp_errors.h</code>. If the return value is 0, then end-of-file was
+  /// reached. It is valid to call Read() multiple times with a completion
+  /// callback to queue up parallel reads from the file, but pending reads
+  /// cannot be interleaved with other operations.
+  int32_t Read(int32_t offset,
+               int32_t max_read_length,
+               const CompletionCallbackWithOutput< std::vector<char> >& cc);
+
   /// Write() writes to an offset in the file.  This function might perform a
   /// partial write. The FileIO object must have been opened with write access.
   ///
@@ -153,6 +188,21 @@ class FileIO : public Resource {
   /// open, then it will be implicitly closed, so you are not required to call
   /// Close().
   void Close();
+
+ private:
+  struct CallbackData1_0 {
+    PP_ArrayOutput output;
+    char* temp_buffer;
+    PP_CompletionCallback original_callback;
+  };
+
+  // Provide backwards-compatability for older Read versions. Converts the
+  // old-style "char*" output buffer of 1.0 to the new "PP_ArrayOutput"
+  // interface in 1.1.
+  //
+  // This takes a heap-allocated CallbackData1_0 struct passed as the user data
+  // and deletes it when the call completes.
+  static void CallbackConverter(void* user_data, int32_t result);
 };
 
 }  // namespace pp