Use ByteStream in downloads system to decouple source and sink.

content/browser/download/download_resource_handler.cc

Index: content/browser/download/download_resource_handler.cc
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index 84ab673c9cd59120e2757d50d579c76167f62092..a4d9e7e01c2e8459190de65ff810dd6d57218ff5 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -8,15 +8,16 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/message_loop_proxy.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/stats_counters.h"
 #include "base/stringprintf.h"
-#include "content/browser/download/download_buffer.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/download/download_request_handle.h"
+#include "content/browser/download/byte_stream.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/renderer_host/resource_dispatcher_host_impl.h"
 #include "content/browser/renderer_host/resource_request_info_impl.h"
@@ -39,6 +40,8 @@ using content::ResourceRequestInfoImpl;
 
 namespace {
 
+static const int kDownloadPipeSize = 100 * 1024;

[darin (slow to review), 2012/05/26 03:59:19]

nit: "pipe" -> "stream" / "byte stream"?

[Randy Smith (Not in Mondays), 2012/05/28 02:51:37]

Done.

+
 void CallStartedCBOnUIThread(
     const DownloadResourceHandler::OnStartedCallback& started_cb,
     DownloadId id,
@@ -49,6 +52,33 @@ void CallStartedCBOnUIThread(
   started_cb.Run(id, error);
 }
 
+// Static function in order to prevent any accidental accesses to
+// DownloadResourceHandler members on the IO thread.

[benjhayden, 2012/05/26 20:42:02]

on the UI thread, right?

[Randy Smith (Not in Mondays), 2012/05/28 02:51:37]

Done.

+static void StartOnUIThread(
+    scoped_ptr<DownloadCreateInfo> info,
+    scoped_ptr<content::ByteStreamOutput> pipe,
+    scoped_refptr<DownloadFileManager> download_file_manager,
+    const DownloadRequestHandle& handle,
+    const DownloadResourceHandler::OnStartedCallback& started_cb) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  DownloadManager* download_manager = handle.GetDownloadManager();
+  if (!download_manager) {
+    // NULL in unittests or if the page closed right after starting the
+    // download.
+    if (!started_cb.is_null())
+      started_cb.Run(DownloadId(), net::ERR_ACCESS_DENIED);
+    return;
+  }
+  DownloadId download_id = download_manager->delegate()->GetNextId();
+  info->download_id = download_id;
+
+  download_file_manager->StartDownload(info.Pass(), pipe.Pass(), handle);
+
+  if (!started_cb.is_null())
+    started_cb.Run(download_id, net::OK);
+}
+
 }  // namespace
 
 DownloadResourceHandler::DownloadResourceHandler(
@@ -60,19 +90,18 @@ DownloadResourceHandler::DownloadResourceHandler(
     net::URLRequest* request,
     const DownloadResourceHandler::OnStartedCallback& started_cb,
     const content::DownloadSaveInfo& save_info)
-    : download_id_(DownloadId::Invalid()),
-      global_id_(render_process_host_id, request_id),
+    : 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),
       last_buffer_size_(0),
       bytes_read_(0),
       pause_count_(0),
-      was_deferred_(false) {
+      was_deferred_(false),
+      on_response_started_called_(false) {
   download_stats::RecordDownloadCount(download_stats::UNTHROTTLED_COUNT);
 }
 
@@ -96,6 +125,11 @@ bool DownloadResourceHandler::OnResponseStarted(
     int request_id,
     content::ResourceResponse* response,
     bool* defer) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // There can be only one (call)
+  DCHECK(!on_response_started_called_);

[benjhayden, 2012/05/26 20:42:02]

What does this buy?

[Randy Smith (Not in Mondays), 2012/05/28 02:51:37]

Turns out that there's a bug in BufferedResourceHandler where a function's
OnResponseStarted can be called twice.  I'm fixing it in a separate CL
(http://codereview.chromium.org/10451023 ) but merging that CL in here so I
don't run into the problem.  This was the 'test' side of the fix.  

+  on_response_started_called_ = true;
+
   VLOG(20) << __FUNCTION__ << "()" << DebugString()
            << " request_id = " << request_id;
   download_start_time_ = base::TimeTicks::Now();
@@ -117,6 +151,17 @@ bool DownloadResourceHandler::OnResponseStarted(
       base::Time::Now(), 0, content_length_, DownloadItem::IN_PROGRESS,
       request_->net_log(), request_info->has_user_gesture(),
       request_info->transition_type()));
+
+  // Create the ByteStream pipe for sending data to the download sink.
+  scoped_ptr<content::ByteStreamOutput> stream_reader;

[darin (slow to review), 2012/05/26 03:59:19]

I know I'm being pedantic, but if stream_reader is the right name for the
variable, then {Prefix}StreamReader is probably the right name for the class. 
Just food for thought.  I know a class name is good when I like using the name
for variables of that type too ;-)

[Randy Smith (Not in Mondays), 2012/05/28 02:51:37]

Done :-} :-J.

+  CreateByteStream(
+      base::MessageLoopProxy::current(),
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE),
+      kDownloadPipeSize, &stream_writer_, &stream_reader);
+  stream_writer_->RegisterCallback(
+      // We're refcounted, so this is a safe callback to pass around.
+      base::Bind(&DownloadResourceHandler::ResumeRequest, this));
+
   info->url_chain = request_->url_chain();
   info->referrer_url = GURL(request_->referrer());
   info->start_time = base::Time::Now();
@@ -163,13 +208,23 @@ bool DownloadResourceHandler::OnResponseStarted(
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&DownloadResourceHandler::StartOnUIThread, this,
-                 base::Passed(&info), request_handle));
+      base::Bind(&StartOnUIThread,
+                 base::Passed(info.Pass()),
+                 base::Passed(stream_reader.Pass()),
+                 scoped_refptr<DownloadFileManager>(download_file_manager_),

[darin (slow to review), 2012/05/26 03:59:19]

nit: I think it is OK to just change download_file_manager_ to be of type
scoped_refptr<DownloadFileManager>.

[Randy Smith (Not in Mondays), 2012/05/28 02:51:37]

Done.

+                 request_handle,
+                 // Pass to StartOnUIThread so that variable
+                 // access is always on IO thread but function
+                 // is called on UI thread.
+                 started_cb_));
+  // Guaranteed to be called in StartOnUIThread
+  started_cb_.Reset();
 
   return true;
 }
 
 void DownloadResourceHandler::CallStartedCB(DownloadId id, net::Error error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (started_cb_.is_null())
     return;
   BrowserThread::PostTask(
@@ -188,6 +243,7 @@ bool DownloadResourceHandler::OnWillStart(int request_id,
 // writing and deletion.
 bool DownloadResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf,
                                          int* buf_size, int min_size) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(buf && buf_size);
   if (!read_buffer_) {
     *buf_size = min_size < 0 ? kReadBufSize : min_size;
@@ -201,6 +257,7 @@ bool DownloadResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf,
 // Pass the buffer to the download file writer.
 bool DownloadResourceHandler::OnReadCompleted(int request_id, int* bytes_read,
                                               bool* defer) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!read_buffer_) {
     // Ignore spurious OnReadCompleted!  Deferring from OnReadCompleted tells
     // the ResourceDispatcherHost that we did not consume the data.
@@ -214,14 +271,6 @@ bool DownloadResourceHandler::OnReadCompleted(int request_id, int* bytes_read,
     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();
@@ -240,27 +289,17 @@ bool DownloadResourceHandler::OnReadCompleted(int request_id, int* bytes_read,
     return true;
   bytes_read_ += *bytes_read;
   DCHECK(read_buffer_);
-  // Swap the data.
-  net::IOBuffer* io_buffer = NULL;
-  read_buffer_.swap(&io_buffer);
-  size_t vector_size = buffer_->AddData(io_buffer, *bytes_read);
-  bool need_update = (vector_size == 1);  // Buffer was empty.
-
-  // 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) {
+  // Take the data ship it down the pipe.  If the pipe is full, pause the
+  // request; the pipe callback will resume it.
+  if (!stream_writer_->Write(read_buffer_, *bytes_read)) {
+    PauseRequest();

[darin (slow to review), 2012/05/26 03:59:19]

I know PauseRequest() looks odd because we are so used to
ResourceDispatcherHostImpl::PauseRequest(), but that is no longer a public
function ;-)  The DRH has its own concept of pausing the request!

[Randy Smith (Not in Mondays), 2012/05/28 02:51:37]

Done.

     *defer = was_deferred_ = true;
-    CheckWriteProgressLater();
+    last_pipe_pause_time_ = now;
   }
 
+  read_buffer_ = NULL;  // Drop our reference.
+
   return true;
 }
 
@@ -268,37 +307,13 @@ bool DownloadResourceHandler::OnResponseCompleted(
     int request_id,
     const net::URLRequestStatus& status,
     const std::string& security_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   VLOG(20) << __FUNCTION__ << "()" << DebugString()
            << " request_id = " << request_id
            << " status.status() = " << status.status()
            << " status.error() = " << status.error();
-  int response = status.is_success() ? request_->GetResponseCode() : 0;
-  if (download_id_.IsValid()) {
-    OnResponseCompletedInternal(request_id, status, security_info, response);
-  } else {
-    // We got cancelled before the task which sets the id ran on the IO thread.
-    // Wait for it.
-    BrowserThread::PostTaskAndReply(
-        BrowserThread::UI, FROM_HERE,
-        base::Bind(&base::DoNothing),
-        base::Bind(&DownloadResourceHandler::OnResponseCompletedInternal, this,
-                   request_id, status, security_info, response));
-  }
-  // Can't trust request_ being value after this point.
-  request_ = NULL;
-  return true;
-}
+  int response_code = status.is_success() ? request_->GetResponseCode() : 0;
 
-void DownloadResourceHandler::OnResponseCompletedInternal(
-    int request_id,
-    const net::URLRequestStatus& status,
-    const std::string& security_info,
-    int response_code) {
-  // NOTE: |request_| may be a dangling pointer at this point.
-  VLOG(20) << __FUNCTION__ << "()"
-           << " request_id = " << request_id
-           << " status.status() = " << status.status()
-           << " status.error() = " << status.error();
   net::Error error_code = net::OK;
   if (status.status() == net::URLRequestStatus::FAILED)
     error_code = static_cast<net::Error>(status.error());  // Normal case.
@@ -342,66 +357,38 @@ void DownloadResourceHandler::OnResponseCompletedInternal(
 
   download_stats::RecordAcceptsRanges(accept_ranges_, bytes_read_);
 
-  // If the callback was already run on the UI thread, this will be a noop.
-  CallStartedCB(download_id_, error_code);
-
-  // We transfer ownership to |DownloadFileManager| to delete |buffer_|,
-  // so that any functions queued up on the FILE thread are executed
-  // before deletion.
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::OnResponseCompleted,
-                 download_file_manager_, download_id_, reason, security_info));
-  buffer_ = NULL;  // The buffer is longer needed by |DownloadResourceHandler|.
-  read_buffer_ = NULL;
-}
-
-void DownloadResourceHandler::OnRequestClosed() {
-  UMA_HISTOGRAM_TIMES("SB2.DownloadDuration",
-                      base::TimeTicks::Now() - download_start_time_);
-}
-
-void DownloadResourceHandler::StartOnUIThread(
-    scoped_ptr<DownloadCreateInfo> info,
-    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;
+  CallStartedCB(DownloadId(), error_code);
 
-  // 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.
+  // Send the info down the pipe.  Conditional is in case we get
+  // OnResponseCompleted without OnResponseStarted.
+  if (stream_writer_.get())
+    stream_writer_->Close(reason);
 
-  download_file_manager_->StartDownload(info.release(), handle);
+  stream_writer_.reset();  // We no longer need the pipe.
+  read_buffer_ = NULL;
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&DownloadResourceHandler::SetDownloadID, this,
-                 download_id));
+  // Stats
+  download_stats::RecordNetworkBandwidth(
+      bytes_read_, base::TimeTicks::Now() - download_start_time_,
+      total_pause_time_);
 
-  CallStartedCB(download_id, net::OK);
+  return true;
 }
 
-void DownloadResourceHandler::SetDownloadID(content::DownloadId id) {
+void DownloadResourceHandler::OnRequestClosed() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  UMA_HISTOGRAM_TIMES("SB2.DownloadDuration",
+                      base::TimeTicks::Now() - download_start_time_);
 
-  download_id_ = id;
-  MaybeResumeRequest();
+  // Can't trust request_ being valid after this point.
+  request_ = NULL;
 }
 
 // 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::SetContentLength(const int64& content_length) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   content_length_ = 0;
   if (content_length > 0)
     content_length_ = content_length;
@@ -409,22 +396,10 @@ void DownloadResourceHandler::SetContentLength(const int64& content_length) {
 
 void DownloadResourceHandler::SetContentDisposition(
     const std::string& content_disposition) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   content_disposition_ = content_disposition;
 }
 
-void DownloadResourceHandler::CheckWriteProgress() {
-  if (!buffer_.get())
-    return;  // The download completed while we were waiting to run.
-
-  if (buffer_->size() > kLoadsToWrite) {
-    // We'll come back later and see if it's okay to unpause the request.
-    CheckWriteProgressLater();
-    return;
-  }
-
-  MaybeResumeRequest();
-}
-
 void DownloadResourceHandler::PauseRequest() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
@@ -436,7 +411,20 @@ void DownloadResourceHandler::ResumeRequest() {
   DCHECK_LT(0, pause_count_);
 
   --pause_count_;
-  MaybeResumeRequest();
+
+  if (!was_deferred_)
+    return;
+  if (pause_count_ > 0)
+    return;
+
+  was_deferred_ = false;
+  if (!last_pipe_pause_time_.is_null()) {
+    total_pause_time_ += (base::TimeTicks::Now() - last_pipe_pause_time_);
+    last_pipe_pause_time_ = base::TimeTicks();

[darin (slow to review), 2012/05/26 03:59:19]

nit: another case where "pipe" is used instead of "byte stream" or "stream"

[Randy Smith (Not in Mondays), 2012/05/28 02:51:37]

Done.

+  }
+  ResourceDispatcherHostImpl::Get()->ResumeDeferredRequest(
+      global_id_.child_id,
+      global_id_.request_id);
 }
 
 void DownloadResourceHandler::CancelRequest() {
@@ -451,7 +439,6 @@ void DownloadResourceHandler::CancelRequest() {
 std::string DownloadResourceHandler::DebugString() const {
   return base::StringPrintf("{"
                             " url_ = " "\"%s\""
-                            " download_id_ = " "%d"
                             " global_id_ = {"
                             " child_id = " "%d"
                             " request_id = " "%d"
@@ -462,7 +449,6 @@ std::string DownloadResourceHandler::DebugString() const {
                             request_ ?
                                 request_->url().spec().c_str() :
                                 "<NULL request>",
-                            download_id_.local(),
                             global_id_.child_id,
                             global_id_.request_id,
                             render_view_id_,
@@ -470,35 +456,15 @@ std::string DownloadResourceHandler::DebugString() const {
 }
 
 DownloadResourceHandler::~DownloadResourceHandler() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
   // 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);
-}
+  CallStartedCB(DownloadId(), net::ERR_ACCESS_DENIED);
 
-void DownloadResourceHandler::CheckWriteProgressLater() {
-  if (!check_write_progress_timer_.IsRunning()) {
-    check_write_progress_timer_.Start(
-        FROM_HERE,
-        base::TimeDelta::FromMilliseconds(kThrottleTimeMs),
-        this,
-        &DownloadResourceHandler::CheckWriteProgress);
-  }
+  // Remove output pipe callback if a pipe exists.
+  if (stream_writer_.get())
+    stream_writer_->RegisterCallback(base::Closure());
 }
 
-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);
-}