Detect file system errors during downloads.

content/browser/download/download_manager.cc

Index: content/browser/download/download_manager.cc
diff --git a/content/browser/download/download_manager.cc b/content/browser/download/download_manager.cc
index 8808a1043de68b512014197942babb429cdeeced..0801a9f4347efc1fa6a360984afa756a55782a0a 100644
--- a/content/browser/download/download_manager.cc
+++ b/content/browser/download/download_manager.cc
@@ -390,24 +390,7 @@ void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
 
 void DownloadManager::OnResponseCompleted(int32 download_id,
                                           int64 size,
-                                          int os_error,
                                           const std::string& hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // ERR_CONNECTION_CLOSED is allowed since a number of servers in the wild
-  // advertise a larger Content-Length than the amount of bytes in the message
-  // body, and then close the connection. Other browsers - IE8, Firefox 4.0.1,
-  // and Safari 5.0.4 - treat the download as complete in this case, so we
-  // follow their lead.
-  if (os_error == 0 || os_error == net::ERR_CONNECTION_CLOSED) {
-    OnAllDataSaved(download_id, size, hash);
-  } else {
-    OnDownloadError(download_id, size, os_error);
-  }
-}
-
-void DownloadManager::OnAllDataSaved(int32 download_id,
-                                     int64 size,
-                                     const std::string& hash) {
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " size = " << size;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -549,24 +532,31 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
 }
 
 void DownloadManager::DownloadCancelled(int32 download_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator it = in_progress_.find(download_id);
-  if (it == in_progress_.end())
+  DownloadItem* download = GetActiveDownload(download_id);
+
+  // A cancel at the right time could remove the download from the
+  // |active_downloads_| map before we get here.
+  if (!download)
     return;
-  DownloadItem* download = it->second;
 
-  VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
-           << " download = " << download->DebugString(true);
+  RemoveFromActiveList(download);
 
-  // Clean up will happen when the history system create callback runs if we
-  // don't have a valid db_handle yet.
-  if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
-    in_progress_.erase(it);
-    active_downloads_.erase(download_id);
-    UpdateDownloadProgress();  // Reflect removal from in_progress_.
-    download_history_->UpdateEntry(download);
-  }
+  DownloadCancelledInternal(download_id, download->request_handle());
+}
+
+void DownloadManager::OnDownloadError(int32 download_id,
+                                      int64 size,
+                                      int error) {
+  DownloadItem* download = GetActiveDownload(download_id);
+  if (!download)
+    return;
 
+  VLOG(20) << __FUNCTION__ << "()" << " Error " << error
+           << " at offset " << download->received_bytes()
+           << " size = " << size;
+
+  RemoveFromActiveList(download);
+  download->Interrupted(size, error);
   DownloadCancelledInternal(download_id, download->request_handle());
 }
 
@@ -581,40 +571,35 @@ void DownloadManager::DownloadCancelledInternal(
           file_manager_, &DownloadFileManager::CancelDownload, download_id));
 }
 
-void DownloadManager::OnDownloadError(int32 download_id,
-                                      int64 size,
-                                      int os_error) {
+DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = active_downloads_.find(download_id);
-  // A cancel at the right time could remove the download from the
-  // |active_downloads_| map before we get here.
   if (it == active_downloads_.end())
-    return;
+    return NULL;
 
   DownloadItem* download = it->second;
 
-  VLOG(20) << __FUNCTION__ << "()" << " Error " << os_error
-           << " at offset " << download->received_bytes()
-           << " for download = " << download->DebugString(true);
+  DCHECK(download);
+  DCHECK_EQ(download_id, download->id());
 
-  download->Interrupted(size, os_error);
+  VLOG(20) << __FUNCTION__ << "()"
+           << " download = " << download->DebugString(true);

[Randy Smith (Not in Mondays), 2011/08/19 20:09:13]

It's up to you, but logging inside of "GetActiveDownload" (which is pure
function, no state changes, and one that may be called from a lot of different
places) seems like overkill.  If I were putting in logging, I have a hard time
imagining a situation in which this would give me useful information (== in
which I wouldn't also want a log statement in the caller, and then ignore the
results from this).

[ahendrickson, 2011/08/22 14:53:30]

Done.

+
+  return download;
+}
+
+void DownloadManager::RemoveFromActiveList(DownloadItem* download) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(download);
 
-  // TODO(ahendrickson) - Remove this when we add resuming of interrupted
-  // downloads, as we will keep the download item around in that case.
-  //
   // Clean up will happen when the history system create callback runs if we
   // don't have a valid db_handle yet.
   if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
-    in_progress_.erase(download_id);
-    active_downloads_.erase(download_id);
+    in_progress_.erase(download->id());
+    active_downloads_.erase(download->id());
     UpdateDownloadProgress();  // Reflect removal from in_progress_.
     download_history_->UpdateEntry(download);
   }
-
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
 }
 
 void DownloadManager::UpdateDownloadProgress() {
@@ -807,6 +792,9 @@ void DownloadManager::FileSelectionCanceled(void* params) {
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
+  // TODO(ahendrickson) -- This currently has no effect.

[Randy Smith (Not in Mondays), 2011/08/19 20:09:13]

Could you enhance this comment to answer the questions:
* What's the actual TODO (i.e. what are you going to be doing in the future)?  
* What's the context/reason it has no effect?

[ahendrickson, 2011/08/22 14:53:30]

Done.

+  RemoveFromActiveList(download);
+
   DownloadCancelledInternal(download_id, download->request_handle());
 }