Detect file system errors during downloads.

chrome/browser/download/download_file_manager.cc

Index: chrome/browser/download/download_file_manager.cc
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index 4d19e6139e8b614a1118aed43d62fd6a4e8cc2ae..71852202dd3811812ab2ea775fdc907df0cebdbb 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -169,11 +169,36 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
   }
 
   DownloadFile* download_file = GetDownloadFile(id);
+  bool had_error = false;
   for (size_t i = 0; i < contents.size(); ++i) {
     net::IOBuffer* data = contents[i].first;
     const int data_len = contents[i].second;
-    if (download_file)
-      download_file->AppendDataToFile(data->data(), data_len);
+    if (!had_error && download_file) {
+      bool write_succeeded =

[cbentzel, 2011/08/16 15:50:36]

If this is going to end up with a net_error code return in a subsequent CL, I'm
fine with using that here rather than a bool. 

[ahendrickson, 2011/08/18 21:52:01]

AppendDataToFile() returns a bool currently.  Another CL will change that.

+          download_file->AppendDataToFile(data->data(), data_len);
+      if (!write_succeeded) {
+        // Write failed: interrupt the download.
+        std::string hash;
+        DownloadManager* download_manager = download_file->GetDownloadManager();
+        had_error = true;
+
+        // Calling this here in case we get more data, to avoid calling
+        // |OnDownloadError()| more than once.
+        CancelDownload(id);

[cbentzel, 2011/08/16 15:50:36]

BUG: I'm not sure the CancelDownload is right.

a] It will lead to a garbage memory dereference below with
download_file->bytes_so_far() because download_file will be deleted.

b] The logic in DownloadManager::OnDownloadError is set up so it's only really
executed the first time - the download is removed from the list so subsequent
calls to OnDownloadError are no-ops.

c] There's logic there to change some of the items, and then a PostTask to the
file thread is called with CancelDownload. Keeping this in the main flow is
probably correct.

[ahendrickson, 2011/08/18 21:52:01]

Fixed.  Now get the number of bytes before canceling.
True.
True, but we're making sure that the DownloadFile is deleted even if we don't
make it back to the file thread due to a race condition.
Calling CancelDownload() twice is OK.

[cbentzel, 2011/08/19 13:02:41]

What race condition?

I agree that calling CancelDownload() twice is OK.

However, it seems like this would be simpler if you just removed the call to
CancelDownload(). This should work correctly without the call, and you end up
with less code. 

[ahendrickson, 2011/08/19 18:11:59]

Hmm, I misspoke a bit, I think.  We might not make it back to the file thread if
we're shutting down, rather than due to a race.

However, there is a race condition that we need to consider.  The scenario we're
worried about is this:

We get a file error in DFM::UpdateDownload(), which then posts a message to
DM::OnDownloadError(), and in turn that posts a message to
DFM::CancelDownload().

Before the 2nd post occurs, we get *another* call to DFM::UpdateDownload(),
which tries to write to the file some more.  It may or may not succeed, but if
it does it could corrupt the file.  A later CL will try to resume uploading, but
could end up with a corrupted file.

One example of the write succeeding the second time but not the first is running
out of space on the disk.  Possibly the second time is trying to write less data
to the file, and happens to fit on the disk.  This would end up with a file that
has a missing section, and possibly a corrupted hash.

Putting the CancelDownload() here is safer.

+
+        if (download_manager) {
+          BrowserThread::PostTask(
+              BrowserThread::UI,
+              FROM_HERE,
+              NewRunnableMethod(
+                  download_manager,
+                  &DownloadManager::OnDownloadError,
+                  id,
+                  download_file->bytes_so_far(),
+                  net::ERR_FAILED));
+        }
+      }
+    }
     data->Release();
   }
 }
@@ -181,10 +206,10 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
 void DownloadFileManager::OnResponseCompleted(
     int id,
     DownloadBuffer* buffer,
-    int os_error,
+    int net_error,
     const std::string& security_info) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id
-           << " os_error = " << os_error
+           << " net_error = " << net_error
            << " security_info = \"" << security_info << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   delete buffer;
@@ -204,11 +229,32 @@ void DownloadFileManager::OnResponseCompleted(
   if (!download_file->GetSha256Hash(&hash))
     hash.clear();
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(
-        download_manager, &DownloadManager::OnResponseCompleted,
-        id, download_file->bytes_so_far(), os_error, hash));
+  // 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 (net_error == net::OK || net_error == net::ERR_CONNECTION_CLOSED) {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        NewRunnableMethod(
+            download_manager,
+            &DownloadManager::OnResponseCompleted,
+            id,
+            download_file->bytes_so_far(),
+            hash));
+  } else {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        NewRunnableMethod(
+            download_manager,
+            &DownloadManager::OnDownloadError,
+            id,
+            download_file->bytes_so_far(),
+            net_error));
+  }
   // We need to keep the download around until the UI thread has finalized
   // the name.
 }
@@ -294,7 +340,7 @@ void DownloadFileManager::RenameInProgressDownloadFile(
   if (!download_file->Rename(full_path)) {
     // Error. Between the time the UI thread generated 'full_path' to the time
     // this code runs, something happened that prevents us from renaming.
-    CancelDownloadOnRename(id);
+    CancelDownloadOnRename(id, net::ERR_FAILED);
   }
 }
 
@@ -342,7 +388,7 @@ void DownloadFileManager::RenameCompletingDownloadFile(
   if (!download_file->Rename(new_path)) {
     // Error. Between the time the UI thread generated 'full_path' to the time
     // this code runs, something happened that prevents us from renaming.
-    CancelDownloadOnRename(id);
+    CancelDownloadOnRename(id, net::ERR_FAILED);
     return;
   }
 
@@ -361,7 +407,7 @@ void DownloadFileManager::RenameCompletingDownloadFile(
 
 // Called only from RenameInProgressDownloadFile and
 // RenameCompletingDownloadFile on the FILE thread.
-void DownloadFileManager::CancelDownloadOnRename(int id) {
+void DownloadFileManager::CancelDownloadOnRename(int id, int rename_error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   DownloadFile* download_file = GetDownloadFile(id);
@@ -372,7 +418,7 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
   if (!download_manager) {
     // Without a download manager, we can't cancel the request normally, so we
     // need to do it here.  The normal path will also update the download
-    // history before cancelling the request.
+    // history before canceling the request.
     download_file->CancelDownloadRequest();
     return;
   }
@@ -380,7 +426,10 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::DownloadCancelled, id));
+                        &DownloadManager::OnDownloadError,
+                        id,
+                        download_file->bytes_so_far(),
+                        rename_error));
 }
 
 void DownloadFileManager::EraseDownload(int id) {