net: Make UploadData::GetContentLength() asynchronous.

net/base/upload_data.cc

Index: net/base/upload_data.cc
diff --git a/net/base/upload_data.cc b/net/base/upload_data.cc
index 424939e4238a20e10ccb4382579c26242ecfe5af..c3630087a43bdf743a672b4161dee020f579eca8 100644
--- a/net/base/upload_data.cc
+++ b/net/base/upload_data.cc
@@ -6,15 +6,29 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/threading/worker_pool.h"
+#include "base/tracked_objects.h"
 #include "net/base/file_stream.h"
 #include "net/base/net_errors.h"
 
 namespace net {
 
+namespace {
+
+// Helper function for GetContentLength().
+void OnGetContentLengthComplete(
+    const UploadData::ContentLengthCallback& callback,
+    uint64* content_length) {
+  callback.Run(*content_length);
+}
+
+}  // namespace
+
 UploadData::Element::Element()
     : type_(TYPE_BYTES),
       file_range_offset_(0),
@@ -68,15 +82,8 @@ uint64 UploadData::Element::GetContentLength() {
     return 0;
 
   int64 length = 0;
-
-  {
-    // TODO(tzik):
-    // file_util::GetFileSize may cause blocking IO.
-    // Temporary allow until fix: http://crbug.com/72001.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    if (!file_util::GetFileSize(file_path_, &length))
+  if (!file_util::GetFileSize(file_path_, &length))
       return 0;
-  }
 
   if (file_range_offset_ >= static_cast<uint64>(length))
     return 0;  // range is beyond eof
@@ -96,11 +103,6 @@ FileStream* UploadData::Element::NewFileStreamForReading() {
     return file;
   }
 
-  // TODO(tzik):
-  // FileStream::Open and FileStream::Seek may cause blocking IO.
-  // Temporary allow until fix: http://crbug.com/72001.
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
-
   scoped_ptr<FileStream> file(new FileStream(NULL));
   int64 rv = file->Open(file_path_,
                       base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ);
@@ -166,15 +168,29 @@ void UploadData::set_chunk_callback(ChunkCallback* callback) {
   chunk_callback_ = callback;
 }
 
-uint64 UploadData::GetContentLength() {
-  if (is_chunked_)
-    return 0;
+void UploadData::GetContentLength(const ContentLengthCallback& callback) {
+  uint64* result = new uint64(0);
+  const bool task_is_slow = true;
+  const bool posted = base::WorkerPool::PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&UploadData::DoGetContentLength, this, result),

[willchan no longer on Chromium, 2012/02/08 20:44:03]

I have to think about this more, but I thought I'd point it out here so you can
think about it too while I am in meetings. This is extending the lifetime of
UploadData. It's possible that all consumers of UploadData are gone and have
released all their references and expect UploadData to go away (and delete the
UploadData::Elements and their FileStreams and what not). I think it's safe to
extend the lifetime a bit (if the WorkerPool task is still outstanding), but
thought I should point out that there's a change in lifetime behavior here that
has some implication.

I pondered suggesting using a base::WeakPtrFactory to avoid extending the
lifetime, but I think it's problematic for us to delete UploadData's Elements
while the DoGetContentLength() operation is underway.

Anyway, some food for thought. I'll think more about it later.

[satorux1, 2012/02/08 21:02:19]

Thank you for sharing the thought.

I think one way to do this without extending the life time is to  compute length
of non-file payloads now, and then compute length of file payloads in the
background. For the latter, we just need file names which can easily be passed
to a callback without referencing 'this'.

However, UploadData::Element::GetContentLength() does some caching not to reopen
the same file twice, which makes it difficult for us to go with the
"non-life-extending" idea. I'd argue that caching here is a premature
optimization and we can remove this behavior. Caching makes code a bit hard to
follow and is one of reasons why a memory leak was introduced
(crrev.com/120952).

+      base::Bind(&OnGetContentLengthComplete, callback, base::Owned(result)),
+      task_is_slow);
+  DCHECK(posted);
+}
 
-  uint64 len = 0;
-  std::vector<Element>::iterator it = elements_.begin();
-  for (; it != elements_.end(); ++it)
-    len += (*it).GetContentLength();
-  return len;
+uint64 UploadData::GetContentLengthSyncHack() {
+  // Temporarily allow until fix: http://crbug.com/72001.
+  base::ThreadRestrictions::ScopedAllowIO allow_io;
+  uint64 content_length = 0;
+  DoGetContentLength(&content_length);
+  return content_length;
+}
+
+uint64 UploadData::GetContentLengthSync() {
+  uint64 content_length = 0;
+  DoGetContentLength(&content_length);
+  return content_length;
 }
 
 bool UploadData::IsInMemory() const {
@@ -197,6 +213,16 @@ void UploadData::SetElements(const std::vector<Element>& elements) {
   elements_ = elements;
 }
 
+void UploadData::DoGetContentLength(uint64* content_length) {
+  *content_length = 0;
+
+  if (is_chunked_)
+    return;
+
+  for (size_t i = 0; i < elements_.size(); ++i)
+    *content_length += elements_[i].GetContentLength();
+}
+
 UploadData::~UploadData() {
 }