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 0c3dca16675ab9f03bee8be3beb5e699cb078464..d657140064e6e9aee25552789d1ab4d9afaca3a6 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -342,6 +342,20 @@ 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));
+  DCHECK(target_state == CANCELLED || target_state == INTERRUPTED);
+
+  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));
@@ -372,27 +386,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
+  // 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; no further code should be executed on this
+    // object.
+  }
 }
 
+
 void DownloadItem::MarkAsComplete() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -492,20 +514,32 @@ void DownloadItem::Observe(int 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; no further code should be executed on this
+    // object.
+  }
 }
 
 void DownloadItem::Delete(DeleteReason reason) {
@@ -536,11 +570,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.
 }