diff options
Diffstat (limited to 'chrome')
17 files changed, 571 insertions, 163 deletions
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc index 71f1eac..28c1f53 100644 --- a/chrome/browser/automation/testing_automation_provider.cc +++ b/chrome/browser/automation/testing_automation_provider.cc @@ -3127,7 +3127,7 @@ void TestingAutomationProvider::PerformActionOnDownload( } else if (action == "cancel") { selected_item->AddObserver(new AutomationProviderDownloadUpdatedObserver( this, reply_message, false)); - selected_item->Cancel(true); + selected_item->Cancel(); } else { AutomationJSONReply(this, reply_message) .SendError(StringPrintf("Invalid action '%s' given.", action.c_str())); diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc index e3055bc..89d72ab 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.cc +++ b/chrome/browser/download/chrome_download_manager_delegate.cc @@ -220,8 +220,8 @@ void ChromeDownloadManagerDelegate::UpdatePathForItemInPersistentStore( } void ChromeDownloadManagerDelegate::RemoveItemFromPersistentStore( - DownloadItem* item) { - download_history_->RemoveEntry(item); + int64 db_handle) { + download_history_->RemoveEntry(db_handle); } void ChromeDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween( diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h index b006e53..e5d7197 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.h +++ b/chrome/browser/download/chrome_download_manager_delegate.h @@ -68,7 +68,7 @@ class ChromeDownloadManagerDelegate virtual void UpdatePathForItemInPersistentStore( DownloadItem* item, const FilePath& new_path) OVERRIDE; - virtual void RemoveItemFromPersistentStore(DownloadItem* item) OVERRIDE; + virtual void RemoveItemFromPersistentStore(int64 db_handle) OVERRIDE; virtual void RemoveItemsFromPersistentStoreBetween( const base::Time remove_begin, const base::Time remove_end) OVERRIDE; diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc index 8c5f3d1..ac39150 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -58,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 }; @@ -73,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); } @@ -92,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. @@ -102,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."; } @@ -194,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); } } @@ -302,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. @@ -321,121 +329,36 @@ class DownloadsObserver : public DownloadManager::Observer, DISALLOW_COPY_AND_ASSIGN(DownloadsObserver); }; -// WaitForFlush() returns after: -// * There are no IN_PROGRESS download items remaining on the -// DownloadManager. -// * There have been two round trip messages through the file and -// IO threads. -// This almost certainly means that a Download cancel has propagated through -// the system. -class DownloadsFlushObserver - : public DownloadManager::Observer, - public DownloadItem::Observer, - public base::RefCountedThreadSafe<DownloadsFlushObserver> { +// Ping through an arbitrary set of threads. Must be run from the UI +// thread. +class ThreadPinger : public base::RefCountedThreadSafe<ThreadPinger> { public: - explicit DownloadsFlushObserver(DownloadManager* download_manager) - : download_manager_(download_manager), - waiting_for_zero_inprogress_(true) {} + ThreadPinger(const BrowserThread::ID ids[], size_t num_ids) : + next_thread_index_(0u), + ids_(ids, ids + num_ids) {} - void WaitForFlush() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - download_manager_->AddObserver(this); + void Ping() { + if (ids_.size() == 0) + return; + NextThread(); ui_test_utils::RunMessageLoop(); } - // DownloadsManager observer methods. - virtual void ModelChanged() { - // Model has changed, so there may be more DownloadItems to observe. - CheckDownloadsInProgress(true); - } - - // DownloadItem observer methods. - virtual void OnDownloadUpdated(DownloadItem* download) { - // No change in DownloadItem set on manager. - CheckDownloadsInProgress(false); - } - virtual void OnDownloadOpened(DownloadItem* download) {} - - protected: - friend class base::RefCountedThreadSafe<DownloadsFlushObserver>; - - virtual ~DownloadsFlushObserver() { - download_manager_->RemoveObserver(this); - for (std::set<DownloadItem*>::iterator it = downloads_observed_.begin(); - it != downloads_observed_.end(); ++it) { - (*it)->RemoveObserver(this); - } - } - private: - // If we're waiting for that flush point, check the number - // of downloads in the IN_PROGRESS state and take appropriate - // action. If requested, also observes all downloads while iterating. - void CheckDownloadsInProgress(bool observe_downloads) { - if (waiting_for_zero_inprogress_) { - int count = 0; - - std::vector<DownloadItem*> downloads; - download_manager_->SearchDownloads(string16(), &downloads); - std::vector<DownloadItem*>::iterator it = downloads.begin(); - for (; it != downloads.end(); ++it) { - if ((*it)->state() == DownloadItem::IN_PROGRESS) - count++; - if (observe_downloads) { - if (downloads_observed_.find(*it) == downloads_observed_.end()) { - (*it)->AddObserver(this); - } - // Download items are forever, and we don't want to make - // assumptions about future state transitions, so once we - // start observing them, we don't stop until destruction. - } - } - - if (count == 0) { - waiting_for_zero_inprogress_ = false; - // Stop observing DownloadItems. We maintain the observation - // of DownloadManager so that we don't have to independently track - // whether we are observing it for conditional destruction. - for (std::set<DownloadItem*>::iterator it = downloads_observed_.begin(); - it != downloads_observed_.end(); ++it) { - (*it)->RemoveObserver(this); - } - downloads_observed_.clear(); - - // Trigger next step. We need to go past the IO thread twice, as - // there's a self-task posting in the IO thread cancel path. - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - NewRunnableMethod(this, - &DownloadsFlushObserver::PingFileThread, 2)); - } - } - } - - void PingFileThread(int cycle) { - BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, - NewRunnableMethod(this, &DownloadsFlushObserver::PingIOThread, - cycle)); - } - - void PingIOThread(int cycle) { - if (--cycle) { + void NextThread() { + if (next_thread_index_ == ids_.size()) { BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - NewRunnableMethod(this, &DownloadsFlushObserver::PingFileThread, - cycle)); + BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask()); } else { + BrowserThread::ID next_id(ids_[next_thread_index_++]); BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask()); + next_id, FROM_HERE, + NewRunnableMethod(this, &ThreadPinger::NextThread)); } } - DownloadManager* download_manager_; - std::set<DownloadItem*> downloads_observed_; - bool waiting_for_zero_inprogress_; - - DISALLOW_COPY_AND_ASSIGN(DownloadsFlushObserver); + size_t next_thread_index_; + std::vector<BrowserThread::ID> ids_; }; // Collect the information from FILE and IO threads needed for the Cancel @@ -506,6 +429,39 @@ class PickSuggestedFileDelegate : public ChromeDownloadManagerDelegate { } }; +// Don't respond to ChooseDownloadPath until a cancel is requested +// out of band. Can handle only one outstanding request at a time +// for a download path. +class BlockCancelFileDelegate : public ChromeDownloadManagerDelegate { + public: + explicit BlockCancelFileDelegate(Profile* profile) + : ChromeDownloadManagerDelegate(profile), + choose_download_path_called_(false), + choose_download_path_data_(NULL) { + SetDownloadManager(profile->GetDownloadManager()); + } + + virtual void ChooseDownloadPath(TabContents* tab_contents, + const FilePath& suggested_path, + void* data) OVERRIDE { + CHECK(!choose_download_path_called_); + choose_download_path_called_ = true; + choose_download_path_data_ = data; + } + + void CancelOutstandingDownloadPathRequest() { + if (choose_download_path_called_) { + if (download_manager_) + download_manager_->FileSelectionCanceled(choose_download_path_data_); + choose_download_path_called_ = false; + choose_download_path_data_ = NULL; + } + } + private: + bool choose_download_path_called_; + void *choose_download_path_data_; +}; + // Get History Information. class DownloadsHistoryDataCollector { public: @@ -623,6 +579,25 @@ class MockDownloadOpeningObserver : public DownloadManager::Observer { DISALLOW_COPY_AND_ASSIGN(MockDownloadOpeningObserver); }; +static const DownloadItem::DownloadState kTerminalStates[] = { + DownloadItem::CANCELLED, + DownloadItem::INTERRUPTED, + DownloadItem::COMPLETE, +}; + +static const DownloadItem::DownloadState kInProgressStates[] = { + DownloadItem::IN_PROGRESS, +}; + +static const BrowserThread::ID kIOFileX2ThreadList[] = { + BrowserThread::IO, BrowserThread::FILE, + BrowserThread::IO, BrowserThread::FILE }; + +static const BrowserThread::ID kExternalTerminationThreadList[] = { + BrowserThread::IO, BrowserThread::IO, BrowserThread::FILE }; + +// Not in anonymous namespace so that friend class from DownloadManager +// can target it. class DownloadTest : public InProcessBrowserTest { public: enum SelectExpectation { @@ -699,6 +674,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 DownloadPrefs::FromDownloadManager( browser->profile()->GetDownloadManager()); @@ -715,12 +696,29 @@ class DownloadTest : public InProcessBrowserTest { browser->profile()->GetDownloadManager(); return new DownloadsObserver( download_manager, num_downloads, - DownloadItem::COMPLETE, // Really done + 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) { @@ -728,9 +726,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 @@ -741,11 +741,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); } @@ -900,12 +902,35 @@ 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); + } + + size_t NumberInProgressDownloads(Browser* browser) { + std::vector<DownloadItem*> downloads; + browser->profile()->GetDownloadManager()->GetInProgressDownloads( + &downloads); + return downloads.size(); + } + + void WaitForIOFileX2() { + scoped_refptr<ThreadPinger> pinger( + new ThreadPinger(kIOFileX2ThreadList, arraysize(kIOFileX2ThreadList))); + pinger->Ping(); + } + // 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). @@ -973,6 +998,39 @@ class DownloadTest : public InProcessBrowserTest { browser->profile()->SetDownloadManagerDelegate(new_delegate); } + // Arrange for select file calls on the given browser from the + // download manager to block until explicitly released. + // Note that the returned pointer is non-owning; the lifetime + // of the object will be that of the profile. + BlockCancelFileDelegate* BlockSelectFile(Browser* browser) { + BlockCancelFileDelegate* new_delegate = + new BlockCancelFileDelegate(browser->profile()); + + DownloadManager* manager = browser->profile()->GetDownloadManager(); + + new_delegate->SetDownloadManager(manager); + manager->set_delegate(new_delegate); + + // Gives ownership to Profile. + browser->profile()->SetDownloadManagerDelegate(new_delegate); + + return new_delegate; + } + + // Wait for an external termination (e.g. signalling + // URLRequestSlowDownloadJob to return an error) to propagate through + // this sytem. This involves hopping over to the IO thread (twice, + // because of possible self-posts from URLRequestJob) then + // chasing the (presumed) notification through the download + // system (FILE->UI). + void WaitForExternalTermination() { + scoped_refptr<ThreadPinger> pinger( + new ThreadPinger( + kExternalTerminationThreadList, + arraysize(kExternalTerminationThreadList))); + pinger->Ping(); + } + private: // Location of the test data. FilePath test_dir_; @@ -1039,7 +1097,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeTypeSelect) { new DownloadsObserver( browser()->profile()->GetDownloadManager(), 1, - DownloadItem::COMPLETE, // Really done + DownloadsObserver::StateSet( + kTerminalStates, kTerminalStates + arraysize(kTerminalStates)), false, // Continue on select file. ON_DANGEROUS_DOWNLOAD_FAIL)); ui_test_utils::NavigateToURLWithDisposition( @@ -1048,12 +1107,168 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeTypeSelect) { observer->WaitForFinished(); EXPECT_TRUE(observer->select_file_dialog_seen()); - // Check state. EXPECT_EQ(1, browser()->tab_count()); CheckDownload(browser(), file, file); CheckDownloadUI(browser(), true, true, file); } +// 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, CancelAtFileSelection) { + ASSERT_TRUE(InitialSetup(true)); + FilePath file(FILE_PATH_LITERAL("download-test1.lib")); + GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); + + BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); + + // 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(); + MessageLoopForUI::current()->RunAllPending(); + + 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()); + + // Return from file selection to release allocated data. + delegate->CancelOutstandingDownloadPathRequest(); +} + +// 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 as if the user had hit cancel. +IN_PROC_BROWSER_TEST_F(DownloadTest, CancelFromFileSelection) { + ASSERT_TRUE(InitialSetup(true)); + FilePath file(FILE_PATH_LITERAL("download-test1.lib")); + GURL url(URLRequestMockHTTPJob::GetMockUrl(file)); + + BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); + + // 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()); + + // (Effectively) Click the Cancel button. This should remove the download + // as it hasn't yet been entered into the history. + delegate->CancelOutstandingDownloadPathRequest(); + MessageLoopForUI::current()->RunAllPending(); + + 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)); + + BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); + + // 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(); + MessageLoopForUI::current()->RunAllPending(); + + 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()); + + // Return from file selection to release allocated data. + delegate->CancelOutstandingDownloadPathRequest(); +} + +// 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); + + BlockCancelFileDelegate* delegate = BlockSelectFile(browser()); + + // 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(); + WaitForExternalTermination(); + + // 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()); + + // 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()); + + // Return from file selection to release allocated data. + delegate->CancelOutstandingDownloadPathRequest(); +} + // Access a file with a viewable mime-type, verify that a download // did not initiate. IN_PROC_BROWSER_TEST_F(DownloadTest, NoDownload) { @@ -1453,25 +1668,37 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, NewWindow) { CheckDownload(browser(), file, file); } +// Create a download, wait until it's visible on the DownloadManager, +// then cancel it. IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) { ASSERT_TRUE(InitialSetup(false)); EXPECT_EQ(1, browser()->tab_count()); - // TODO(rdsmith): Fragile code warning! The code below relies on the - // DownloadsObserver only finishing when the new download has reached - // the state of being entered into the history and being user-visible - // (that's what's required for the Remove to be valid and for the - // download shelf to be visible). By the pure semantics of + // The code below is slightly fragile. Currently, + // the DownloadsObserver will only finish when the new download has + // reached the state of being entered into the history and being + // user-visible. However, by the pure semantics of // DownloadsObserver, that's not guaranteed; DownloadItems are created // in the IN_PROGRESS state and made known to the DownloadManager // immediately, so any ModelChanged event on the DownloadManager after - // navigation would allow the observer to return. However, the only - // ModelChanged() event the code will currently fire is in - // OnCreateDownloadEntryComplete, at which point the download item will - // be in the state we need. - // The right way to fix this is to create finer grained states on the - // DownloadItem, and wait for the state that indicates the item has been - // entered in the history and made visible in the UI. + // navigation would allow the observer to return. At some point we'll + // probably change the DownloadManager to fire a ModelChanged event + // earlier in download processing. This test should continue to work, + // as a cancel is valid at any point during download process. However, + // at that point it'll be testing two different things, depending on + // what state the download item has reached when it is cancelled: + // a) cancelling from a pre-history entry when the only record of a + // download item is on the active_downloads_ queue, and b) cancelling + // from a post-history entry when the download is present on the + // history_downloads_ list. + // + // Note that the above is why we don't do any UI testing here--we don't + // know whether or not the download item has been made visible to the user. + + // TODO(rdsmith): After we fire ModelChanged events at finer granularity, + // split this test into subtests that tests canceling from each separate + // download item state. Also include UI testing as appropriate in those + // split tests. // Create a download, wait until it's started, and confirm // we're in the expected state. @@ -1482,27 +1709,199 @@ 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()); // Cancel the download and wait for download system quiesce. downloads[0]->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); - scoped_refptr<DownloadsFlushObserver> flush_observer( - new DownloadsFlushObserver(browser()->profile()->GetDownloadManager())); - flush_observer->WaitForFlush(); + ASSERT_EQ(0u, NumberInProgressDownloads(browser())); + WaitForIOFileX2(); // Get the important info from other threads and check it. scoped_refptr<CancelTestDataCollector> info(new CancelTestDataCollector()); info->WaitForDataCollected(); EXPECT_EQ(0, info->rdh_pending_requests()); EXPECT_EQ(0, info->dfm_pending_downloads()); +} + +// 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); + EXPECT_EQ(DownloadItem::INTERRUPTED, 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(); + + DownloadItem* download1; + std::vector<DownloadItem*> downloads; + GetPersistentDownloads(browser(), &downloads); + ASSERT_EQ(1u, downloads.size()); + download1 = downloads[0]; + ASSERT_EQ(DownloadItem::IN_PROGRESS, download1->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(); + WaitForExternalTermination(); + + // Should still be visible, with INTERRUPTED state. + GetPersistentDownloads(browser(), &downloads); + ASSERT_EQ(1u, downloads.size()); + DownloadItem* download = downloads[0]; + ASSERT_EQ(download, download1); + 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(); - // Using "DownloadItem::Remove" follows the discard dangerous download path, - // which completely removes the browser from the shelf and closes the shelf - // if it was there. Download panel stays open on ChromeOS. + GetPersistentDownloads(browser(), &downloads); + ASSERT_EQ(0u, downloads.size()); CheckDownloadUI(browser(), false, true, FilePath()); } @@ -1520,7 +1919,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(); @@ -1632,8 +2031,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_history.cc b/chrome/browser/download/download_history.cc index 3da198b..9c9a26e 100644 --- a/chrome/browser/download/download_history.cc +++ b/chrome/browser/download/download_history.cc @@ -116,14 +116,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() <= DownloadItem::kUninitializedHandle) + if (db_handle <= DownloadItem::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 6d6b237..9ef445c 100644 --- a/chrome/browser/download/download_history.h +++ b/chrome/browser/download/download_history.h @@ -47,8 +47,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_model.cc b/chrome/browser/download/download_item_model.cc index 891a5ed..f5fec1b 100644 --- a/chrome/browser/download/download_item_model.cc +++ b/chrome/browser/download/download_item_model.cc @@ -26,7 +26,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_unittest.cc b/chrome/browser/download/download_manager_unittest.cc index 3380046..6a2974b 100644 --- a/chrome/browser/download/download_manager_unittest.cc +++ b/chrome/browser/download/download_manager_unittest.cc @@ -534,7 +534,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)); @@ -630,7 +630,7 @@ TEST_F(DownloadManagerTest, DownloadFileErrorTest) { download_item_model->GetStatusText()); // Clean up. - download->Cancel(true); + download->Cancel(); message_loop_.RunAllPending(); } @@ -676,10 +676,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/ui/cocoa/download/download_item_controller.mm b/chrome/browser/ui/cocoa/download/download_item_controller.mm index eb854ba..d89cfdc 100644 --- a/chrome/browser/ui/cocoa/download/download_item_controller.mm +++ b/chrome/browser/ui/cocoa/download/download_item_controller.mm @@ -353,7 +353,7 @@ class DownloadShelfContextMenuMac : public DownloadShelfContextMenu { base::Time::Now() - creationTime_); DownloadItem* download = bridge_->download_model()->download(); if (download->IsPartialDownload()) - download->Cancel(true); + download->Cancel(); download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); // WARNING: we are deleted at this point. Don't access 'this'. } diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.cc b/chrome/browser/ui/gtk/download/download_item_gtk.cc index 5f9218e..b0d61a1 100644 --- a/chrome/browser/ui/gtk/download/download_item_gtk.cc +++ b/chrome/browser/ui/gtk/download/download_item_gtk.cc @@ -878,6 +878,6 @@ void DownloadItemGtk::OnDangerousDecline(GtkWidget* button) { UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", base::Time::Now() - creation_time_); if (get_download()->IsPartialDownload()) - get_download()->Cancel(true); + get_download()->Cancel(); get_download()->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); } diff --git a/chrome/browser/ui/panels/panel_browsertest.cc b/chrome/browser/ui/panels/panel_browsertest.cc index f3b29b1..ec8c538 100644 --- a/chrome/browser/ui/panels/panel_browsertest.cc +++ b/chrome/browser/ui/panels/panel_browsertest.cc @@ -890,7 +890,7 @@ class DownloadObserver : public DownloadManager::Observer { return; EXPECT_EQ(1U, downloads.size()); - downloads.front()->Cancel(false); // Don't actually need to download it. + downloads.front()->Cancel(); // Don't actually need to download it. saw_download_ = true; EXPECT_TRUE(waiting_); diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc index 02010f7..79ecc01 100644 --- a/chrome/browser/ui/views/download/download_item_view.cc +++ b/chrome/browser/ui/views/download/download_item_view.cc @@ -654,7 +654,7 @@ void DownloadItemView::ButtonPressed( UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", base::Time::Now() - creation_time_); if (download_->IsPartialDownload()) - download_->Cancel(true); + download_->Cancel(); download_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); // WARNING: we are deleted at this point. Don't access 'this'. } else if (sender == save_button_) { diff --git a/chrome/browser/ui/webui/active_downloads_ui.cc b/chrome/browser/ui/webui/active_downloads_ui.cc index 3d5af17..5114b9f 100644 --- a/chrome/browser/ui/webui/active_downloads_ui.cc +++ b/chrome/browser/ui/webui/active_downloads_ui.cc @@ -270,7 +270,7 @@ void ActiveDownloadsHandler::HandleCancelDownload(const ListValue* args) { DownloadItem* item = GetDownloadById(args); if (item) { if (item->IsPartialDownload()) - item->Cancel(true); + item->Cancel(); item->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); } } diff --git a/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc b/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc index 9e54398..1130256 100644 --- a/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc +++ b/chrome/browser/ui/webui/chromeos/imageburner/imageburner_ui.cc @@ -509,7 +509,7 @@ void WebUIHandler::ProcessError(int message_id) { // We don't want to process Download Cancelled signal. active_download_item_->RemoveObserver(this); if (active_download_item_->IsPartialDownload()) - active_download_item_->Cancel(true); + active_download_item_->Cancel(); active_download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); active_download_item_ = NULL; CleanupDownloadObjects(); diff --git a/chrome/browser/ui/webui/downloads_dom_handler.cc b/chrome/browser/ui/webui/downloads_dom_handler.cc index 52e986c..5f74dd2 100644 --- a/chrome/browser/ui/webui/downloads_dom_handler.cc +++ b/chrome/browser/ui/webui/downloads_dom_handler.cc @@ -314,7 +314,7 @@ void DownloadsDOMHandler::HandleCancel(const ListValue* args) { CountDownloadsDOMEvents(DOWNLOADS_DOM_EVENT_CANCEL); DownloadItem* file = GetDownloadByValue(args); if (file) - file->Cancel(true); + file->Cancel(); } void DownloadsDOMHandler::HandleClearAll(const ListValue* args) { diff --git a/chrome/test/data/download-dangerous.jar b/chrome/test/data/download-dangerous.jar new file mode 100644 index 0000000..28f58bc --- /dev/null +++ b/chrome/test/data/download-dangerous.jar @@ -0,0 +1,2 @@ +Contents of file not important; will be marked as dangerous based on +content-type and extension. diff --git a/chrome/test/data/download-dangerous.jar.mock-http-headers b/chrome/test/data/download-dangerous.jar.mock-http-headers new file mode 100644 index 0000000..e675398 --- /dev/null +++ b/chrome/test/data/download-dangerous.jar.mock-http-headers @@ -0,0 +1,6 @@ +HTTP/1.1 200 OK +Content-Type: application/x-jar +Content-Disposition: attachment; filename="download-dangerous.jar" +Content-Length: 97 +Date: Mon, 13 Nov 2006 21:38:09 GMT +Expires: Tue, 14 Nov 2006 19:23:58 GMT |