Use defer out-params instead of ResourceDispatcherHostImpl::PauseRequest(...true)

content/browser/download/download_resource_handler.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.
 
 #include "content/browser/download/download_resource_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 51 matching lines @@
     const content::DownloadSaveInfo& save_info)
     : download_id_(DownloadId::Invalid()),
       global_id_(render_process_host_id, request_id),
       render_view_id_(render_view_id),
       content_length_(0),
       download_file_manager_(download_file_manager),
       request_(request),
       started_cb_(started_cb),
       save_info_(save_info),
       buffer_(new content::DownloadBuffer),
-      is_paused_(false),
       last_buffer_size_(0),
-      bytes_read_(0) {
+      bytes_read_(0),
+      pause_count_(0),
+      was_deferred_(false) {
   download_stats::RecordDownloadCount(download_stats::UNTHROTTLED_COUNT);
 }
 
 bool DownloadResourceHandler::OnUploadProgress(int request_id,
                                                uint64 position,
                                                uint64 size) {
   return true;
 }
 
 // Not needed, as this event handler ought to be the final resource.
 bool DownloadResourceHandler::OnRequestRedirected(
     int request_id,
     const GURL& url,
     content::ResourceResponse* response,
     bool* defer) {
   return true;
 }
 
 // Send the download creation information to the download thread.
 bool DownloadResourceHandler::OnResponseStarted(
     int request_id,
-    content::ResourceResponse* response) {
+    content::ResourceResponse* response,
+    bool* defer) {
   VLOG(20) << __FUNCTION__ << "()" << DebugString()
            << " request_id = " << request_id;
   download_start_time_ = base::TimeTicks::Now();
 
   // If it's a download, we don't want to poison the cache with it.
   request_->StopCaching();
 
   std::string content_disposition;
   request_->GetResponseHeaderByName("content-disposition",
                                     &content_disposition);
-  set_content_disposition(content_disposition);
-  set_content_length(response->content_length);
+  SetContentDisposition(content_disposition);
+  SetContentLength(response->content_length);
 
   const ResourceRequestInfoImpl* request_info =
       ResourceRequestInfoImpl::ForRequest(request_);
 
   // Deleted in DownloadManager.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo(
       base::Time::Now(), 0, content_length_, DownloadItem::IN_PROGRESS,
       request_->net_log(), request_info->has_user_gesture(),
       request_info->transition_type()));
   info->url_chain = request_->url_chain();
   info->referrer_url = GURL(request_->referrer());
   info->start_time = base::Time::Now();
   info->received_bytes = save_info_.offset;
   info->total_bytes = content_length_;
   info->state = DownloadItem::IN_PROGRESS;
   info->has_user_gesture = request_info->has_user_gesture();
   info->content_disposition = content_disposition_;
   info->mime_type = response->mime_type;
   info->remote_address = request_->GetSocketAddress().host();
   download_stats::RecordDownloadMimeType(info->mime_type);
 
-  DownloadRequestHandle request_handle(global_id_.child_id,
+  DownloadRequestHandle request_handle(this, global_id_.child_id,
                                        render_view_id_, global_id_.request_id);
 
   // Get the last modified time and etag.
   const net::HttpResponseHeaders* headers = request_->response_headers();
   if (headers) {
     std::string last_modified_hdr;
     std::string etag;
     if (headers->EnumerateHeader(NULL, "Last-Modified", &last_modified_hdr))
       info->last_modified = last_modified_hdr;
     if (headers->EnumerateHeader(NULL, "ETag", &etag))
@@ skipping 11 matching lines @@
                                           "Accept-Ranges",
                                           &accept_ranges_)) {
     accept_ranges_ = "";
   }
 
   info->prompt_user_for_save_location =
       save_info_.prompt_for_save_location && save_info_.file_path.empty();
   info->referrer_charset = request_->context()->referrer_charset();
   info->save_info = save_info_;
 
-
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&DownloadResourceHandler::StartOnUIThread, this,
                  base::Passed(&info), request_handle));
 
-  // We can't start saving the data before we create the file on disk and have a
-  // download id. The request will be un-paused in
-  // DownloadFileManager::CreateDownloadFile.
-  ResourceDispatcherHostImpl::Get()->PauseRequest(global_id_.child_id,
-                                                  global_id_.request_id,
-                                                  true);
-
   return true;
 }
 
 void DownloadResourceHandler::CallStartedCB(DownloadId id, net::Error error) {
   if (started_cb_.is_null())
     return;
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&CallStartedCBOnUIThread, started_cb_, id, error));
   started_cb_.Reset();
@@ skipping 13 matching lines @@
   if (!read_buffer_) {
     *buf_size = min_size < 0 ? kReadBufSize : min_size;
     last_buffer_size_ = *buf_size;
     read_buffer_ = new net::IOBuffer(*buf_size);
   }
   *buf = read_buffer_.get();
   return true;
 }
 
 // Pass the buffer to the download file writer.
-bool DownloadResourceHandler::OnReadCompleted(int request_id, int* bytes_read) {
+bool DownloadResourceHandler::OnReadCompleted(int request_id, int* bytes_read,
+                                              bool* defer) {
+  if (!read_buffer_) {
+    // Ignore spurious OnReadCompleted!  Deferring from OnReadCompleted tells
+    // the ResourceDispatcherHost that we did not consume the data.
+    // ResumeDeferredRequest then repeats the last OnReadCompleted call.
+    // TODO(darin): Fix the ResourceDispatcherHost to avoid this hack!
+    return true;
+  }
+
+  if (pause_count_ > 0) {
+    *defer = was_deferred_ = true;
+    return true;
+  }
+
+  if (download_id_ == DownloadId::Invalid()) {
+    // We can't start saving the data before we create the file on disk and
+    // have a download id. The request will be un-paused in
+    // DownloadFileManager::CreateDownloadFile.
+    *defer = was_deferred_ = true;
+    return true;
+  }
+
   base::TimeTicks now(base::TimeTicks::Now());
   if (!last_read_time_.is_null()) {
     double seconds_since_last_read = (now - last_read_time_).InSecondsF();
     if (now == last_read_time_)
       // Use 1/10 ms as a "very small number" so that we avoid
       // divide-by-zero error and still record a very high potential bandwidth.
       seconds_since_last_read = 0.00001;
 
     double actual_bandwidth = (*bytes_read)/seconds_since_last_read;
     double potential_bandwidth = last_buffer_size_/seconds_since_last_read;
@@ skipping 14 matching lines @@
   // We are passing ownership of this buffer to the download file manager.
   if (need_update) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&DownloadFileManager::UpdateDownload,
                    download_file_manager_, download_id_, buffer_));
   }
 
   // We schedule a pause outside of the read loop if there is too much file
   // writing work to do.
-  if (vector_size > kLoadsToWrite)
-    StartPauseTimer();
+  if (vector_size > kLoadsToWrite) {
+    *defer = was_deferred_ = true;
+    CheckWriteProgressLater();
+  }
 
   return true;
 }
 
 bool DownloadResourceHandler::OnResponseCompleted(
     int request_id,
     const net::URLRequestStatus& status,
     const std::string& security_info) {
   VLOG(20) << __FUNCTION__ << "()" << DebugString()
            << " request_id = " << request_id
@@ skipping 93 matching lines @@
     const DownloadRequestHandle& handle) {
   DownloadManager* download_manager = handle.GetDownloadManager();
   if (!download_manager) {
     // NULL in unittests or if the page closed right after starting the
     // download.
     CallStartedCB(download_id_, net::ERR_ACCESS_DENIED);
     return;
   }
   DownloadId download_id = download_manager->delegate()->GetNextId();
   info->download_id = download_id;
+
+  // NOTE: StartDownload triggers creation of the download destination file
+  // that will hold the downloaded data.  SetDownloadID unblocks the
+  // DownloadResourceHandler to begin forwarding network data to the download
+  // destination file.  The sequence of these two steps is critical as creation
+  // of the downloaded destination file has to happen before we attempt to
+  // append data to it.  Both of those operations happen on the FILE thread.
+
+  download_file_manager_->StartDownload(info.release(), handle);

[Randy Smith (Not in Mondays), 2012/05/18 20:17:02]

Actually, when I think about this a little bit more, it scares me, in that the
file thread might generate posts to the IO thread (specifically in the case of
an error--I think there are currently cancels that target the IO thread without
going through the UI thread) which then will try to propagate failures to the UI
thread with a download id that hasn't been set.  It's an unlikely race, and it
may be an impossible one (in that I think the FILE->IO cancels only occur if the
download manager doesn't exist) but I'd feel better if we left the high level
logic alone, i.e. start the download in a paused state and have it be resumed
from the file thread after the download file is created.

Note that all of this is going to go down the toilet with a very satisfying
flushing sound as soon as I land http://codereview.chromium.org/10392111/.  But
my preference would be not to rock the boat at the inter-thread race level until
that point.

[darin (slow to review), 2012/05/18 23:09:05]

DownloadFileManager::StartDownload() gets called on the UI thread from
DownloadResourceHandler::StartOnUIThread(), and that calls
DownloadFileManager::CreateDownloadFile() on the FILE thread. 
CreateDownloadFile() just posts messages to the UI thread.

I don't see any problems.  What am I missing?

[Randy Smith (Not in Mondays), 2012/05/19 00:42:14]

You're right, though I'm still nervous.  What I was remembering was the
occasionally DFM::CancelDownloadOnRename->DF::CancelDownloadRequest->IO, but
there isn't any such pathway in the creation path.  It's fragile code (IMO), but
that doesn't make it unusual in context :-J and it'll go away soon, so I'm good
with it.  

+
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(&DownloadResourceHandler::set_download_id, this,
-                 info->download_id));
-  // It's safe to continue on with download initiation before we have
-  // confirmation that that download_id_ has been set on the IO thread, as any
-  // messages generated by the UI thread that affect the IO thread will be
-  // behind the message posted above.
-  download_file_manager_->StartDownload(info.release(), handle);
+      base::Bind(&DownloadResourceHandler::SetDownloadID, this,
+                 download_id));
+
   CallStartedCB(download_id, net::OK);
 }
 
-void DownloadResourceHandler::set_download_id(content::DownloadId id) {
+void DownloadResourceHandler::SetDownloadID(content::DownloadId id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
   download_id_ = id;
+  MaybeResumeRequest();
 }
 
 // If the content-length header is not present (or contains something other
 // than numbers), the incoming content_length is -1 (unknown size).
 // Set the content length to 0 to indicate unknown size to DownloadManager.
-void DownloadResourceHandler::set_content_length(const int64& content_length) {
+void DownloadResourceHandler::SetContentLength(const int64& content_length) {
   content_length_ = 0;
   if (content_length > 0)
     content_length_ = content_length;
 }
 
-void DownloadResourceHandler::set_content_disposition(
+void DownloadResourceHandler::SetContentDisposition(
     const std::string& content_disposition) {
   content_disposition_ = content_disposition;
 }
 
 void DownloadResourceHandler::CheckWriteProgress() {
   if (!buffer_.get())
     return;  // The download completed while we were waiting to run.
 
-  size_t contents_size = buffer_->size();
+  if (buffer_->size() > kLoadsToWrite) {
+    // We'll come back later and see if it's okay to unpause the request.
+    CheckWriteProgressLater();
+    return;
+  }
 
-  bool should_pause = contents_size > kLoadsToWrite;
+  MaybeResumeRequest();
+}
 
-  // We'll come back later and see if it's okay to unpause the request.
-  if (should_pause)
-    StartPauseTimer();
+void DownloadResourceHandler::PauseRequest() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  if (is_paused_ != should_pause) {
-    ResourceDispatcherHostImpl::Get()->PauseRequest(global_id_.child_id,
-                                                    global_id_.request_id,
-                                                    should_pause);
-    is_paused_ = should_pause;
-  }
+  ++pause_count_;
+}
+
+void DownloadResourceHandler::ResumeRequest() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK_LT(0, pause_count_);
+
+  --pause_count_;
+  MaybeResumeRequest();
+}
+
+void DownloadResourceHandler::CancelRequest() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  ResourceDispatcherHostImpl::Get()->CancelRequest(
+      global_id_.child_id,
+      global_id_.request_id,
+      false);
 }
 
 DownloadResourceHandler::~DownloadResourceHandler() {
   // This won't do anything if the callback was called before.
   // If it goes through, it will likely be because OnWillStart() returned
   // false somewhere in the chain of resource handlers.
   CallStartedCB(download_id_, net::ERR_ACCESS_DENIED);
 }
 
-void DownloadResourceHandler::StartPauseTimer() {
-  if (!pause_timer_.IsRunning())
-    pause_timer_.Start(FROM_HERE,
-                       base::TimeDelta::FromMilliseconds(kThrottleTimeMs), this,
-                       &DownloadResourceHandler::CheckWriteProgress);
+void DownloadResourceHandler::CheckWriteProgressLater() {
+  if (!check_write_progress_timer_.IsRunning()) {
+    check_write_progress_timer_.Start(
+        FROM_HERE,
+        base::TimeDelta::FromMilliseconds(kThrottleTimeMs),
+        this,
+        &DownloadResourceHandler::CheckWriteProgress);
+  }
+}
+
+void DownloadResourceHandler::MaybeResumeRequest() {
+  if (!was_deferred_)
+    return;
+
+  if (pause_count_ > 0)
+    return;
+  if (download_id_ == DownloadId::Invalid())
+    return;
+  if (buffer_.get() && (buffer_->size() > kLoadsToWrite))
+    return;
+
+  was_deferred_ = false;
+  ResourceDispatcherHostImpl::Get()->ResumeDeferredRequest(
+      global_id_.child_id,
+      global_id_.request_id);
 }
 
 std::string DownloadResourceHandler::DebugString() const {
   return base::StringPrintf("{"
                             " url_ = " "\"%s\""
                             " download_id_ = " "%d"
                             " global_id_ = {"
                             " child_id = " "%d"
                             " request_id = " "%d"
                             " }"
                             " render_view_id_ = " "%d"
                             " save_info_.file_path = \"%" PRFilePath "\""
                             " }",
                             request_ ?
                                 request_->url().spec().c_str() :
                                 "<NULL request>",
                             download_id_.local(),
                             global_id_.child_id,
                             global_id_.request_id,
                             render_view_id_,
                             save_info_.file_path.value().c_str());
 }