Modified cancel and interrupt processing to avoid race with history.

chrome/browser/download/download_item.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/download/download_item.h"
 
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/i18n/case_conversion.h"
@@ skipping 323 matching lines @@
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   received_bytes_ = bytes_so_far;
 
   // If we've received more data than we were expecting (bad server info?),
   // revert to 'unknown size mode'.
   if (received_bytes_ > total_bytes_)
     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));
+
+  VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
+  DCHECK(IsInProgress());
+
+  download_util::RecordDownloadCount(download_util::CANCELLED_COUNT);

[Randy Smith (Not in Mondays), 2011/06/30 23:05:13]

This recording is incorrect--it should be only in the callers.

[Randy Smith (Not in Mondays), 2011/07/05 20:28:44]

Done.

+
+  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));
 
   update_timer_.Start(base::TimeDelta::FromMilliseconds(kUpdateTimeMs), this,
                       &DownloadItem::UpdateObservers);
 }
 
 void DownloadItem::StopProgressTimer() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
@@ skipping 10 matching lines @@
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!IsInProgress()) {
     NOTREACHED();
     return;
   }
   UpdateSize(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 downoad item is persisted

[Randy Smith (Not in Mondays), 2011/06/30 23:05:13]

"download"

[Randy Smith (Not in Mondays), 2011/07/05 20:28:44]

Done.

+// 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);
 
-  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.
+    return;
+  }
 }
 
+
 void DownloadItem::MarkAsComplete() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(all_data_saved_);
   state_ = COMPLETE;
   UpdateObservers();
 }
 
 void DownloadItem::OnAllDataSaved(int64 size) {
@@ skipping 79 matching lines @@
   registrar_.Remove(this,
                     NotificationType::CRX_INSTALLER_DONE,
                     source);
 
   auto_opened_ = true;
   DCHECK(all_data_saved_);
 
   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;
+
+  UpdateSize(size);
   last_os_error_ = os_error;
-  UpdateSize(size);
-  StopProgressTimer();
+
+  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;
+  }
 }
 
 void DownloadItem::Delete(DeleteReason reason) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   switch (reason) {
     case DELETE_DUE_TO_USER_DISCARD:
       UMA_HISTOGRAM_ENUMERATION("Download.UserDiscard", GetDangerType(),
                                 DANGEROUS_TYPE_MAX);
       break;
     case DELETE_DUE_TO_BROWSER_SHUTDOWN:
       UMA_HISTOGRAM_ENUMERATION("Download.Discard", GetDangerType(),
                                 DANGEROUS_TYPE_MAX);
       break;
     default:
       NOTREACHED();
   }
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       NewRunnableFunction(&DeleteDownloadedFile, full_path_));
   Remove();
   // We have now been deleted.
 }
 
 void DownloadItem::Remove() {

[Randy Smith (Not in Mondays), 2011/06/30 23:05:13]

We need a new UMA count here for removals (or maybe to add to the cancel count
if we actually cancel it).

[Randy Smith (Not in Mondays), 2011/07/05 20:28:44]

Done.

   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   download_manager_->AssertQueueStateConsistent(this);
-  Cancel(true);
+  if (IsInProgress())
+    StopInternal(CANCELLED);
   download_manager_->AssertQueueStateConsistent(this);
 
-  state_ = REMOVING;
-  download_manager_->RemoveDownload(db_handle_);
+  download_manager_->RemoveDownload(this);
   // We have now been deleted.
 }
 
 bool DownloadItem::TimeRemaining(base::TimeDelta* remaining) const {
   if (total_bytes_ <= 0)
     return false;  // We never received the content_length for this download.
 
   int64 speed = CurrentSpeed();
   if (speed == 0)
     return false;
@@ skipping 283 matching lines @@
         state_info_.target_name.value().c_str(),
         full_path().value().c_str());
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
 
   description += " }";
 
   return description;
 }