Modified cancel and interrupt processing to avoid race with history.

Avoid racing with the history callback by unilaterally removing
DownloadItem from queues on cancel/interrupt.  This keeps the
state<->queue correspondence cleaner, and avoids leaving things on
queues during shutdown.  It might also fix 85408; we'll see :-}.

BUG=85408
TEST=

Review URL: http://codereview.chromium.org/7294013

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

12 files changed, 713 insertions, 270 deletions

diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 7bebb65..908b665 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -28,6 +28,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/shell_dialogs.h"
 #include "chrome/browser/ui/webui/active_downloads_ui.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_paths.h"
@@ -57,6 +58,7 @@ const FilePath kLargeThemePath(FILE_PATH_LITERAL("extensions/theme2.crx"));
 enum DangerousDownloadAction {
   ON_DANGEROUS_DOWNLOAD_ACCEPT,  // Accept the download
   ON_DANGEROUS_DOWNLOAD_DENY,  // Deny the download
+  ON_DANGEROUS_DOWNLOAD_IGNORE,  // Don't do anything; calling code will handle.
   ON_DANGEROUS_DOWNLOAD_FAIL  // Fail if a dangerous download is seen
 };
 
@@ -72,7 +74,7 @@ void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager,
                            int32 download_id) {
   DownloadItem* download = download_manager->GetDownloadItem(download_id);
   ASSERT_TRUE(download->IsPartialDownload());
-  download->Cancel(true);
+  download->Cancel();
   download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
 }
 
@@ -91,8 +93,10 @@ void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager,
 class DownloadsObserver : public DownloadManager::Observer,
                           public DownloadItem::Observer {
  public:
+  typedef std::set<DownloadItem::DownloadState> StateSet;
+
   // Create an object that will be considered finished when |wait_count|
-  // download items have entered state |download_finished_state|.
+  // download items have entered any states in |download_finished_states|.
   // If |finish_on_select_file| is true, the object will also be
   // considered finished if the DownloadManager raises a
   // SelectFileDialogDisplayed() notification.
@@ -101,20 +105,21 @@ class DownloadsObserver : public DownloadManager::Observer,
   // to treat as completion events.
   DownloadsObserver(DownloadManager* download_manager,
                     size_t wait_count,
-                    DownloadItem::DownloadState download_finished_state,
+                    StateSet download_finished_states,
                     bool finish_on_select_file,
                     DangerousDownloadAction dangerous_download_action)
       : download_manager_(download_manager),
         wait_count_(wait_count),
         finished_downloads_at_construction_(0),
         waiting_(false),
-        download_finished_state_(download_finished_state),
+        download_finished_states_(download_finished_states),
         finish_on_select_file_(finish_on_select_file),
         select_file_dialog_seen_(false),
         dangerous_download_action_(dangerous_download_action) {
     download_manager_->AddObserver(this);  // Will call initial ModelChanged().
     finished_downloads_at_construction_ = finished_downloads_.size();
-    EXPECT_NE(DownloadItem::REMOVING, download_finished_state)
+    EXPECT_TRUE(download_finished_states.find(DownloadItem::REMOVING) ==
+                download_finished_states.end())
         << "Waiting for REMOVING is not supported.  Try COMPLETE.";
   }
 
@@ -193,12 +198,16 @@ class DownloadsObserver : public DownloadManager::Observer,
           ADD_FAILURE() << "Unexpected dangerous download item.";
           break;
 
+        case ON_DANGEROUS_DOWNLOAD_IGNORE:
+          break;
+
         default:
           NOTREACHED();
       }
     }
 
-    if (download->state() == download_finished_state_) {
+    if (download_finished_states_.find(download->state()) !=
+        download_finished_states_.end()) {
       DownloadInFinalState(download);
     }
   }
@@ -301,8 +310,8 @@ class DownloadsObserver : public DownloadManager::Observer,
   // all downloads completing.
   bool waiting_;
 
-  // The state on which to consider the DownloadItem finished.
-  DownloadItem::DownloadState download_finished_state_;
+  // The states on which to consider the DownloadItem finished.
+  StateSet download_finished_states_;
 
   // True if we should transition the DownloadsObserver to finished if
   // the select file dialog comes up.
@@ -490,6 +499,177 @@ class CancelTestDataCollector
   DISALLOW_COPY_AND_ASSIGN(CancelTestDataCollector);
 };
 
+static const DownloadItem::DownloadState kTerminalStates[] = {
+  DownloadItem::CANCELLED,
+  DownloadItem::INTERRUPTED,
+  DownloadItem::COMPLETE,
+};
+
+static const DownloadItem::DownloadState kInProgressStates[] = {
+  DownloadItem::IN_PROGRESS,
+};
+
+// Get History Information.
+class DownloadsHistoryDataCollector {
+ public:
+  explicit DownloadsHistoryDataCollector(int64 download_db_handle,
+                                         DownloadManager* manager)
+      : result_valid_(false),
+        download_db_handle_(download_db_handle) {
+    DCHECK(manager);
+    HistoryService* hs =
+        manager->profile()->GetHistoryService(Profile::EXPLICIT_ACCESS);
+    DCHECK(hs);
+    hs->QueryDownloads(
+        &callback_consumer_,
+        NewCallback(this,
+                    &DownloadsHistoryDataCollector::OnQueryDownloadsComplete));
+
+    // Cannot complete immediately because the history backend runs on a
+    // separate thread, so we can assume that the RunMessageLoop below will
+    // be exited by the Quit in OnQueryDownloadsComplete.
+    ui_test_utils::RunMessageLoop();
+  }
+
+  bool GetDownloadsHistoryEntry(DownloadHistoryInfo* result) {
+    DCHECK(result);
+    *result = result_;
+    return result_valid_;
+  }
+
+ private:
+  void OnQueryDownloadsComplete(
+      std::vector<DownloadHistoryInfo>* entries) {
+    result_valid_ = false;
+    for (std::vector<DownloadHistoryInfo>::const_iterator it = entries->begin();
+         it != entries->end(); ++it) {
+      if (it->db_handle == download_db_handle_) {
+        result_ = *it;
+        result_valid_ = true;
+        break;
+      }
+    }
+    MessageLoopForUI::current()->Quit();
+  }
+
+  DownloadHistoryInfo result_;
+  bool result_valid_;
+  int64 download_db_handle_;
+  CancelableRequestConsumer callback_consumer_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadsHistoryDataCollector);
+};
+
+// Mock that simulates a permissions dialog where the user denies
+// permission to install.  TODO(skerner): This could be shared with
+// extensions tests.  Find a common place for this class.
+class MockAbortExtensionInstallUI : public ExtensionInstallUI {
+ public:
+  MockAbortExtensionInstallUI() : ExtensionInstallUI(NULL) {}
+
+  // Simulate a user abort on an extension installation.
+  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
+    delegate->InstallUIAbort(true);
+    MessageLoopForUI::current()->Quit();
+  }
+
+  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
+  virtual void OnInstallFailure(const std::string& error) {}
+};
+
+// Mock that simulates a permissions dialog where the user allows
+// installation.
+class MockAutoConfirmExtensionInstallUI : public ExtensionInstallUI {
+ public:
+  explicit MockAutoConfirmExtensionInstallUI(Profile* profile) :
+      ExtensionInstallUI(profile) {}
+
+  // Proceed without confirmation prompt.
+  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
+    delegate->InstallUIProceed();
+  }
+
+  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
+  virtual void OnInstallFailure(const std::string& error) {}
+};
+
+// While any objects of this class exists, all DownloadItems added to the
+// specified download manager will have opening mocked out.
+class MockDownloadOpeningObserver : public DownloadManager::Observer {
+ public:
+  explicit MockDownloadOpeningObserver(DownloadManager* manager)
+      : download_manager_(manager) {
+    download_manager_->AddObserver(this);
+  }
+
+  ~MockDownloadOpeningObserver() {
+    download_manager_->RemoveObserver(this);
+  }
+
+  // DownloadManager::Observer
+  virtual void ModelChanged() {
+    std::vector<DownloadItem*> downloads;
+    download_manager_->SearchDownloads(string16(), &downloads);
+
+    for (std::vector<DownloadItem*>::iterator it = downloads.begin();
+         it != downloads.end(); ++it) {
+      (*it)->TestMockDownloadOpen();
+    }
+  }
+
+ private:
+  DownloadManager* download_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockDownloadOpeningObserver);
+};
+
+// No-ops select files from the download manager.
+// Returns a cancel callback on destruction.
+class MockSelectFileDialog : public SelectFileDialog {
+ public:
+  explicit MockSelectFileDialog(SelectFileDialog::Listener* listener)
+      : SelectFileDialog(listener),
+        listener_(listener),
+        params_(NULL),
+        active_(false) {}
+  ~MockSelectFileDialog() {
+    if (listener_)
+      listener_->FileSelectionCanceled(params_);
+  }
+
+  static MockSelectFileDialog* Create(Listener* listener) {
+    return new MockSelectFileDialog(listener);
+  }
+
+  virtual bool IsRunning(gfx::NativeWindow owning_window) const {
+    return active_;
+  }
+
+  virtual void ListenerDestroyed() {
+    listener_ = NULL;
+  }
+
+  virtual void SelectFileImpl(SelectFileDialog::Type type,
+                              const string16& title,
+                              const FilePath& default_path,
+                              const FileTypeInfo* file_types,
+                              int file_type_index,
+                              const FilePath::StringType& default_extension,
+                              gfx::NativeWindow owning_window,
+                              void *params) {
+    active_ = true;
+    params_ = params;
+  }
+ private:
+  SelectFileDialog::Listener* listener_;
+  void* params_;
+  bool active_;
+};
+
+}  // namespace
+
+// Not in anonymous namespace so that friend class from DownloadManager
+// can target it.
 class DownloadTest : public InProcessBrowserTest {
  public:
   enum SelectExpectation {
@@ -566,6 +746,12 @@ class DownloadTest : public InProcessBrowserTest {
     return true;
   }
 
+  // For tests that want to test system reaction to files
+  // going away underneath them.
+  void DeleteDownloadsDirectory() {
+    EXPECT_TRUE(downloads_directory_.Delete());
+  }
+
   DownloadPrefs* GetDownloadPrefs(Browser* browser) {
     return browser->profile()->GetDownloadManager()->download_prefs();
   }
@@ -583,12 +769,29 @@ class DownloadTest : public InProcessBrowserTest {
         browser->profile()->GetDownloadManager();
     return new DownloadsObserver(
         download_manager, num_downloads,
-        DownloadItem::COMPLETE,  // Really done
-        false,                   // Bail on select file
+        DownloadsObserver::StateSet(
+            kTerminalStates, kTerminalStates + arraysize(kTerminalStates)),
+        true,                   // Bail on select file
         ON_DANGEROUS_DOWNLOAD_FAIL);
   }
 
   // Create a DownloadsObserver that will wait for the
+  // specified number of downloads to finish, and is
+  // ok with dangerous downloads.  Note that use of this
+  // waiter is conditional on accepting the dangerous download.
+  DownloadsObserver* CreateDangerousWaiter(
+      Browser* browser, int num_downloads) {
+    DownloadManager* download_manager =
+        browser->profile()->GetDownloadManager();
+    return new DownloadsObserver(
+        download_manager, num_downloads,
+        DownloadsObserver::StateSet(
+            kTerminalStates, kTerminalStates + arraysize(kTerminalStates)),
+        true,                   // Bail on select file
+        ON_DANGEROUS_DOWNLOAD_IGNORE);
+  }
+
+  // Create a DownloadsObserver that will wait for the
   // specified number of downloads to start.
   DownloadsObserver* CreateInProgressWaiter(Browser* browser,
                                             int num_downloads) {
@@ -596,9 +799,11 @@ class DownloadTest : public InProcessBrowserTest {
         browser->profile()->GetDownloadManager();
     return new DownloadsObserver(
         download_manager, num_downloads,
-        DownloadItem::IN_PROGRESS,      // Has started
+        DownloadsObserver::StateSet(
+            kInProgressStates,
+            kInProgressStates + arraysize(kInProgressStates)),
         true,                           // Bail on select file
-        ON_DANGEROUS_DOWNLOAD_FAIL);
+        ON_DANGEROUS_DOWNLOAD_IGNORE);
   }
 
   // Create a DownloadsObserver that will wait for the
@@ -609,11 +814,13 @@ class DownloadTest : public InProcessBrowserTest {
       int num_downloads,
       DownloadItem::DownloadState final_state,
       DangerousDownloadAction dangerous_download_action) {
+    DownloadsObserver::StateSet states;
+    states.insert(final_state);
     DownloadManager* download_manager =
         browser->profile()->GetDownloadManager();
     return new DownloadsObserver(
         download_manager, num_downloads,
-        final_state,
+        states,
         true,                         // Bail on select file
         dangerous_download_action);
   }
@@ -662,6 +869,19 @@ class DownloadTest : public InProcessBrowserTest {
         ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
   }
 
+  // Mock or unmock the select file dialog for the download manager
+  // associated with the specified browser.
+  void NoopSelectFileDialog(Browser* browser, bool mock) {
+    if (mock) {
+      scoped_refptr<SelectFileDialog> dialog(
+          MockSelectFileDialog::Create(
+              browser->profile()->GetDownloadManager()));
+      browser->profile()->GetDownloadManager()->SetSelectFileDialog(dialog);
+    } else {
+      browser->profile()->GetDownloadManager()->SetSelectFileDialog(NULL);
+    }
+  }
+
   // Should only be called when the download is known to have finished
   // (in error or not).
   // Returning false indicates a failure of the function, and should be asserted
@@ -768,12 +988,22 @@ class DownloadTest : public InProcessBrowserTest {
     return true;
   }
 
-  void GetDownloads(Browser* browser, std::vector<DownloadItem*>* downloads) {
+  void GetPersistentDownloads(Browser* browser,
+                              std::vector<DownloadItem*>* downloads) {
     DCHECK(downloads);
+    downloads->clear();
     DownloadManager* manager = browser->profile()->GetDownloadManager();
     manager->SearchDownloads(string16(), downloads);
   }
 
+  void GetInProgressDownloads(Browser* browser,
+                              std::vector<DownloadItem*>* downloads) {
+    downloads->clear();
+    DCHECK(downloads);
+    DownloadManager* manager = browser->profile()->GetDownloadManager();
+    manager->GetInProgressDownloads(downloads);
+  }
+
   // Check that the download UI (shelf on non-chromeos or panel on chromeos)
   // is visible or not as expected. Additionally, check that the filename
   // is present in the UI (currently only on chromeos).
@@ -823,120 +1053,6 @@ class DownloadTest : public InProcessBrowserTest {
   ScopedTempDir downloads_directory_;
 };
 
-// Get History Information.
-class DownloadsHistoryDataCollector {
- public:
-  explicit DownloadsHistoryDataCollector(int64 download_db_handle,
-                                         DownloadManager* manager)
-      : result_valid_(false),
-        download_db_handle_(download_db_handle) {
-    HistoryService* hs =
-        manager->profile()->GetHistoryService(Profile::EXPLICIT_ACCESS);
-    DCHECK(hs);
-    hs->QueryDownloads(
-        &callback_consumer_,
-        NewCallback(this,
-                    &DownloadsHistoryDataCollector::OnQueryDownloadsComplete));
-
-    // Cannot complete immediately because the history backend runs on a
-    // separate thread, so we can assume that the RunMessageLoop below will
-    // be exited by the Quit in OnQueryDownloadsComplete.
-    ui_test_utils::RunMessageLoop();
-  }
-
-  bool GetDownloadsHistoryEntry(DownloadHistoryInfo* result) {
-    DCHECK(result);
-    *result = result_;
-    return result_valid_;
-  }
-
- private:
-  void OnQueryDownloadsComplete(
-      std::vector<DownloadHistoryInfo>* entries) {
-    result_valid_ = false;
-    for (std::vector<DownloadHistoryInfo>::const_iterator it = entries->begin();
-         it != entries->end(); ++it) {
-      if (it->db_handle == download_db_handle_) {
-        result_ = *it;
-        result_valid_ = true;
-      }
-    }
-    MessageLoopForUI::current()->Quit();
-  }
-
-  DownloadHistoryInfo result_;
-  bool result_valid_;
-  int64 download_db_handle_;
-  CancelableRequestConsumer callback_consumer_;
-
-  DISALLOW_COPY_AND_ASSIGN(DownloadsHistoryDataCollector);
-};
-
-// Mock that simulates a permissions dialog where the user denies
-// permission to install.  TODO(skerner): This could be shared with
-// extensions tests.  Find a common place for this class.
-class MockAbortExtensionInstallUI : public ExtensionInstallUI {
- public:
-  MockAbortExtensionInstallUI() : ExtensionInstallUI(NULL) {}
-
-  // Simulate a user abort on an extension installation.
-  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
-    delegate->InstallUIAbort(true);
-    MessageLoopForUI::current()->Quit();
-  }
-
-  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
-  virtual void OnInstallFailure(const std::string& error) {}
-};
-
-// Mock that simulates a permissions dialog where the user allows
-// installation.
-class MockAutoConfirmExtensionInstallUI : public ExtensionInstallUI {
- public:
-  explicit MockAutoConfirmExtensionInstallUI(Profile* profile) :
-      ExtensionInstallUI(profile) {}
-
-  // Proceed without confirmation prompt.
-  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
-    delegate->InstallUIProceed();
-  }
-
-  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
-  virtual void OnInstallFailure(const std::string& error) {}
-};
-
-}  // namespace
-
-// While an object of this class exists, it will mock out download
-// opening for all downloads created on the specified download manager.
-class MockDownloadOpeningObserver : public DownloadManager::Observer {
- public:
-  explicit MockDownloadOpeningObserver(DownloadManager* manager)
-      : download_manager_(manager) {
-    download_manager_->AddObserver(this);
-  }
-
-  ~MockDownloadOpeningObserver() {
-    download_manager_->RemoveObserver(this);
-  }
-
-  // DownloadManager::Observer
-  virtual void ModelChanged() {
-    std::vector<DownloadItem*> downloads;
-    download_manager_->SearchDownloads(string16(), &downloads);
-
-    for (std::vector<DownloadItem*>::iterator it = downloads.begin();
-         it != downloads.end(); ++it) {
-      (*it)->TestMockDownloadOpen();
-    }
-  }
-
- private:
-  DownloadManager* download_manager_;
-
-  DISALLOW_COPY_AND_ASSIGN(MockDownloadOpeningObserver);
-};
-
 // NOTES:
 //
 // Files for these tests are found in DIR_TEST_DATA (currently
@@ -980,22 +1096,109 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CheckInternetZone) {
 }
 #endif
 
-// Put up a Select File dialog when the file is downloaded, due to its MIME
-// type.
-//
-// This test runs correctly, but leaves behind turds in the test user's
-// download directory because of http://crbug.com/62099.  No big loss; it
-// was primarily confirming DownloadsObserver wait on select file dialog
-// functionality anyway.
-IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DownloadMimeTypeSelect) {
+// Put up a Select File dialog when the file is downloaded, due to
+// prompt_for_download==true argument to InitialSetup().
+// Confirm that we can cancel the download in that state.
+IN_PROC_BROWSER_TEST_F(DownloadTest, CancelFromFileSelection) {
+  ASSERT_TRUE(InitialSetup(true));
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // Setup select file interposition.
+  NoopSelectFileDialog(browser(), true);
+
+  // Download the file and wait.  We expect the Select File dialog to appear.
+  DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG);
+
+  std::vector<DownloadItem*> active_downloads, history_downloads;
+  GetInProgressDownloads(browser(), &active_downloads);
+  ASSERT_EQ(1u, active_downloads.size());
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, active_downloads[0]->state());
+  GetPersistentDownloads(browser(), &history_downloads);
+  EXPECT_EQ(0u, history_downloads.size());
+
+  // This should remove the download as it hasn't yet been entered into
+  // the history.
+  active_downloads[0]->Cancel();
+
+  GetInProgressDownloads(browser(), &active_downloads);
+  EXPECT_EQ(0u, active_downloads.size());
+  GetPersistentDownloads(browser(), &history_downloads);
+  EXPECT_EQ(0u, history_downloads.size());
+
+  // Check state.
+  EXPECT_EQ(1, browser()->tab_count());
+  // Since we exited while the Select File dialog was visible, there should not
+  // be anything in the download shelf and so it should not be visible.
+  CheckDownloadUI(browser(), false, false, FilePath());
+}
+
+// Put up a Select File dialog when the file is downloaded, due to
+// prompt_for_download==true argument to InitialSetup().
+// Confirm that we can remove the download in that state.
+IN_PROC_BROWSER_TEST_F(DownloadTest, RemoveFromFileSelection) {
   ASSERT_TRUE(InitialSetup(true));
   FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
   GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
 
-  // Download the file and wait.  We expect the Select File dialog to appear
-  // due to the MIME type.
+  // Setup select file interposition.
+  NoopSelectFileDialog(browser(), true);
+
+  // Download the file and wait.  We expect the Select File dialog to appear.
+  DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG);
+
+  std::vector<DownloadItem*> active_downloads, history_downloads;
+  GetInProgressDownloads(browser(), &active_downloads);
+  ASSERT_EQ(1u, active_downloads.size());
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, active_downloads[0]->state());
+  GetPersistentDownloads(browser(), &history_downloads);
+  EXPECT_EQ(0u, history_downloads.size());
+
+  // Confirm the file can be successfully removed from the select file
+  // dialog blocked state.
+  active_downloads[0]->Remove();
+
+  GetInProgressDownloads(browser(), &active_downloads);
+  EXPECT_EQ(0u, active_downloads.size());
+  GetPersistentDownloads(browser(), &history_downloads);
+  EXPECT_EQ(0u, history_downloads.size());
+
+  EXPECT_EQ(1, browser()->tab_count());
+  // Since we exited while the Select File dialog was visible, there should not
+  // be anything in the download shelf and so it should not be visible.
+  CheckDownloadUI(browser(), false, false, FilePath());
+}
+
+// Put up a Select File dialog when the file is downloaded, due to
+// prompt_for_download==true argument to InitialSetup().
+// Confirm that an error coming in from the network works properly
+// when in that state.
+IN_PROC_BROWSER_TEST_F(DownloadTest, InterruptFromFileSelection) {
+  ASSERT_TRUE(InitialSetup(true));
+  GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl);
+
+  // Download the file and wait.  We expect the Select File dialog to appear.
   DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG);
 
+  std::vector<DownloadItem*> active_downloads, history_downloads;
+  GetInProgressDownloads(browser(), &active_downloads);
+  ASSERT_EQ(1u, active_downloads.size());
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, active_downloads[0]->state());
+  GetPersistentDownloads(browser(), &history_downloads);
+  EXPECT_EQ(0u, history_downloads.size());
+
+  // Complete the download with error.
+  GURL error_url(URLRequestSlowDownloadJob::kErrorFinishDownloadUrl);
+  ui_test_utils::NavigateToURL(browser(), error_url);
+  MessageLoopForUI::current()->RunAllPending();
+
+  // Confirm that a download error before entry into history
+  // deletes the download.
+  GetInProgressDownloads(browser(), &active_downloads);
+  EXPECT_EQ(0u, active_downloads.size());
+  GetPersistentDownloads(browser(), &history_downloads);
+  EXPECT_EQ(0u, history_downloads.size());
+
   // Check state.
   EXPECT_EQ(1, browser()->tab_count());
   // Since we exited while the Select File dialog was visible, there should not
@@ -1431,8 +1634,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) {
   observer->WaitForFinished();
 
   std::vector<DownloadItem*> downloads;
-  browser()->profile()->GetDownloadManager()->SearchDownloads(
-      string16(), &downloads);
+  GetPersistentDownloads(browser(), &downloads);
   ASSERT_EQ(1u, downloads.size());
   ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->state());
   CheckDownloadUI(browser(), true, true, FilePath());
@@ -1455,6 +1657,183 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) {
   CheckDownloadUI(browser(), false, true, FilePath());
 }
 
+// Do a dangerous download and confirm that the download does
+// not complete until user accept, and that all states are
+// correct along the way.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerous) {
+  ASSERT_TRUE(InitialSetup(false));
+  FilePath file(FILE_PATH_LITERAL("download-dangerous.jar"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  EXPECT_EQ(1, browser()->tab_count());
+
+  scoped_ptr<DownloadsObserver> observer(
+      CreateInProgressWaiter(browser(), 1));
+  ui_test_utils::NavigateToURL(browser(), url);
+  observer->WaitForFinished();
+
+  // We should have one download, in history, and it should
+  // still be dangerous.
+  std::vector<DownloadItem*> downloads;
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  DownloadItem* download = downloads[0];
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
+  EXPECT_EQ(DownloadItem::DANGEROUS, download->safety_state());
+  EXPECT_EQ(DownloadItem::DANGEROUS_FILE, download->GetDangerType());
+  // In ChromeOS, popup will be up, but file name will be unrecognizable.
+  CheckDownloadUI(browser(), true, true, FilePath());
+
+  // See if accepting completes the download and changes the safety
+  // state.
+  scoped_ptr<DownloadsObserver> completion_observer(
+      CreateDangerousWaiter(browser(), 1));
+  AcceptDangerousDownload(browser()->profile()->GetDownloadManager(),
+                          download->id());
+  completion_observer->WaitForFinished();
+
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  ASSERT_EQ(downloads[0], download);
+  EXPECT_EQ(DownloadItem::COMPLETE, download->state());
+  EXPECT_EQ(DownloadItem::DANGEROUS_BUT_VALIDATED, download->safety_state());
+  CheckDownloadUI(browser(), true, true, file);
+}
+
+// Confirm that a dangerous download that gets a file error before
+// completion ends in the right state (currently cancelled because file
+// errors are non-resumable).  Note that this is really testing
+// to make sure errors from the final rename are propagated properly.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousFileError) {
+  ASSERT_TRUE(InitialSetup(false));
+  FilePath file(FILE_PATH_LITERAL("download-dangerous.jar"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  EXPECT_EQ(1, browser()->tab_count());
+
+  scoped_ptr<DownloadsObserver> observer(
+      CreateInProgressWaiter(browser(), 1));
+  ui_test_utils::NavigateToURL(browser(), url);
+  observer->WaitForFinished();
+
+  // We should have one download, in history, and it should
+  // still be dangerous.
+  std::vector<DownloadItem*> downloads;
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  DownloadItem* download = downloads[0];
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
+  EXPECT_EQ(DownloadItem::DANGEROUS, download->safety_state());
+  EXPECT_EQ(DownloadItem::DANGEROUS_FILE, download->GetDangerType());
+  // In ChromeOS, popup will be up, but file name will be unrecognizable.
+  CheckDownloadUI(browser(), true, true, FilePath());
+
+  // Accept it after nuking the directory into which it's being downloaded;
+  // that should complete the download with an error.
+  DeleteDownloadsDirectory();
+  scoped_ptr<DownloadsObserver> completion_observer(
+      CreateDangerousWaiter(browser(), 1));
+  AcceptDangerousDownload(browser()->profile()->GetDownloadManager(),
+                          download->id());
+  completion_observer->WaitForFinished();
+
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  ASSERT_EQ(downloads[0], download);
+  // Persistent errors currently -> CANCELLED.
+  EXPECT_EQ(DownloadItem::CANCELLED, download->state());
+  EXPECT_EQ(DownloadItem::DANGEROUS_BUT_VALIDATED, download->safety_state());
+  // In ChromeOS, popup will still be up, but the file will have been
+  // deleted.
+  CheckDownloadUI(browser(), true, true, FilePath());
+}
+
+// Confirm that declining a dangerous download erases it from living memory.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadDangerousDecline) {
+  ASSERT_TRUE(InitialSetup(false));
+  FilePath file(FILE_PATH_LITERAL("download-dangerous.jar"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  EXPECT_EQ(1, browser()->tab_count());
+
+  scoped_ptr<DownloadsObserver> observer(
+      CreateInProgressWaiter(browser(), 1));
+  ui_test_utils::NavigateToURL(browser(), url);
+  observer->WaitForFinished();
+
+  // We should have one download, in history, and it should
+  // still be dangerous.
+  std::vector<DownloadItem*> downloads;
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  DownloadItem* download = downloads[0];
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->state());
+  EXPECT_EQ(DownloadItem::DANGEROUS, download->safety_state());
+  EXPECT_EQ(DownloadItem::DANGEROUS_FILE, download->GetDangerType());
+  CheckDownloadUI(browser(), true, true, FilePath());
+
+  DenyDangerousDownload(browser()->profile()->GetDownloadManager(),
+                        download->id());
+
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(0u, downloads.size());
+  CheckDownloadUI(browser(), false, true, FilePath());
+}
+
+// Fail a download with a network error partway through, and make sure the
+// state is INTERRUPTED and the error is propagated.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadInterrupted) {
+  ASSERT_TRUE(InitialSetup(false));
+  GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl);
+
+  scoped_ptr<DownloadsObserver> observer(
+      CreateInProgressWaiter(browser(), 1));
+  ui_test_utils::NavigateToURL(browser(), url);
+  observer->WaitForFinished();
+
+  std::vector<DownloadItem*> downloads;
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->state());
+  FilePath filename;
+  net::FileURLToFilePath(url, &filename);
+  CheckDownloadUI(browser(), true, true,
+                  download_util::GetCrDownloadPath(filename.BaseName()));
+
+  // Fail the download
+  GURL error_url(URLRequestSlowDownloadJob::kErrorFinishDownloadUrl);
+  ui_test_utils::NavigateToURL(browser(), error_url);
+  MessageLoopForUI::current()->RunAllPending();
+
+  // Should still be visible, with INTERRUPTED state.
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  DownloadItem* download = downloads[0];
+  ASSERT_EQ(DownloadItem::INTERRUPTED, download->state());
+  // TODO(rdsmith): Confirm error provided by URLRequest is shown
+  // in DownloadItem.
+  CheckDownloadUI(browser(), true, true, FilePath());
+
+  // Confirm cancel does nothing.
+  download->Cancel();
+  MessageLoopForUI::current()->RunAllPending();
+
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  ASSERT_EQ(download, downloads[0]);
+  ASSERT_EQ(DownloadItem::INTERRUPTED, download->state());
+  CheckDownloadUI(browser(), true, true, FilePath());
+
+  // Confirm remove gets rid of it.
+  download->Remove();
+  download = NULL;
+  MessageLoopForUI::current()->RunAllPending();
+
+  GetPersistentDownloads(browser(), &downloads);
+  ASSERT_EQ(0u, downloads.size());
+  CheckDownloadUI(browser(), false, true, FilePath());
+}
+
 // Confirm a download makes it into the history properly.
 IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryCheck) {
   ASSERT_TRUE(InitialSetup(false));
@@ -1469,7 +1848,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryCheck) {
 
   // Get details of what downloads have just happened.
   std::vector<DownloadItem*> downloads;
-  GetDownloads(browser(), &downloads);
+  GetPersistentDownloads(browser(), &downloads);
   ASSERT_EQ(1u, downloads.size());
   int64 db_handle = downloads[0]->db_handle();
 
@@ -1565,8 +1944,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, AutoOpen) {
 
   // Find the download and confirm it was opened.
   std::vector<DownloadItem*> downloads;
-  browser()->profile()->GetDownloadManager()->SearchDownloads(
-      string16(), &downloads);
+  GetPersistentDownloads(browser(), &downloads);
   ASSERT_EQ(1u, downloads.size());
   EXPECT_EQ(DownloadItem::COMPLETE, downloads[0]->state());
   EXPECT_TRUE(downloads[0]->opened());
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index 1b6b267..5cd0b37 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -384,7 +384,7 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::DownloadCancelled, id));
+                        &DownloadManager::CancelDownload, id));
 }
 
 void DownloadFileManager::EraseDownload(int id) {
diff --git a/chrome/browser/download/download_history.cc b/chrome/browser/download/download_history.cc
index 115541a..a44ee4d 100644
--- a/chrome/browser/download/download_history.cc
+++ b/chrome/browser/download/download_history.cc
@@ -120,14 +120,14 @@ void DownloadHistory::UpdateDownloadPath(DownloadItem* download_item,
     hs->UpdateDownloadPath(new_path, download_item->db_handle());
 }
 
-void DownloadHistory::RemoveEntry(DownloadItem* download_item) {
+void DownloadHistory::RemoveEntry(int64 db_handle) {
   // No update necessary if the download was initiated while in incognito mode.
-  if (download_item->db_handle() <= kUninitializedHandle)
+  if (db_handle <= kUninitializedHandle)
     return;
 
   HistoryService* hs = profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
   if (hs)
-    hs->RemoveDownload(download_item->db_handle());
+    hs->RemoveDownload(db_handle);
 }
 
 void DownloadHistory::RemoveEntriesBetween(const base::Time remove_begin,
diff --git a/chrome/browser/download/download_history.h b/chrome/browser/download/download_history.h
index f4b4881..44bdf60 100644
--- a/chrome/browser/download/download_history.h
+++ b/chrome/browser/download/download_history.h
@@ -51,8 +51,10 @@ class DownloadHistory {
   void UpdateDownloadPath(DownloadItem* download_item,
                           const FilePath& new_path);
 
-  // Removes |download_item| from the history database.
-  void RemoveEntry(DownloadItem* download_item);
+  // Removes the download identified by |db_handle| from the history database.
+  // |db_handle| is used instead of the DownloadItem pointer to allow
+  // removal after deletion of the download item.
+  void RemoveEntry(int64 db_handle);
 
   // Removes download-related history entries in the given time range.
   void RemoveEntriesBetween(const base::Time remove_begin,
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 0c3dca1..d657140 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.
 }
 
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index 28afd07..7571caf 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -163,16 +163,11 @@ class DownloadItem : public NotificationObserver {
   // Received a new chunk of data
   void Update(int64 bytes_so_far);
 
-  // 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);
+  // 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();
 
   // Called by external code (SavePackage) using the DownloadItem interface
   // to display progress when the DownloadItem should be considered complete.
@@ -184,10 +179,14 @@ class DownloadItem : public NotificationObserver {
   // Called when the downloaded file is removed.
   void OnDownloadedFileRemoved();
 
-  // Download operation had an error.
+  // 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 |os_error| is the error
   // code that the operation received.
-  void Interrupted(int64 size, int os_error);
+  void Interrupt(int64 size, int os_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
@@ -353,6 +352,10 @@ class DownloadItem : public NotificationObserver {
   // Internal helper for maintaining consistent received and total sizes.
   void UpdateSize(int64 size);
 
+  // Internal function to do the main work of cancelling or
+  // interrupting a download.
+  void StopInternal(DownloadState target_state);
+
   // Called when the entire download operation (including renaming etc)
   // is completed.
   void Completed();
diff --git a/chrome/browser/download/download_item_model.cc b/chrome/browser/download/download_item_model.cc
index 5c8e793..47744c6 100644
--- a/chrome/browser/download/download_item_model.cc
+++ b/chrome/browser/download/download_item_model.cc
@@ -25,7 +25,7 @@ DownloadItemModel::DownloadItemModel(DownloadItem* download)
 }
 
 void DownloadItemModel::CancelTask() {
-  download_->Cancel(true /* update history service */);
+  download_->Cancel();
 }
 
 string16 DownloadItemModel::GetStatusText() {
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 3e0cf36..3970475 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -105,8 +105,7 @@ void DownloadManager::Shutdown() {
       // from all queues.
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
-      download->Cancel(false);
-      download_history_->UpdateEntry(download);
+      download->Cancel();
     }
   }
 
@@ -648,6 +647,15 @@ void DownloadManager::OnResponseCompleted(int32 download_id,
   }
 }
 
+void DownloadManager::CancelDownload(int32 download_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadMap::iterator it = active_downloads_.find(download_id);
+  if (it == active_downloads_.end())
+    return;
+
+  it->second->Cancel();
+}
+
 void DownloadManager::OnAllDataSaved(int32 download_id,
                                      int64 size,
                                      const std::string& hash) {
@@ -728,8 +736,8 @@ void DownloadManager::AssertQueueStateConsistent(DownloadItem* download) {
 
   CHECK(ContainsKey(active_downloads_, download->id()) ==
         (download->state() == DownloadItem::IN_PROGRESS));
-  CHECK(ContainsKey(in_progress_, download->id()) ==
-        (download->state() == DownloadItem::IN_PROGRESS));
+  // Would check in_progress_, but removal from that queue happens
+  // before transition from IN_PROGRESS for legacy reasons, so skipping.
 }
 
 bool DownloadManager::IsDownloadReadyForCompletion(DownloadItem* download) {
@@ -826,32 +834,24 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
   download_history_->UpdateDownloadPath(item, full_path);
 }
 
-void DownloadManager::DownloadCancelled(int32 download_id) {
+void DownloadManager::DownloadStopped(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator it = in_progress_.find(download_id);
-  if (it == in_progress_.end())
+  DownloadMap::iterator it = active_downloads_.find(download_id);
+  if (it == active_downloads_.end())
     return;
   DownloadItem* download = it->second;
 
   VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
            << " download = " << download->DebugString(true);
 
-  // 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() != DownloadHistory::kUninitializedHandle) {
-    in_progress_.erase(it);
-    active_downloads_.erase(download_id);
-    UpdateAppIcon();  // Reflect removal from in_progress_.
-    download_history_->UpdateEntry(download);
-  }
+  in_progress_.erase(download_id);
+  active_downloads_.erase(download_id);
+  UpdateAppIcon();  // Reflect removal from in_progress_.
 
-  DownloadCancelledInternal(download_id, download->request_handle());
-}
+  // The history service should always outlive the DownloadManager.
+  download_history_->UpdateEntry(download);
 
-void DownloadManager::DownloadCancelledInternal(
-    int download_id, const DownloadRequestHandle& request_handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle.CancelRequest();
+  download->request_handle().CancelRequest();
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ -875,24 +875,7 @@ void DownloadManager::OnDownloadError(int32 download_id,
            << " at offset " << download->received_bytes()
            << " for download = " << download->DebugString(true);
 
-  download->Interrupted(size, os_error);
-
-  // TODO(ahendrickson) - Remove this when we add resuming of interrupted
-  // downloads, as we will keep the download item around in that case.
-  //
-  // 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() != DownloadHistory::kUninitializedHandle) {
-    in_progress_.erase(download_id);
-    active_downloads_.erase(download_id);
-    UpdateAppIcon();  // Reflect removal from in_progress_.
-    download_history_->UpdateEntry(download);
-  }
-
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
+  download->Interrupt(size, os_error);
 }
 
 void DownloadManager::UpdateAppIcon() {
@@ -900,17 +883,15 @@ void DownloadManager::UpdateAppIcon() {
     status_updater_->Update();
 }
 
-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;
-  download_history_->RemoveEntry(download);
+void DownloadManager::RemoveDownload(DownloadItem* download) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // Silently ignores request if db handle indicates that the download
+  // isn't in the history.
+  download_history_->RemoveEntry(download->db_handle());
 
   // Remove from our tables and delete.
-  history_downloads_.erase(it);
+  if (download->db_handle() != DownloadHistory::kUninitializedHandle)
+    history_downloads_.erase(download->db_handle());
   int downloads_count = downloads_.erase(download);
   DCHECK_EQ(1, downloads_count);
 
@@ -1061,6 +1042,11 @@ int64 DownloadManager::GetInProgressDownloadCount() {
   return in_progress_.size();
 }
 
+void DownloadManager::SetSelectFileDialog(
+    scoped_refptr<SelectFileDialog> file_dialog) {
+  select_file_dialog_ = file_dialog;
+}
+
 int64 DownloadManager::GetReceivedDownloadBytes() {
   DCHECK(IsDownloadProgressKnown());
   int64 received_bytes = 0;
@@ -1119,7 +1105,7 @@ void DownloadManager::FileSelectionCanceled(void* params) {
   VLOG(20) << __FUNCTION__ << "()"
            << " download = " << download->DebugString(true);
 
-  DownloadCancelledInternal(download_id, download->request_handle());
+  download->Cancel();
 }
 
 // TODO(phajdan.jr): This is apparently not being exercised in tests.
@@ -1180,6 +1166,9 @@ void DownloadManager::AddDownloadItemToHistory(DownloadItem* download,
                                                int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
+           << " download = " << download->DebugString(false);
+
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
@@ -1188,9 +1177,10 @@ void DownloadManager::AddDownloadItemToHistory(DownloadItem* download,
   if (db_handle == DownloadHistory::kUninitializedHandle)
     db_handle = download_history_->GetNextFakeDbHandle();
 
+  // We shouldn't be here if the download is no longer in progress.
   // TODO(rdsmith): Convert to DCHECK() when http://crbug.com/84508
   // is fixed.
-  CHECK_NE(DownloadHistory::kUninitializedHandle, db_handle);
+  CHECK(download->IsInProgress());
 
   DCHECK(download->db_handle() == DownloadHistory::kUninitializedHandle);
   download->set_db_handle(db_handle);
@@ -1205,13 +1195,25 @@ void DownloadManager::AddDownloadItemToHistory(DownloadItem* download,
 void DownloadManager::OnCreateDownloadEntryComplete(int32 download_id,
                                                     int64 db_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
+           << " download_id = " << download_id;
   DownloadItem* download = GetActiveDownloadItem(download_id);
-  if (!download)
+  if (!download) {
+    // The download was cancelled while the history system was entering it.
+    // We resolve this race by turning around and deleting it in the
+    // history system (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 history system insertion).
+
+    // Make sure we haven't already been shutdown (the callback raced
+    // with shutdown), as that would mean that the history service has
+    // also been shutdown.  In that case, the history will be cleaned up
+    // on next browser start.
+    if (download_history_.get())
+      download_history_->RemoveEntry(db_handle);
     return;
-
-  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << db_handle
-           << " download_id = " << download_id
-           << " download = " << download->DebugString(true);
+  }
 
   AddDownloadItemToHistory(download, db_handle);
 
@@ -1222,23 +1224,7 @@ void DownloadManager::OnCreateDownloadEntryComplete(int32 download_id,
   // Inform interested objects about the new download.
   NotifyModelChanged();
 
-  // 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 {
-    DCHECK(download->IsCancelled())
-        << " download = " << download->DebugString(true);
-    in_progress_.erase(download_id);
-    active_downloads_.erase(download_id);
-    download_history_->UpdateEntry(download);
-    download->UpdateObservers();
-  }
+  MaybeCompleteDownload(download);
 }
 
 void DownloadManager::ShowDownloadInBrowser(DownloadItem* download) {
@@ -1283,6 +1269,16 @@ 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/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 828c83c..b5b220e 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -123,9 +123,22 @@ class DownloadManager
   void OnResponseCompleted(int32 download_id, int64 size, int os_error,
                            const std::string& hash);
 
-  // Called from a view when a user clicks a UI button or link.
-  void DownloadCancelled(int32 download_id);
-  void RemoveDownload(int64 download_handle);
+  // Called to initiate download cancel on behalf of an off-thread portion
+  // of the download system; redirects to DownloadItem.
+  void CancelDownload(int32 download_handle);
+
+  // 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(int32 download_id);
+
+  // 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);
 
   // Determine if the download is ready for completion, i.e. has had
   // all data saved, and completed the filename determination and
@@ -267,10 +280,6 @@ class DownloadManager
   DownloadItem* GetDownloadItem(int id);
 
  private:
-  // For testing.
-  friend class DownloadManagerTest;
-  friend class MockDownloadManager;
-
   // This class is used to let an incognito DownloadManager observe changes to
   // a normal DownloadManager, to propagate ModelChanged() calls from the parent
   // DownloadManager to the observers of the incognito DownloadManager.
@@ -296,6 +305,11 @@ class DownloadManager
   friend class DeleteTask<DownloadManager>;
   friend class OtherDownloadManagerObserver;
 
+  // For testing.
+  friend class DownloadManagerTest;
+  friend class MockDownloadManager;
+  friend class DownloadTest;
+
   virtual ~DownloadManager();
 
   // Called on the FILE thread to check the existence of a downloaded file.
@@ -324,10 +338,6 @@ class DownloadManager
   void ContinueDownloadWithPath(DownloadItem* download,
                                 const FilePath& chosen_file);
 
-  // Download cancel helper function.
-  void DownloadCancelledInternal(int download_id,
-                                 const DownloadRequestHandle& request_handle);
-
   // All data has been downloaded.
   // |hash| is sha256 hash for the downloaded file. It is empty when the hash
   // is not available.
@@ -342,6 +352,16 @@ class DownloadManager
   // 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);
+
+  // Replace the file selection dialog used for the download manager.
+  // This is intended to be used for testing only.
+  void SetSelectFileDialog(scoped_refptr<SelectFileDialog> file_dialog);
+
   // Get the download item from the active map.  Useful when the item is not
   // yet in the history map.
   DownloadItem* GetActiveDownloadItem(int id);
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 1b80445..72533f1 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -458,7 +458,7 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
                                        simple_size,
                                        simple_total));
 
-  download->Cancel(true);
+  download->Cancel();
 
   EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
   EXPECT_TRUE(observer->hit_state(DownloadItem::INTERRUPTED));
@@ -515,10 +515,10 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
 
   download_file->AppendDataToFile(kTestData, kTestDataLen);
 
-  download->Cancel(false);
+  download->Cancel();
   message_loop_.RunAllPending();
 
-  EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
+  EXPECT_TRUE(GetActiveDownloadItem(0) == NULL);
   EXPECT_TRUE(observer->hit_state(DownloadItem::IN_PROGRESS));
   EXPECT_TRUE(observer->hit_state(DownloadItem::CANCELLED));
   EXPECT_FALSE(observer->hit_state(DownloadItem::INTERRUPTED));
diff --git a/chrome/browser/download/download_request_handle.h b/chrome/browser/download/download_request_handle.h
index 0f26697..00a40c7 100644
--- a/chrome/browser/download/download_request_handle.h
+++ b/chrome/browser/download/download_request_handle.h
@@ -37,11 +37,15 @@ class DownloadRequestHandle {
   TabContents* GetTabContents() const;
   DownloadManager* GetDownloadManager() const;
 
-  // Pause or resume the matching URL request.
+  // 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.
   void PauseRequest() const;
   void ResumeRequest() const;
 
-  // Cancel the request
+  // Cancel the request.  Note that while this does not
+  // modify the DownloadRequestHandle, it does modify the
+  // request the handle refers to.
   void CancelRequest() const;
 
   std::string DebugString() const;
diff --git a/chrome/browser/download/download_util.h b/chrome/browser/download/download_util.h
index 90f669a..51ec993 100644
--- a/chrome/browser/download/download_util.h
+++ b/chrome/browser/download/download_util.h
@@ -165,6 +165,9 @@ enum DownloadCountTypes {
   // Downloads that were interrupted by the OS.
   INTERRUPTED_COUNT,
 
+  // Downloads that were removed by the user.
+  REMOVED_COUNT,
+
   DOWNLOAD_COUNT_TYPES_LAST_ENTRY
 };