Revert 101510 - 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

TBR=rdsmith@chromium.org
Review URL: http://codereview.chromium.org/7922013

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

10 files changed, 124 insertions, 140 deletions

diff --git a/content/browser/download/download_item.cc b/content/browser/download/download_item.cc
index a88a438..0809872 100644
--- a/content/browser/download/download_item.cc
+++ b/content/browser/download/download_item.cc
@@ -352,27 +352,24 @@ void DownloadItem::Update(int64 bytes_so_far) {
   UpdateObservers();
 }
 
-void DownloadItem::Cancel() {
+// Triggered by a user action.
+void DownloadItem::Cancel(bool update_history) {
   // 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())
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
+  if (!IsPartialDownload()) {
+    // Small downloads might be complete before this method has
+    // a chance to run.
     return;
-
-  TransitionTo(CANCELLED);
+  }
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
-  // 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.
-  }
+  TransitionTo(CANCELLED);
+  StopProgressTimer();
+  if (update_history)
+    download_manager_->DownloadCancelledInternal(this);
 }
 
 void DownloadItem::MarkAsComplete() {
@@ -431,22 +428,10 @@ void DownloadItem::Completed() {
 }
 
 void DownloadItem::TransitionTo(DownloadState new_state) {
-  DownloadState old_state = state_;
-  if (old_state == new_state)
+  if (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();
 }
 
@@ -468,32 +453,20 @@ void DownloadItem::UpdateTarget() {
     state_info_.target_name = full_path_.BaseName();
 }
 
-void DownloadItem::Interrupt(int64 size, net::Error net_error) {
+void DownloadItem::Interrupted(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;
 
-  UpdateSize(size);
   last_error_ = net_error;
-
-  TransitionTo(INTERRUPTED);
-
+  UpdateSize(size);
+  StopProgressTimer();
   download_stats::RecordDownloadInterrupted(net_error,
                                             received_bytes_,
                                             total_bytes_);
-
-  // 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.
-  }
+  TransitionTo(INTERRUPTED);
 }
 
 void DownloadItem::Delete(DeleteReason reason) {
@@ -524,14 +497,11 @@ void DownloadItem::Remove() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   download_manager_->AssertQueueStateConsistent(this);
-  if (IsInProgress()) {
-    TransitionTo(CANCELLED);
-    download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
-  }
+  Cancel(true);
   download_manager_->AssertQueueStateConsistent(this);
-  download_stats::RecordDownloadCount(download_stats::REMOVED_COUNT);
 
-  download_manager_->RemoveDownload(this);
+  TransitionTo(REMOVING);
+  download_manager_->RemoveDownload(db_handle_);
   // We have now been deleted.
 }
 
diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h
index 7a81aa0..07c43fe 100644
--- a/content/browser/download/download_item.h
+++ b/content/browser/download/download_item.h
@@ -157,11 +157,16 @@ class CONTENT_EXPORT DownloadItem {
   // Received a new chunk of data
   void Update(int64 bytes_so_far);
 
-  // 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();
+  // 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);
 
   // Called by external code (SavePackage) using the DownloadItem interface
   // to display progress when the DownloadItem should be considered complete.
@@ -177,14 +182,10 @@ class CONTENT_EXPORT DownloadItem {
   // Called when the downloaded file is removed.
   void OnDownloadedFileRemoved();
 
-  // 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);
+  // 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);
 
   // 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
@@ -356,8 +357,7 @@ class CONTENT_EXPORT DownloadItem {
   void StartProgressTimer();
   void StopProgressTimer();
 
-  // Does the actual work of transition state; all state
-  // transitions should go through this.
+  // Call to 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 2681e72..06bf42f5 100644
--- a/content/browser/download/download_manager.cc
+++ b/content/browser/download/download_manager.cc
@@ -105,7 +105,8 @@ void DownloadManager::Shutdown() {
       // from all queues.
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
-      download->Cancel();
+      download->Cancel(false);
+      delegate_->UpdateItemInPersistentStore(download);
     }
   }
 
@@ -497,19 +498,23 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
   delegate_->UpdatePathForItemInPersistentStore(item, full_path);
 }
 
-void DownloadManager::DownloadStopped(DownloadItem* download) {
+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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  CHECK(ContainsKey(active_downloads_, download->id()));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  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);
-
+  RemoveFromActiveList(download);
   // This function is called from the DownloadItem, so DI state
   // should already have been updated.
   AssertQueueStateConsistent(download);
@@ -531,7 +536,9 @@ void DownloadManager::OnDownloadError(int32 download_id,
            << " size = " << size
            << " download = " << download->DebugString(true);
 
-  download->Interrupt(size, error);
+  RemoveFromActiveList(download);
+  download->Interrupted(size, error);
+  download->OffThreadCancel(file_manager_);
 }
 
 DownloadItem* DownloadManager::GetActiveDownload(int32 download_id) {
@@ -548,6 +555,20 @@ 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();
 }
@@ -577,9 +598,14 @@ int DownloadManager::RemoveDownloadItems(
   return num_deleted;
 }
 
-void DownloadManager::RemoveDownload(DownloadItem* download) {
-  // Make history update.  Ignores if db_handle isn't in history.
-  delegate_->RemoveItemFromPersistentStore(download->db_handle());
+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);
 
   // Remove from our tables and delete.
   int downloads_count = RemoveDownloadItems(DownloadVector(1, download));
@@ -729,7 +755,12 @@ void DownloadManager::FileSelectionCanceled(void* params) {
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  download->Cancel();
+  // 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_);
 }
 
 // Operations posted to us from the history service ----------------------------
@@ -800,22 +831,8 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id,
                                                            int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* download = GetActiveDownloadItem(download_id);
-  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);
+  if (!download)
     return;
-  }
 
   VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
            << " download_id = " << download_id
@@ -826,10 +843,27 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id,
   base::debug::Alias(&largest_handle);
   CHECK(!ContainsKey(history_downloads_, db_handle));
 
-  CHECK(download->IsInProgress());
   AddDownloadItemToHistory(download, db_handle);
 
-  MaybeCompleteDownload(download);
+  // 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();
+  }
 }
 
 void DownloadManager::ShowDownloadInBrowser(DownloadItem* download) {
@@ -868,16 +902,6 @@ 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 965bdcd..06803ce 100644
--- a/content/browser/download/download_manager.h
+++ b/content/browser/download/download_manager.h
@@ -122,24 +122,22 @@ 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);
 
-  // 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 DownloadItem to handle the DownloadManager portion of a
+  // Cancel; should not be called from other locations.
+  void DownloadCancelledInternal(DownloadItem* download);
 
-  // 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);
+  // Called from a view when a user clicks a UI button or link.
+  void RemoveDownload(int64 download_handle);
 
   // Determine if the download is ready for completion, i.e. has had
   // all data saved, and completed the filename determination and
@@ -275,16 +273,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();
@@ -307,18 +305,17 @@ 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 be72f11..55d25f6 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(int64 db_handle) = 0;
+  virtual void RemoveItemFromPersistentStore(DownloadItem* item) = 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 aba8dc1..c65ebe6 100644
--- a/content/browser/download/download_request_handle.h
+++ b/content/browser/download/download_request_handle.h
@@ -37,15 +37,11 @@ class DownloadRequestHandle {
   TabContents* GetTabContents() const;
   DownloadManager* GetDownloadManager() const;
 
-  // 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.
+  // Pause or resume the matching URL request.
   void PauseRequest() const;
   void ResumeRequest() const;
 
-  // Cancel the request.  Note that while this does not
-  // modify the DownloadRequestHandle, it does modify the
-  // request the handle refers to.
+  // Cancel the request
   void CancelRequest() const;
 
   std::string DebugString() const;
diff --git a/content/browser/download/download_stats.h b/content/browser/download/download_stats.h
index d80cba2..7ca7a23 100644
--- a/content/browser/download/download_stats.h
+++ b/content/browser/download/download_stats.h
@@ -64,9 +64,6 @@ 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 dba6b8c..e5b8b66 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(
-    int64 db_handle) {
+    DownloadItem* item) {
 }
 
 void MockDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween(
diff --git a/content/browser/download/mock_download_manager_delegate.h b/content/browser/download/mock_download_manager_delegate.h
index 453f3449..fc3f8f2 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(int64 db_handle) OVERRIDE;
+  virtual void RemoveItemFromPersistentStore(DownloadItem* item) 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 f0c2be1..1560dd0 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();
+    download_->Cancel(false);
     FinalizeDownloadEntry();
   }
 }