When the download folder does not exist, change the download folder to a user's "Downloads"

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 72e89b53551f587d29ca578bfb803b1d21238cff..b197e43e9ed245fbb9190fe0ffd3ac569756d004 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -532,9 +532,31 @@ void DownloadItem::Remove() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  download_manager_->AssertQueueStateConsistent(this);
+  // TODO(haraken): The following AssertQueueStateConsistent() fails
+  // if this Remove() is called after download_manager_->StartDownload()
+  // but before download_manager_->ContinueDownloadWithPath().
+  // In that case, download->state_ is IN_PROGRESS but the download id
+  // is not yet registered to download_manager_->in_progress_.
+  // This violates 'CHECK(ContainsKey(in_progress_, download->id()) ==
+  // (download->state() == DownloadItem::IN_PROGRESS));'
+  // in AssertQueueStateConsistent().
+  //
+  // download_manager_->AssertQueueStateConsistent(this);
+

[haraken1, 2011/06/22 18:01:58]

In fact, DownloadManagerTest.StartDownload fails at AssertQueueStateConsistent()
for the following reason:

(1) TEST_F(DownloadManagerTest, StartDownload) calls ~DownloadManagerTest() in
the end.
(2) ~DownloadManagerTest() calls DownloadManager::Shutdown().
(3) DownloadManager::Shutdown() calls DownloadItem::Delete() for (not all but)
some downloads in the test.
(4) DownloadItem::Delete() calls DownloadItem::Remove().
(5) In DownloadItem::Remove(), state_ is IN_PROGRESS but its download id is not
yet registered to DownloadManager::in_progress_, because the registration is
done only at DownloadManager::ContinueDownloadWithPath() but 
DownloadManager::ContinueDownloadWithPath() is never called in the scenario
above. 
(6) DownloadManager::AssertQueueStateConsistent() is called and
'CHECK(ContainsKey(in_progress_, download->id()) == download->state() ==
DownloadItem::IN_PROGRESS))' in DownloadManager::AssertQueueStateConsistent()
fails.

The reason why the previous code does not cause this problem is that
(fortunately) the previous DownloadManagerTest.StartDownload always called
DownloadManager::ContinueDownloadWithPath() for all the downloads that entered
the call path I described above. Here, whether
DownloadManager::ContinueDownloadWithPath() is called or not is determined by
kStartDownloadCases[i].expected_save_as in DownloadManagerTest.StartDownload.
Since we had to change the value of kStartDownloadCases[i].expected_save_as from
false to true (See the comments in download_manager_unittest.cc for more details
for this change), this problem appeared.

[Randy Smith (Not in Mondays), 2011/06/23 20:24:42]

Based on your description, it sounds as if if we can change the test so that we
can revert the expect_save_as change in download_manager_unittest.cc, this
problem will go away, so I'll assume that that'll work until you tell me
differently.  Please do *not* remove the AssertQueueStateConsistent calls; they
represent invariants in the download system during normal operation that we
should make sure the tests respect.

[haraken1, 2011/06/24 01:57:25]

I reverted this code back to the original, since I could revert
DownloadManagerTest.StartDownload to the original and thus this problem
disappeared. 

Just an out-of-curiosity question: Aren't there any practical cases that tracks
the call path I described above and thus the invariant collapses? i.e.:

(1) DownloadManager::Shutdown() is called in the condition that StartDownload()
is already called but ContinueDownloadWithPath() is not yet called. 
(2) The download is dangerous and thus the DownloadManager::Shutdown() calls
DownloadItem::Delete().
(3) The problem I described occurs.

   Cancel(true);
-  download_manager_->AssertQueueStateConsistent(this);
+
+  // TODO(haraken): The following AssertQueueStateConsistent() fails
+  // if this Remove() is called after download_manager_->StartDownload()
+  // but before download_manager_->ContinueDownloadWithPath().
+  // In that case, since the download id is not yet registered to
+  // download_manager_->in_progress_, download_manager_->DownloadCancelled()
+  // called inside the above Cancel() returns immediately without
+  // clearing download_manager_->active_downloads_. Then, this violates
+  // 'CHECK(ContainsKey(active_downloads_, download->id()) ==
+  // (download->state() == DownloadItem::IN_PROGRESS));'
+  // in AssertQueueStateConsistent().
+  //
+  // download_manager_->AssertQueueStateConsistent(this);
 
   state_ = REMOVING;
   download_manager_->RemoveDownload(db_handle_);