Avoid disk accesses on the wrong thread in URLRequestFileJob

net/url_request/url_request_file_job.cc

Index: net/url_request/url_request_file_job.cc
===================================================================
--- net/url_request/url_request_file_job.cc	(revision 145483)
+++ net/url_request/url_request_file_job.cc	(working copy)
@@ -42,54 +42,17 @@
 
 namespace net {
 
-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,
                                      const FilePath& file_path)
     : URLRequestJob(request, request->context()->network_delegate()),
       file_path_(file_path),
       stream_(NULL),
-      is_directory_(false),
-      remaining_bytes_(0) {
+      remaining_bytes_(0),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+  meta_info_.file_exists_ = false;
+  meta_info_.file_size_ = -1;
+  meta_info_.is_directory_ = false;
+  meta_info_.mime_type_result_ = false;

[wtc, 2012/07/13 22:38:55]

Please define a default constructor for the FileMetaInfo
structure.  Then you don't need to manually initialize its
members here.

 }
 
 // static
@@ -119,25 +82,21 @@
 }
 
 void URLRequestFileJob::Start() {
-  DCHECK(!async_resolver_);
-  async_resolver_ = new AsyncResolver(this);
-  base::WorkerPool::PostTask(
+  FileMetaInfo* meta_info = new FileMetaInfo;

[wtc, 2012/07/13 22:38:55]

Will meta_info be deleted by the WorkerPool because of the
use of base::Owned(meta_info) on line 92?

[pivanof, 2012/07/13 23:51:21]

Yes, that was my intention.

+  base::WorkerPool::PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&AsyncResolver::Resolve, async_resolver_.get(), file_path_),
+      base::Bind(&URLRequestFileJob::FetchMetaInfo, file_path_,
+                 base::Unretained(meta_info)),
+      base::Bind(&URLRequestFileJob::DidFetchMetaInfo,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(meta_info)),
       true);
 }
 
 void URLRequestFileJob::Kill() {
-  // URL requests should not block on the disk!
-  //   http://code.google.com/p/chromium/issues/detail?id=59849
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
-  stream_.CloseSync();
+  stream_.CloseAndCancelAsync();
+  weak_ptr_factory_.InvalidateWeakPtrs();
 
-  if (async_resolver_) {
-    async_resolver_->Cancel();
-    async_resolver_ = NULL;
-  }
-
   URLRequestJob::Kill();
 }
 
@@ -159,7 +118,7 @@
 
   int rv = stream_.Read(dest, dest_size,
                         base::Bind(&URLRequestFileJob::DidRead,
-                                   base::Unretained(this)));
+                                   weak_ptr_factory_.GetWeakPtr()));
   if (rv >= 0) {
     // Data is immediately available.
     *bytes_read = rv;
@@ -179,7 +138,7 @@
 
 bool URLRequestFileJob::IsRedirectResponse(GURL* location,
                                            int* http_status_code) {
-  if (is_directory_) {
+  if (meta_info_.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();
@@ -221,12 +180,9 @@
 }
 
 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_);
-  return GetMimeTypeFromFile(file_path_, mime_type);
+  *mime_type = meta_info_.mime_type_;

[wtc, 2012/07/13 22:38:55]

Nit: it seems that we only need to set *mime_type if
meta_info_.mime_type_result_ is true.

[pivanof, 2012/07/13 23:51:21]

I've also thought like that. But just in case of my insufficient knowledge I
kept the functionality as it was before. Do you think I should change that?

+  return meta_info_.mime_type_result_;
 }
 
 void URLRequestFileJob::SetExtraRequestHeaders(
@@ -263,20 +219,23 @@
 }
 
 URLRequestFileJob::~URLRequestFileJob() {
-  DCHECK(!async_resolver_);
 }
 
-void URLRequestFileJob::DidResolve(
-    bool exists, const base::PlatformFileInfo& file_info) {
-  async_resolver_ = NULL;
+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);
+  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_);
+}
 
-  // 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,
@@ -285,25 +244,32 @@
   // 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 (!exists) {
-    rv = ERR_FILE_NOT_FOUND;
-  } else if (!is_directory_) {
-    // 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);
+  if (!meta_info_.file_exists_) {
+    DidOpen(ERR_FILE_NOT_FOUND);
+    return;
   }
+  if (meta_info_.is_directory_) {
+    DidOpen(OK);
+    return;
+  }
 
-  if (rv != OK) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
+  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));
     return;
   }
 
-  if (!byte_range_.ComputeBounds(file_info.size)) {
+  if (!byte_range_.ComputeBounds(meta_info_.file_size_)) {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
@@ -313,21 +279,24 @@
                      byte_range_.first_byte_position() + 1;
   DCHECK_GE(remaining_bytes_, 0);
 
-  // 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;
-    }
+  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)
+      DidSeek(-1);

[wtc, 2012/07/13 22:38:55]

BUG?: Why do we pass -1 to DidSeek here?  It seems that
we should pass rv instead.

[pivanof, 2012/07/13 23:51:21]

-1 is to force DidSeek to go to the path of error processing. Passing rv could
make sense in two cases:
 - Seek() could finish synchronously and return the resulting position;
 - DidSeek() paid attention to input value in case of an error.
Neither of these cases can actually happen although abstractly thinking maybe
asynchronous Seek() finishing synchronously is theoretically possible. Do we
want to make code safe for this theoretical possibility?

[pivanof, 2012/07/14 02:28:45]

Just noticed that comment to FileStream::Seek() says "Upon success,
ERR_IO_PENDING is returned". So Seek() cannot finish synchronously and if its
result is not ERR_IO_PENDING then it was unsuccessful. So -1 here cannot be a
bug.

+  } else {
+    DidSeek(byte_range_.first_byte_position());

[wtc, 2012/07/13 22:38:55]

This DidSeek call is a little hard to understand because
we didn't call Seek in this case.  (I figured out why it's
necessary.)

[pivanof, 2012/07/13 23:51:21]

Is adding a comment explaining that would be good enough?

   }
+}
 
+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_);
   NotifyHeadersComplete();
 }