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,32 @@
 
 namespace net {
 
-class URLRequestFileJob::AsyncResolver
-    : public base::RefCountedThreadSafe<URLRequestFileJob::AsyncResolver> {
- public:
-  explicit AsyncResolver(URLRequestFileJob* owner)
-      : owner_(owner), owner_loop_(MessageLoop::current()) {
-  }
+namespace {
 
-  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 ResolveAndGetMimeType(const FilePath& file_path,
+                           bool* exists,
+                           base::PlatformFileInfo* file_info,
+                           bool* mime_type_result,
+                           std::string* mime_type) {
+  *exists = file_util::GetFileInfo(file_path, file_info);
+  // On Windows GetMimeTypeFromFile() goes to the registry. Thus it should be
+  // done in WorkerPool.
+  *mime_type_result = GetMimeTypeFromFile(file_path, mime_type);
+}
 
-  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) {
+      mime_type_result_(false),
+      file_size_(-1),
+      remaining_bytes_(0),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 // static
@@ -119,25 +97,30 @@
 }
 
 void URLRequestFileJob::Start() {
-  DCHECK(!async_resolver_);
-  async_resolver_ = new AsyncResolver(this);
-  base::WorkerPool::PostTask(
+  bool* exists = new bool;
+  base::PlatformFileInfo* file_info = new base::PlatformFileInfo;

[eroman, 2012/07/12 19:13:16]

This feels a bit crowded now with the extra parameters.

I suggest packaging them into a single struct, and then just doing one
allocation. For instance:

struct ResolveAndGetMimeTypeResult {
  bool exists;
  base::PlatformFileInfo file_info;
  bool mime_type_result;
  std::string read_mime_type;
};

You could also use this struct to replace the following member variables:

  bool is_directory_;
  bool mime_type_result_;
  std::string mime_type_;
  int64 file_size_;

Since it would be sufficient to have:
  ResolveAndGetMimeTypeResult resolve_result_;


(Note, I'm not sure if PlatformFileInfo supports default assignment. If it
doesn't might want to have the struct simply encapsulate {is_directory,
file_size} instead).

+  bool* mime_type_result = new bool;
+  std::string* read_mime_type = new std::string;
+  base::WorkerPool::PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&AsyncResolver::Resolve, async_resolver_.get(), file_path_),
+      base::Bind(&ResolveAndGetMimeType, file_path_,
+                 base::Unretained(exists),
+                 base::Unretained(file_info),
+                 base::Unretained(mime_type_result),
+                 base::Unretained(read_mime_type)),
+      base::Bind(&URLRequestFileJob::DidResolveAndMimeType,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Owned(exists),
+                 base::Owned(file_info),
+                 base::Owned(mime_type_result),
+                 base::Owned(read_mime_type)),
       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 +142,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;
@@ -221,12 +204,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 = mime_type_;
+  return mime_type_result_;
 }
 
 void URLRequestFileJob::SetExtraRequestHeaders(
@@ -263,20 +243,18 @@
 }
 
 URLRequestFileJob::~URLRequestFileJob() {
-  DCHECK(!async_resolver_);
 }
 
-void URLRequestFileJob::DidResolve(
-    bool exists, const base::PlatformFileInfo& file_info) {
-  async_resolver_ = NULL;
+void URLRequestFileJob::DidResolveAndMimeType(
+      const bool* exists,
+      const base::PlatformFileInfo* file_info,
+      const bool* mime_type_result,
+      const std::string* read_mime_type) {
+  is_directory_ = file_info->is_directory;
+  file_size_ = file_info->size;
+  mime_type_result_ = *mime_type_result;
+  mime_type_ = *read_mime_type;
 
-  // We may have been orphaned...
-  if (!request_)
-    return;
-
-  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 +263,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 (!(*exists)) {
+    DidOpen(ERR_FILE_NOT_FOUND);
+    return;
   }
+  if (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(file_size_)) {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
@@ -313,21 +298,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);
+  } else {
+    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_);
   NotifyHeadersComplete();
 }