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

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 1227651c99ee033ea1fd6c5ace6fbf152c871306..5986a6443e3586e9db94e12381ea53c40da50dac 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -34,6 +34,20 @@ 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,
@@ -159,10 +173,11 @@ bool DownloadResourceHandler::OnResponseStarted(
 }
 
 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();
 }
 
@@ -298,10 +313,7 @@ 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.
-  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
@@ -326,6 +338,7 @@ void DownloadResourceHandler::StartOnUIThread(
   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();
@@ -381,6 +394,10 @@ void DownloadResourceHandler::CheckWriteProgress() {
 }
 
 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() {
@@ -401,7 +418,9 @@ std::string DownloadResourceHandler::DebugString() const {
                             " 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,