Added callback to DownloadUrl() so we can find download failures.

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 16 matching lines @@
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request_context.h"
 
 using content::BrowserThread;
 using content::DownloadId;
 using content::DownloadItem;
 using content::DownloadManager;
 
+namespace {
+
+void CallStartedCBOnUIThread(
+    const DownloadResourceHandler::OnStartedCallback& started_cb,
+    DownloadId id,
+    net::Error error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (started_cb.is_null())

[Randy Smith (Not in Mondays), 2012/03/07 21:16:57]

When you null a callback, doesn't that mean that running it does nothing?  If
so, you can get rid of this conditional (and maybe replace it with a comment if
it's confusing).

[ahendrickson, 2012/03/08 21:33:07]

I tried it, and got a NULL function pointer which caused a crash.

[Randy Smith (Not in Mondays), 2012/03/09 19:14:02]

I spent a bit of time digging around to see if there was an already existing way
to do this and didn't find it, but I have a hard time believing there isn't
(e.g. when someone passes in a default constructed callback, we sometimes just
run it, and nothing blows up, so assigning a default constructed callback should
do it?).  But I'm ok with this for DRH; I would have objected more strongly to
the extra routine in RDH, but that's gone now :-}.

+    return;
+  started_cb.Run(id, error);
+}
+
+}  // namespace
+
 DownloadResourceHandler::DownloadResourceHandler(
     ResourceDispatcherHost* rdh,
     int render_process_host_id,
     int render_view_id,
     int request_id,
     const GURL& url,
     DownloadFileManager* download_file_manager,
     net::URLRequest* request,
     const DownloadResourceHandler::OnStartedCallback& started_cb,
     const DownloadSaveInfo& save_info)
@@ skipping 105 matching lines @@
 
   // 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.
   rdh_->PauseRequest(global_id_.child_id, global_id_.request_id, true);
 
   return true;
 }
 
 void DownloadResourceHandler::CallStartedCB(DownloadId id, net::Error error) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (started_cb_.is_null())
     return;
-  started_cb_.Run(id, error);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&CallStartedCBOnUIThread, started_cb_, id, error));

[Randy Smith (Not in Mondays), 2012/03/07 21:16:57]

As noted elsewhere, I think this can be simplified to avoid needing
CallStartedCBOnUIThread.

   started_cb_.Reset();
 }
 
 bool DownloadResourceHandler::OnWillStart(int request_id,
                                           const GURL& url,
                                           bool* defer) {
   return true;
 }
 
 // Create a new buffer, which will be handed to the download thread for file
@@ skipping 115 matching lines @@
         default:
           reason = DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
           break;
       }
     }
   }
 
   download_stats::RecordAcceptsRanges(accept_ranges_, bytes_read_);
 
   // If the callback was already run on the UI thread, this will be a noop.
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&DownloadResourceHandler::CallStartedCB, this,
-                 download_id_, error_code));
+  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;
   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
@@ skipping 35 matching lines @@
     StartPauseTimer();
 
   if (is_paused_ != should_pause) {
     rdh_->PauseRequest(global_id_.child_id, global_id_.request_id,
                        should_pause);
     is_paused_ = should_pause;
   }
 }
 
 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);
 }
 
 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_->url().spec().c_str(),
+                            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());
 }