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));

[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));
 
   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 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);
 
-  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));
 
   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;

[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) {
   // 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);
@@ skipping 10 matching lines @@
       NewRunnableFunction(&DeleteDownloadedFile, full_path_));
   Remove();
   // We have now been deleted.
 }
 
 void DownloadItem::Remove() {
   // 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_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.
 }
 
 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;
 }