Modified cancel and interrupt processing to avoid race with history.

chrome/browser/download/download_item.cc

Index: chrome/browser/download/download_item.cc
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 7f75dc19169707189890a634d6c9d5df4385b3ff..f58496b31a01c4354b18fba47014cac17f4356f1 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -341,6 +341,19 @@ void DownloadItem::UpdateSize(int64 bytes_so_far) {
     total_bytes_ = 0;
 }
 
+void DownloadItem::StopInternal(DownloadState target_state) {
+  // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

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

Could you add a DCHECK that target_state is either CANCELLED or INTERRUPTED?

[Randy Smith (Not in Mondays), 2011/07/15 20:49:08]

Sorry, I don't know how I missed these.  Done.

+
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
+  DCHECK(IsInProgress());
+
+  state_ = target_state;
+  download_manager_->DownloadStopped(download_id_);
+  UpdateObservers();
+  StopProgressTimer();
+}
+
 void DownloadItem::StartProgressTimer() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -371,27 +384,35 @@ void DownloadItem::Update(int64 bytes_so_far) {
   UpdateObservers();
 }
 
-// Triggered by a user action.
-void DownloadItem::Cancel(bool update_history) {
+
+// May be triggered by user action or because of various kinds of error.
+// Note that a cancel that occurs before a download item is persisted
+// into the history system will result in a removal of the Download
+// from the system.
+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;
-  }
+
+  StopInternal(CANCELLED);
 
   download_util::RecordDownloadCount(download_util::CANCELLED_COUNT);
 
-  state_ = CANCELLED;
-  UpdateObservers();
-  StopProgressTimer();
-  if (update_history)
-    download_manager_->DownloadCancelled(download_id_);
+  // History insertion is the point at which we have finalized download

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

Why not put this in StopInternal()?

For a call from Remove(), you could set db_handle_ to
DownloadHistory::kUninitializedHandle first.  Or, if you still needed to call
AssertQueueStateConsistent() afterwards, add a flag to prevent it from removing
the download.

[Randy Smith (Not in Mondays), 2011/07/15 20:49:08]

I don't feel comfortable doing this, both because I'm uncomfortable playing
games with the db_handle to signal to a function that it shouldn't do part of
what it would normally do (primary) and because the "delete self but continue on
in self's methods" pattern is a dangerous one, and I'd rather not push the
delete even further down the call stack *and* make it conditional so that
callers have more of a chance to keep using a deleted items members.  So unless
you feel strongly about this, I'll pass.

+  // details and persist them if something goes wrong.  Before history
+  // insertion, interrupt or cancel results in download removal.
+  if (db_handle() == DownloadHistory::kUninitializedHandle) {
+    download_manager_->RemoveDownload(this);
+    // We are now deleted.
+    return;

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

Nit: Redundant.

[Randy Smith (Not in Mondays), 2011/07/15 20:49:08]

I see why I did it (a more neurotic version of the above, worrying about code
being inserted after the curly brace in the future that would then access the
deleted items member variables), but you're quite right.  Replaced with a
comment.

+  }
 }
 
+
 void DownloadItem::MarkAsComplete() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -491,20 +512,32 @@ void DownloadItem::Observe(NotificationType type,
   Completed();
 }
 
-void DownloadItem::Interrupted(int64 size, int os_error) {
+void DownloadItem::Interrupt(int64 size, int os_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;
-  state_ = INTERRUPTED;
-  last_os_error_ = os_error;
+
   UpdateSize(size);
-  StopProgressTimer();
+  last_os_error_ = os_error;
+
+  StopInternal(INTERRUPTED);
+
   download_util::RecordDownloadInterrupted(os_error,
                                            received_bytes_,
                                            total_bytes_);
-  UpdateObservers();
+
+  // 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() == DownloadHistory::kUninitializedHandle) {
+    download_manager_->RemoveDownload(this);
+    // We are now deleted.
+    return;

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

Nit: Redundant.

[Randy Smith (Not in Mondays), 2011/07/15 20:49:08]

Done.

+  }
 }
 
 void DownloadItem::Delete(DeleteReason reason) {
@@ -535,11 +568,14 @@ void DownloadItem::Remove() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   download_manager_->AssertQueueStateConsistent(this);
-  Cancel(true);
+  if (IsInProgress()) {
+    StopInternal(CANCELLED);
+    download_util::RecordDownloadCount(download_util::CANCELLED_COUNT);
+  }
   download_manager_->AssertQueueStateConsistent(this);
+  download_util::RecordDownloadCount(download_util::REMOVED_COUNT);
 
-  state_ = REMOVING;
-  download_manager_->RemoveDownload(db_handle_);
+  download_manager_->RemoveDownload(this);
   // We have now been deleted.
 }