Avoid disk accesses on the wrong thread in URLRequestFileJob

net/url_request/url_request_file_job.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // For loading files, we make use of overlapped i/o to ensure that reading from
 // the filesystem (e.g., a network filesystem) does not block the calling
 // thread.  An alternative approach would be to use a background thread or pool
 // of threads, but it seems better to leverage the operating system's ability
 // to do background file reads for us.
 //
@@ skipping 24 matching lines @@
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/http/http_util.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_error_job.h"
 #include "net/url_request/url_request_file_dir_job.h"
 
 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
 URLRequestJob* URLRequestFileJob::Factory(URLRequest* request,
                                           const std::string& scheme) {
   FilePath file_path;
   const bool is_file = FileURLToFilePath(request->url(), &file_path);
 
   // Check file access permissions.
   if (!IsFileAccessAllowed(*request, file_path))
     return new URLRequestErrorJob(request, ERR_ACCESS_DENIED);
 
   // We need to decide whether to create URLRequestFileJob for file access or
   // URLRequestFileDirJob for directory access. To avoid accessing the
   // filesystem, we only look at the path string here.
   // The code in the URLRequestFileJob::Start() method discovers that a path,
   // which doesn't end with a slash, should really be treated as a directory,
   // and it then redirects to the URLRequestFileDirJob.
   if (is_file &&
       file_util::EndsWithSeparator(file_path) &&
       file_path.IsAbsolute())
     return new URLRequestFileDirJob(request, file_path);
 
   // Use a regular file request job for all non-directories (including invalid
   // file names).
   return new URLRequestFileJob(request, file_path);
 }
 
 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();
-
-  if (async_resolver_) {
-    async_resolver_->Cancel();
-    async_resolver_ = NULL;
-  }
+  stream_.CloseAndCancelAsync();
+  weak_ptr_factory_.InvalidateWeakPtrs();
 
   URLRequestJob::Kill();
 }
 
 bool URLRequestFileJob::ReadRawData(IOBuffer* dest, int dest_size,
                                     int *bytes_read) {
   DCHECK_NE(dest_size, 0);
   DCHECK(bytes_read);
   DCHECK_GE(remaining_bytes_, 0);
 
   if (remaining_bytes_ < dest_size)
     dest_size = static_cast<int>(remaining_bytes_);
 
   // If we should copy zero bytes because |remaining_bytes_| is zero, short
   // circuit here.
   if (!dest_size) {
     *bytes_read = 0;
     return true;
   }
 
   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;
     remaining_bytes_ -= rv;
     DCHECK_GE(remaining_bytes_, 0);
     return true;
   }
 
   // Otherwise, a read error occured.  We may just need to wait...
   if (rv == ERR_IO_PENDING) {
     SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
   } else {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
   }
   return false;
 }
 
 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();
     new_path.push_back('/');
     GURL::Replacements replacements;
     replacements.SetPathStr(new_path);
 
     *location = request_->url().ReplaceComponents(replacements);
     *http_status_code = 301;  // simulate a permanent redirect
     return true;
@@ skipping 21 matching lines @@
 #endif
 }
 
 Filter* URLRequestFileJob::SetupFilter() const {
   // Bug 9936 - .svgz files needs to be decompressed.
   return LowerCaseEqualsASCII(file_path_.Extension(), ".svgz")
       ? Filter::GZipFactory() : NULL;
 }
 
 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(
     const HttpRequestHeaders& headers) {
   std::string range_header;
   if (headers.GetHeader(HttpRequestHeaders::kRange, &range_header)) {
     // We only care about "Range" header here.
     std::vector<HttpByteRange> ranges;
     if (HttpUtil::ParseRangeHeader(range_header, &ranges)) {
       if (ranges.size() == 1) {
@@ skipping 16 matching lines @@
   const URLRequestContext* context = request.context();
   if (!context)
     return false;
   const NetworkDelegate* delegate = context->network_delegate();
   if (delegate)
     return delegate->CanAccessFile(request, path);
   return false;
 }
 
 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,
   // we will append trailing slash and redirect to FileDirJob.
   // A special case is "\" on Windows. We should resolve as invalid.
   // 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 (rv != OK) {
-    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, rv));
+  if (meta_info_.is_directory_) {
+    DidOpen(OK);
     return;
   }
 
-  if (!byte_range_.ComputeBounds(file_info.size)) {
+  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(meta_info_.file_size_)) {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
     return;
   }
 
   remaining_bytes_ = byte_range_.last_byte_position() -
                      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();
 }
 
 void URLRequestFileJob::DidRead(int result) {
   if (result > 0) {
     SetStatus(URLRequestStatus());  // Clear the IO_PENDING status
   } else if (result == 0) {
     NotifyDone(URLRequestStatus());
   } else {
     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED, result));
   }
 
   remaining_bytes_ -= result;
   DCHECK_GE(remaining_bytes_, 0);
 
   NotifyReadComplete(result);
 }
 
 }  // namespace net