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

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 514 matching lines @@
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       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);
+  // 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_);
   // 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.
 
@@ skipping 286 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;
 }