Revert 168626 - Avoid disk accesses on the wrong thread in URLRequestFileJob

while opening and positioning in the file.


Patch from Pavel Ivanov <paivanof@gmail.com>

BUG=59849,114783
TEST=net_unittests

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

TBR=paivanof@gmail.com
Review URL: https://codereview.chromium.org/11418080

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

3 files changed, 103 insertions, 133 deletions

diff --git a/net/url_request/url_request_file_job.cc b/net/url_request/url_request_file_job.cc
index 0c703e1..c60b313 100644
--- a/net/url_request/url_request_file_job.cc
+++ b/net/url_request/url_request_file_job.cc
@@ -45,21 +45,54 @@
 
 namespace net {
 
-URLRequestFileJob::FileMetaInfo::FileMetaInfo()
-    : file_size(0),
-      mime_type_result(false),
-      file_exists(false),
-      is_directory(false) {
-}
+class URLRequestFileJob::AsyncResolver
+    : public base::RefCountedThreadSafe<URLRequestFileJob::AsyncResolver> {
+ public:
+  explicit AsyncResolver(URLRequestFileJob* owner)
+      : owner_(owner), owner_loop_(MessageLoop::current()) {
+  }
+
+  void Resolve(const FilePath& file_path) {
+    base::PlatformFileInfo file_info;
+    bool exists = file_util::GetFileInfo(file_path, &file_info);
+    base::AutoLock locked(lock_);
+    if (owner_loop_) {
+      owner_loop_->PostTask(
+          FROM_HERE,
+          base::Bind(&AsyncResolver::ReturnResults, this, exists, file_info));
+    }
+  }
+
+  void Cancel() {
+    owner_ = NULL;
+
+    base::AutoLock locked(lock_);
+    owner_loop_ = NULL;
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<URLRequestFileJob::AsyncResolver>;
+
+  ~AsyncResolver() {}
+
+  void ReturnResults(bool exists, const base::PlatformFileInfo& file_info) {
+    if (owner_)
+      owner_->DidResolve(exists, file_info);
+  }
+
+  URLRequestFileJob* owner_;
+
+  base::Lock lock_;
+  MessageLoop* owner_loop_;
+};
 
 URLRequestFileJob::URLRequestFileJob(URLRequest* request,
                                      NetworkDelegate* network_delegate,
                                      const FilePath& file_path)
     : URLRequestJob(request, network_delegate),
       file_path_(file_path),
-      stream_(new FileStream(NULL)),
-      remaining_bytes_(0),
-      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      is_directory_(false),
+      remaining_bytes_(0) {
 }
 
 // static
@@ -91,20 +124,21 @@ URLRequestJob* URLRequestFileJob::Factory(URLRequest* request,
 }
 
 void URLRequestFileJob::Start() {
-  FileMetaInfo* meta_info = new FileMetaInfo();
-  base::WorkerPool::PostTaskAndReply(
+  DCHECK(!async_resolver_);
+  async_resolver_ = new AsyncResolver(this);
+  base::WorkerPool::PostTask(
       FROM_HERE,
-      base::Bind(&URLRequestFileJob::FetchMetaInfo, file_path_,
-                 base::Unretained(meta_info)),
-      base::Bind(&URLRequestFileJob::DidFetchMetaInfo,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 base::Owned(meta_info)),
+      base::Bind(&AsyncResolver::Resolve, async_resolver_.get(), file_path_),
       true);
 }
 
 void URLRequestFileJob::Kill() {
   stream_.reset();
-  weak_ptr_factory_.InvalidateWeakPtrs();
+
+  if (async_resolver_) {
+    async_resolver_->Cancel();
+    async_resolver_ = NULL;
+  }
 
   URLRequestJob::Kill();
 }
@@ -127,7 +161,7 @@ bool URLRequestFileJob::ReadRawData(IOBuffer* dest, int dest_size,
 
   int rv = stream_->Read(dest, dest_size,
                          base::Bind(&URLRequestFileJob::DidRead,
-                                    weak_ptr_factory_.GetWeakPtr()));
+                                    base::Unretained(this)));
   if (rv >= 0) {
     // Data is immediately available.
     *bytes_read = rv;
@@ -147,7 +181,7 @@ bool URLRequestFileJob::ReadRawData(IOBuffer* dest, int dest_size,
 
 bool URLRequestFileJob::IsRedirectResponse(GURL* location,
                                            int* http_status_code) {
-  if (meta_info_.is_directory) {
+  if (is_directory_) {
     // This happens when we discovered the file is a directory, so needs a
     // slash at the end of the path.
     std::string new_path = request_->url().path();
@@ -189,12 +223,12 @@ Filter* URLRequestFileJob::SetupFilter() const {
 }
 
 bool URLRequestFileJob::GetMimeType(std::string* mime_type) const {
+  // URL requests should not block on the disk!  On Windows this goes to the
+  // registry.
+  //   http://code.google.com/p/chromium/issues/detail?id=59849
+  base::ThreadRestrictions::ScopedAllowIO allow_io;
   DCHECK(request_);
-  if (meta_info_.mime_type_result) {
-    *mime_type = meta_info_.mime_type;
-    return true;
-  }
-  return false;
+  return GetMimeTypeFromFile(file_path_, mime_type);
 }
 
 void URLRequestFileJob::SetExtraRequestHeaders(
@@ -219,25 +253,20 @@ void URLRequestFileJob::SetExtraRequestHeaders(
 }
 
 URLRequestFileJob::~URLRequestFileJob() {
+  DCHECK(!async_resolver_);
 }
 
-void URLRequestFileJob::FetchMetaInfo(const FilePath& file_path,
-                                      FileMetaInfo* meta_info) {
-  base::PlatformFileInfo platform_info;
-  meta_info->file_exists = file_util::GetFileInfo(file_path, &platform_info);
-  if (meta_info->file_exists) {
-    meta_info->file_size = platform_info.size;
-    meta_info->is_directory = platform_info.is_directory;
-  }
-  // On Windows GetMimeTypeFromFile() goes to the registry. Thus it should be
-  // done in WorkerPool.
-  meta_info->mime_type_result = GetMimeTypeFromFile(file_path,
-                                                    &meta_info->mime_type);
-}
+void URLRequestFileJob::DidResolve(
+    bool exists, const base::PlatformFileInfo& file_info) {
+  async_resolver_ = NULL;
+
+  // We may have been orphaned...
+  if (!request_)
+    return;
 
-void URLRequestFileJob::DidFetchMetaInfo(const FileMetaInfo* meta_info) {
-  meta_info_ = *meta_info;
+  is_directory_ = file_info.is_directory;
 
+  int rv = OK;
   // We use URLRequestFileJob to handle files as well as directories without
   // trailing slash.
   // If a directory does not exist, we return ERR_FILE_NOT_FOUND. Otherwise,
@@ -246,32 +275,27 @@ void URLRequestFileJob::DidFetchMetaInfo(const FileMetaInfo* meta_info) {
   // However, Windows resolves "\" to "C:\", thus reports it as existent.
   // So what happens is we append it with trailing slash and redirect it to
   // FileDirJob where it is resolved as invalid.
-  if (!meta_info_.file_exists) {
-    DidOpen(ERR_FILE_NOT_FOUND);
-    return;
-  }
-  if (meta_info_.is_directory) {
-    DidOpen(OK);
-    return;
+  if (!exists) {
+    rv = ERR_FILE_NOT_FOUND;
+  } else if (!is_directory_) {
+    stream_.reset(new FileStream(NULL));
+
+    // URL requests should not block on the disk!
+    //   http://code.google.com/p/chromium/issues/detail?id=59849
+    base::ThreadRestrictions::ScopedAllowIO allow_io;
+
+    int flags = base::PLATFORM_FILE_OPEN |
+                base::PLATFORM_FILE_READ |
+                base::PLATFORM_FILE_ASYNC;
+    rv = stream_->OpenSync(file_path_, flags);
   }
 
-  int flags = base::PLATFORM_FILE_OPEN |
-              base::PLATFORM_FILE_READ |
-              base::PLATFORM_FILE_ASYNC;
-  int rv = stream_->Open(file_path_, flags,
-                         base::Bind(&URLRequestFileJob::DidOpen,
-                                    weak_ptr_factory_.GetWeakPtr()));
-  if (rv != ERR_IO_PENDING)
-    DidOpen(rv);
-}
-
-void URLRequestFileJob::DidOpen(int result) {
-  if (result != OK) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
+  if (rv != OK) {
+    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
     return;
   }
 
-  if (!byte_range_.ComputeBounds(meta_info_.file_size)) {
+  if (!byte_range_.ComputeBounds(file_info.size)) {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
@@ -281,28 +305,19 @@ void URLRequestFileJob::DidOpen(int result) {
                      byte_range_.first_byte_position() + 1;
   DCHECK_GE(remaining_bytes_, 0);
 
-  if (remaining_bytes_ > 0 && byte_range_.first_byte_position() != 0) {
-    int rv = stream_->Seek(FROM_BEGIN, byte_range_.first_byte_position(),
-                           base::Bind(&URLRequestFileJob::DidSeek,
-                                      weak_ptr_factory_.GetWeakPtr()));
-    if (rv != ERR_IO_PENDING) {
-      // stream_->Seek() failed, so pass an intentionally erroneous value
-      // into DidSeek().
-      DidSeek(-1);
+  // URL requests should not block on the disk!
+  //   http://code.google.com/p/chromium/issues/detail?id=59849
+  {
+    base::ThreadRestrictions::ScopedAllowIO allow_io;
+    // Do the seek at the beginning of the request.
+    if (remaining_bytes_ > 0 &&
+        byte_range_.first_byte_position() != 0 &&
+        byte_range_.first_byte_position() !=
+        stream_->SeekSync(FROM_BEGIN, byte_range_.first_byte_position())) {
+      NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
+                                  ERR_REQUEST_RANGE_NOT_SATISFIABLE));
+      return;
     }
-  } else {
-    // We didn't need to call stream_->Seek() at all, so we pass to DidSeek()
-    // the value that would mean seek success. This way we skip the code
-    // handling seek failure.
-    DidSeek(byte_range_.first_byte_position());
-  }
-}
-
-void URLRequestFileJob::DidSeek(int64 result) {
-  if (result != byte_range_.first_byte_position()) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
-                                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
-    return;
   }
 
   set_expected_content_size(remaining_bytes_);
diff --git a/net/url_request/url_request_file_job.h b/net/url_request/url_request_file_job.h
index 9c2199c..15c7d62 100644
--- a/net/url_request/url_request_file_job.h
+++ b/net/url_request/url_request_file_job.h
@@ -9,7 +9,6 @@
 #include <vector>
 
 #include "base/file_path.h"
-#include "base/memory/weak_ptr.h"
 #include "net/base/net_export.h"
 #include "net/http/http_byte_range.h"
 #include "net/url_request/url_request.h"
@@ -55,49 +54,23 @@ class NET_EXPORT URLRequestFileJob : public URLRequestJob {
   FilePath file_path_;
 
  private:
-  // Meta information about the file. It's used as a member in the
-  // URLRequestFileJob and also passed between threads because disk access is
-  // necessary to obtain it.
-  struct FileMetaInfo {
-    FileMetaInfo();
-
-    // Size of the file.
-    int64 file_size;
-    // Mime type associated with the file.
-    std::string mime_type;
-    // Result returned from GetMimeTypeFromFile(), i.e. flag showing whether
-    // obtaining of the mime type was successful.
-    bool mime_type_result;
-    // Flag showing whether the file exists.
-    bool file_exists;
-    // Flag showing whether the file name actually refers to a directory.
-    bool is_directory;
-  };
-
-  // Fetches file info on a background thread.
-  static void FetchMetaInfo(const FilePath& file_path,
-                            FileMetaInfo* meta_info);
-
   // Callback after fetching file info on a background thread.
-  void DidFetchMetaInfo(const FileMetaInfo* meta_info);
-
-  // Callback after opening file on a background thread.
-  void DidOpen(int result);
-
-  // Callback after seeking to the beginning of |byte_range_| in the file
-  // on a background thread.
-  void DidSeek(int64 result);
+  void DidResolve(bool exists, const base::PlatformFileInfo& file_info);
 
   // Callback after data is asynchronously read from the file.
   void DidRead(int result);
 
   scoped_ptr<FileStream> stream_;
-  FileMetaInfo meta_info_;
+  bool is_directory_;
 
   HttpByteRange byte_range_;
   int64 remaining_bytes_;
 
-  base::WeakPtrFactory<URLRequestFileJob> weak_ptr_factory_;
+  // The initial file metadata is fetched on a background thread.
+  // AsyncResolver runs that task.
+  class AsyncResolver;
+  friend class AsyncResolver;
+  scoped_refptr<AsyncResolver> async_resolver_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestFileJob);
 };
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index 57198c5..5486a90 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -563,24 +563,6 @@ TEST_F(URLRequestTest, FileTest) {
   }
 }
 
-TEST_F(URLRequestTest, FileTestCancel) {
-  FilePath app_path;
-  PathService::Get(base::FILE_EXE, &app_path);
-  GURL app_url = FilePathToFileURL(app_path);
-
-  TestDelegate d;
-  {
-    URLRequest r(app_url, &d, &default_context_);
-
-    r.Start();
-    EXPECT_TRUE(r.is_pending());
-    r.Cancel();
-  }
-  // Async cancelation should be safe even when URLRequest has been already
-  // destroyed.
-  MessageLoop::current()->RunUntilIdle();
-}
-
 TEST_F(URLRequestTest, FileTestFullSpecifiedRange) {
   const size_t buffer_size = 4000;
   scoped_array<char> buffer(new char[buffer_size]);