Modified cancel and interrupt processing to avoid race with history.

chrome/browser/download/download_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_manager.h"
 
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
@@ skipping 88 matching lines @@
       // The user hasn't accepted it, so we need to remove it
       // from the disk.  This may or may not result in it being
       // removed from the DownloadManager queues and deleted
       // (specifically, DownloadManager::RemoveDownload only
       // removes and deletes it if it's known to the history service)
       // so the only thing we know after calling this function is that
       // the download was deleted if-and-only-if it was removed
       // from all queues.
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
-      download->Cancel(false);
-      download_history_->UpdateEntry(download);
+      download->Cancel();
     }
   }
 
   // At this point, all dangerous downloads have had their files removed
   // and all in progress downloads have been cancelled.  We can now delete
   // anything left.
 
   // Copy downloads_ to separate container so as not to set off checks
   // in DownloadItem destruction.
   DownloadSet downloads_to_delete;
@@ skipping 516 matching lines @@
   // 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::CancelDownload(int32 download_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadMap::iterator it = active_downloads_.find(download_id);
+  if (it == active_downloads_.end())
+    return;
+
+  it->second->Cancel();
+}
+
 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));
 
   // If it's not in active_downloads_, that means it was cancelled; just
   // ignore the notification.
   if (active_downloads_.count(download_id) == 0)
@@ skipping 54 matching lines @@
     // TODO(rdsmith): Somewhat painful; make sure to disable in
     // release builds after resolution of http://crbug.com/85408.
     for (DownloadMap::iterator it = history_downloads_.begin();
          it != history_downloads_.end(); ++it) {
       CHECK(it->second != download);
     }
   }
 
   CHECK(ContainsKey(active_downloads_, download->id()) ==
         (download->state() == DownloadItem::IN_PROGRESS));
-  CHECK(ContainsKey(in_progress_, download->id()) ==
-        (download->state() == DownloadItem::IN_PROGRESS));
+  // Would check in_progress_, but removal from that queue happens
+  // before transition from IN_PROGRESS for legacy reasons, so skipping.
 }
 
 bool DownloadManager::IsDownloadReadyForCompletion(DownloadItem* download) {
   // If we don't have all the data, the download is not ready for
   // completion.
   if (!download->all_data_saved())
     return false;
 
   // If the download is dangerous, but not yet validated, it's not ready for
   // completion.
@@ skipping 76 matching lines @@
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::CompleteDownload, download_id));
 
   if (uniquifier)
     item->set_path_uniquifier(uniquifier);
 
   item->OnDownloadRenamedToFinalName(full_path);
   download_history_->UpdateDownloadPath(item, full_path);
 }
 
-void DownloadManager::DownloadCancelled(int32 download_id) {
+void DownloadManager::DownloadStopped(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator it = in_progress_.find(download_id);
-  if (it == in_progress_.end())
+  DownloadMap::iterator it = active_downloads_.find(download_id);
+  if (it == active_downloads_.end())
     return;
   DownloadItem* download = it->second;
 
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
-  // 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);
-    UpdateAppIcon();  // Reflect removal from in_progress_.
-    download_history_->UpdateEntry(download);
-  }
+  in_progress_.erase(download_id);
+  active_downloads_.erase(download_id);
+  UpdateAppIcon();  // Reflect removal from in_progress_.
 
-  DownloadCancelledInternal(download_id, download->request_handle());
-}
+  // The history service should always outlive the DownloadManager.
+  download_history_->UpdateEntry(download);
 
-void DownloadManager::DownloadCancelledInternal(
-    int download_id, DownloadRequestHandle request_handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle.CancelRequest();
+  download->request_handle().CancelRequest();
 
+  // TODO(ahendrickson) - Remove this when we add resuming of interrupted
+  // downloads, as we will keep the download item around in that case.
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::CancelDownload, download_id));
 }
 
 void DownloadManager::OnDownloadError(int32 download_id,
                                       int64 size,
                                       int os_error) {
   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;
 
   DownloadItem* download = it->second;
 
   VLOG(20) << __FUNCTION__ << "()" << " Error " << os_error
            << " at offset " << download->received_bytes()
            << " for download = " << download->DebugString(true);
 
-  download->Interrupted(size, os_error);
-
-  // 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);
-    UpdateAppIcon();  // Reflect removal from in_progress_.
-    download_history_->UpdateEntry(download);
-  }
-
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
+  download->Interrupt(size, os_error);
 }
 
 void DownloadManager::UpdateAppIcon() {
   if (status_updater_)
     status_updater_->Update();
 }
 
-void DownloadManager::RemoveDownload(int64 download_handle) {
-  DownloadMap::iterator it = history_downloads_.find(download_handle);
-  if (it == history_downloads_.end())
-    return;
-
-  // Make history update.
-  DownloadItem* download = it->second;
-  download_history_->RemoveEntry(download);
+void DownloadManager::RemoveDownload(DownloadItem* download) {
+  // Silently ignores request if db handle indicates that the download

[ahendrickson, 2011/07/11 20:05:48]

Add

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

now that it's called from another file.

[Randy Smith (Not in Mondays), 2011/07/13 21:11:38]

Done.

+  // isn't in the history.
+  download_history_->RemoveEntry(download->db_handle());
 
   // Remove from our tables and delete.
-  history_downloads_.erase(it);
+  if (download->db_handle() != DownloadHistory::kUninitializedHandle)
+    history_downloads_.erase(download->db_handle());
   int downloads_count = downloads_.erase(download);
   DCHECK_EQ(1, downloads_count);
 
   // Tell observers to refresh their views.
   NotifyModelChanged();
 
   delete download;
 }
 
 int DownloadManager::RemoveDownloadsBetween(const base::Time remove_begin,
@@ skipping 188 matching lines @@
   int32 download_id = *id_ptr;
   delete id_ptr;
 
   DownloadItem* download = GetActiveDownloadItem(download_id);
   if (!download)
     return;
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  DownloadCancelledInternal(download_id, download->request_handle());
+  download->Cancel();
 }
 
 // TODO(phajdan.jr): This is apparently not being exercised in tests.
 bool DownloadManager::IsDangerous(const DownloadItem& download,
                                   const DownloadStateInfo& state,
                                   bool visited_referrer_before) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   bool auto_open = ShouldOpenFileBasedOnExtension(state.suggested_path);
   download_util::DownloadDangerLevel danger_level =
@@ skipping 39 matching lines @@
     VLOG(20) << __FUNCTION__ << "()" << i << ">"
              << " download = " << download->DebugString(true);
   }
   NotifyModelChanged();
   CheckForHistoryFilesRemoval();
 }
 
 void DownloadManager::AddDownloadItemToHistory(DownloadItem* download,
                                                int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
+           << " download = " << download->DebugString(false);
 
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
   // DownloadItems with the same invalid db_handle, so we need to assign a
   // unique |db_handle| here.
   if (db_handle == DownloadHistory::kUninitializedHandle)
     db_handle = download_history_->GetNextFakeDbHandle();
 
+  // The download shouldn't have been returned from GetActiveDownloadItem()
+  // if it's not in progress.
   // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508
   // is fixed.
-  CHECK_NE(DownloadHistory::kUninitializedHandle, db_handle);
+  CHECK(download->IsInProgress());
 
   DCHECK(download->db_handle() == DownloadHistory::kUninitializedHandle);
   download->set_db_handle(db_handle);
 
   DCHECK(!ContainsKey(history_downloads_, download->db_handle()));
   history_downloads_[download->db_handle()] = download;
 }
 
 // Once the new DownloadItem's creation info has been committed to the history
 // service, we associate the DownloadItem with the db handle, update our
 // 'history_downloads_' map and inform observers.
 void DownloadManager::OnCreateDownloadEntryComplete(int32 download_id,
                                                     int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
+           << " download_id = " << download_id;
   DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
+  if (!download) {
+    // The download was cancelled while the history system was entering it.
+    // We resolve this race by turning around and deleting it in the
+    // history system (implicitly treating it as a failure in download
+    // initiation, which is appropriate as the only places the cancel could
+    // have come from were in resolving issues (like the file name) which
+    // we need to have resolved for history system insertion).
+
+    // Make sure we haven't already been shutdown (the callback raced
+    // with shutdown), as that would mean that the history service has
+    // also been shutdown.  In that case, the history will be cleaned up
+    // on next browser start.
+    if (download_history_.get())
+      download_history_->RemoveEntry(db_handle);
     return;
-
-  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
-           << " download_id = " << download_id
-           << " download = " << download->DebugString(true);
+  }
 
   AddDownloadItemToHistory(download, db_handle);
 
   // Show in the appropriate browser UI.
   // This includes buttons to save or cancel, for a dangerous download.
   ShowDownloadInBrowser(download);
 
   // Inform interested objects about the new download.
   NotifyModelChanged();
 
-  // If the download is still in progress, try to complete it.
-  //
-  // Otherwise, download has been cancelled or interrupted before we've
-  // received the DB handle.  We post one final message to the history
-  // service so that it can be properly in sync with the DownloadItem's
-  // completion status, and also inform any observers so that they get
-  // more than just the start notification.
-  if (download->IsInProgress()) {
-    MaybeCompleteDownload(download);
-  } else {
-    DCHECK(download->IsCancelled())
-        << " download = " << download->DebugString(true);
-    in_progress_.erase(download_id);
-    active_downloads_.erase(download_id);
-    download_history_->UpdateEntry(download);
-    download->UpdateObservers();
-  }
+  MaybeCompleteDownload(download);
 }
 
 void DownloadManager::ShowDownloadInBrowser(DownloadItem* download) {
   // The 'contents' may no longer exist if the user closed the tab before we
   // get this start completion event. If it does, tell the origin TabContents
   // to display its download shelf.
   DownloadRequestHandle request_handle = download->request_handle();
   TabContents* contents = request_handle.GetTabContents();
   TabContentsWrapper* wrapper = NULL;
   if (contents)
@@ skipping 28 matching lines @@
   // not its id, so we have to iterate.
   for (DownloadMap::iterator it = history_downloads_.begin();
        it != history_downloads_.end(); ++it) {
     DownloadItem* item = it->second;
     if (item->id() == download_id)
       return item;
   }
   return NULL;
 }
 
+void DownloadManager::GetInProgressDownloads(
+    std::vector<DownloadItem*>* result) {
+  DCHECK(result);
+
+  for (DownloadMap::iterator it = active_downloads_.begin();
+       it != active_downloads_.end(); ++it) {
+    result->push_back(it->second);
+  }
+}
+
 DownloadItem* DownloadManager::GetActiveDownloadItem(int download_id) {
   DCHECK(ContainsKey(active_downloads_, download_id));
   DownloadItem* download = active_downloads_[download_id];
   DCHECK(download != NULL);
   return download;
 }
 
 // Confirm that everything in all maps is also in |downloads_|, and that
 // everything in |downloads_| is also in some other map.
 void DownloadManager::AssertContainersConsistent() const {
@@ skipping 60 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }