Made the cancel from CancelDownloadOnRename go through the full cancel path,

and added a few more checks for 85408.

BUG=85408


Review URL: http://codereview.chromium.org/7749013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@98474 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/download/download_browsertest.cc

@@ -583,7 +583,7 @@ class DownloadTest : public InProcessBrowserTest {
     return new DownloadsObserver(
         download_manager, num_downloads,
         DownloadItem::COMPLETE,  // Really done
-        false,                   // Bail on select file
+        true,                    // Bail on select file
         ON_DANGEROUS_DOWNLOAD_FAIL);
   }
 

content/browser/download/download_file_manager.cc

@@ -364,7 +364,7 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::DownloadCancelled, id));
+                        &DownloadManager::CancelDownload, id));
 }
 
 void DownloadFileManager::EraseDownload(int id) {

content/browser/download/download_item.cc

@@ -367,7 +367,7 @@ void DownloadItem::Cancel(bool update_history) {
   TransitionTo(CANCELLED);
   StopProgressTimer();
   if (update_history)
-    download_manager_->DownloadCancelled(download_id_);
+    download_manager_->DownloadCancelledInternal(this);
 }
 
 void DownloadItem::MarkAsComplete() {
@@ -703,6 +703,16 @@ FilePath DownloadItem::GetUserVerifiedFilePath() const {
       GetTargetFilePath() : full_path_;
 }
 
+void DownloadItem::OffThreadCancel(DownloadFileManager* file_manager) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  request_handle_.CancelRequest();
+
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      NewRunnableMethod(
+          file_manager, &DownloadFileManager::CancelDownload, download_id_));
+}
+
 void DownloadItem::Init(bool active) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

content/browser/download/download_item.h

@@ -170,7 +170,7 @@ class DownloadItem {
   // to display progress when the DownloadItem should be considered complete.
   void MarkAsComplete();
 
-  // Called by the delegate after it delayed completing the download in 
+  // Called by the delegate after it delayed completing the download in
   // DownloadManagerDelegate::ShouldCompleteDownload.
   void CompleteDelayedDownload();
 
@@ -322,6 +322,14 @@ class DownloadItem {
     return state_info_.target_name != full_path_.BaseName();
   }
 
+  // Cancels the off-thread aspects of the download.
+  // TODO(rdsmith): This should be private and only called from
+  // DownloadItem::Cancel/Interrupt; it isn't now because we can't
+  // call those functions from
+  // DownloadManager::FileSelectionCancelled() without doing some
+  // rewrites of the DownloadManager queues.
+  void OffThreadCancel(DownloadFileManager* file_manager);
+
   std::string DebugString(bool verbose) const;
 
 #ifdef UNIT_TEST

content/browser/download/download_manager.cc

@@ -52,7 +52,8 @@ DownloadManager::DownloadManager(DownloadManagerDelegate* delegate,
       browser_context_(NULL),
       file_manager_(NULL),
       status_updater_(status_updater->AsWeakPtr()),
-      delegate_(delegate) {
+      delegate_(delegate),
+      largest_db_handle_in_history_(DownloadItem::kUninitializedHandle) {
   if (status_updater_)
     status_updater_->AddDelegate(this);
 }
@@ -288,6 +289,8 @@ void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
                                             browser_context_->IsOffTheRecord());
   int32 download_id = info->download_id;
   DCHECK(!ContainsKey(in_progress_, download_id));
+
+  // TODO(rdsmith): Remove after http://crbug.com/85408 resolved.
   CHECK(!ContainsKey(active_downloads_, download_id));
   downloads_.insert(download);
   active_downloads_[download_id] = download;
@@ -506,38 +509,36 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
   delegate_->UpdatePathForItemInPersistentStore(item, full_path);
 }
 
-void DownloadManager::DownloadCancelled(int32 download_id) {
+void DownloadManager::CancelDownload(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator it = in_progress_.find(download_id);
-  if (it == in_progress_.end())
+  DownloadItem* download = GetDownloadItem(download_id);
+  if (!download)
     return;
-  DownloadItem* download = it->second;
 
-  VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
+  download->Cancel(true);
+}
+
+void DownloadManager::DownloadCancelledInternal(DownloadItem* download) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  int download_id = download->id();
+
+  VLOG(20) << __FUNCTION__ << "()"
            << " 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() != DownloadItem::kUninitializedHandle) {
-    in_progress_.erase(it);
+    in_progress_.erase(download_id);
     active_downloads_.erase(download_id);
     UpdateDownloadProgress();  // Reflect removal from in_progress_.
     delegate_->UpdateItemInPersistentStore(download);
+
+    // This function is called from the DownloadItem, so DI state
+    // should already have been updated.
     AssertQueueStateConsistent(download);
   }
 
-  DownloadCancelledInternal(download_id, download->request_handle());
-}
-
-void DownloadManager::DownloadCancelledInternal(
-    int download_id, const DownloadRequestHandle& request_handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle.CancelRequest();
-
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
+  download->OffThreadCancel(file_manager_);
 }
 
 void DownloadManager::OnDownloadError(int32 download_id,
@@ -762,7 +763,7 @@ void DownloadManager::FileSelectionCanceled(void* params) {
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  DownloadCancelledInternal(download_id, download->request_handle());
+  download->OffThreadCancel(file_manager_);
 }
 
 // Operations posted to us from the history service ----------------------------
@@ -771,13 +772,21 @@ void DownloadManager::FileSelectionCanceled(void* params) {
 // 'DownloadPersistentStoreInfo's in sorted order (by ascending start_time).
 void DownloadManager::OnPersistentStoreQueryComplete(
     std::vector<DownloadPersistentStoreInfo>* entries) {
+  // TODO(rdsmith): Remove this and related logic when
+  // http://crbug.com/84508 is fixed.
+  largest_db_handle_in_history_ = 0;
+
   for (size_t i = 0; i < entries->size(); ++i) {
     DownloadItem* download = new DownloadItem(this, entries->at(i));
+    // TODO(rdsmith): Remove after http://crbug.com/85408 resolved.
     CHECK(!ContainsKey(history_downloads_, download->db_handle()));
     downloads_.insert(download);
     history_downloads_[download->db_handle()] = download;
     VLOG(20) << __FUNCTION__ << "()" << i << ">"
              << " download = " << download->DebugString(true);
+
+    if (download->db_handle() > largest_db_handle_in_history_)
+      largest_db_handle_in_history_ = download->db_handle();
   }
   NotifyModelChanged();
   CheckForHistoryFilesRemoval();
@@ -794,6 +803,8 @@ void DownloadManager::AddDownloadItemToHistory(DownloadItem* download,
   DCHECK(download->db_handle() == DownloadItem::kUninitializedHandle);
   download->set_db_handle(db_handle);
 
+  // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508
+  // is fixed.
   CHECK(!ContainsKey(history_downloads_, download->db_handle()));
   history_downloads_[download->db_handle()] = download;
 
@@ -830,6 +841,11 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id,
            << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
+  // TODO(rdsmith): Remove after http://crbug.com/85408 resolved.
+  CHECK(!ContainsKey(history_downloads_, download->db_handle()));
+  int64 largest_handle = largest_db_handle_in_history_;
+  base::debug::Alias(&largest_handle);
+
   AddDownloadItemToHistory(download, db_handle);
 
   // If the download is still in progress, try to complete it.
@@ -842,6 +858,8 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id,
   if (download->IsInProgress()) {
     MaybeCompleteDownload(download);
   } else {
+    // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508
+    // is fixed.
     CHECK(download->IsCancelled())
         << " download = " << download->DebugString(true);
     in_progress_.erase(download_id);
@@ -979,6 +997,11 @@ void DownloadManager::OnSavePageItemAddedToPersistentStore(int32 download_id,
     return;
   }
 
+  // TODO(rdsmith): Remove after http://crbug.com/85408 resolved.
+  CHECK(!ContainsKey(history_downloads_, download->db_handle()));
+  int64 largest_handle = largest_db_handle_in_history_;
+  base::debug::Alias(&largest_handle);
+
   AddDownloadItemToHistory(download, db_handle);
 
   // Finalize this download if it finished before the history callback.

content/browser/download/download_manager.h

@@ -117,8 +117,15 @@ class DownloadManager
   void OnResponseCompleted(int32 download_id, int64 size, int os_error,
                            const std::string& hash);
 
+  // Offthread target for cancelling a particular download.  Will be a no-op
+  // if the download has already been cancelled.
+  void CancelDownload(int32 download_id);
+
+  // Called from DownloadItem to handle the DownloadManager portion of a
+  // Cancel; should not be called from other locations.
+  void DownloadCancelledInternal(DownloadItem* download);
+
   // Called from a view when a user clicks a UI button or link.
-  void DownloadCancelled(int32 download_id);
   void RemoveDownload(int64 download_handle);
 
   // Determine if the download is ready for completion, i.e. has had
@@ -278,10 +285,6 @@ class DownloadManager
   void ContinueDownloadWithPath(DownloadItem* download,
                                 const FilePath& chosen_file);
 
-  // Download cancel helper function.
-  void DownloadCancelledInternal(int download_id,
-                                 const DownloadRequestHandle& request_handle);
-
   // All data has been downloaded.
   // |hash| is sha256 hash for the downloaded file. It is empty when the hash
   // is not available.
@@ -378,6 +381,10 @@ class DownloadManager
   // Allows an embedder to control behavior. Guaranteed to outlive this object.
   DownloadManagerDelegate* delegate_;
 
+  // TODO(rdsmith): Remove when http://crbug.com/84508 is fixed.
+  // For debugging only.
+  int64 largest_db_handle_in_history_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadManager);
 };