Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(480)

Unified Diff: net/url_request/url_request_file_job.cc

Issue 10695110: Avoid disk accesses on the wrong thread in URLRequestFileJob (Closed) Base URL: https://src.chromium.org/chrome/trunk/src/
Patch Set: Created 8 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 parame
+ 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();
}

Powered by Google App Engine
This is Rietveld 408576698