In BlobURLRequestJob, open files asynchronously to avoid blocking the IO thread

(and tripping thread restriction asserts).

The bug was found while trying to get FileWriter ui_tests to pass
(see http://codereview.chromium.org/6609040/).

This change also changes all callers to pass in a file_thread_proxy so
the class can assume it's there and pass it to FileUtilProxy.

Finally, it adds a BlobURLRequestJob test case that reads a file larger
than the buffer size, to better exercise the job's behavior when ReadRawData()
is called multiple times.

BUG=75548
TEST=test_shell_tests,ui_tests

Review URL: http://codereview.chromium.org/6612051

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

3 files changed, 124 insertions, 72 deletions

diff --git a/webkit/blob/blob_url_request_job.cc b/webkit/blob/blob_url_request_job.cc
index 58e96c6..0de56ac 100644
--- a/webkit/blob/blob_url_request_job.cc
+++ b/webkit/blob/blob_url_request_job.cc
@@ -12,6 +12,7 @@
 #include "base/message_loop_proxy.h"
 #include "base/string_number_conversions.h"
 #include "base/threading/thread_restrictions.h"
+#include "net/base/file_stream.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_request_headers.h"
@@ -41,6 +42,10 @@ static const char kHTTPRequestedRangeNotSatisfiableText[] =
     "Requested Range Not Satisfiable";
 static const char kHTTPInternalErrorText[] = "Internal Server Error";
 
+static const int kFileOpenFlags = base::PLATFORM_FILE_OPEN |
+                                  base::PLATFORM_FILE_READ |
+                                  base::PLATFORM_FILE_ASYNC;
+
 BlobURLRequestJob::BlobURLRequestJob(
     net::URLRequest* request,
     BlobData* blob_data,
@@ -58,13 +63,18 @@ BlobURLRequestJob::BlobURLRequestJob(
       read_buf_offset_(0),
       read_buf_size_(0),
       read_buf_remaining_bytes_(0),
+      bytes_to_read_(0),
       error_(false),
       headers_set_(false),
       byte_range_set_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
+  DCHECK(file_thread_proxy_);
 }
 
 BlobURLRequestJob::~BlobURLRequestJob() {
+  // FileStream's destructor won't close it for us because we passed in our own
+  // file handle.
+  CloseStream();
 }
 
 void BlobURLRequestJob::Start() {
@@ -90,8 +100,15 @@ void BlobURLRequestJob::DidStart() {
   CountSize();
 }
 
+void BlobURLRequestJob::CloseStream() {
+  if (stream_ != NULL) {
+    stream_->Close();
+    stream_.reset(NULL);
+  }
+}
+
 void BlobURLRequestJob::Kill() {
-  stream_.Close();
+  CloseStream();
 
   net::URLRequestJob::Kill();
   callback_factory_.RevokeAll();
@@ -99,28 +116,10 @@ void BlobURLRequestJob::Kill() {
 }
 
 void BlobURLRequestJob::ResolveFile(const FilePath& file_path) {
-  // If the file thread proxy is provided, we can use it get the file info.
-  if (file_thread_proxy_) {
-    base::FileUtilProxy::GetFileInfo(
-          file_thread_proxy_,
-          file_path,
-          callback_factory_.NewCallback(&BlobURLRequestJob::DidResolve));
-    return;
-  }
-
-  // Otherwise, we use current thread, i.e. IO thread, as this is the case when
-  // we run the unittest or test shell.
-  // TODO(jianli): Consider using the proxy of current thread.
-  base::PlatformFileInfo file_info;
-  bool exists = file_util::GetFileInfo(file_path, &file_info);
-
-  // Continue asynchronously.
-  MessageLoop::current()->PostTask(
-      FROM_HERE,
-      method_factory_.NewRunnableMethod(
-          &BlobURLRequestJob::DidResolve,
-          exists ? base::PLATFORM_FILE_OK : base::PLATFORM_FILE_ERROR_NOT_FOUND,
-          file_info));
+  base::FileUtilProxy::GetFileInfo(
+        file_thread_proxy_,
+        file_path,
+        callback_factory_.NewCallback(&BlobURLRequestJob::DidResolve));
 }
 
 void BlobURLRequestJob::DidResolve(base::PlatformFileError rv,
@@ -255,6 +254,17 @@ bool BlobURLRequestJob::ReadLoop(int* bytes_read) {
   return true;
 }
 
+int BlobURLRequestJob::ComputeBytesToRead() const {
+  int64 current_item_remaining_bytes =
+      item_length_list_[item_index_] - current_item_offset_;
+  int bytes_to_read = (read_buf_remaining_bytes_ > current_item_remaining_bytes)
+      ? static_cast<int>(current_item_remaining_bytes)
+      : read_buf_remaining_bytes_;
+  if (bytes_to_read > remaining_bytes_)
+    bytes_to_read = static_cast<int>(remaining_bytes_);
+  return bytes_to_read;
+}
+
 bool BlobURLRequestJob::ReadItem() {
   // Are we done with reading all the blob data?
   if (remaining_bytes_ == 0)
@@ -267,77 +277,85 @@ bool BlobURLRequestJob::ReadItem() {
     return false;
   }
 
-  const BlobData::Item& item = blob_data_->items().at(item_index_);
-
   // Compute the bytes to read for current item.
-  int64 current_item_remaining_bytes =
-      item_length_list_[item_index_] - current_item_offset_;
-  int bytes_to_read = (read_buf_remaining_bytes_ > current_item_remaining_bytes)
-      ? static_cast<int>(current_item_remaining_bytes)
-      : read_buf_remaining_bytes_;
-  if (bytes_to_read > remaining_bytes_)
-    bytes_to_read = static_cast<int>(remaining_bytes_);
+  bytes_to_read_ = ComputeBytesToRead();
 
   // If nothing to read for current item, advance to next item.
-  if (bytes_to_read == 0) {
+  if (bytes_to_read_ == 0) {
     AdvanceItem();
     return ReadItem();
   }
 
   // Do the reading.
+  const BlobData::Item& item = blob_data_->items().at(item_index_);
   switch (item.type()) {
     case BlobData::TYPE_DATA:
-      return ReadBytes(item, bytes_to_read);
+      return ReadBytes(item);
     case BlobData::TYPE_FILE:
-      return ReadFile(item, bytes_to_read);
+      return DispatchReadFile(item);
     default:
       DCHECK(false);
       return false;
   }
 }
 
-bool BlobURLRequestJob::ReadBytes(const BlobData::Item& item,
-                                  int bytes_to_read) {
-  DCHECK(read_buf_remaining_bytes_ >= bytes_to_read);
+bool BlobURLRequestJob::ReadBytes(const BlobData::Item& item) {
+  DCHECK(read_buf_remaining_bytes_ >= bytes_to_read_);
 
   memcpy(read_buf_->data() + read_buf_offset_,
          &item.data().at(0) + item.offset() + current_item_offset_,
-         bytes_to_read);
+         bytes_to_read_);
 
-  AdvanceBytesRead(bytes_to_read);
+  AdvanceBytesRead(bytes_to_read_);
   return true;
 }
 
-bool BlobURLRequestJob::ReadFile(const BlobData::Item& item,
-                                 int bytes_to_read) {
-  DCHECK(read_buf_remaining_bytes_ >= bytes_to_read);
+bool BlobURLRequestJob::DispatchReadFile(const BlobData::Item& item) {
+  // If the stream already exists, keep reading from it.
+  if (stream_ != NULL)
+    return ReadFile(item);
 
-  // Open the file if not yet.
-  if (!stream_.IsOpen()) {
-    // stream_.Open() and stream_.Seek() block the IO thread.
-    // See http://crbug.com/75548.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    int rv = stream_.Open(item.file_path(), base::PLATFORM_FILE_OPEN |
-        base::PLATFORM_FILE_READ | base::PLATFORM_FILE_ASYNC);
-    if (rv != net::OK) {
-      NotifyFailure(net::ERR_FAILED);
-      return false;
-    }
+  base::FileUtilProxy::CreateOrOpen(
+      file_thread_proxy_, item.file_path(), kFileOpenFlags,
+      callback_factory_.NewCallback(&BlobURLRequestJob::DidOpen));
+  SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
+  return false;
+}
+
+void BlobURLRequestJob::DidOpen(base::PlatformFileError rv,
+                                base::PassPlatformFile file,
+                                bool created) {
+  if (rv != base::PLATFORM_FILE_OK) {
+    NotifyFailure(net::ERR_FAILED);
+    return;
+  }
+
+  DCHECK(!stream_.get());
+  stream_.reset(new net::FileStream(file.ReleaseValue(), kFileOpenFlags));
 
-    // Seek the file if needed.
+  const BlobData::Item& item = blob_data_->items().at(item_index_);
+  {
+    // stream_.Seek() blocks the IO thread, see http://crbug.com/75548.
+    base::ThreadRestrictions::ScopedAllowIO allow_io;
     int64 offset = current_item_offset_ + static_cast<int64>(item.offset());
-    if (offset > 0) {
-      if (offset != stream_.Seek(net::FROM_BEGIN, offset)) {
-        NotifyFailure(net::ERR_FAILED);
-        return false;
-      }
+    if (offset > 0 && offset != stream_->Seek(net::FROM_BEGIN, offset)) {
+      NotifyFailure(net::ERR_FAILED);
+      return;
     }
   }
 
+  ReadFile(item);
+}
+
+bool BlobURLRequestJob::ReadFile(const BlobData::Item& item) {
+  DCHECK(stream_.get());
+  DCHECK(stream_->IsOpen());
+  DCHECK(read_buf_remaining_bytes_ >= bytes_to_read_);
+
   // Start the asynchronous reading.
-  int rv = stream_.Read(read_buf_->data() + read_buf_offset_,
-                        bytes_to_read,
-                        &io_callback_);
+  int rv = stream_->Read(read_buf_->data() + read_buf_offset_,
+                         bytes_to_read_,
+                         &io_callback_);
 
   // If I/O pending error is returned, we just need to wait.
   if (rv == net::ERR_IO_PENDING) {
@@ -352,7 +370,11 @@ bool BlobURLRequestJob::ReadFile(const BlobData::Item& item,
   }
 
   // Otherwise, data is immediately available.
-  AdvanceBytesRead(rv);
+  if (GetStatus().is_io_pending())
+    DidRead(rv);
+  else
+    AdvanceBytesRead(rv);
+
   return true;
 }
 
@@ -380,8 +402,7 @@ void BlobURLRequestJob::DidRead(int result) {
 
 void BlobURLRequestJob::AdvanceItem() {
   // Close the stream if the current item is a file.
-  if (stream_.IsOpen())
-    stream_.Close();
+  CloseStream();
 
   // Advance to the next item.
   item_index_++;
diff --git a/webkit/blob/blob_url_request_job.h b/webkit/blob/blob_url_request_job.h
index 2dec522..c5669bf 100644
--- a/webkit/blob/blob_url_request_job.h
+++ b/webkit/blob/blob_url_request_job.h
@@ -11,7 +11,6 @@
 #include "base/scoped_ptr.h"
 #include "base/task.h"
 #include "net/base/completion_callback.h"
-#include "net/base/file_stream.h"
 #include "net/http/http_byte_range.h"
 #include "net/url_request/url_request_job.h"
 #include "webkit/blob/blob_data.h"
@@ -21,6 +20,10 @@ class MessageLoopProxy;
 struct PlatformFileInfo;
 }
 
+namespace net {
+class FileStream;
+}
+
 namespace webkit_blob {
 
 // A request job that handles reading blob URLs.
@@ -41,15 +44,18 @@ class BlobURLRequestJob : public net::URLRequestJob {
   virtual void SetExtraRequestHeaders(const net::HttpRequestHeaders& headers);
 
  private:
+  void CloseStream();
   void ResolveFile(const FilePath& file_path);
   void CountSize();
   void Seek(int64 offset);
   void AdvanceItem();
   void AdvanceBytesRead(int result);
+  int ComputeBytesToRead() const;
   bool ReadLoop(int* bytes_read);
   bool ReadItem();
-  bool ReadBytes(const BlobData::Item& item, int bytes_to_read);
-  bool ReadFile(const BlobData::Item& item, int bytes_to_read);
+  bool ReadBytes(const BlobData::Item& item);
+  bool DispatchReadFile(const BlobData::Item& item);
+  bool ReadFile(const BlobData::Item& item);
   void HeadersCompleted(int status_code, const std::string& status_txt);
   int ReadCompleted();
   void NotifySuccess();
@@ -58,6 +64,9 @@ class BlobURLRequestJob : public net::URLRequestJob {
   void DidStart();
   void DidResolve(base::PlatformFileError rv,
                   const base::PlatformFileInfo& file_info);
+  void DidOpen(base::PlatformFileError rv,
+               base::PassPlatformFile file,
+               bool created);
   void DidRead(int result);
 
   base::ScopedCallbackFactory<BlobURLRequestJob> callback_factory_;
@@ -65,7 +74,7 @@ class BlobURLRequestJob : public net::URLRequestJob {
   scoped_refptr<base::MessageLoopProxy> file_thread_proxy_;
   net::CompletionCallbackImpl<BlobURLRequestJob> io_callback_;
   std::vector<int64> item_length_list_;
-  net::FileStream stream_;
+  scoped_ptr<net::FileStream> stream_;
   size_t item_index_;
   int64 total_size_;
   int64 current_item_offset_;
@@ -74,6 +83,7 @@ class BlobURLRequestJob : public net::URLRequestJob {
   int read_buf_offset_;
   int read_buf_size_;
   int read_buf_remaining_bytes_;
+  int bytes_to_read_;
   bool error_;
   bool headers_set_;
   bool byte_range_set_;
diff --git a/webkit/blob/blob_url_request_job_unittest.cc b/webkit/blob/blob_url_request_job_unittest.cc
index 1d7cd6c..5d318a3 100644
--- a/webkit/blob/blob_url_request_job_unittest.cc
+++ b/webkit/blob/blob_url_request_job_unittest.cc
@@ -7,6 +7,7 @@
 
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/message_loop_proxy.h"
 #include "base/scoped_temp_dir.h"
 #include "base/task.h"
 #include "base/threading/thread.h"
@@ -249,8 +250,10 @@ class BlobURLRequestJobTest : public testing::Test {
     request_.reset(new net::URLRequest(GURL("blob:blah"),
                                        url_request_delegate_.get()));
     request_->set_method(method);
-    blob_url_request_job_ = new BlobURLRequestJob(request_.get(),
-                                                  blob_data, NULL);
+    blob_url_request_job_ = new BlobURLRequestJob(
+        request_.get(),
+        blob_data,
+        base::MessageLoopProxy::CreateForCurrentThread());
 
     // Start the request.
     if (!extra_headers.IsEmpty())
@@ -283,6 +286,20 @@ class BlobURLRequestJobTest : public testing::Test {
     TestSuccessRequest(blob_data, kTestFileData1);
   }
 
+  void TestGetLargeFileRequest() {
+    scoped_refptr<BlobData> blob_data(new BlobData());
+    FilePath large_temp_file = temp_dir_.path().AppendASCII("LargeBlob.dat");
+    std::string large_data;
+    large_data.reserve(kBufferSize * 5);
+    for (int i = 0; i < kBufferSize * 5; ++i)
+      large_data.append(1, static_cast<char>(i % 256));
+    ASSERT_EQ(static_cast<int>(large_data.size()),
+              file_util::WriteFile(large_temp_file, large_data.data(),
+                                   large_data.size()));
+    blob_data->AppendFile(large_temp_file, 0, -1, base::Time());
+    TestSuccessRequest(blob_data, large_data);
+  }
+
   void TestGetNonExistentFileRequest() {
     FilePath non_existent_file =
         temp_file1_.InsertBeforeExtension(FILE_PATH_LITERAL("-na"));
@@ -409,6 +426,10 @@ TEST_F(BlobURLRequestJobTest, TestGetSimpleFileRequest) {
   RunTestOnIOThread(&BlobURLRequestJobTest::TestGetSimpleFileRequest);
 }
 
+TEST_F(BlobURLRequestJobTest, TestGetLargeFileRequest) {
+  RunTestOnIOThread(&BlobURLRequestJobTest::TestGetLargeFileRequest);
+}
+
 TEST_F(BlobURLRequestJobTest, TestGetSlicedDataRequest) {
   RunTestOnIOThread(&BlobURLRequestJobTest::TestGetSlicedDataRequest);
 }