Detect file system errors during downloads.

chrome/browser/download/download_file_manager.cc

 // Copyright (c) 2011 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 "chrome/browser/download/download_file_manager.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/task.h"
@@ skipping 151 matching lines @@
 // DownloadFile has been deleted.
 void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   std::vector<DownloadBuffer::Contents> contents;
   {
     base::AutoLock auto_lock(buffer->lock);
     contents.swap(buffer->contents);
   }
 
   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();
   }
 }
 
 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;
   DownloadFile* download_file = GetDownloadFile(id);
   if (!download_file)
     return;
 
   download_file->Finish();
 
   DownloadManager* download_manager = download_file->GetDownloadManager();
   if (!download_manager) {
     CancelDownload(id);
     return;
   }
 
   std::string hash;
   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.
 }
 
 // This method will be sent via a user action, or shutdown on the UI thread, and
 // run on the download thread. Since this message has been sent from the UI
 // thread, the download may have already completed and won't exist in our map.
 void DownloadFileManager::CancelDownload(int id) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id;
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ skipping 65 matching lines @@
   DownloadFile* download_file = GetDownloadFile(id);
   if (!download_file)
     return;
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download_file = " << download_file->DebugString();
 
   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);
   }
 }
 
 // The DownloadManager in the UI thread has provided a final name for the
 // download specified by 'id'. Rename the download that's in the process
 // of completing.
 //
 // There are 2 possible rename cases where this method can be called:
 // 1. foo.crdownload -> foo (final, safe)
 // 2. Unconfirmed.xxx.crdownload -> xxx (final, validated)
@@ skipping 27 matching lines @@
     uniquifier = download_util::GetUniquePathNumber(new_path);
     if (uniquifier > 0) {
       download_util::AppendNumberToPath(&new_path, uniquifier);
     }
   }
 
   // Rename the file, overwriting if necessary.
   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;
   }
 
 #if defined(OS_MACOSX)
   // Done here because we only want to do this once; see
   // http://crbug.com/13120 for details.
   download_file->AnnotateWithSourceInformation();
 #endif
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
           download_manager, &DownloadManager::OnDownloadRenamedToFinalName, id,
           new_path, uniquifier));
 }
 
 // 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);
   if (!download_file)
     return;
 
   DownloadManager* download_manager = download_file->GetDownloadManager();
   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;
   }
 
   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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   if (!ContainsKey(downloads_, id))
     return;
 
   DownloadFile* download_file = downloads_[id];
 
   VLOG(20) << " " << __FUNCTION__ << "()"
            << " id = " << id
            << " download_file = " << download_file->DebugString();
 
   downloads_.erase(id);
 
   delete download_file;
 
   if (downloads_.empty())
     StopUpdateTimer();
 }