Make cancel remove cancelled download from active queues at time of cancel.

Also add various tests required or enabled by this change.

This changes two aspects of Cancel semantics (for downloads, not save page):
a) Cancel can now be called anytime during the lifetime of a download, and
b) if it is called before the history callback occurs, the download will be removed from the system (no where to store it persistently while waiting)

BUG=85408

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

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

10 files changed, 140 insertions, 124 deletions

diff --git a/content/browser/download/download_item.cc b/content/browser/download/download_item.cc
index 0809872..a88a438 100644
--- a/content/browser/download/download_item.cc
+++ b/content/browser/download/download_item.cc
@@ -352,24 +352,27 @@ void DownloadItem::Update(int64 bytes_so_far) {
   UpdateObservers();
 }
 
-// Triggered by a user action.
-void DownloadItem::Cancel(bool update_history) {
+void DownloadItem::Cancel() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  if (!IsPartialDownload()) {
-    // Small downloads might be complete before this method has
-    // a chance to run.
+
+  // Small downloads might be complete before we have a chance to run.
+  if (!IsInProgress())
     return;
-  }
+
+  TransitionTo(CANCELLED);
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
-  TransitionTo(CANCELLED);
-  StopProgressTimer();
-  if (update_history)
-    download_manager_->DownloadCancelledInternal(this);
+  // History insertion is the point at which we have finalized download
+  // details and persist them if something goes wrong.  Before history
+  // insertion, interrupt or cancel results in download removal.
+  if (db_handle() == DownloadItem::kUninitializedHandle) {
+    download_manager_->RemoveDownload(this);
+    // We are now deleted; no further code should be executed on this
+    // object.
+  }
 }
 
 void DownloadItem::MarkAsComplete() {
@@ -428,10 +431,22 @@ void DownloadItem::Completed() {
 }
 
 void DownloadItem::TransitionTo(DownloadState new_state) {
-  if (state_ == new_state)
+  DownloadState old_state = state_;
+  if (old_state == new_state)
     return;
 
+  // Check for disallowed state transitions.
+  CHECK(!(old_state == IN_PROGRESS && new_state == REMOVING));
+
   state_ = new_state;
+
+  // Do special operations for transitions from an active state.
+  if (old_state == IN_PROGRESS &&
+      (new_state == CANCELLED || new_state == INTERRUPTED)) {
+    download_manager_->DownloadStopped(this);
+    StopProgressTimer();
+  }
+
   UpdateObservers();
 }
 
@@ -453,20 +468,32 @@ void DownloadItem::UpdateTarget() {
     state_info_.target_name = full_path_.BaseName();
 }
 
-void DownloadItem::Interrupted(int64 size, net::Error net_error) {
+void DownloadItem::Interrupt(int64 size, net::Error net_error) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
 
+  // Small downloads might be complete before we have a chance to run.
   if (!IsInProgress())
     return;
 
-  last_error_ = net_error;
   UpdateSize(size);
-  StopProgressTimer();
+  last_error_ = net_error;
+
+  TransitionTo(INTERRUPTED);
+
   download_stats::RecordDownloadInterrupted(net_error,
                                             received_bytes_,
                                             total_bytes_);
-  TransitionTo(INTERRUPTED);
+
+  // History insertion is the point at which we have finalized download
+  // details and persist them if something goes wrong.  Before history
+  // insertion, interrupt or cancel results in download removal.
+  if (db_handle() == DownloadItem::kUninitializedHandle) {
+    download_manager_->RemoveDownload(this);
+    // We are now deleted; no further code should be executed on this
+    // object.
+  }
 }
 
 void DownloadItem::Delete(DeleteReason reason) {
@@ -497,11 +524,14 @@ void DownloadItem::Remove() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   download_manager_->AssertQueueStateConsistent(this);
-  Cancel(true);
+  if (IsInProgress()) {
+    TransitionTo(CANCELLED);
+    download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
+  }
   download_manager_->AssertQueueStateConsistent(this);
+  download_stats::RecordDownloadCount(download_stats::REMOVED_COUNT);
 
-  TransitionTo(REMOVING);
-  download_manager_->RemoveDownload(db_handle_);
+  download_manager_->RemoveDownload(this);
   // We have now been deleted.
 }
 
diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h
index 07c43fe..7a81aa0 100644
--- a/content/browser/download/download_item.h
+++ b/content/browser/download/download_item.h
@@ -157,16 +157,11 @@ class CONTENT_EXPORT DownloadItem {
   // Received a new chunk of data
   void Update(int64 bytes_so_far);
 
-  // Cancel the download operation. We need to distinguish between cancels at
-  // exit (DownloadManager destructor) from user interface initiated cancels
-  // because at exit, the history system may not exist, and any updates to it
-  // require AddRef'ing the DownloadManager in the destructor which results in
-  // a DCHECK failure. Set 'update_history' to false when canceling from at
-  // exit to prevent this crash. This may result in a difference between the
-  // downloaded file's size on disk, and what the history system's last record
-  // of it is. At worst, we'll end up re-downloading a small portion of the file
-  // when resuming a download (assuming the server supports byte ranges).
-  void Cancel(bool update_history);
+  // Cancel the download operation.  This may be called at any time; if
+  // it is called before all download attributes have been finalized and
+  // the download entered into the history, it will remove the download from
+  // the system.
+  void Cancel();
 
   // Called by external code (SavePackage) using the DownloadItem interface
   // to display progress when the DownloadItem should be considered complete.
@@ -182,10 +177,14 @@ class CONTENT_EXPORT DownloadItem {
   // Called when the downloaded file is removed.
   void OnDownloadedFileRemoved();
 
-  // Download operation had an error.
-  // |size| is the amount of data received at interruption.
-  // |error| is the network error code that the operation received.
-  void Interrupted(int64 size, net::Error error);
+  // Download operation had an error; call to interrupt the processing.
+  // Note that if the download attributes haven't yet been finalized and
+  // the download entered into the history (which implies that it hasn't
+  // yet been made visible in the UI), this call will remove the
+  // download from the system.
+  // |size| is the amount of data received so far, and |net_error| is the
+  // error code that the operation received.
+  void Interrupt(int64 size, net::Error net_error);
 
   // Deletes the file from disk and removes the download from the views and
   // history.  |user| should be true if this is the result of the user clicking
@@ -357,7 +356,8 @@ class CONTENT_EXPORT DownloadItem {
   void StartProgressTimer();
   void StopProgressTimer();
 
-  // Call to transition state; all state transitions should go through this.
+  // Does the actual work of transition state; all state
+  // transitions should go through this.
   void TransitionTo(DownloadState new_state);
 
   // Called when safety_state_ should be recomputed from is_dangerous_file
diff --git a/content/browser/download/download_manager.cc b/content/browser/download/download_manager.cc
index 06bf42f5..2681e72 100644
--- a/content/browser/download/download_manager.cc
+++ b/content/browser/download/download_manager.cc
@@ -105,8 +105,7 @@ void DownloadManager::Shutdown() {
       // from all queues.
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
-      download->Cancel(false);
-      delegate_->UpdateItemInPersistentStore(download);
+      download->Cancel();
     }
   }
 
@@ -498,23 +497,19 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
   delegate_->UpdatePathForItemInPersistentStore(item, full_path);
 }
 
-void DownloadManager::CancelDownload(int32 download_id) {
-  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;
-
-  download->Cancel(true);
-}
-
-void DownloadManager::DownloadCancelledInternal(DownloadItem* download) {
+void DownloadManager::DownloadStopped(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  CHECK(ContainsKey(active_downloads_, download->id()));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  RemoveFromActiveList(download);
+  in_progress_.erase(download->id());
+  active_downloads_.erase(download->id());
+  UpdateDownloadProgress();  // Reflect removal from in_progress_.
+  if (download->db_handle() != DownloadItem::kUninitializedHandle)
+    delegate_->UpdateItemInPersistentStore(download);
+
   // This function is called from the DownloadItem, so DI state
   // should already have been updated.
   AssertQueueStateConsistent(download);
@@ -536,9 +531,7 @@ void DownloadManager::OnDownloadError(int32 download_id,
            << " size = " << size
            << " download = " << download->DebugString(true);
 
-  RemoveFromActiveList(download);
-  download->Interrupted(size, error);
-  download->OffThreadCancel(file_manager_);
+  download->Interrupt(size, error);
 }
 
 DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) {
@@ -555,20 +548,6 @@ DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) {
   return download;
 }
 
-void DownloadManager::RemoveFromActiveList(DownloadItem* download) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(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() != DownloadItem::kUninitializedHandle) {
-    in_progress_.erase(download->id());
-    active_downloads_.erase(download->id());
-    UpdateDownloadProgress();  // Reflect removal from in_progress_.
-    delegate_->UpdateItemInPersistentStore(download);
-  }
-}
-
 void DownloadManager::UpdateDownloadProgress() {
   delegate_->DownloadProgressUpdated();
 }
@@ -598,14 +577,9 @@ int DownloadManager::RemoveDownloadItems(
   return num_deleted;
 }
 
-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;
-  delegate_->RemoveItemFromPersistentStore(download);
+void DownloadManager::RemoveDownload(DownloadItem* download) {
+  // Make history update.  Ignores if db_handle isn't in history.
+  delegate_->RemoveItemFromPersistentStore(download->db_handle());
 
   // Remove from our tables and delete.
   int downloads_count = RemoveDownloadItems(DownloadVector(1, download));
@@ -755,12 +729,7 @@ void DownloadManager::FileSelectionCanceled(void* params) {
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  // TODO(ahendrickson) -- This currently has no effect, as the download is
-  // not put on the active list until the file selection is complete.  Need
-  // to put it on the active list earlier in the process.
-  RemoveFromActiveList(download);
-
-  download->OffThreadCancel(file_manager_);
+  download->Cancel();
 }
 
 // Operations posted to us from the history service ----------------------------
@@ -831,8 +800,22 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id,
                                                            int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
+  if (!download) {
+    // The download was cancelled while the persistent store was entering it.
+    // We resolve this race by turning around and deleting it in the
+    // persistent store (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 persistent store insertion).
+
+    // Make sure we haven't already been shutdown (the callback raced
+    // with shutdown), as that would mean that we no longer have access
+    // to the persistent store.  In that case, the history will be cleaned up
+    // on next persistent store query.
+    if (shutdown_needed_)
+      delegate_->RemoveItemFromPersistentStore(db_handle);
     return;
+  }
 
   VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
            << " download_id = " << download_id
@@ -843,27 +826,10 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id,
   base::debug::Alias(&largest_handle);
   CHECK(!ContainsKey(history_downloads_, db_handle));
 
+  CHECK(download->IsInProgress());
   AddDownloadItemToHistory(download, db_handle);
 
-  // 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 {
-    // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508
-    // is fixed.
-    CHECK(download->IsCancelled())
-        << " download = " << download->DebugString(true);
-    in_progress_.erase(download_id);
-    active_downloads_.erase(download_id);
-    delegate_->UpdateItemInPersistentStore(download);
-    download->UpdateObservers();
-  }
+  MaybeCompleteDownload(download);
 }
 
 void DownloadManager::ShowDownloadInBrowser(DownloadItem* download) {
@@ -902,6 +868,16 @@ DownloadItem* DownloadManager::GetDownloadItem(int download_id) {
   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];
diff --git a/content/browser/download/download_manager.h b/content/browser/download/download_manager.h
index 06803ce..965bdcd 100644
--- a/content/browser/download/download_manager.h
+++ b/content/browser/download/download_manager.h
@@ -122,22 +122,24 @@ class CONTENT_EXPORT DownloadManager
   void OnResponseCompleted(int32 download_id, int64 size,
                            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 when there is an error in the download.
   // |download_id| is the ID of the download.
   // |size| is the number of bytes that are currently downloaded.
   // |error| is a download error code.  Indicates what caused the interruption.
   void OnDownloadError(int32 download_id, int64 size, net::Error error);
 
-  // Called from DownloadItem to handle the DownloadManager portion of a
-  // Cancel; should not be called from other locations.
-  void DownloadCancelledInternal(DownloadItem* download);
+  // This routine is called from the DownloadItem when a
+  // request is cancelled or interrupted.  It removes the download
+  // from all internal queues holding in-progress work, and takes care
+  // of the off-thread aspects of the cancel (stopping the request,
+  // cancelling the download on the file thread).
+  void DownloadStopped(DownloadItem* download);
 
-  // Called from a view when a user clicks a UI button or link.
-  void RemoveDownload(int64 download_handle);
+  // Called from DownloadItem when the download is being removed.
+  // Takes care of all history operations, modifying internal queues,
+  // and notifying DownloadManager observers, and actually deletes
+  // the DownloadItem.
+  void RemoveDownload(DownloadItem* download);
 
   // Determine if the download is ready for completion, i.e. has had
   // all data saved, and completed the filename determination and
@@ -273,16 +275,16 @@ class CONTENT_EXPORT DownloadManager
   typedef std::set<DownloadItem*> DownloadSet;
   typedef base::hash_map<int64, DownloadItem*> DownloadMap;
 
+  friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
+  friend class DeleteTask<DownloadManager>;
+  friend class base::RefCountedThreadSafe<DownloadManager,
+                                          BrowserThread::DeleteOnUIThread>;
+
   // For testing.
   friend class DownloadManagerTest;
   friend class MockDownloadManager;
   friend class DownloadTest;
 
-  friend class base::RefCountedThreadSafe<DownloadManager,
-                                          BrowserThread::DeleteOnUIThread>;
-  friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
-  friend class DeleteTask<DownloadManager>;
-
   void set_delegate(DownloadManagerDelegate* delegate) { delegate_ = delegate; }
 
   virtual ~DownloadManager();
@@ -305,17 +307,18 @@ class CONTENT_EXPORT DownloadManager
   // Returns NULL if the download is not active.
   DownloadItem* GetActiveDownload(int32 download_id);
 
-  // Removes |download| from the active and in progress maps.
-  // Called when the download is cancelled or has an error.
-  // Does nothing if the download is not in the history DB.
-  void RemoveFromActiveList(DownloadItem* download);
-
   // Updates the delegate about the overall download progress.
   void UpdateDownloadProgress();
 
   // Inform observers that the model has changed.
   void NotifyModelChanged();
 
+  // Return all in progress downloads.  This includes downloads that
+  // have not yet been entered into the history (all other accessors
+  // only return downloads that have been entered into the history).
+  // This is intended to be used for testing only.
+  void GetInProgressDownloads(std::vector<DownloadItem*>* result);
+
   // Debugging routine to confirm relationship between below
   // containers; no-op if NDEBUG.
   void AssertContainersConsistent() const;
diff --git a/content/browser/download/download_manager_delegate.h b/content/browser/download/download_manager_delegate.h
index 55d25f6..be72f11 100644
--- a/content/browser/download/download_manager_delegate.h
+++ b/content/browser/download/download_manager_delegate.h
@@ -84,7 +84,7 @@ class DownloadManagerDelegate {
 
   // Notifies the delegate that it should remove the download item from its
   // persistent store.
-  virtual void RemoveItemFromPersistentStore(DownloadItem* item) = 0;
+  virtual void RemoveItemFromPersistentStore(int64 db_handle) = 0;
 
   // Notifies the delegate to remove downloads from the given time range.
   virtual void RemoveItemsFromPersistentStoreBetween(
diff --git a/content/browser/download/download_request_handle.h b/content/browser/download/download_request_handle.h
index c65ebe6..aba8dc1 100644
--- a/content/browser/download/download_request_handle.h
+++ b/content/browser/download/download_request_handle.h
@@ -37,11 +37,15 @@ class DownloadRequestHandle {
   TabContents* GetTabContents() const;
   DownloadManager* GetDownloadManager() const;
 
-  // Pause or resume the matching URL request.
+  // Pause or resume the matching URL request.  Note that while these
+  // do not modify the DownloadRequestHandle, they do modify the
+  // request the handle refers to.
   void PauseRequest() const;
   void ResumeRequest() const;
 
-  // Cancel the request
+  // Cancel the request.  Note that while this does not
+  // modify the DownloadRequestHandle, it does modify the
+  // request the handle refers to.
   void CancelRequest() const;
 
   std::string DebugString() const;
diff --git a/content/browser/download/download_stats.h b/content/browser/download/download_stats.h
index 7ca7a23..d80cba2 100644
--- a/content/browser/download/download_stats.h
+++ b/content/browser/download/download_stats.h
@@ -64,6 +64,9 @@ enum DownloadCountTypes {
   // Counts iterations of the BaseFile::AppendDataToFile() loop.
   WRITE_LOOP_COUNT,
 
+  // Downloads that were removed by the user.
+  REMOVED_COUNT,
+
   DOWNLOAD_COUNT_TYPES_LAST_ENTRY
 };
 
diff --git a/content/browser/download/mock_download_manager_delegate.cc b/content/browser/download/mock_download_manager_delegate.cc
index e5b8b66..dba6b8c 100644
--- a/content/browser/download/mock_download_manager_delegate.cc
+++ b/content/browser/download/mock_download_manager_delegate.cc
@@ -65,7 +65,7 @@ void MockDownloadManagerDelegate::UpdatePathForItemInPersistentStore(
 }
 
 void MockDownloadManagerDelegate::RemoveItemFromPersistentStore(
-    DownloadItem* item) {
+    int64 db_handle) {
 }
 
 void MockDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween(
diff --git a/content/browser/download/mock_download_manager_delegate.h b/content/browser/download/mock_download_manager_delegate.h
index fc3f8f2..453f3449 100644
--- a/content/browser/download/mock_download_manager_delegate.h
+++ b/content/browser/download/mock_download_manager_delegate.h
@@ -31,7 +31,7 @@ class MockDownloadManagerDelegate : public DownloadManagerDelegate {
   virtual void UpdatePathForItemInPersistentStore(
       DownloadItem* item,
       const FilePath& new_path) OVERRIDE;
-  virtual void RemoveItemFromPersistentStore(DownloadItem* item) OVERRIDE;
+  virtual void RemoveItemFromPersistentStore(int64 db_handle) OVERRIDE;
   virtual void RemoveItemsFromPersistentStoreBetween(
       const base::Time remove_begin,
       const base::Time remove_end) OVERRIDE;
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index 1560dd0..f0c2be1 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -626,7 +626,7 @@ void SavePackage::Stop() {
 
   // Inform the DownloadItem we have canceled whole save page job.
   if (download_) {
-    download_->Cancel(false);
+    download_->Cancel();
     FinalizeDownloadEntry();
   }
 }