diff options
| -rw-r--r-- | chrome/browser/download/base_file.cc | 34 | ||||
| -rw-r--r-- | chrome/browser/download/base_file.h | 7 | ||||
| -rw-r--r-- | chrome/browser/download/base_file_unittest.cc | 59 | ||||
| -rw-r--r-- | chrome/browser/download/download_browsertest.cc | 2 | ||||
| -rw-r--r-- | chrome/browser/download/download_file_manager.cc | 178 | ||||
| -rw-r--r-- | chrome/browser/download/download_file_manager.h | 45 | ||||
| -rw-r--r-- | chrome/browser/download/download_item.cc | 37 | ||||
| -rw-r--r-- | chrome/browser/download/download_item.h | 4 | ||||
| -rw-r--r-- | chrome/browser/download/download_manager.cc | 142 | ||||
| -rw-r--r-- | chrome/browser/download/download_manager.h | 26 | ||||
| -rw-r--r-- | chrome/browser/download/download_manager_unittest.cc | 117 | ||||
| -rw-r--r-- | chrome/browser/download/download_util.cc | 4 | ||||
| -rw-r--r-- | chrome/browser/download/save_file_manager.cc | 8 | ||||
| -rw-r--r-- | chrome/browser/download/save_package.cc | 6 | ||||
| -rw-r--r-- | chrome/browser/ui/gtk/dialogs_gtk.cc | 3 |
15 files changed, 394 insertions, 278 deletions
diff --git a/chrome/browser/download/base_file.cc b/chrome/browser/download/base_file.cc index 9229aca..5f5a53b 100644 --- a/chrome/browser/download/base_file.cc +++ b/chrome/browser/download/base_file.cc @@ -6,6 +6,7 @@ #include "base/crypto/secure_hash.h" #include "base/file_util.h" +#include "base/format_macros.h" #include "base/logging.h" #include "base/stringprintf.h" #include "net/base/file_stream.h" @@ -30,20 +31,23 @@ BaseFile::BaseFile(const FilePath& full_path, file_stream_(file_stream), bytes_so_far_(received_bytes), power_save_blocker_(true), - calculate_hash_(false) { + calculate_hash_(false), + detached_(false) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); memset(sha256_hash_, 0, sizeof(sha256_hash_)); } BaseFile::~BaseFile() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - if (in_progress()) - Cancel(); - Close(); + if (detached_) + Close(); + else + Cancel(); // Will delete the file. } bool BaseFile::Initialize(bool calculate_hash) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(!detached_); calculate_hash_ = calculate_hash; @@ -58,6 +62,7 @@ bool BaseFile::Initialize(bool calculate_hash) { bool BaseFile::AppendDataToFile(const char* data, size_t data_len) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(!detached_); if (!file_stream_.get()) return false; @@ -140,9 +145,16 @@ bool BaseFile::Rename(const FilePath& new_path) { return true; } +void BaseFile::Detach() { + detached_ = true; +} + void BaseFile::Cancel() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(!detached_); + Close(); + if (!full_path_.empty()) file_util::Delete(full_path_, false); } @@ -157,6 +169,7 @@ void BaseFile::Finish() { } bool BaseFile::GetSha256Hash(std::string* hash) { + DCHECK(!detached_); if (!calculate_hash_ || in_progress()) return false; hash->assign(reinterpret_cast<const char*>(sha256_hash_), @@ -166,6 +179,8 @@ bool BaseFile::GetSha256Hash(std::string* hash) { void BaseFile::AnnotateWithSourceInformation() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(!detached_); + #if defined(OS_WIN) // Sets the Zone to tell Windows that this file comes from the internet. // We ignore the return value because a failure is not fatal. @@ -180,9 +195,10 @@ void BaseFile::AnnotateWithSourceInformation() { bool BaseFile::Open() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(!detached_); DCHECK(!full_path_.empty()); - // Create a new file steram if it is not provided. + // Create a new file stream if it is not provided. if (!file_stream_.get()) { file_stream_.reset(new net::FileStream); if (file_stream_->Open(full_path_, @@ -220,7 +236,11 @@ void BaseFile::Close() { } std::string BaseFile::DebugString() const { - return base::StringPrintf("{ source_url_ = \"%s\" full_path_ = \"%s\" }", + return base::StringPrintf("{ source_url_ = \"%s\"" + " full_path_ = \"%" PRFilePath "\"" + " bytes_so_far_ = %" PRId64 " detached_ = %c }", source_url_.spec().c_str(), - full_path_.value().c_str()); + full_path_.value().c_str(), + bytes_so_far_, + detached_ ? 'T' : 'F'); } diff --git a/chrome/browser/download/base_file.h b/chrome/browser/download/base_file.h index cf4bb24..1fc25aa 100644 --- a/chrome/browser/download/base_file.h +++ b/chrome/browser/download/base_file.h @@ -42,6 +42,9 @@ class BaseFile { // Rename the download file. Returns true on success. virtual bool Rename(const FilePath& full_path); + // Detach the file so it is not deleted on destruction. + virtual void Detach(); + // Abort the download and automatically close the file. void Cancel(); @@ -95,6 +98,10 @@ class BaseFile { unsigned char sha256_hash_[kSha256HashLen]; + // Indicates that this class no longer owns the associated file, and so + // won't delete it on destruction. + bool detached_; + DISALLOW_COPY_AND_ASSIGN(BaseFile); }; diff --git a/chrome/browser/download/base_file_unittest.cc b/chrome/browser/download/base_file_unittest.cc index c6176fb..5eeb685 100644 --- a/chrome/browser/download/base_file_unittest.cc +++ b/chrome/browser/download/base_file_unittest.cc @@ -19,7 +19,9 @@ const char kTestData3[] = "Final line."; class BaseFileTest : public testing::Test { public: - BaseFileTest() : file_thread_(BrowserThread::FILE, &message_loop_) { + BaseFileTest() + : expect_file_survives_(false), + file_thread_(BrowserThread::FILE, &message_loop_) { } virtual void SetUp() { @@ -33,17 +35,20 @@ class BaseFileTest : public testing::Test { EXPECT_EQ(static_cast<int64>(expected_data_.size()), base_file_->bytes_so_far()); + FilePath full_path = base_file_->full_path(); + if (!expected_data_.empty()) { // Make sure the data has been properly written to disk. std::string disk_data; - EXPECT_TRUE(file_util::ReadFileToString(base_file_->full_path(), - &disk_data)); + EXPECT_TRUE(file_util::ReadFileToString(full_path, &disk_data)); EXPECT_EQ(expected_data_, disk_data); } // Make sure the mock BrowserThread outlives the BaseFile to satisfy // thread checks inside it. base_file_.reset(); + + EXPECT_EQ(expect_file_survives_, file_util::PathExists(full_path)); } void AppendDataToFile(const std::string& data) { @@ -63,6 +68,9 @@ class BaseFileTest : public testing::Test { // Temporary directory for renamed downloads. ScopedTempDir temp_dir_; + // Expect the file to survive deletion of the BaseFile instance. + bool expect_file_survives_; + private: // Keep track of what data should be saved to the disk file. std::string expected_data_; @@ -88,6 +96,51 @@ TEST_F(BaseFileTest, Cancel) { EXPECT_NE(FilePath().value(), base_file_->full_path().value()); } +// Write data to the file and detach it, so it doesn't get deleted +// automatically when base_file_ is destructed. +TEST_F(BaseFileTest, WriteAndDetach) { + ASSERT_TRUE(base_file_->Initialize(false)); + AppendDataToFile(kTestData1); + base_file_->Finish(); + base_file_->Detach(); + expect_file_survives_ = true; +} + +// Write data to the file and detach it, and calculate its sha256 hash. +TEST_F(BaseFileTest, WriteWithHashAndDetach) { + ASSERT_TRUE(base_file_->Initialize(true)); + AppendDataToFile(kTestData1); + base_file_->Finish(); + + std::string hash; + base_file_->GetSha256Hash(&hash); + EXPECT_EQ("0B2D3F3F7943AD64B860DF94D05CB56A8A97C6EC5768B5B70B930C5AA7FA9ADE", + base::HexEncode(hash.data(), hash.size())); + + base_file_->Detach(); + expect_file_survives_ = true; +} + +// Rename the file after writing to it, then detach. +TEST_F(BaseFileTest, WriteThenRenameAndDetach) { + ASSERT_TRUE(base_file_->Initialize(false)); + + FilePath initial_path(base_file_->full_path()); + EXPECT_TRUE(file_util::PathExists(initial_path)); + FilePath new_path(temp_dir_.path().AppendASCII("NewFile")); + EXPECT_FALSE(file_util::PathExists(new_path)); + + AppendDataToFile(kTestData1); + + EXPECT_TRUE(base_file_->Rename(new_path)); + EXPECT_FALSE(file_util::PathExists(initial_path)); + EXPECT_TRUE(file_util::PathExists(new_path)); + + base_file_->Finish(); + base_file_->Detach(); + expect_file_survives_ = true; +} + // Write data to the file once. TEST_F(BaseFileTest, SingleWrite) { ASSERT_TRUE(base_file_->Initialize(false)); diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc index 84ae546..8d346b5 100644 --- a/chrome/browser/download/download_browsertest.cc +++ b/chrome/browser/download/download_browsertest.cc @@ -147,7 +147,7 @@ class DownloadsObserver : public DownloadManager::Observer, } } - virtual void SelectFileDialogDisplayed() { + virtual void SelectFileDialogDisplayed(int32 /* id */) { select_file_dialog_seen_ = true; SignalIfFinished(); } diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc index 4b7e860..7a1c298 100644 --- a/chrome/browser/download/download_file_manager.cc +++ b/chrome/browser/download/download_file_manager.cc @@ -54,7 +54,6 @@ DownloadFileManager::DownloadFileManager(ResourceDispatcherHost* rdh) DownloadFileManager::~DownloadFileManager() { DCHECK(downloads_.empty()); - DCHECK(downloads_with_final_name_.empty()); } void DownloadFileManager::Shutdown() { @@ -67,7 +66,6 @@ void DownloadFileManager::Shutdown() { void DownloadFileManager::OnShutdown() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); StopUpdateTimer(); - downloads_with_final_name_.clear(); STLDeleteValues(&downloads_); } @@ -211,31 +209,27 @@ void DownloadFileManager::OnResponseCompleted(int id, DownloadBuffer* buffer) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); delete buffer; DownloadFileMap::iterator it = downloads_.find(id); - if (it != downloads_.end()) { - DownloadFile* download = it->second; - download->Finish(); - - DownloadManager* download_manager = download->GetDownloadManager(); - if (download_manager) { - std::string hash; - if (!download->GetSha256Hash(&hash)) - hash.clear(); - - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - NewRunnableMethod( - download_manager, &DownloadManager::OnAllDataSaved, - id, download->bytes_so_far(), hash)); - } + if (it == downloads_.end()) + return; + + DownloadFile* download = it->second; + download->Finish(); - // We need to keep the download around until the UI thread has finalized - // the name. - if (ContainsKey(downloads_with_final_name_, id)) - EraseDownload(id); + DownloadManager* download_manager = download->GetDownloadManager(); + if (!download_manager) { + CancelDownload(id); + return; } - if (downloads_.empty()) - StopUpdateTimer(); + std::string hash; + if (!download->GetSha256Hash(&hash)) + hash.clear(); + + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + NewRunnableMethod( + download_manager, &DownloadManager::OnAllDataSaved, + id, download->bytes_so_far(), hash)); } // This method will be sent via a user action, or shutdown on the UI thread, and @@ -245,18 +239,32 @@ void DownloadFileManager::CancelDownload(int id) { VLOG(20) << __FUNCTION__ << "()" << " id = " << id; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DownloadFileMap::iterator it = downloads_.find(id); - if (it != downloads_.end()) { - DownloadFile* download = it->second; - VLOG(20) << __FUNCTION__ << "()" - << " download = " << download->DebugString(); - download->Cancel(); - - if (ContainsKey(downloads_with_final_name_, id)) - EraseDownload(id); - } + if (it == downloads_.end()) + return; - if (downloads_.empty()) - StopUpdateTimer(); + DownloadFile* download = it->second; + VLOG(20) << __FUNCTION__ << "()" + << " download = " << download->DebugString(); + download->Cancel(); + + EraseDownload(id); +} + +void DownloadFileManager::CompleteDownload(int id) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + + if (!ContainsKey(downloads_, id)) + return; + + DownloadFile* download_file = downloads_[id]; + + VLOG(20) << " " << __FUNCTION__ << "()" + << " id = " << id + << " download_file = " << download_file->DebugString(); + + download_file->Detach(); + + EraseDownload(id); } void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) { @@ -271,7 +279,6 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) { if (download_file->GetDownloadManager() == manager) { download_file->CancelDownloadRequest(resource_dispatcher_host_); to_remove.insert(download_file); - downloads_with_final_name_.erase(download_file->id()); } } @@ -286,18 +293,22 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) { // The DownloadManager in the UI thread has provided an intermediate .crdownload // name for the download specified by 'id'. Rename the in progress download. -void DownloadFileManager::OnIntermediateDownloadName( - int id, const FilePath& full_path, DownloadManager* download_manager) { +// +// There are 2 possible rename cases where this method can be called: +// 1. tmp -> foo.crdownload (not final, safe) +// 2. tmp-> Unconfirmed.xxx.crdownload (not final, dangerous) +void DownloadFileManager::RenameInProgressDownloadFile( + int id, const FilePath& full_path) { VLOG(20) << __FUNCTION__ << "()" << " id = " << id << " full_path = \"" << full_path.value() << "\""; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - DownloadFileMap::iterator it = downloads_.find(id); - if (it == downloads_.end()) + + DownloadFile* download = GetDownloadFile(id); + if (!download) return; - DCHECK(!ContainsKey(downloads_with_final_name_, id)); - DownloadFile* download = it->second; VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString(); + if (!download->Rename(full_path)) { // Error. Between the time the UI thread generated 'full_path' to the time // this code runs, something happened that prevents us from renaming. @@ -306,16 +317,14 @@ void DownloadFileManager::OnIntermediateDownloadName( } // The DownloadManager in the UI thread has provided a final name for the -// download specified by 'id'. Rename the in progress download, and remove it -// from our table if it has been completed or cancelled already. +// download specified by 'id'. Rename the completed download. // // There are 2 possible rename cases where this method can be called: -// 1. foo.crdownload -> foo -// 2. tmp-> Unconfirmed.xxx.crdownload -// We don't call this function before a safe temp file has been renamed (in -// that case tmp -> foo.crdownload occurs in |OnIntermediateDownloadName|). -void DownloadFileManager::OnFinalDownloadName( - int id, const FilePath& full_path, DownloadManager* download_manager) { +// 1. foo.crdownload -> foo (final, safe) +// 2. Unconfirmed.xxx.crdownload -> xxx (final, validated) +void DownloadFileManager::RenameFinishedDownloadFile( + int id, const FilePath& full_path) +{ VLOG(20) << __FUNCTION__ << "()" << " id = " << id << " full_path = \"" << full_path.value() << "\""; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); @@ -323,36 +332,48 @@ void DownloadFileManager::OnFinalDownloadName( DownloadFile* download = GetDownloadFile(id); if (!download) return; + + DCHECK(download->GetDownloadManager()); + DownloadManager* download_manager = download->GetDownloadManager(); + VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString(); - DCHECK(!ContainsKey(downloads_with_final_name_, id)); - if (download->Rename(full_path)) { - downloads_with_final_name_[id] = download; -#if defined(OS_MACOSX) - // Done here because we only want to do this once; see - // http://crbug.com/13120 for details. - download->AnnotateWithSourceInformation(); -#endif - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - NewRunnableMethod( - download_manager, &DownloadManager::DownloadRenamedToFinalName, id, - full_path)); - } else { + + int uniquifier = 0; + FilePath new_path = full_path; + // Make our name unique at this point, as if a dangerous file is + // downloading and a 2nd download is started for a file with the same + // name, they would have the same path. This is because we uniquify + // the name on download start, and at that time the first file does + // not exists yet, so the second file gets the same name. + // This should not happen in the SAFE case, and we check for that in the UI + // thread. + uniquifier = download_util::GetUniquePathNumber(new_path); + if (uniquifier > 0) { + download_util::AppendNumberToPath(&new_path, uniquifier); + } + + // Rename the file, overwriting if necessary. + if (!download->Rename(new_path)) { // Error. Between the time the UI thread generated 'full_path' to the time // this code runs, something happened that prevents us from renaming. CancelDownloadOnRename(id); + return; } - // If the download has completed before we got this final name, we remove it - // from our in progress map. - if (!download->in_progress()) - EraseDownload(id); +#if defined(OS_MACOSX) + // Done here because we only want to do this once; see + // http://crbug.com/13120 for details. + download->AnnotateWithSourceInformation(); +#endif - if (downloads_.empty()) - StopUpdateTimer(); + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + NewRunnableMethod( + download_manager, &DownloadManager::OnDownloadRenamedToFinalName, id, + new_path, uniquifier)); } -// Called only from OnFinalDownloadName or OnIntermediateDownloadName +// Called only from RenameInProgressDownloadFile and RenameFinishedDownloadFile // on the FILE thread. void DownloadFileManager::CancelDownloadOnRename(int id) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); @@ -363,6 +384,9 @@ void DownloadFileManager::CancelDownloadOnRename(int id) { DownloadManager* download_manager = download->GetDownloadManager(); if (!download_manager) { + // Without a download manager, we can't cancel the request normally, so we + // need to do it here. The normal path will also update the download + // history before cancelling the request. download->CancelDownloadRequest(resource_dispatcher_host_); return; } @@ -374,15 +398,21 @@ void DownloadFileManager::CancelDownloadOnRename(int id) { } void DownloadFileManager::EraseDownload(int id) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + if (!ContainsKey(downloads_, id)) return; DownloadFile* download_file = downloads_[id]; - downloads_.erase(id); + VLOG(20) << " " << __FUNCTION__ << "()" + << " id = " << id + << " download_file = " << download_file->DebugString(); - if (ContainsKey(downloads_with_final_name_, id)) - downloads_with_final_name_.erase(id); + downloads_.erase(id); delete download_file; + + if (downloads_.empty()) + StopUpdateTimer(); } diff --git a/chrome/browser/download/download_file_manager.h b/chrome/browser/download/download_file_manager.h index a0abc99..7b8a0b4 100644 --- a/chrome/browser/download/download_file_manager.h +++ b/chrome/browser/download/download_file_manager.h @@ -43,6 +43,7 @@ #include <map> #include "base/basictypes.h" +#include "base/gtest_prod_util.h" #include "base/hash_tables.h" #include "base/ref_counted.h" #include "base/timer.h" @@ -61,10 +62,6 @@ class URLRequestContextGetter; // Manages all in progress downloads. class DownloadFileManager : public base::RefCountedThreadSafe<DownloadFileManager> { - - // For testing. - friend class DownloadManagerTest; - public: explicit DownloadFileManager(ResourceDispatcherHost* rdh); @@ -78,26 +75,34 @@ class DownloadFileManager void StartDownload(DownloadCreateInfo* info); // Handlers for notifications sent from the IO thread and run on the - // download thread. + // FILE thread. void UpdateDownload(int id, DownloadBuffer* buffer); - void CancelDownload(int id); void OnResponseCompleted(int id, DownloadBuffer* buffer); + // Handlers for notifications sent from the UI thread and run on the + // FILE thread. These are both terminal actions with respect to the + // download file, as far as the DownloadFileManager is concerned -- if + // anything happens to the download file after they are called, it will + // be ignored. + void CancelDownload(int id); + void CompleteDownload(int id); + // Called on FILE thread by DownloadManager at the beginning of its shutdown. void OnDownloadManagerShutdown(DownloadManager* manager); // The DownloadManager in the UI thread has provided an intermediate - // .crdownload name for the download specified by 'id'. - void OnIntermediateDownloadName(int id, const FilePath& full_path, - DownloadManager* download_manager); + // .crdownload name for the download specified by |id|. + void RenameInProgressDownloadFile(int id, const FilePath& full_path); - // The download manager has provided a final name for a download. Sent from - // the UI thread and run on the download thread. - void OnFinalDownloadName(int id, const FilePath& full_path, - DownloadManager* download_manager); + // The DownloadManager in the UI thread has provided a final name for the + // download specified by |id|. Sent from the UI thread and run on the + // FILE thread. + void RenameFinishedDownloadFile(int id, const FilePath& full_path); private: friend class base::RefCountedThreadSafe<DownloadFileManager>; + friend class DownloadManagerTest; + FRIEND_TEST_ALL_PREFIXES(DownloadManagerTest, StartDownload); ~DownloadFileManager(); @@ -122,12 +127,12 @@ class DownloadFileManager // Called only on the download thread. DownloadFile* GetDownloadFile(int id); - // Called only from OnFinalDownloadName or OnIntermediateDownloadName - // on the FILE thread. + // Called only from RenameInProgressDownloadFile and + // RenameFinishedDownloadFile on the FILE thread. void CancelDownloadOnRename(int id); - // Erases the download file with the given the download |id| and removes - // it from the maps. + // Called when the DownloadFileManager has finished with the download file. + // Will delete the download file if it hasn't been detached. void EraseDownload(int id); // Unique ID for each DownloadFile. @@ -135,13 +140,9 @@ class DownloadFileManager typedef base::hash_map<int, DownloadFile*> DownloadFileMap; - // A map of all in progress downloads. It owns the download files, - // although they may also show up in |downloads_with_final_name_|. + // A map of all in progress downloads. It owns the download files. DownloadFileMap downloads_; - // download files are owned by |downloads_|. - DownloadFileMap downloads_with_final_name_; - // Schedule periodic updates of the download progress. This timer // is controlled from the FILE thread, and posts updates to the UI thread. base::RepeatingTimer<DownloadFileManager> update_timer_; diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc index d593608..3c303d0 100644 --- a/chrome/browser/download/download_item.cc +++ b/chrome/browser/download/download_item.cc @@ -359,6 +359,8 @@ void DownloadItem::OnAllDataSaved(int64 size) { } void DownloadItem::Finished() { + VLOG(20) << " " << __FUNCTION__ << "() " + << DebugString(false); // Handle chrome extensions explicitly and skip the shell execute. if (is_extension_install()) { download_util::OpenChromeExtension(download_manager_->profile(), @@ -433,7 +435,7 @@ int DownloadItem::PercentComplete() const { void DownloadItem::Rename(const FilePath& full_path) { VLOG(20) << " " << __FUNCTION__ << "()" - << " full_path = " << full_path.value() + << " full_path = \"" << full_path.value() << "\"" << DebugString(true); DCHECK(!full_path.empty()); full_path_ = full_path; @@ -447,6 +449,8 @@ void DownloadItem::TogglePause() { } void DownloadItem::OnNameFinalized() { + VLOG(20) << " " << __FUNCTION__ << "() " + << DebugString(true); name_finalized_ = true; // The download file is meant to be completed if both the filename is @@ -459,34 +463,43 @@ void DownloadItem::OnNameFinalized() { } } -void DownloadItem::OnSafeDownloadFinished(DownloadFileManager* file_manager) { - DCHECK_EQ(SAFE, safety_state()); +void DownloadItem::OnDownloadFinished(DownloadFileManager* file_manager) { + VLOG(20) << " " << __FUNCTION__ << "() " + << " needs rename = " << NeedsRename() + << " " << DebugString(true); + DCHECK_NE(DANGEROUS, safety_state()); DCHECK(file_manager); + if (NeedsRename()) { BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, NewRunnableMethod( - file_manager, &DownloadFileManager::OnFinalDownloadName, - id(), GetTargetFilePath(), make_scoped_refptr(download_manager_))); + file_manager, &DownloadFileManager::RenameFinishedDownloadFile, + id(), GetTargetFilePath())); return; } Finished(); + + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + NewRunnableMethod( + file_manager, &DownloadFileManager::CompleteDownload, id())); } void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) { VLOG(20) << " " << __FUNCTION__ << "()" - << " full_path = " << full_path.value(); - bool needed_rename = NeedsRename(); + << " full_path = " << full_path.value() + << " needed rename = " << NeedsRename() + << " " << DebugString(false); + DCHECK(NeedsRename()); Rename(full_path); OnNameFinalized(); - if (needed_rename && safety_state() == SAFE) { - // This was called from OnSafeDownloadFinished; continue to call - // DownloadFinished. - Finished(); - } + // This was called from OnDownloadFinished; continue to call + // DownloadFinished. + Finished(); } bool DownloadItem::MatchesQuery(const string16& query) const { diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h index 5eadea8..9ffff60 100644 --- a/chrome/browser/download/download_item.h +++ b/chrome/browser/download/download_item.h @@ -202,10 +202,10 @@ class DownloadItem { // Called when the name of the download is finalized. void OnNameFinalized(); - // Called when the download is finished for safe downloads. + // Called when the download is finished. // This may perform final rename if necessary and will eventually call // DownloadManager::DownloadFinished(). - void OnSafeDownloadFinished(DownloadFileManager* file_manager); + void OnDownloadFinished(DownloadFileManager* file_manager); // Called when the file name for the download is renamed to its final name. void OnDownloadRenamedToFinalName(const FilePath& full_path); diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc index f2e6ec7..c24f096 100644 --- a/chrome/browser/download/download_manager.cc +++ b/chrome/browser/download/download_manager.cc @@ -439,7 +439,8 @@ void DownloadManager::OnPathExistenceAvailable(DownloadCreateInfo* info) { info->suggested_path, &file_type_info, 0, FILE_PATH_LITERAL(""), owning_window, info); - FOR_EACH_OBSERVER(Observer, observers_, SelectFileDialogDisplayed()); + FOR_EACH_OBSERVER(Observer, observers_, + SelectFileDialogDisplayed(info->download_id)); } else { // No prompting for download, just continue with the suggested name. info->path = info->suggested_path; @@ -485,30 +486,29 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info) { UpdateAppIcon(); // Reflect entry into in_progress_. // Rename to intermediate name. + FilePath download_path; if (info->IsDangerous()) { // The download is not safe. We can now rename the file to its - // tentative name using OnFinalDownloadName (the actual final - // name after user confirmation will be set in - // ProceedWithFinishedDangerousDownload). - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - NewRunnableMethod( - file_manager_, &DownloadFileManager::OnFinalDownloadName, - download->id(), info->path, make_scoped_refptr(this))); + // tentative name using RenameInProgressDownloadFile. + // NOTE: The |Rename| below will be a no-op for dangerous files, as we're + // renaming it to the same name. + download_path = info->path; } else { // The download is a safe download. We need to // rename it to its intermediate '.crdownload' path. The final // name after user confirmation will be set from - // DownloadItem::OnSafeDownloadFinished. - FilePath download_path = download_util::GetCrDownloadPath(info->path); - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - NewRunnableMethod( - file_manager_, &DownloadFileManager::OnIntermediateDownloadName, - download->id(), download_path, make_scoped_refptr(this))); - download->Rename(download_path); + // DownloadItem::OnDownloadFinished. + download_path = download_util::GetCrDownloadPath(info->path); } + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + NewRunnableMethod( + file_manager_, &DownloadFileManager::RenameInProgressDownloadFile, + download->id(), download_path)); + + download->Rename(download_path); + download_history_->AddEntry(*info, download, NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete)); } @@ -627,30 +627,8 @@ void DownloadManager::MaybeCompleteDownload(DownloadItem* download) { download->MarkAsComplete(); download_history_->UpdateEntry(download); - switch (download->safety_state()) { - case DownloadItem::DANGEROUS: - // If this a dangerous download not yet validated by the user, don't do - // anything. When the user notifies us, it will trigger a call to - // ProceedWithFinishedDangerousDownload. - NOTREACHED(); - return; - case DownloadItem::DANGEROUS_BUT_VALIDATED: - // The dangerous download has been validated by the user. We first - // need to rename the downloaded file from its temporary name to - // its final name. We will continue the download processing in the - // callback. - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - NewRunnableMethod( - this, &DownloadManager::ProceedWithFinishedDangerousDownload, - download->db_handle(), - download->full_path(), download->target_name())); - return; - case DownloadItem::SAFE: - // The download is safe; just finish it. - download->OnSafeDownloadFinished(file_manager_); - return; - } + // Finish the download. + download->OnDownloadFinished(file_manager_); } void DownloadManager::RemoveFromActiveList(int32 download_id) { @@ -658,72 +636,39 @@ void DownloadManager::RemoveFromActiveList(int32 download_id) { active_downloads_.erase(download_id); } -void DownloadManager::DownloadRenamedToFinalName(int download_id, - const FilePath& full_path) { +void DownloadManager::OnDownloadRenamedToFinalName(int download_id, + const FilePath& full_path, + int uniquifier) { VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id - << " full_path = \"" << full_path.value() << "\""; + << " full_path = \"" << full_path.value() << "\"" + << " uniquifier = " << uniquifier; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DownloadItem* item = GetDownloadItem(download_id); if (!item) return; - item->OnDownloadRenamedToFinalName(full_path); -} - -// Called on the file thread. Renames the downloaded file to its original name. -void DownloadManager::ProceedWithFinishedDangerousDownload( - int64 download_handle, - const FilePath& path, - const FilePath& original_name) { - bool success = false; - FilePath new_path; - int uniquifier = 0; - if (file_util::PathExists(path)) { - new_path = path.DirName().Append(original_name); - // Make our name unique at this point, as if a dangerous file is downloading - // and a 2nd download is started for a file with the same name, they would - // have the same path. This is because we uniquify the name on download - // start, and at that time the first file does not exists yet, so the second - // file gets the same name. - uniquifier = download_util::GetUniquePathNumber(new_path); - if (uniquifier > 0) - download_util::AppendNumberToPath(&new_path, uniquifier); - success = file_util::Move(path, new_path); - } else { - NOTREACHED(); - } BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - NewRunnableMethod(this, &DownloadManager::DangerousDownloadRenamed, - download_handle, success, new_path, uniquifier)); -} + BrowserThread::FILE, FROM_HERE, + NewRunnableMethod( + file_manager_, &DownloadFileManager::CompleteDownload, download_id)); -// Call from the file thread when the finished dangerous download was renamed. -void DownloadManager::DangerousDownloadRenamed(int64 download_handle, - bool success, - const FilePath& new_path, - int new_path_uniquifier) { - VLOG(20) << __FUNCTION__ << "()" << " download_handle = " << download_handle - << " success = " << success - << " new_path = \"" << new_path.value() << "\"" - << " new_path_uniquifier = " << new_path_uniquifier; - DownloadMap::iterator it = history_downloads_.find(download_handle); - if (it == history_downloads_.end()) { - NOTREACHED(); + if ((item->safety_state() == DownloadItem::SAFE) && (uniquifier != 0)) { + // File name conflict: the file name we expected to use at the start + // of the safe download was taken. + // TODO(ahendrickson): Warn the user that we're cancelling the download. + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + NewRunnableMethod( + file_manager_, &DownloadFileManager::CancelDownload, download_id)); return; } - DownloadItem* download = it->second; - // If we failed to rename the file, we'll just keep the name as is. - if (success) { - // We need to update the path uniquifier so that the UI shows the right - // name when calling GetFileNameToReportUser(). - download->set_path_uniquifier(new_path_uniquifier); - RenameDownload(download, new_path); - } + if (uniquifier) + item->set_path_uniquifier(uniquifier); - // Continue the download finished sequence. - download->Finished(); + item->OnDownloadRenamedToFinalName(full_path); + download_history_->UpdateDownloadPath(item, full_path); } void DownloadManager::DownloadCancelled(int32 download_id) { @@ -753,6 +698,7 @@ void DownloadManager::DownloadCancelled(int32 download_id) { void DownloadManager::DownloadCancelledInternal(int download_id, int render_process_id, int request_id) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); // Cancel the network request. RDH is guaranteed to outlive the IO thread. BrowserThread::PostTask( BrowserThread::IO, FROM_HERE, @@ -791,12 +737,6 @@ void DownloadManager::UpdateAppIcon() { status_updater_->Update(); } -void DownloadManager::RenameDownload(DownloadItem* download, - const FilePath& new_path) { - download->Rename(new_path); - download_history_->UpdateDownloadPath(download, new_path); -} - void DownloadManager::PauseDownloadRequest(ResourceDispatcherHost* rdh, int render_process_id, int request_id, diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h index 070693cd..42d4ed9 100644 --- a/chrome/browser/download/download_manager.h +++ b/chrome/browser/download/download_manager.h @@ -83,7 +83,8 @@ class DownloadManager // Called immediately after the DownloadManager puts up a select file // dialog. - virtual void SelectFileDialogDisplayed() {} + // |id| indicates which download opened the dialog. + virtual void SelectFileDialogDisplayed(int32 id) {} protected: virtual ~Observer() {} @@ -135,7 +136,11 @@ class DownloadManager void MaybeCompleteDownload(DownloadItem* download); // Called when the download is renamed to its final name. - void DownloadRenamedToFinalName(int download_id, const FilePath& full_path); + // |uniquifier| is a number used to make unique names for the file. It is + // only valid for the DANGEROUS_BUT_VALIDATED state of the download item. + void OnDownloadRenamedToFinalName(int download_id, + const FilePath& full_path, + int uniquifier); // Remove downloads after remove_begin (inclusive) and before remove_end // (exclusive). You may pass in null Time values to do an unbounded delete @@ -282,26 +287,9 @@ class DownloadManager int render_process_id, int request_id); - // Renames a finished dangerous download from its temporary file name to its - // real file name. - // Invoked on the file thread. - void ProceedWithFinishedDangerousDownload(int64 download_handle, - const FilePath& path, - const FilePath& original_name); - - // Invoked on the UI thread when a dangerous downloaded file has been renamed. - void DangerousDownloadRenamed(int64 download_handle, - bool success, - const FilePath& new_path, - int new_path_uniquifier); - // Updates the app icon about the overall download progress. void UpdateAppIcon(); - // Changes the paths and file name of the specified |download|, propagating - // the change to the history system. - void RenameDownload(DownloadItem* download, const FilePath& new_path); - // Makes the ResourceDispatcherHost pause/un-pause a download request. // Called on the IO thread. void PauseDownloadRequest(ResourceDispatcherHost* rdh, diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc index 683a993..a857d34 100644 --- a/chrome/browser/download/download_manager_unittest.cc +++ b/chrome/browser/download/download_manager_unittest.cc @@ -3,7 +3,9 @@ // found in the LICENSE file. #include <string> +#include <set> +#include "base/scoped_ptr.h" #include "base/string_util.h" #include "build/build_config.h" #include "chrome/browser/download/download_file.h" @@ -28,7 +30,8 @@ class DownloadManagerTest : public testing::Test { DownloadManagerTest() : profile_(new TestingProfile()), download_manager_(new MockDownloadManager(&download_status_updater_)), - ui_thread_(BrowserThread::UI, &message_loop_) { + ui_thread_(BrowserThread::UI, &message_loop_), + file_thread_(BrowserThread::FILE, &message_loop_) { download_manager_->Init(profile_.get()); } @@ -52,6 +55,7 @@ class DownloadManagerTest : public testing::Test { scoped_refptr<DownloadFileManager> file_manager_; MessageLoopForUI message_loop_; BrowserThread ui_thread_; + BrowserThread file_thread_; DownloadFileManager* file_manager() { if (!file_manager_) { @@ -126,34 +130,6 @@ const struct { true, }, }; -} // namespace - -TEST_F(DownloadManagerTest, StartDownload) { - BrowserThread io_thread(BrowserThread::IO, &message_loop_); - PrefService* prefs = profile_->GetPrefs(); - prefs->SetFilePath(prefs::kDownloadDefaultDirectory, FilePath()); - download_manager_->download_prefs()->EnableAutoOpenBasedOnExtension( - FilePath(FILE_PATH_LITERAL("example.pdf"))); - - for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kStartDownloadCases); ++i) { - prefs->SetBoolean(prefs::kPromptForDownload, - kStartDownloadCases[i].prompt_for_download); - - DownloadCreateInfo info; - info.prompt_user_for_save_location = kStartDownloadCases[i].save_as; - info.url = GURL(kStartDownloadCases[i].url); - info.mime_type = kStartDownloadCases[i].mime_type; - - download_manager_->StartDownload(&info); - message_loop_.RunAllPending(); - - EXPECT_EQ(kStartDownloadCases[i].expected_save_as, - info.prompt_user_for_save_location); - } -} - -namespace { - const struct { FilePath::StringType suggested_path; bool is_dangerous_file; @@ -191,8 +167,8 @@ const struct { class MockDownloadFile : public DownloadFile { public: - explicit MockDownloadFile(DownloadCreateInfo* info) - : DownloadFile(info, NULL), renamed_count_(0) { } + MockDownloadFile(DownloadCreateInfo* info, DownloadManager* manager) + : DownloadFile(info, manager), renamed_count_(0) { } virtual ~MockDownloadFile() { Destructed(); } MOCK_METHOD1(Rename, bool(const FilePath&)); MOCK_METHOD0(Destructed, void()); @@ -210,16 +186,87 @@ class MockDownloadFile : public DownloadFile { int renamed_count_; }; +// This is an observer that records what download IDs have opened a select +// file dialog. +class SelectFileObserver : public DownloadManager::Observer { + public: + explicit SelectFileObserver(DownloadManager* download_manager) + : download_manager_(download_manager) { + DCHECK(download_manager_); + download_manager_->AddObserver(this); + } + + ~SelectFileObserver() { + download_manager_->RemoveObserver(this); + } + + // Downloadmanager::Observer functions. + virtual void ModelChanged() {} + virtual void ManagerGoingDown() {} + virtual void SelectFileDialogDisplayed(int32 id) { + file_dialog_ids_.insert(id); + } + + bool ShowedFileDialogForId(int32 id) { + return file_dialog_ids_.find(id) != file_dialog_ids_.end(); + } + + private: + std::set<int32> file_dialog_ids_; + DownloadManager* download_manager_; +}; + } // namespace +TEST_F(DownloadManagerTest, StartDownload) { + BrowserThread io_thread(BrowserThread::IO, &message_loop_); + PrefService* prefs = profile_->GetPrefs(); + prefs->SetFilePath(prefs::kDownloadDefaultDirectory, FilePath()); + download_manager_->download_prefs()->EnableAutoOpenBasedOnExtension( + FilePath(FILE_PATH_LITERAL("example.pdf"))); + + for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kStartDownloadCases); ++i) { + prefs->SetBoolean(prefs::kPromptForDownload, + kStartDownloadCases[i].prompt_for_download); + + SelectFileObserver observer(download_manager_); + DownloadCreateInfo* info = new DownloadCreateInfo; + info->download_id = static_cast<int>(i); + info->prompt_user_for_save_location = kStartDownloadCases[i].save_as; + info->url = GURL(kStartDownloadCases[i].url); + info->mime_type = kStartDownloadCases[i].mime_type; + download_manager_->CreateDownloadItem(info); + + DownloadFile* download(new DownloadFile(info, download_manager_)); + AddDownloadToFileManager(info->download_id, download); + download->Initialize(false); + download_manager_->StartDownload(info); + message_loop_.RunAllPending(); + + // NOTE: At this point, |AttachDownloadItem| will have been run if we don't + // need to prompt the user, so |info| could have been destructed. + // This means that we can't check any of its values. + // However, SelectFileObserver will have recorded any attempt to open the + // select file dialog. + EXPECT_EQ(kStartDownloadCases[i].expected_save_as, + observer.ShowedFileDialogForId(i)); + + // If the Save As dialog pops up, it never reached + // DownloadManager::AttachDownloadItem(), and never deleted info or + // completed. This cleans up info. + // Note that DownloadManager::FileSelectionCanceled() is never called. + if (observer.ShowedFileDialogForId(i)) { + delete info; + } + } +} + TEST_F(DownloadManagerTest, DownloadRenameTest) { using ::testing::_; using ::testing::CreateFunctor; using ::testing::Invoke; using ::testing::Return; - BrowserThread file_thread(BrowserThread::FILE, &message_loop_); - for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDownloadRenameCases); ++i) { // |info| will be destroyed in download_manager_. DownloadCreateInfo* info(new DownloadCreateInfo); @@ -229,7 +276,7 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) { info->is_dangerous_url = kDownloadRenameCases[i].is_dangerous_url; FilePath new_path(kDownloadRenameCases[i].suggested_path); - MockDownloadFile* download(new MockDownloadFile(info)); + MockDownloadFile* download(new MockDownloadFile(info, download_manager_)); AddDownloadToFileManager(info->download_id, download); // |download| is owned by DownloadFileManager. @@ -253,9 +300,11 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) { if (kDownloadRenameCases[i].finish_before_rename) { download_manager_->OnAllDataSaved(i, 1024, std::string("fake_hash")); + message_loop_.RunAllPending(); download_manager_->FileSelected(new_path, i, info); } else { download_manager_->FileSelected(new_path, i, info); + message_loop_.RunAllPending(); download_manager_->OnAllDataSaved(i, 1024, std::string("fake_hash")); } diff --git a/chrome/browser/download/download_util.cc b/chrome/browser/download/download_util.cc index f00b40b..62ff15f 100644 --- a/chrome/browser/download/download_util.cc +++ b/chrome/browser/download/download_util.cc @@ -757,7 +757,9 @@ void CancelDownloadRequest(ResourceDispatcherHost* rdh, int render_process_id, int request_id) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - rdh->CancelRequest(render_process_id, request_id, false); + // |rdh| may be NULL in unit tests. + if (rdh) + rdh->CancelRequest(render_process_id, request_id, false); } void NotifyDownloadInitiated(int render_process_id, int render_view_id) { diff --git a/chrome/browser/download/save_file_manager.cc b/chrome/browser/download/save_file_manager.cc index 8240b9f..dfce49d 100644 --- a/chrome/browser/download/save_file_manager.cc +++ b/chrome/browser/download/save_file_manager.cc @@ -262,10 +262,16 @@ void SaveFileManager::SaveFinished(int save_id, const GURL& save_url, int render_process_id, bool is_success) { + VLOG(20) << " " << __FUNCTION__ << "()" + << " save_id = " << save_id + << " save_url = \"" << save_url.spec() << "\"" + << " is_success = " << is_success; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); SaveFileMap::iterator it = save_file_map_.find(save_id); if (it != save_file_map_.end()) { SaveFile* save_file = it->second; + VLOG(20) << " " << __FUNCTION__ << "()" + << " save_file = " << save_file->DebugString(); BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, NewRunnableMethod( @@ -273,6 +279,7 @@ void SaveFileManager::SaveFinished(int save_id, save_file->bytes_so_far(), is_success)); save_file->Finish(); + save_file->Detach(); } else if (save_id == -1) { // Before saving started, we got error. We still call finish process. DCHECK(!save_url.is_empty()); @@ -438,6 +445,7 @@ void SaveFileManager::SaveLocalFile(const GURL& original_file_url, // Close the save file before the copy operation. save_file->Finish(); + save_file->Detach(); DCHECK(original_file_url.SchemeIsFile()); FilePath file_path; diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc index abb0a21..0c29107 100644 --- a/chrome/browser/download/save_package.cc +++ b/chrome/browser/download/save_package.cc @@ -1018,6 +1018,9 @@ void SavePackage::OnReceivedSerializedHtmlData(const GURL& frame_url, if (flag == WebPageSerializerClient::AllFramesAreFinished) { for (SaveUrlItemMap::iterator it = in_progress_items_.begin(); it != in_progress_items_.end(); ++it) { + VLOG(20) << " " << __FUNCTION__ << "()" + << " save_id = " << it->second->save_id() + << " url = \"" << it->second->url().spec() << "\""; BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, NewRunnableMethod(file_manager_, @@ -1053,6 +1056,9 @@ void SavePackage::OnReceivedSerializedHtmlData(const GURL& frame_url, // Current frame is completed saving, call finish in file thread. if (flag == WebPageSerializerClient::CurrentFrameIsFinished) { + VLOG(20) << " " << __FUNCTION__ << "()" + << " save_id = " << save_item->save_id() + << " url = \"" << save_item->url().spec() << "\""; BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, NewRunnableMethod(file_manager_, diff --git a/chrome/browser/ui/gtk/dialogs_gtk.cc b/chrome/browser/ui/gtk/dialogs_gtk.cc index c33a7d6..98a4025 100644 --- a/chrome/browser/ui/gtk/dialogs_gtk.cc +++ b/chrome/browser/ui/gtk/dialogs_gtk.cc @@ -163,8 +163,7 @@ FilePath* SelectFileDialogImpl::last_opened_path_ = NULL; // static SelectFileDialog* SelectFileDialog::Create(Listener* listener) { - DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)); - DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); return new SelectFileDialogImpl(listener); } |