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::FileMetaInfo::FileMetaInfo()
+    : file_size(-1),
+      mime_type_result(false),
+      file_exists(false),
+      is_directory(false) {
+}
 
 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) {
+      stream_(new FileStream(NULL)),
+      remaining_bytes_(0),
+      weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 // 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;
+  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_.reset();

[wtc, 2012/08/14 18:00:40]

Could you explain why it is necessary to delete the FileStream
object here?  Is it not enough to just close the FileStream
as the original code does?

[pivanof, 2012/08/15 06:36:50]

We can't call CloseSync() because it will make IO on the wrong thread.
CloseAsync() requires a callback parameter and passing NULL to it looks gross
(and I don't know whether it will work). The only way of closing the FileStream
left is destructor.

+  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)));
+  int rv = stream_->Read(dest, dest_size,
+                         base::Bind(&URLRequestFileJob::DidRead,
+                                    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);
+  if (meta_info_.mime_type_result) {
+    *mime_type = meta_info_.mime_type;
+    return true;
+  }
+  return false;

[wtc, 2012/08/14 18:00:40]

Nit: this can be rewritten as:

  if (meta_info_.mime_type_result)
    *mime_type = meta_info_.mime_type;
  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 13 matching lines @@
 // static
 bool URLRequestFileJob::IsFileAccessAllowed(const URLRequest& request,
                                             const FilePath& path) {
   const NetworkDelegate* delegate = request.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;

[wtc, 2012/08/14 18:00:40]

It seems that platform_info may contain junk if the
file_util::GetFileInfo() call failed.  So perhaps this should
be written as:

  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;
  }

[wtc, 2012/11/14 21:05:01]

This change is probably worth making. I am not 100% sure
though, because we still need to set meta_info->file_size and
meta_info->is_directory to some dummy values if
file_util::GetFileInfo() fails.
file_util::GetFileInfo(

[wtc, 2012/11/17 02:15:29]

pivanof wrote:
The dummy values are for extra protection, just in
case someone still uses meta_info->file_size and
meta_info->is_directory even though meta_info->file_exists
is false. We can set meta_info->file_size to -1 and
meta_info->is_directory to false in that case.

NOTE: platform_info.size is initialized to 0, whereas
meta_info->file_size is initialized to -1. It would be
nice to be consistent.

+  // 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) {
+      // stream_->Seek() didn't finish successfully, so pass an intentionally

[wtc, 2012/08/14 18:00:40]

Nit: didn't finish successfully => failed

+      // erroneous value into DidSeek().
+      DidSeek(-1);
     }
+  } else {
+    // We didn't need to call stream_->Seek() at all, so let's pretend we
+    // actually did that and the FileStream has positioned where we need it
+    // to be (it won't be true only when remaining_bytes_ <= 0).

[wtc, 2012/08/14 18:00:40]

I don't understand the sentence in parentheses
  (it won't be true only when remaining_bytes_ <= 0)

I suggest we just remove that sentence.

[pivanof, 2012/08/15 06:36:50]

What I meant is that statement before parenthesis (FileStream is positioned
where we need it to be) will be actually true except one case -- when
|remaining_bytes_| <= 0. Maybe I need rephrase?

[wtc, 2012/08/17 00:24:04]

I suggest something like this:
  // We didn't need to call stream_->Seek() at all, so pass
  // the value that would mean seek success to DidSeek() to
  // skip the code for 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_);
   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