net: Make UploadDataStream::Read() asynchronous

net/base/upload_file_element_reader.cc

Index: net/base/upload_file_element_reader.cc
diff --git a/net/base/upload_file_element_reader.cc b/net/base/upload_file_element_reader.cc
index 86be79d15095a58996d03d44c57abf0874ee3828..88c8e93a45d6826e6447a63715ba30722f0ac8d5 100644
--- a/net/base/upload_file_element_reader.cc
+++ b/net/base/upload_file_element_reader.cc
@@ -10,6 +10,7 @@
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/worker_pool.h"
 #include "net/base/file_stream.h"
+#include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 
 namespace net {
@@ -20,7 +21,7 @@ namespace {
 // UploadFileElementReader::GetContentLength() when set to non-zero.
 uint64 overriding_content_length = 0;
 
-// This method is used to implement Init().
+// This function is used to implement Init().
 void InitInternal(const FilePath& path,
                   uint64 range_offset,
                   uint64 range_length,
@@ -72,6 +73,35 @@ void InitInternal(const FilePath& path,
   *out_result = OK;
 }
 
+// This function is used to implement Read().
+void ReadInternal(scoped_refptr<IOBuffer> buf,
+                  int buf_length,
+                  uint64 bytes_remaining,
+                  FileStream* file_stream,
+                  int* result) {
+  DCHECK_LT(0, buf_length);
+
+  const uint64 num_bytes_to_read =
+      std::min(bytes_remaining, static_cast<uint64>(buf_length));
+
+  if (num_bytes_to_read > 0) {
+    int num_bytes_consumed = 0;
+    // file_stream is NULL if the target file is
+    // missing or not readable.
+    if (file_stream) {
+      num_bytes_consumed = file_stream->ReadSync(buf->data(),
+                                                 num_bytes_to_read);
+    }
+    if (num_bytes_consumed <= 0) {
+      // If there's less data to read than we initially observed, then
+      // pad with zero.  Otherwise the server will hang waiting for the
+      // rest of the data.
+      memset(buf->data(), 0, num_bytes_to_read);
+    }
+  }
+  *result = num_bytes_to_read;
+}
+
 }  // namespace
 
 UploadFileElementReader::UploadFileElementReader(
@@ -143,32 +173,42 @@ uint64 UploadFileElementReader::BytesRemaining() const {
   return bytes_remaining_;
 }
 
-int UploadFileElementReader::ReadSync(char* buf, int buf_length) {
+int UploadFileElementReader::Read(IOBuffer* buf,
+                                  int buf_length,
+                                  const CompletionCallback& callback) {
+  if (BytesRemaining() <= 0)

[willchan no longer on Chromium, 2012/09/24 23:37:08]

Why <= 0? It returns a uint64.

[hashimoto, 2012/09/25 10:47:56]

Replaced with '=='.

+    return 0;
+
+  // Save the value of file_stream_.get() before base::Passed() invalidats it.

[willchan no longer on Chromium, 2012/09/24 23:37:08]

invalidates

Also, why does this make sense? file_stream_.get() gets called before
base::Passed(&file_stream_) anyway.

[hashimoto, 2012/09/25 10:47:56]

We must save the value here because the C++ standard does not specify the
evaluation order of function arguments. (ISO/IEC 14882:2011 8.3.6.9)
This can result in a real crash.
See http://crrev.com/156365 as an example.

+  FileStream* file_stream_ptr = file_stream_.get();
+  int* result = new int;
+  // Pass the ownership of file_stream_ to the worker pool to safely perform
+  // operation even when |this| is destructed before the read completes.
+  const bool posted = base::WorkerPool::PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&ReadInternal,
+                 scoped_refptr<IOBuffer>(buf),
+                 buf_length,
+                 BytesRemaining(),
+                 file_stream_ptr,
+                 result),
+      base::Bind(&UploadFileElementReader::OnReadCompleted,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(&file_stream_),
+                 base::Owned(result),
+                 callback),
+      true /* task_is_slow */);
+  DCHECK(posted);
+  return ERR_IO_PENDING;
+}
+
+int UploadFileElementReader::ReadSync(IOBuffer* buf, int buf_length) {
   // Temporarily allow until fix: http://crbug.com/72001.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
-  DCHECK_LT(0, buf_length);
-
-  const uint64 num_bytes_to_read =
-      static_cast<int>(std::min(BytesRemaining(),
-                                static_cast<uint64>(buf_length)));
-  if (num_bytes_to_read > 0) {
-    int num_bytes_consumed = 0;
-    // file_stream_ is NULL if the target file is
-    // missing or not readable.
-    if (file_stream_.get()) {
-      num_bytes_consumed =
-          file_stream_->ReadSync(buf, num_bytes_to_read);
-    }
-    if (num_bytes_consumed <= 0) {
-      // If there's less data to read than we initially observed, then
-      // pad with zero.  Otherwise the server will hang waiting for the
-      // rest of the data.
-      memset(buf, 0, num_bytes_to_read);
-    }
-  }
-  DCHECK_GE(bytes_remaining_, num_bytes_to_read);
-  bytes_remaining_ -= num_bytes_to_read;
-  return num_bytes_to_read;
+  int result = 0;
+  ReadInternal(buf, buf_length, BytesRemaining(), file_stream_.get(), &result);
+  OnReadCompleted(file_stream_.Pass(), &result, CompletionCallback());
+  return result;
 }
 
 void UploadFileElementReader::OnInitCompleted(
@@ -183,6 +223,17 @@ void UploadFileElementReader::OnInitCompleted(
     callback.Run(*result);
 }
 
+void UploadFileElementReader::OnReadCompleted(
+    scoped_ptr<FileStream> file_stream,
+    int* result,
+    const CompletionCallback& callback) {
+  file_stream_.swap(file_stream);
+  DCHECK_GE(static_cast<int>(bytes_remaining_), *result);
+  bytes_remaining_ -= *result;
+  if (!callback.is_null())
+    callback.Run(*result);
+}
+
 UploadFileElementReader::ScopedOverridingContentLengthForTests::
 ScopedOverridingContentLengthForTests(uint64 value) {
   overriding_content_length = value;