Revert 159281 - Shift ordering in DownloadManagerImpl shutdown so that COMPLETING states

will still be on active_downloads_.

R=benjhayden@chromium.org
BUG=152535


Review URL: https://chromiumcodereview.appspot.com/11000036

TBR=rdsmith@chromium.org
Review URL: https://codereview.chromium.org/11012007

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

3 files changed, 2 insertions, 69 deletions

diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index e362f2f..46030b1 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -471,6 +471,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtFinalRename) {
 // release.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) {
   // Setup new factory.
+  // Setup new factory.
   DownloadFileWithDelayFactory* file_factory =
       new DownloadFileWithDelayFactory();
   GetDownloadFileManager()->SetFileFactoryForTesting(
@@ -524,66 +525,4 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) {
   EXPECT_EQ(DownloadItem::COMPLETE, items[0]->GetState());
 }
 
-// Try to shutdown just after we release the download file, by delaying
-// release.
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, ShutdownAtRelease) {
-  // Setup new factory.
-  DownloadFileWithDelayFactory* file_factory =
-      new DownloadFileWithDelayFactory();
-  GetDownloadFileManager()->SetFileFactoryForTesting(
-      scoped_ptr<content::DownloadFileFactory>(file_factory).Pass());
-
-  // Create a download
-  FilePath file(FILE_PATH_LITERAL("download-test.lib"));
-  NavigateToURL(shell(), URLRequestMockHTTPJob::GetMockUrl(file));
-
-  // Wait until the first (intermediate file) rename and execute the callback.
-  file_factory->WaitForSomeCallback();
-  std::vector<base::Closure> callbacks;
-  file_factory->GetAllDetachCallbacks(&callbacks);
-  ASSERT_TRUE(callbacks.empty());
-  file_factory->GetAllRenameCallbacks(&callbacks);
-  ASSERT_EQ(1u, callbacks.size());
-  callbacks[0].Run();
-  callbacks.clear();
-
-  // Wait until the second (final) rename callback is posted.
-  file_factory->WaitForSomeCallback();
-  file_factory->GetAllDetachCallbacks(&callbacks);
-  ASSERT_TRUE(callbacks.empty());
-  file_factory->GetAllRenameCallbacks(&callbacks);
-  ASSERT_EQ(1u, callbacks.size());
-
-  // Call it.
-  callbacks[0].Run();
-  callbacks.clear();
-
-  // Confirm download isn't complete yet.
-  std::vector<DownloadItem*> items;
-  DownloadManagerForShell(shell())->GetAllDownloads(&items);
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
-
-  // Cancel the download; confirm cancel fails anyway.
-  ASSERT_EQ(1u, items.size());
-  items[0]->Cancel(true);
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
-  RunAllPendingInMessageLoop();
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
-
-  // Get the detach callback that should have been produced by the above.
-  file_factory->WaitForSomeCallback();
-  file_factory->GetAllRenameCallbacks(&callbacks);
-  ASSERT_TRUE(callbacks.empty());
-  file_factory->GetAllDetachCallbacks(&callbacks);
-  ASSERT_EQ(1u, callbacks.size());
-
-  // Shutdown the download manager.  Can't get at any downloads after this,
-  // so this is basically just checking for crashes.
-  DownloadManagerForShell(shell())->Shutdown();
-
-  // Run the detach callback; shouldn't cause any problems.
-  callbacks[0].Run();
-  callbacks.clear();
-}
-
 }  // namespace content
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index 72d8c26..93293cb 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -218,8 +218,6 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
 
     // Between commit point (dispatch of download file release) and
     // completed.  Embedder may be opening the file in this state.
-    // Note that the DownloadItem may be deleted (by shutdown) in this
-    // state.
     COMPLETING_INTERNAL,
 
     // After embedder has had a chance to auto-open.  User may now open
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index e16dbb3..bfd5ac0 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -320,12 +320,8 @@ void DownloadManagerImpl::Shutdown() {
   // and all in progress downloads have been cancelled.  We can now delete
   // anything left.
 
-  // We delete the downloads before clearing the active_downloads_ map
-  // so that downloads in the COMPLETING_INTERNAL state (which will have
-  // ignored the Cancel() above) will still show up in active_downloads_
-  // in order to satisfy the invariants enforced in AssertStateConsistent().
-  STLDeleteValues(&downloads_);
   active_downloads_.clear();
+  STLDeleteValues(&downloads_);
   downloads_.clear();
 
   // We'll have nothing more to report to the observers after this point.