net: Make UploadDataStream::Init() asynchronous.

net/base/upload_data_stream.cc

Index: net/base/upload_data_stream.cc
diff --git a/net/base/upload_data_stream.cc b/net/base/upload_data_stream.cc
index 6c78f95569bbc49f2482c3c7ba3679f919fa2144..de1bdf6cfc91c93d3359b3216090556c0ace18b2 100644
--- a/net/base/upload_data_stream.cc
+++ b/net/base/upload_data_stream.cc
@@ -5,14 +5,54 @@
 #include "net/base/upload_data_stream.h"
 
 #include "base/file_util.h"
+#include "base/location.h"
 #include "base/logging.h"
 #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 {
 
+namespace {
+
+// This function is used to implement UploadDataStream::Init().
+void InitInternal(scoped_refptr<UploadData> upload_data,

[eroman, 2012/08/09 05:29:33]

UploadData is not threadsafe, however now it is being passed off to a worker
thread and used there. I do not believe this is safe.

(Even just calling AddRef() implicitly by the scoped_refptr<> can be risky
business when done from multiple threads and the base class was RefCounted
rather than RefCountedThreadSafe).

[hashimoto, 2012/08/09 13:35:02]

Oh I thought touching UploadData on a worker thread is safe because
UploadData::GetContentLength is doing it.
It seems we have to fix GetContentLength first.
How about making UploadData RecCountedThreadSafe and adding a class comment "Do
not touch this class while asynchronous operation is in progress" for
UploadData?
It is safe as long as all public methods of UploadData are called only on one
thread (i.e. IO thread) and only private methods touch UploadData on worker
threads.

If we are able to touch UploadData on worker threads, it becomes very easy to
avoid code duplication between synchronous and asynchronous implementations.
(Synchronous implementation should be maintained unless we fix Chrome Frame
radically)

+                  uint64* total_size,
+                  int* result) {
+  *total_size = upload_data->GetContentLengthSync();
+
+  // If the underlying file has been changed and the expected file
+  // modification time is set, treat it as error. Note that the expected
+  // modification time from WebKit is based on time_t precision. So we
+  // have to convert both to time_t to compare. This check is used for
+  // sliced files.
+  const std::vector<UploadData::Element>& elements = *upload_data->elements();
+  for (size_t i = 0; i < elements.size(); ++i) {
+    const UploadData::Element& element = elements[i];
+    if (element.type() == UploadData::TYPE_FILE &&
+        !element.expected_file_modification_time().is_null()) {
+      base::PlatformFileInfo info;
+      if (file_util::GetFileInfo(element.file_path(), &info) &&
+          element.expected_file_modification_time().ToTimeT() !=
+          info.last_modified.ToTimeT()) {
+        *result = ERR_UPLOAD_FILE_CHANGED;
+        return;
+      }
+    }
+  }
+
+  // Reset the offset, as upload_data_ may already be read (i.e. UploadData
+  // can be reused for a new UploadDataStream).
+  upload_data->ResetOffset();
+
+  *result = OK;
+  return;

[eroman, 2012/08/09 05:29:33]

delete this

+}
+
+}  // namespace
+
 bool UploadDataStream::merge_chunks_ = true;
 
 // static
@@ -26,47 +66,46 @@ UploadDataStream::UploadDataStream(UploadData* upload_data)
       element_index_(0),
       total_size_(0),
       current_position_(0),
-      initialized_successfully_(false) {
+      initialized_successfully_(false),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 UploadDataStream::~UploadDataStream() {
 }
 
-int UploadDataStream::Init() {
+int UploadDataStream::Init(const CompletionCallback& callback) {
   DCHECK(!initialized_successfully_);
+  // Fast path. Initialize synchronously if the data is in memory.
+  if (upload_data_->IsInMemory())
+    return InitSync();
+
+  uint64* total_size = new uint64(0);
+  int* result = new int(OK);
+  const bool posted = base::WorkerPool::PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&InitInternal, upload_data_, total_size, result),
+      base::Bind(&UploadDataStream::OnInitComplete,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback,
+                 base::Owned(total_size),
+                 base::Owned(result)),
+      true /* task_is_slow */);
+  DCHECK(posted);
+  return ERR_IO_PENDING;
+}
 
+int UploadDataStream::InitSync() {
+  DCHECK(!initialized_successfully_);
+  int result = OK;
   {
     base::ThreadRestrictions::ScopedAllowIO allow_io;
-    total_size_ = upload_data_->GetContentLengthSync();
-  }
-
-  // If the underlying file has been changed and the expected file
-  // modification time is set, treat it as error. Note that the expected
-  // modification time from WebKit is based on time_t precision. So we
-  // have to convert both to time_t to compare. This check is used for
-  // sliced files.
-  const std::vector<UploadData::Element>& elements = *upload_data_->elements();
-  for (size_t i = 0; i < elements.size(); ++i) {
-    const UploadData::Element& element = elements[i];
-    if (element.type() == UploadData::TYPE_FILE &&
-        !element.expected_file_modification_time().is_null()) {
-      // Temporarily allow until fix: http://crbug.com/72001.
-      base::ThreadRestrictions::ScopedAllowIO allow_io;
-      base::PlatformFileInfo info;
-      if (file_util::GetFileInfo(element.file_path(), &info) &&
-          element.expected_file_modification_time().ToTimeT() !=
-          info.last_modified.ToTimeT()) {
-        return ERR_UPLOAD_FILE_CHANGED;
-      }
-    }
+    InitInternal(upload_data_, &total_size_, &result);
   }
-
-  // Reset the offset, as upload_data_ may already be read (i.e. UploadData
-  // can be reused for a new UploadDataStream).
-  upload_data_->ResetOffset();
-
-  initialized_successfully_ = true;
-  return OK;
+  // Passing a null callback to OnInitComplete() since we return the result
+  // synchronously here and there is no need to run callback.
+  CompletionCallback null_callback;
+  OnInitComplete(null_callback, &total_size_, &result);
+  return result;
 }
 
 int UploadDataStream::Read(IOBuffer* buf, int buf_len) {
@@ -94,6 +133,17 @@ int UploadDataStream::Read(IOBuffer* buf, int buf_len) {
   return bytes_copied;
 }
 
+void UploadDataStream::OnInitComplete(const CompletionCallback& callback,
+                                      uint64* total_size,
+                                      int* result) {
+  total_size_ = *total_size;
+  if (*result == OK)
+    initialized_successfully_ = true;
+
+  if (!callback.is_null())
+    callback.Run(*result);
+}
+
 bool UploadDataStream::IsEOF() const {
   const std::vector<UploadData::Element>& elements = *upload_data_->elements();