diff options
Diffstat (limited to 'chromium/content/browser/download')
25 files changed, 847 insertions, 492 deletions
diff --git a/chromium/content/browser/download/base_file.cc b/chromium/content/browser/download/base_file.cc index e34970f7868..735793270ff 100644 --- a/chromium/content/browser/download/base_file.cc +++ b/chromium/content/browser/download/base_file.cc @@ -208,7 +208,7 @@ std::string BaseFile::DebugString() const { DownloadInterruptReason BaseFile::CalculatePartialHash( const std::string& hash_to_expect) { - secure_hash_.reset(crypto::SecureHash::Create(crypto::SecureHash::SHA256)); + secure_hash_ = crypto::SecureHash::Create(crypto::SecureHash::SHA256); if (bytes_so_far_ == 0) return DOWNLOAD_INTERRUPT_REASON_NONE; diff --git a/chromium/content/browser/download/docs/save-page-as.md b/chromium/content/browser/download/docs/save-page-as.md index 2661289b375..a4b6fd24071 100644 --- a/chromium/content/browser/download/docs/save-page-as.md +++ b/chromium/content/browser/download/docs/save-page-as.md @@ -16,10 +16,9 @@ are described by their code comments or by their code structure). * UI-thread object * SaveFileCreateInfo::SaveFileSource enum - * classifies `SaveItem` and `SaveFile` processing into 3 flavours: + * classifies `SaveItem` and `SaveFile` processing into 2 flavours: * `SAVE_FILE_FROM_NET` (see `SaveFileResourceHandler`) * `SAVE_FILE_FROM_DOM` (see "Complete HTML" section below) - * `SAVE_FILE_FROM_FILE` (see `SaveFileManager::SaveLocalFile`) * SaveItem class * tracks saving a single file @@ -68,8 +67,8 @@ Very high-level flow of saving a page as "Complete HTML": * Step 1: `SavePackage` asks all frames for "savable resources" and creates `SaveItem` for each of files that need to be saved -* Step 2: `SavePackage` first processes `SAVE_FILE_FROM_NET` and - `SAVE_FILE_FROM_FILE` `SaveItem`s and asks `SaveFileManager` to save +* Step 2: `SavePackage` first processes `SAVE_FILE_FROM_NET` + `SaveItem`s and asks `SaveFileManager` to save them. * Step 3: `SavePackage` handles remaining `SAVE_FILE_FROM_DOM` `SaveItem`s and @@ -88,13 +87,16 @@ Very high-level flow of saving a page as MHTML: API) or by an embedder of `WebContents` (since this is public API of //content). -* Step 2: `MHTMLGenerationManager` coordinates generation of the MHTML file - by sequentially (one-at-a-time) asking each frame to write its portion - of MHTML to a file handle. Other classes (i.e. `SavePackage` and/or - `SaveFileManager`) are not used at this step at all. +* Step 2: `MHTMLGenerationManager` creates a new instance of + `MHTMLGenerationManager::Job` that coordinates generation of + the MHTML file by sequentially (one-at-a-time) asking each + frame to write its portion of MHTML to a file handle. Other + classes (i.e. `SavePackage` and/or `SaveFileManager`) are not + used at this step at all. -* Step 3: When done `MHTMLGenerationManager` calls a completion callback - which in case of Save-Page-As will end up in +* Step 3: When done `MHTMLGenerationManager` destroys + `MHTMLGenerationManager::Job` instance and calls a completion + callback which in case of Save-Page-As will end up in `SavePackage::OnMHTMLGenerated`. Note: MHTML format is by default disabled in Save-Page-As UI on Windows, MacOS @@ -106,8 +108,8 @@ changed using `--save-page-as-mhtml` command line switch. Very high-level flow of saving a page as "HTML Only": -* `SavePackage` creates only a single `SaveItem` (either `SAVE_FILE_FROM_NET` or - `SAVE_FILE_FROM_FILE`) and asks `SaveFileManager` to process it +* `SavePackage` creates only a single `SaveItem` (always `SAVE_FILE_FROM_NET`) + and asks `SaveFileManager` to process it (as in the Complete HTML individual SaveItem handling above.). diff --git a/chromium/content/browser/download/download_browsertest.cc b/chromium/content/browser/download/download_browsertest.cc index 5d78e1c7a82..352f7a22b1b 100644 --- a/chromium/content/browser/download/download_browsertest.cc +++ b/chromium/content/browser/download/download_browsertest.cc @@ -32,7 +32,6 @@ #include "content/browser/loader/resource_dispatcher_host_impl.h" #include "content/browser/web_contents/web_contents_impl.h" #include "content/public/browser/download_danger_type.h" -#include "content/public/browser/power_save_blocker.h" #include "content/public/browser/resource_dispatcher_host_delegate.h" #include "content/public/browser/resource_throttle.h" #include "content/public/common/content_features.h" @@ -48,6 +47,7 @@ #include "content/shell/browser/shell_browser_context.h" #include "content/shell/browser/shell_download_manager_delegate.h" #include "content/shell/browser/shell_network_delegate.h" +#include "device/power_save_blocker/power_save_blocker.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "net/test/embedded_test_server/http_request.h" #include "net/test/embedded_test_server/http_response.h" @@ -122,13 +122,14 @@ static DownloadManagerImpl* DownloadManagerForShell(Shell* shell) { class DownloadFileWithDelay : public DownloadFileImpl { public: - DownloadFileWithDelay(std::unique_ptr<DownloadSaveInfo> save_info, - const base::FilePath& default_download_directory, - std::unique_ptr<ByteStreamReader> stream, - const net::BoundNetLog& bound_net_log, - std::unique_ptr<PowerSaveBlocker> power_save_blocker, - base::WeakPtr<DownloadDestinationObserver> observer, - base::WeakPtr<DownloadFileWithDelayFactory> owner); + DownloadFileWithDelay( + std::unique_ptr<DownloadSaveInfo> save_info, + const base::FilePath& default_download_directory, + std::unique_ptr<ByteStreamReader> stream, + const net::BoundNetLog& bound_net_log, + std::unique_ptr<device::PowerSaveBlocker> power_save_blocker, + base::WeakPtr<DownloadDestinationObserver> observer, + base::WeakPtr<DownloadFileWithDelayFactory> owner); ~DownloadFileWithDelay() override; @@ -193,7 +194,7 @@ DownloadFileWithDelay::DownloadFileWithDelay( const base::FilePath& default_download_directory, std::unique_ptr<ByteStreamReader> stream, const net::BoundNetLog& bound_net_log, - std::unique_ptr<PowerSaveBlocker> power_save_blocker, + std::unique_ptr<device::PowerSaveBlocker> power_save_blocker, base::WeakPtr<DownloadDestinationObserver> observer, base::WeakPtr<DownloadFileWithDelayFactory> owner) : DownloadFileImpl(std::move(save_info), @@ -254,9 +255,11 @@ DownloadFile* DownloadFileWithDelayFactory::CreateFile( std::unique_ptr<ByteStreamReader> stream, const net::BoundNetLog& bound_net_log, base::WeakPtr<DownloadDestinationObserver> observer) { - std::unique_ptr<PowerSaveBlocker> psb(PowerSaveBlocker::Create( - PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, - PowerSaveBlocker::kReasonOther, "Download in progress")); + std::unique_ptr<device::PowerSaveBlocker> psb(new device::PowerSaveBlocker( + device::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, + device::PowerSaveBlocker::kReasonOther, "Download in progress", + BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI), + BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE))); return new DownloadFileWithDelay(std::move(save_info), default_download_directory, std::move(stream), @@ -291,12 +294,13 @@ void DownloadFileWithDelayFactory::WaitForSomeCallback() { class CountingDownloadFile : public DownloadFileImpl { public: - CountingDownloadFile(std::unique_ptr<DownloadSaveInfo> save_info, - const base::FilePath& default_downloads_directory, - std::unique_ptr<ByteStreamReader> stream, - const net::BoundNetLog& bound_net_log, - std::unique_ptr<PowerSaveBlocker> power_save_blocker, - base::WeakPtr<DownloadDestinationObserver> observer) + CountingDownloadFile( + std::unique_ptr<DownloadSaveInfo> save_info, + const base::FilePath& default_downloads_directory, + std::unique_ptr<ByteStreamReader> stream, + const net::BoundNetLog& bound_net_log, + std::unique_ptr<device::PowerSaveBlocker> power_save_blocker, + base::WeakPtr<DownloadDestinationObserver> observer) : DownloadFileImpl(std::move(save_info), default_downloads_directory, std::move(stream), @@ -350,9 +354,11 @@ class CountingDownloadFileFactory : public DownloadFileFactory { std::unique_ptr<ByteStreamReader> stream, const net::BoundNetLog& bound_net_log, base::WeakPtr<DownloadDestinationObserver> observer) override { - std::unique_ptr<PowerSaveBlocker> psb(PowerSaveBlocker::Create( - PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, - PowerSaveBlocker::kReasonOther, "Download in progress")); + std::unique_ptr<device::PowerSaveBlocker> psb(new device::PowerSaveBlocker( + device::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, + device::PowerSaveBlocker::kReasonOther, "Download in progress", + BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI), + BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE))); return new CountingDownloadFile(std::move(save_info), default_downloads_directory, std::move(stream), @@ -548,13 +554,6 @@ class TestRequestStartHandler { class DownloadContentTest : public ContentBrowserTest { protected: void SetUpOnMainThread() override { - // Enable downloads resumption. - base::FeatureList::ClearInstanceForTesting(); - std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList); - feature_list->InitializeFromCommandLine(features::kDownloadResumption.name, - std::string()); - base::FeatureList::SetInstance(std::move(feature_list)); - ASSERT_TRUE(downloads_directory_.CreateUniqueTempDir()); test_delegate_.reset(new TestShellDownloadManagerDelegate()); @@ -2253,8 +2252,8 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CookiePolicy) { // Download the file. SetupEnsureNoPendingDownloads(); std::unique_ptr<DownloadUrlParameters> download_parameters( - DownloadUrlParameters::FromWebContents(shell()->web_contents(), - origin_two.GetURL("/bar"))); + DownloadUrlParameters::CreateForWebContentsMainFrame( + shell()->web_contents(), origin_two.GetURL("/bar"))); std::unique_ptr<DownloadTestObserver> observer(CreateWaiter(shell(), 1)); DownloadManagerForShell(shell())->DownloadUrl(std::move(download_parameters)); observer->WaitForFinished(); diff --git a/chromium/content/browser/download/download_item_impl.cc b/chromium/content/browser/download/download_item_impl.cc index 56c8bcb13e0..be68b07963b 100644 --- a/chromium/content/browser/download/download_item_impl.cc +++ b/chromium/content/browser/download/download_item_impl.cc @@ -31,7 +31,6 @@ #include "base/format_macros.h" #include "base/guid.h" #include "base/logging.h" -#include "base/memory/ptr_util.h" #include "base/metrics/histogram.h" #include "base/stl_util.h" #include "base/strings/string_util.h" @@ -95,10 +94,6 @@ static void DownloadFileCancel(std::unique_ptr<DownloadFile> download_file) { download_file->Cancel(); } -bool IsDownloadResumptionEnabled() { - return base::FeatureList::IsEnabled(features::kDownloadResumption); -} - } // namespace const uint32_t DownloadItem::kInvalidId = 0; @@ -375,7 +370,7 @@ void DownloadItemImpl::Resume() { case INTERRUPTED_INTERNAL: auto_resume_count_ = 0; // User input resets the counter. - ResumeInterruptedDownload(); + ResumeInterruptedDownload(ResumptionRequestSource::USER); UpdateObservers(); return; @@ -483,9 +478,8 @@ bool DownloadItemImpl::CanResume() const { ResumeMode resume_mode = GetResumeMode(); // Only allow Resume() calls if the resumption mode requires a user // action. - return IsDownloadResumptionEnabled() && - (resume_mode == RESUME_MODE_USER_RESTART || - resume_mode == RESUME_MODE_USER_CONTINUE); + return resume_mode == RESUME_MODE_USER_RESTART || + resume_mode == RESUME_MODE_USER_CONTINUE; } case MAX_DOWNLOAD_INTERNAL_STATE: @@ -844,9 +838,6 @@ std::string DownloadItemImpl::DebugString(bool verbose) const { DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const { DCHECK_CURRENTLY_ON(BrowserThread::UI); - if (!IsDownloadResumptionEnabled()) - return RESUME_MODE_INVALID; - // Only support resumption for HTTP(S). if (!GetURL().SchemeIsHTTPOrHTTPS()) return RESUME_MODE_INVALID; @@ -1183,9 +1174,9 @@ void DownloadItemImpl::Start( int64_t offset = new_create_info.save_info->offset; std::unique_ptr<crypto::SecureHash> hash_state = - base::WrapUnique(new_create_info.save_info->hash_state - ? new_create_info.save_info->hash_state->Clone() - : nullptr); + new_create_info.save_info->hash_state + ? new_create_info.save_info->hash_state->Clone() + : nullptr; // Interrupted downloads also need a target path. if (target_path_.empty()) { @@ -1878,14 +1869,12 @@ void DownloadItemImpl::AutoResumeIfValid() { auto_resume_count_++; - ResumeInterruptedDownload(); + ResumeInterruptedDownload(ResumptionRequestSource::AUTOMATIC); } -void DownloadItemImpl::ResumeInterruptedDownload() { +void DownloadItemImpl::ResumeInterruptedDownload( + ResumptionRequestSource source) { DCHECK_CURRENTLY_ON(BrowserThread::UI); - if (!IsDownloadResumptionEnabled()) - return; - // If we're not interrupted, ignore the request; our caller is drunk. if (state_ != INTERRUPTED_INTERNAL) return; @@ -1914,7 +1903,7 @@ void DownloadItemImpl::ResumeInterruptedDownload() { // request will not be dropped if the WebContents (and by extension, the // associated renderer) goes away before a response is received. std::unique_ptr<DownloadUrlParameters> download_params( - new DownloadUrlParameters(GetURL(), -1, -1, -1, + new DownloadUrlParameters(GetURL(), storage_partition->GetURLRequestContext())); download_params->set_file_path(GetFullPath()); download_params->set_offset(GetReceivedBytes()); @@ -1930,6 +1919,9 @@ void DownloadItemImpl::ResumeInterruptedDownload() { Referrer(GetReferrerUrl(), blink::WebReferrerPolicyAlways)); TransitionTo(RESUMING_INTERNAL); + RecordDownloadSource(source == ResumptionRequestSource::USER + ? INITIATED_BY_MANUAL_RESUMPTION + : INITIATED_BY_AUTOMATIC_RESUMPTION); delegate_->ResumeInterruptedDownload(std::move(download_params), GetId()); // Just in case we were interrupted while paused. is_paused_ = false; diff --git a/chromium/content/browser/download/download_item_impl.h b/chromium/content/browser/download/download_item_impl.h index ba5b2ae0d3c..802c5af06ea 100644 --- a/chromium/content/browser/download/download_item_impl.h +++ b/chromium/content/browser/download/download_item_impl.h @@ -476,7 +476,8 @@ class CONTENT_EXPORT DownloadItemImpl void AutoResumeIfValid(); - void ResumeInterruptedDownload(); + enum class ResumptionRequestSource { AUTOMATIC, USER }; + void ResumeInterruptedDownload(ResumptionRequestSource source); // Update origin information based on the response to a download resumption // request. Should only be called if the resumption request was successful. diff --git a/chromium/content/browser/download/download_item_impl_unittest.cc b/chromium/content/browser/download/download_item_impl_unittest.cc index 4fd8b8756b0..c9389638c22 100644 --- a/chromium/content/browser/download/download_item_impl_unittest.cc +++ b/chromium/content/browser/download/download_item_impl_unittest.cc @@ -244,12 +244,6 @@ class DownloadItemTest : public testing::Test { public: DownloadItemTest() : delegate_(), next_download_id_(DownloadItem::kInvalidId + 1) { - base::FeatureList::ClearInstanceForTesting(); - std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList); - feature_list->InitializeFromCommandLine(features::kDownloadResumption.name, - std::string()); - base::FeatureList::SetInstance(std::move(feature_list)); - create_info_.reset(new DownloadCreateInfo()); create_info_->save_info = std::unique_ptr<DownloadSaveInfo>(new DownloadSaveInfo()); diff --git a/chromium/content/browser/download/download_manager_impl.cc b/chromium/content/browser/download/download_manager_impl.cc index 22caf42d9d2..e5584bd4c06 100644 --- a/chromium/content/browser/download/download_manager_impl.cc +++ b/chromium/content/browser/download/download_manager_impl.cc @@ -470,7 +470,6 @@ void DownloadManagerImpl::OnSavePackageSuccessfullyFinished( void DownloadManagerImpl::ResumeInterruptedDownload( std::unique_ptr<content::DownloadUrlParameters> params, uint32_t id) { - RecordDownloadSource(INITIATED_BY_RESUMPTION); BrowserThread::PostTaskAndReplyWithResult( BrowserThread::IO, FROM_HERE, base::Bind(&BeginDownload, base::Passed(¶ms), diff --git a/chromium/content/browser/download/download_request_core.cc b/chromium/content/browser/download/download_request_core.cc index 4ab373f1877..fa768758946 100644 --- a/chromium/content/browser/download/download_request_core.cc +++ b/chromium/content/browser/download/download_request_core.cc @@ -29,9 +29,9 @@ #include "content/public/browser/download_item.h" #include "content/public/browser/download_manager_delegate.h" #include "content/public/browser/navigation_entry.h" -#include "content/public/browser/power_save_blocker.h" #include "content/public/browser/resource_context.h" #include "content/public/browser/web_contents.h" +#include "device/power_save_blocker/power_save_blocker.h" #include "net/base/elements_upload_data_stream.h" #include "net/base/io_buffer.h" #include "net/base/load_flags.h" @@ -204,9 +204,11 @@ DownloadRequestCore::DownloadRequestCore(net::URLRequest* request, DCHECK(request_); DCHECK(delegate_); RecordDownloadCount(UNTHROTTLED_COUNT); - power_save_blocker_ = PowerSaveBlocker::Create( - PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, - PowerSaveBlocker::kReasonOther, "Download in progress"); + power_save_blocker_.reset(new device::PowerSaveBlocker( + device::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, + device::PowerSaveBlocker::kReasonOther, "Download in progress", + BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI), + BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE))); DownloadRequestData* request_data = DownloadRequestData::Get(request_); if (request_data) { save_info_ = request_data->TakeSaveInfo(); @@ -523,23 +525,19 @@ std::string DownloadRequestCore::DebugString() const { DownloadInterruptReason DownloadRequestCore::HandleRequestStatus( const net::URLRequestStatus& status) { net::Error error_code = net::OK; - if (status.status() == net::URLRequestStatus::FAILED || - // Note cancels as failures too. - status.status() == net::URLRequestStatus::CANCELED) { + if (!status.is_success()) { error_code = static_cast<net::Error>(status.error()); // Normal case. // Make sure that at least the fact of failure comes through. if (error_code == net::OK) error_code = net::ERR_FAILED; } - // ERR_CONTENT_LENGTH_MISMATCH and ERR_INCOMPLETE_CHUNKED_ENCODING are - // allowed since a number of servers in the wild close the connection too - // early by mistake. Other browsers - IE9, Firefox 11.0, and Safari 5.1.4 - - // treat downloads as complete in both cases, so we follow their lead. - if (error_code == net::ERR_CONTENT_LENGTH_MISMATCH || - error_code == net::ERR_INCOMPLETE_CHUNKED_ENCODING) { + // ERR_CONTENT_LENGTH_MISMATCH is allowed since a number of servers in the + // wild close the connection too early by mistake. Other browsers - IE9, + // Firefox 11.0, and Safari 5.1.4 - treat downloads as complete in both cases, + // so we follow their lead. + if (error_code == net::ERR_CONTENT_LENGTH_MISMATCH) error_code = net::OK; - } DownloadInterruptReason reason = ConvertNetErrorToInterruptReason( error_code, DOWNLOAD_INTERRUPT_FROM_NETWORK); diff --git a/chromium/content/browser/download/download_request_core.h b/chromium/content/browser/download/download_request_core.h index 84eb2e0a935..646911641b5 100644 --- a/chromium/content/browser/download/download_request_core.h +++ b/chromium/content/browser/download/download_request_core.h @@ -19,6 +19,10 @@ #include "content/public/browser/download_save_info.h" #include "content/public/browser/download_url_parameters.h" +namespace device { +class PowerSaveBlocker; +} // namespace device + namespace net { class HttpResponseHeaders; class URLRequest; @@ -29,7 +33,6 @@ namespace content { class DownloadManagerImpl; class ByteStreamReader; class ByteStreamWriter; -class PowerSaveBlocker; struct DownloadCreateInfo; // This class encapsulates the core logic for reading data from a URLRequest and @@ -140,7 +143,7 @@ class CONTENT_EXPORT DownloadRequestCore // Keeps the system from sleeping while this is alive. If the // system enters power saving mode while a request is alive, it can cause the // request to fail and the associated download will be interrupted. - std::unique_ptr<PowerSaveBlocker> power_save_blocker_; + std::unique_ptr<device::PowerSaveBlocker> power_save_blocker_; // The following are used to collect stats. base::TimeTicks download_start_time_; diff --git a/chromium/content/browser/download/download_resource_handler.cc b/chromium/content/browser/download/download_resource_handler.cc index fabe1d62919..7798f926ad6 100644 --- a/chromium/content/browser/download/download_resource_handler.cc +++ b/chromium/content/browser/download/download_resource_handler.cc @@ -16,6 +16,7 @@ #include "content/browser/download/download_interrupt_reasons_impl.h" #include "content/browser/download/download_manager_impl.h" #include "content/browser/download/download_request_handle.h" +#include "content/browser/frame_host/frame_tree_node.h" #include "content/browser/loader/resource_dispatcher_host_impl.h" #include "content/browser/loader/resource_request_info_impl.h" #include "content/public/browser/browser_thread.h" @@ -23,6 +24,7 @@ #include "content/public/browser/navigation_entry.h" #include "content/public/browser/render_frame_host.h" #include "content/public/browser/web_contents.h" +#include "content/public/common/browser_side_navigation_policy.h" #include "content/public/common/resource_response.h" namespace content { @@ -42,11 +44,22 @@ static void StartOnUIThread( std::unique_ptr<ByteStreamReader> stream, int render_process_id, int render_frame_id, + int frame_tree_node_id, const DownloadUrlParameters::OnStartedCallback& started_cb) { DCHECK_CURRENTLY_ON(BrowserThread::UI); RenderFrameHost* frame_host = RenderFrameHost::FromID(render_process_id, render_frame_id); + + // PlzNavigate: navigations don't have associated RenderFrameHosts. Get the + // SiteInstance from the FrameTreeNode. + if (!frame_host && IsBrowserSideNavigationEnabled()) { + FrameTreeNode* frame_tree_node = + FrameTreeNode::GloballyFindByID(frame_tree_node_id); + if (frame_tree_node) + frame_host = frame_tree_node->current_frame_host(); + } + DownloadManager* download_manager = info->request_handle->GetDownloadManager(); if (!download_manager || !frame_host) { @@ -203,7 +216,8 @@ void DownloadResourceHandler::OnStart( BrowserThread::UI, FROM_HERE, base::Bind(&StartOnUIThread, base::Passed(&create_info), base::Passed(&tab_info_), base::Passed(&stream_reader), - render_process_id, render_frame_id, callback)); + render_process_id, render_frame_id, + request_info->frame_tree_node_id(), callback)); } void DownloadResourceHandler::OnReadyToRead() { diff --git a/chromium/content/browser/download/download_stats.h b/chromium/content/browser/download/download_stats.h index a029a0b4f34..8da7f022304 100644 --- a/chromium/content/browser/download/download_stats.h +++ b/chromium/content/browser/download/download_stats.h @@ -107,9 +107,16 @@ enum DownloadSource { // Fomerly INITIATED_BY_PEPPER_SAVE. DOWNLOAD_SOURCE_UNUSED_3, - // A request that was initiated as a result of resuming an interrupted - // download. - INITIATED_BY_RESUMPTION, + // Formerly INITIATED_BY_RESUMPTION. + DOWNLOAD_SOURCE_UNUSED_4, + + // A request that was initiated as a result of manually resuming an + // interrupted download. + INITIATED_BY_MANUAL_RESUMPTION, + + // A request that was initiated as a result of automatically resuming an + // interrupted download. + INITIATED_BY_AUTOMATIC_RESUMPTION, DOWNLOAD_SOURCE_LAST_ENTRY }; diff --git a/chromium/content/browser/download/drag_download_file.cc b/chromium/content/browser/download/drag_download_file.cc index 468c26f5c21..cd68969fb0d 100644 --- a/chromium/content/browser/download/drag_download_file.cc +++ b/chromium/content/browser/download/drag_download_file.cc @@ -65,8 +65,11 @@ class DragDownloadFile::DragDownloadFileUI : public DownloadItem::Observer { BrowserContext::GetDownloadManager(web_contents_->GetBrowserContext()); RecordDownloadSource(INITIATED_BY_DRAG_N_DROP); + // TODO(https://crbug.com/614134) This should use the frame actually + // containing the link being dragged rather than the main frame of the tab. std::unique_ptr<content::DownloadUrlParameters> params( - DownloadUrlParameters::FromWebContents(web_contents_, url_)); + DownloadUrlParameters::CreateForWebContentsMainFrame( + web_contents_, url_)); params->set_referrer(referrer_); params->set_referrer_encoding(referrer_encoding_); params->set_callback(base::Bind(&DragDownloadFileUI::OnDownloadStarted, diff --git a/chromium/content/browser/download/drag_download_file_browsertest.cc b/chromium/content/browser/download/drag_download_file_browsertest.cc index 8986c391350..5821c3015bd 100644 --- a/chromium/content/browser/download/drag_download_file_browsertest.cc +++ b/chromium/content/browser/download/drag_download_file_browsertest.cc @@ -14,7 +14,6 @@ #include "content/browser/download/drag_download_util.h" #include "content/browser/web_contents/web_contents_impl.h" #include "content/public/browser/content_browser_client.h" -#include "content/public/browser/power_save_blocker.h" #include "content/public/common/content_client.h" #include "content/public/test/content_browser_test.h" #include "content/public/test/content_browser_test_utils.h" @@ -23,6 +22,7 @@ #include "content/shell/browser/shell.h" #include "content/shell/browser/shell_browser_context.h" #include "content/shell/browser/shell_download_manager_delegate.h" +#include "device/power_save_blocker/power_save_blocker.h" #include "net/test/url_request/url_request_mock_http_job.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/chromium/content/browser/download/mhtml_generation_browsertest.cc b/chromium/content/browser/download/mhtml_generation_browsertest.cc index 00f3aeb1964..8a8052efd3d 100644 --- a/chromium/content/browser/download/mhtml_generation_browsertest.cc +++ b/chromium/content/browser/download/mhtml_generation_browsertest.cc @@ -11,6 +11,10 @@ #include "base/files/scoped_temp_dir.h" #include "base/macros.h" #include "base/run_loop.h" +#include "base/strings/utf_string_conversions.h" +#include "content/browser/renderer_host/render_process_host_impl.h" +#include "content/common/frame_messages.h" +#include "content/public/browser/render_process_host.h" #include "content/public/browser/web_contents.h" #include "content/public/common/mhtml_generation_params.h" #include "content/public/test/browser_test_utils.h" @@ -18,10 +22,13 @@ #include "content/public/test/content_browser_test_utils.h" #include "content/public/test/test_utils.h" #include "content/shell/browser/shell.h" +#include "net/base/filename_util.h" #include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +#include "third_party/WebKit/public/web/WebFindOptions.h" +#include "third_party/WebKit/public/web/WebFrameSerializerCacheControlPolicy.h" using testing::ContainsRegex; using testing::HasSubstr; @@ -29,6 +36,58 @@ using testing::Not; namespace content { +namespace { + +// A dummy WebContentsDelegate which tracks the results of a find operation. +class FindTrackingDelegate : public WebContentsDelegate { + public: + FindTrackingDelegate(const std::string& search) + : search_(search), matches_(-1) {} + + // Returns number of results. + int Wait(WebContents* web_contents) { + WebContentsDelegate* old_delegate = web_contents->GetDelegate(); + web_contents->SetDelegate(this); + + blink::WebFindOptions options; + options.matchCase = false; + + web_contents->Find(global_request_id++, base::UTF8ToUTF16(search_), + options); + run_loop_.Run(); + + web_contents->SetDelegate(old_delegate); + + return matches_; + } + + void FindReply(WebContents* web_contents, + int request_id, + int number_of_matches, + const gfx::Rect& selection_rect, + int active_match_ordinal, + bool final_update) override { + if (final_update) { + matches_ = number_of_matches; + run_loop_.Quit(); + } + } + + static int global_request_id; + + private: + std::string search_; + int matches_; + base::RunLoop run_loop_; + + DISALLOW_COPY_AND_ASSIGN(FindTrackingDelegate); +}; + +// static +int FindTrackingDelegate::global_request_id = 0; + +} // namespace + class MHTMLGenerationTest : public ContentBrowserTest { public: MHTMLGenerationTest() : has_mhtml_callback_run_(false), file_size_(0) {} @@ -46,7 +105,10 @@ class MHTMLGenerationTest : public ContentBrowserTest { void GenerateMHTML(const MHTMLGenerationParams& params, const GURL& url) { NavigateToURL(shell(), url); + GenerateMHTMLForCurrentPage(params); + } + void GenerateMHTMLForCurrentPage(const MHTMLGenerationParams& params) { base::RunLoop run_loop; shell()->web_contents()->GenerateMHTML( @@ -65,6 +127,68 @@ class MHTMLGenerationTest : public ContentBrowserTest { return file_size; } + void TestOriginalVsSavedPage( + const GURL& url, + const MHTMLGenerationParams params, + int expected_number_of_frames, + const std::vector<std::string>& expected_substrings, + const std::vector<std::string>& forbidden_substrings_in_saved_page, + bool skip_verification_of_original_page = false) { + // Navigate to the test page and verify if test expectations + // are met (this is mostly a sanity check - a failure to meet + // expectations would probably mean that there is a test bug + // (i.e. that we got called with wrong expected_foo argument). + NavigateToURL(shell(), url); + if (!skip_verification_of_original_page) { + AssertExpectationsAboutCurrentTab(expected_number_of_frames, + expected_substrings, + std::vector<std::string>()); + } + + GenerateMHTML(params, url); + ASSERT_FALSE(HasFailure()); + + // Stop the test server (to make sure the locally saved page + // is self-contained / won't try to open original resources). + ASSERT_TRUE(embedded_test_server()->ShutdownAndWaitUntilComplete()); + + // Open the saved page and verify if test expectations are + // met (i.e. if the same expectations are met for "after" + // [saved version of the page] as for the "before" + // [the original version of the page]. + NavigateToURL(shell(), net::FilePathToFileURL(params.file_path)); + AssertExpectationsAboutCurrentTab(expected_number_of_frames, + expected_substrings, + forbidden_substrings_in_saved_page); + } + + void AssertExpectationsAboutCurrentTab( + int expected_number_of_frames, + const std::vector<std::string>& expected_substrings, + const std::vector<std::string>& forbidden_substrings) { + int actual_number_of_frames = + shell()->web_contents()->GetAllFrames().size(); + EXPECT_EQ(expected_number_of_frames, actual_number_of_frames); + + for (const auto& expected_substring : expected_substrings) { + FindTrackingDelegate delegate(expected_substring); + int actual_number_of_matches = delegate.Wait(shell()->web_contents()); + EXPECT_EQ(1, actual_number_of_matches) + << "Verifying that \"" << expected_substring << "\" appears " + << "exactly once in the text of web contents of " + << shell()->web_contents()->GetURL().spec(); + } + + for (const auto& forbidden_substring : forbidden_substrings) { + FindTrackingDelegate delegate(forbidden_substring); + int actual_number_of_matches = delegate.Wait(shell()->web_contents()); + EXPECT_EQ(0, actual_number_of_matches) + << "Verifying that \"" << forbidden_substring << "\" doesn't " + << "appear in the text of web contents of " + << shell()->web_contents()->GetURL().spec(); + } + } + bool has_mhtml_callback_run() const { return has_mhtml_callback_run_; } int64_t file_size() const { return file_size_; } @@ -102,6 +226,121 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTML) { HasSubstr("Content-Transfer-Encoding: quoted-printable")); } +class GenerateMHTMLAndExitRendererMessageFilter : public BrowserMessageFilter { + public: + GenerateMHTMLAndExitRendererMessageFilter( + RenderProcessHostImpl* render_process_host) + : BrowserMessageFilter(FrameMsgStart), + render_process_host_(render_process_host) {} + + protected: + ~GenerateMHTMLAndExitRendererMessageFilter() override {} + + private: + bool OnMessageReceived(const IPC::Message& message) override { + if (message.type() == FrameHostMsg_SerializeAsMHTMLResponse::ID) { + // After |return false| below, this IPC message will be handled by the + // product code as illustrated below. (1), (2), (3) depict points in time + // when product code runs on UI and FILE threads. (X), (Y), (Z) depict + // when we want test-injected tasks to run - for the repro, (Z) has to + // happen between (1) and (3). (Y?) and (Z?) depict when test tasks can + // theoretically happen and ruin the repro. + // + // IO thread UI thread FILE thread + // --------- --------- ----------- + // | | | + // WE ARE HERE | | + // | | | + // after |return false| | | + // +--------------->+ | + // | | | + // | (X) | + // | | | + // | | (Y?) + // | (Z?) | + // | | | + // (1) | MHTMLGenerationManager | + // | ::OnSerializeAsMHTMLResponse | + // | +-------------------->+ + // | | | + // | | (Y) + // | | | + // (2) | | MHTMLGenerationManager::Job + // | | ::CloseFileOnFileThread + // | | | + // | (Z) | + // | test needs to inject | + // | fast renderer shutdown | + // | HERE - between (1) and (3) | + // | | | + // | | | + // | +<--------------------+ + // | | | + // (3) | MHTMLGenerationManager | + // | ::OnFileClosed | + // | | | + // + // We hope that (Z) happens between (1) and (3) by doing the following: + // - From here post TaskX to UI thread. (X) is guaranteed to happen + // before timepoint (1) (because posting of (1) happens after + // |return false| / before we post TaskX below). + // - From (X) post TaskY to FILE thread. Because this posting is done + // before (1), we can guarantee that (Y) will happen before (2). + // - From (Y) post TaskZ to UI thread. Because this posting is done + // before (2), we can guarantee that (Z) will happen before (3). + // - We cannot really guarantee that (Y) and (Z) happen *after* (1) - i.e. + // execution at (Y?) and (Z?) instead is possible. In practice, + // bouncing off of UI and FILE thread does mean (Z) happens after (1). + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, base::Bind( + &GenerateMHTMLAndExitRendererMessageFilter::TaskX, + base::Unretained(this))); + } + + return false; + }; + + void TaskX() { + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, base::Bind( + &GenerateMHTMLAndExitRendererMessageFilter::TaskY, + base::Unretained(this))); + } + + void TaskY() { + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, base::Bind( + &GenerateMHTMLAndExitRendererMessageFilter::TaskZ, + base::Unretained(this))); + } + + void TaskZ() { + render_process_host_->FastShutdownIfPossible(); + } + + RenderProcessHostImpl* render_process_host_; + + DISALLOW_COPY_AND_ASSIGN(GenerateMHTMLAndExitRendererMessageFilter); +}; + +// Regression test for the crash/race from https://crbug.com/612098. +IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLAndExitRenderer) { + NavigateToURL(shell(), embedded_test_server()->GetURL("/simple_page.html")); + + RenderProcessHostImpl* render_process_host = + static_cast<RenderProcessHostImpl*>( + shell()->web_contents()->GetRenderProcessHost()); + scoped_refptr<BrowserMessageFilter> filter = + new GenerateMHTMLAndExitRendererMessageFilter(render_process_host); + render_process_host->AddFilter(filter.get()); + + base::FilePath path(temp_dir_.path()); + path = path.Append(FILE_PATH_LITERAL("test.mht")); + GenerateMHTMLForCurrentPage(MHTMLGenerationParams(path)); + + EXPECT_GT(ReadFileSizeFromDisk(path), 100); // Verify the actual file size. +} + IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, InvalidPath) { base::FilePath path(FILE_PATH_LITERAL("/invalid/file/path")); @@ -161,7 +400,7 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLIgnoreNoStore) { GURL url(embedded_test_server()->GetURL("/nostore.html")); - // Generate MHTML without specifying the FAIL_FOR_NO_STORE_MAIN_FRAME policy. + // Generate MHTML without specifying the FailForNoStoreMainFrame policy. GenerateMHTML(path, url); // We expect that there wasn't an error (file size -1 indicates an error.) @@ -183,10 +422,10 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLObeyNoStoreMainFrame) { GURL url(embedded_test_server()->GetURL("/nostore.html")); - // Generate MHTML, specifying the FAIL_FOR_NO_STORE_MAIN_FRAME policy. + // Generate MHTML, specifying the FailForNoStoreMainFrame policy. MHTMLGenerationParams params(path); params.cache_control_policy = - content::MHTMLCacheControlPolicy::FAIL_FOR_NO_STORE_MAIN_FRAME; + blink::WebFrameSerializerCacheControlPolicy::FailForNoStoreMainFrame; GenerateMHTML(params, url); // We expect that there was an error (file size -1 indicates an error.) @@ -199,6 +438,114 @@ IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLObeyNoStoreMainFrame) { EXPECT_THAT(mhtml, Not(HasSubstr("test body"))); } +IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, + GenerateMHTMLIgnoreNoStoreSubFrame) { + base::FilePath path(temp_dir_.path()); + path = path.Append(FILE_PATH_LITERAL("test.mht")); + + GURL url(embedded_test_server()->GetURL("/page_with_nostore_iframe.html")); + + // Generate MHTML, specifying the FailForNoStoreMainFrame policy. + MHTMLGenerationParams params(path); + params.cache_control_policy = + blink::WebFrameSerializerCacheControlPolicy::FailForNoStoreMainFrame; + + GenerateMHTML(params, url); + // We expect that there was no error (file size -1 indicates an error.) + EXPECT_LT(0, file_size()); + + std::string mhtml; + ASSERT_TRUE(base::ReadFileToString(path, &mhtml)); + + EXPECT_THAT(mhtml, HasSubstr("Main Frame")); + // Make sure that no-store subresources exist in this mode. + EXPECT_THAT(mhtml, HasSubstr("no-store test body")); + EXPECT_THAT(mhtml, ContainsRegex("Content-Location:.*nostore.jpg")); +} + +IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, GenerateMHTMLObeyNoStoreSubFrame) { + base::FilePath path(temp_dir_.path()); + path = path.Append(FILE_PATH_LITERAL("test.mht")); + + GURL url(embedded_test_server()->GetURL("/page_with_nostore_iframe.html")); + + // Generate MHTML, specifying the FailForNoStoreMainFrame policy. + MHTMLGenerationParams params(path); + params.cache_control_policy = blink::WebFrameSerializerCacheControlPolicy:: + SkipAnyFrameOrResourceMarkedNoStore; + + GenerateMHTML(params, url); + // We expect that there was no error (file size -1 indicates an error.) + EXPECT_LT(0, file_size()); + + std::string mhtml; + ASSERT_TRUE(base::ReadFileToString(path, &mhtml)); + + EXPECT_THAT(mhtml, HasSubstr("Main Frame")); + // Make sure the contents are missing. + EXPECT_THAT(mhtml, Not(HasSubstr("no-store test body"))); + // This image comes from a resource marked no-store. + EXPECT_THAT(mhtml, Not(ContainsRegex("Content-Location:.*nostore.jpg"))); +} + +// TODO(crbug.com/615291): These fail on Android under some circumstances. +#if defined(OS_ANDROID) +#define MAYBE_ViewedMHTMLContainsNoStoreContentIfNoCacheControlPolicy \ + DISABLED_ViewedMHTMLContainsNoStoreContentIfNoCacheControlPolicy +#define MAYBE_ViewedMHTMLDoesNotContainNoStoreContent \ + DISABLED_ViewedMHTMLDoesNotContainNoStoreContent +#else +#define MAYBE_ViewedMHTMLContainsNoStoreContentIfNoCacheControlPolicy \ + ViewedMHTMLContainsNoStoreContentIfNoCacheControlPolicy +#define MAYBE_ViewedMHTMLDoesNotContainNoStoreContent \ + ViewedMHTMLDoesNotContainNoStoreContent +#endif + +IN_PROC_BROWSER_TEST_F( + MHTMLGenerationTest, + MAYBE_ViewedMHTMLContainsNoStoreContentIfNoCacheControlPolicy) { + // Generate MHTML, specifying the FailForNoStoreMainFrame policy. + base::FilePath path(temp_dir_.path()); + path = path.Append(FILE_PATH_LITERAL("test.mht")); + MHTMLGenerationParams params(path); + + // No special cache control options so we should see both frames. + std::vector<std::string> expectations = { + "Main Frame, normal headers.", "Cache-Control: no-store test body", + }; + std::vector<std::string> forbidden; + TestOriginalVsSavedPage( + embedded_test_server()->GetURL("/page_with_nostore_iframe.html"), params, + 2 /* expected number of frames */, expectations, forbidden); + + std::string mhtml; + ASSERT_TRUE(base::ReadFileToString(params.file_path, &mhtml)); +} + +IN_PROC_BROWSER_TEST_F(MHTMLGenerationTest, + MAYBE_ViewedMHTMLDoesNotContainNoStoreContent) { + // Generate MHTML, specifying the FailForNoStoreMainFrame policy. + base::FilePath path(temp_dir_.path()); + path = path.Append(FILE_PATH_LITERAL("test.mht")); + MHTMLGenerationParams params(path); + params.cache_control_policy = blink::WebFrameSerializerCacheControlPolicy:: + SkipAnyFrameOrResourceMarkedNoStore; + + // No special cache control options so we should see both frames. + std::vector<std::string> expectations = { + "Main Frame, normal headers.", + }; + std::vector<std::string> forbidden = { + "Cache-Control: no-store test body", + }; + TestOriginalVsSavedPage( + embedded_test_server()->GetURL("/page_with_nostore_iframe.html"), params, + 2 /* expected number of frames */, expectations, forbidden); + + std::string mhtml; + ASSERT_TRUE(base::ReadFileToString(params.file_path, &mhtml)); +} + // Test suite that allows testing --site-per-process against cross-site frames. // See http://dev.chromium.org/developers/design-documents/site-isolation. class MHTMLGenerationSitePerProcessTest : public MHTMLGenerationTest { diff --git a/chromium/content/browser/download/mhtml_generation_manager.cc b/chromium/content/browser/download/mhtml_generation_manager.cc index 60feca041a6..dc4ef122c21 100644 --- a/chromium/content/browser/download/mhtml_generation_manager.cc +++ b/chromium/content/browser/download/mhtml_generation_manager.cc @@ -44,11 +44,15 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver { const GenerateMHTMLCallback& callback() const { return callback_; } + // Indicates whether we expect a message from the |sender| at this time. + // We expect only one message per frame - therefore calling this method + // will always clear |frame_tree_node_id_of_busy_frame_|. + bool IsMessageFromFrameExpected(RenderFrameHostImpl* sender); + // Handler for FrameHostMsg_SerializeAsMHTMLResponse (a notification from the // renderer that the MHTML generation for previous frame has finished). // Returns |true| upon success; |false| otherwise. bool OnSerializeAsMHTMLResponse( - RenderFrameHostImpl* sender, const std::set<std::string>& digests_of_uris_of_serialized_resources); // Sends IPC to the renderer, asking for MHTML generation of the next frame. @@ -75,6 +79,8 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver { int exit_code) override; void RenderProcessHostDestroyed(RenderProcessHost* host) override; + void MarkAsFinished(); + private: static int64_t CloseFileOnFileThread(base::File file); void AddFrame(RenderFrameHost* render_frame_host); @@ -119,6 +125,11 @@ class MHTMLGenerationManager::Job : public RenderProcessHostObserver { // The callback to call once generation is complete. const GenerateMHTMLCallback callback_; + // Whether the job is finished (set to true only for the short duration of + // time between MHTMLGenerationManager::JobFinished is called and the job is + // destroyed by MHTMLGenerationManager::OnFileClosed). + bool is_finished_; + // RAII helper for registering this Job as a RenderProcessHost observer. ScopedObserver<RenderProcessHost, MHTMLGenerationManager::Job> observed_renderer_process_host_; @@ -136,6 +147,7 @@ MHTMLGenerationManager::Job::Job(int job_id, mhtml_boundary_marker_(net::GenerateMimeMultipartBoundary()), salt_(base::GenerateGUID()), callback_(callback), + is_finished_(false), observed_renderer_process_host_(this) { DCHECK_CURRENTLY_ON(BrowserThread::UI); web_contents->ForEachFrame(base::Bind( @@ -222,6 +234,15 @@ void MHTMLGenerationManager::Job::RenderProcessExited( MHTMLGenerationManager::GetInstance()->RenderProcessExited(this); } +void MHTMLGenerationManager::Job::MarkAsFinished() { + DCHECK(!is_finished_); + is_finished_ = true; + + // Stopping RenderProcessExited notifications is needed to avoid calling + // JobFinished twice. See also https://crbug.com/612098. + observed_renderer_process_host_.RemoveAll(); +} + void MHTMLGenerationManager::Job::AddFrame(RenderFrameHost* render_frame_host) { auto* rfhi = static_cast<RenderFrameHostImpl*>(render_frame_host); int frame_tree_node_id = rfhi->frame_tree_node()->frame_tree_node_id(); @@ -255,18 +276,21 @@ void MHTMLGenerationManager::Job::CloseFile( callback); } -bool MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse( - RenderFrameHostImpl* sender, - const std::set<std::string>& digests_of_uris_of_serialized_resources) { - // Sanitize renderer input / reject unexpected messages. +bool MHTMLGenerationManager::Job::IsMessageFromFrameExpected( + RenderFrameHostImpl* sender) { int sender_id = sender->frame_tree_node()->frame_tree_node_id(); - if (sender_id != frame_tree_node_id_of_busy_frame_) { - ReceivedBadMessage(sender->GetProcess(), - bad_message::DWNLD_INVALID_SERIALIZE_AS_MHTML_RESPONSE); - return false; // Report failure. - } + if (sender_id != frame_tree_node_id_of_busy_frame_) + return false; + + // We only expect one message per frame - let's make sure subsequent messages + // from the same |sender| will be rejected. frame_tree_node_id_of_busy_frame_ = FrameTreeNode::kFrameTreeNodeInvalidId; + return true; +} + +bool MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse( + const std::set<std::string>& digests_of_uris_of_serialized_resources) { // Renderer should be deduping resources with the same uris. DCHECK_EQ(0u, base::STLSetIntersection<std::set<std::string>>( digests_of_already_serialized_uris_, @@ -276,7 +300,7 @@ bool MHTMLGenerationManager::Job::OnSerializeAsMHTMLResponse( digests_of_uris_of_serialized_resources.end()); if (pending_frame_tree_node_ids_.empty()) - return true; // Report success. + return true; // Report success - all frames have been processed. return SendToNextRenderFrame(); } @@ -323,7 +347,8 @@ void MHTMLGenerationManager::OnSerializeAsMHTMLResponse( DCHECK_CURRENTLY_ON(BrowserThread::UI); Job* job = FindJob(job_id); - if (!job) { + if (!job || !job->IsMessageFromFrameExpected(sender)) { + NOTREACHED(); ReceivedBadMessage(sender->GetProcess(), bad_message::DWNLD_INVALID_SERIALIZE_AS_MHTML_RESPONSE); return; @@ -335,7 +360,7 @@ void MHTMLGenerationManager::OnSerializeAsMHTMLResponse( } if (!job->OnSerializeAsMHTMLResponse( - sender, digests_of_uris_of_serialized_resources)) { + digests_of_uris_of_serialized_resources)) { JobFinished(job, JobStatus::FAILURE); return; } @@ -388,6 +413,7 @@ void MHTMLGenerationManager::JobFinished(Job* job, JobStatus job_status) { DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK(job); + job->MarkAsFinished(); job->CloseFile( base::Bind(&MHTMLGenerationManager::OnFileClosed, base::Unretained(this), // Safe b/c |this| is a singleton. diff --git a/chromium/content/browser/download/mhtml_generation_manager.h b/chromium/content/browser/download/mhtml_generation_manager.h index d360cce0ac6..50d5bebc702 100644 --- a/chromium/content/browser/download/mhtml_generation_manager.h +++ b/chromium/content/browser/download/mhtml_generation_manager.h @@ -28,6 +28,10 @@ class WebContents; // The class and all of its members live on the UI thread. Only static methods // are executed on other threads. +// +// MHTMLGenerationManager is a singleton. Each call to SaveMHTML method creates +// a new instance of MHTMLGenerationManager::Job that tracks generation of a +// single MHTML file. class MHTMLGenerationManager { public: static MHTMLGenerationManager* GetInstance(); @@ -37,8 +41,8 @@ class MHTMLGenerationManager { // generated file. On failure |file_size| is -1. typedef base::Callback<void(int64_t file_size)> GenerateMHTMLCallback; - // Instructs the render view to generate a MHTML representation of the current - // page for |web_contents|. + // Instructs the RenderFrames in |web_contents| to generate a MHTML + // representation of the current page. void SaveMHTML(WebContents* web_contents, const MHTMLGenerationParams& params, const GenerateMHTMLCallback& callback); diff --git a/chromium/content/browser/download/save_file_manager.cc b/chromium/content/browser/download/save_file_manager.cc index 527a7a18fbb..aed8eb0e0ca 100644 --- a/chromium/content/browser/download/save_file_manager.cc +++ b/chromium/content/browser/download/save_file_manager.cc @@ -19,7 +19,6 @@ #include "content/browser/web_contents/web_contents_impl.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/render_frame_host.h" -#include "net/base/filename_util.h" #include "net/base/io_buffer.h" #include "url/gurl.h" @@ -324,41 +323,6 @@ void SaveFileManager::CancelSave(SaveItemId save_item_id) { } } -// It is possible that SaveItem which has specified save_item_id has been -// canceled -// before this function runs. So if we can not find corresponding SaveFile by -// using specified save_item_id, just return. -void SaveFileManager::SaveLocalFile(const GURL& original_file_url, - SaveItemId save_item_id, - SavePackageId save_package_id) { - DCHECK_CURRENTLY_ON(BrowserThread::FILE); - SaveFile* save_file = LookupSaveFile(save_item_id); - if (!save_file) - return; - // If it has finished, just return. - if (!save_file->InProgress()) - return; - - // Close the save file before the copy operation. - save_file->Finish(); - save_file->Detach(); - - DCHECK(original_file_url.SchemeIsFile()); - base::FilePath file_path; - net::FileURLToFilePath(original_file_url, &file_path); - // If we can not get valid file path from original URL, treat it as - // disk error. - if (file_path.empty()) - SaveFinished(save_item_id, save_package_id, false); - - // Copy the local file to the temporary file. It will be renamed to its - // final name later. - bool success = base::CopyFile(file_path, save_file->FullPath()); - if (!success) - base::DeleteFile(save_file->FullPath(), false); - SaveFinished(save_item_id, save_package_id, success); -} - void SaveFileManager::OnDeleteDirectoryOrFile(const base::FilePath& full_path, bool is_dir) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); diff --git a/chromium/content/browser/download/save_file_resource_handler.cc b/chromium/content/browser/download/save_file_resource_handler.cc index 7cba445aad5..2c1c5526ca4 100644 --- a/chromium/content/browser/download/save_file_resource_handler.cc +++ b/chromium/content/browser/download/save_file_resource_handler.cc @@ -16,13 +16,15 @@ namespace content { -SaveFileResourceHandler::SaveFileResourceHandler(net::URLRequest* request, - SaveItemId save_item_id, - SavePackageId save_package_id, - int render_process_host_id, - int render_frame_routing_id, - const GURL& url, - SaveFileManager* manager) +SaveFileResourceHandler::SaveFileResourceHandler( + net::URLRequest* request, + SaveItemId save_item_id, + SavePackageId save_package_id, + int render_process_host_id, + int render_frame_routing_id, + const GURL& url, + SaveFileManager* manager, + AuthorizationState authorization_state) : ResourceHandler(request), save_item_id_(save_item_id), save_package_id_(save_package_id), @@ -30,7 +32,8 @@ SaveFileResourceHandler::SaveFileResourceHandler(net::URLRequest* request, render_frame_routing_id_(render_frame_routing_id), url_(url), content_length_(0), - save_manager_(manager) {} + save_manager_(manager), + authorization_state_(authorization_state) {} SaveFileResourceHandler::~SaveFileResourceHandler() { } @@ -57,7 +60,7 @@ bool SaveFileResourceHandler::OnResponseStarted(ResourceResponse* response, } bool SaveFileResourceHandler::OnWillStart(const GURL& url, bool* defer) { - return true; + return authorization_state_ == AuthorizationState::AUTHORIZED; } bool SaveFileResourceHandler::OnBeforeNetworkStart(const GURL& url, @@ -68,6 +71,7 @@ bool SaveFileResourceHandler::OnBeforeNetworkStart(const GURL& url, bool SaveFileResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf, int* buf_size, int min_size) { + DCHECK_EQ(AuthorizationState::AUTHORIZED, authorization_state_); DCHECK(buf && buf_size); if (!read_buffer_.get()) { *buf_size = min_size < 0 ? kReadBufSize : min_size; @@ -78,6 +82,7 @@ bool SaveFileResourceHandler::OnWillRead(scoped_refptr<net::IOBuffer>* buf, } bool SaveFileResourceHandler::OnReadCompleted(int bytes_read, bool* defer) { + DCHECK_EQ(AuthorizationState::AUTHORIZED, authorization_state_); DCHECK(read_buffer_.get()); // We are passing ownership of this buffer to the save file manager. scoped_refptr<net::IOBuffer> buffer; @@ -93,6 +98,9 @@ void SaveFileResourceHandler::OnResponseCompleted( const net::URLRequestStatus& status, const std::string& security_info, bool* defer) { + if (authorization_state_ != AuthorizationState::AUTHORIZED) + DCHECK(!status.is_success()); + BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, base::Bind(&SaveFileManager::SaveFinished, save_manager_, save_item_id_, diff --git a/chromium/content/browser/download/save_file_resource_handler.h b/chromium/content/browser/download/save_file_resource_handler.h index e559e44a63e..2644dcc124f 100644 --- a/chromium/content/browser/download/save_file_resource_handler.h +++ b/chromium/content/browser/download/save_file_resource_handler.h @@ -25,13 +25,26 @@ class SaveFileManager; // Forwards data to the save thread. class SaveFileResourceHandler : public ResourceHandler { public: + // Unauthorized requests are cancelled from OnWillStart callback. + // + // This way of handling unauthorized requests allows unified handling of all + // SaveFile requests - communicating the failure to OnResponseCompleted + // happens in a generic, typical way, reusing common infrastructure code + // (rather than forcing an ad-hoc, Save-File-specific call to + // OnResponseCompleted from ResourceDispatcherHostImpl::BeginSaveFile). + enum class AuthorizationState { + AUTHORIZED, + NOT_AUTHORIZED, + }; + SaveFileResourceHandler(net::URLRequest* request, SaveItemId save_item_id, SavePackageId save_package_id, int render_process_host_id, int render_frame_routing_id, const GURL& url, - SaveFileManager* manager); + SaveFileManager* manager, + AuthorizationState authorization_state); ~SaveFileResourceHandler() override; // ResourceHandler Implementation: @@ -88,6 +101,8 @@ class SaveFileResourceHandler : public ResourceHandler { int64_t content_length_; SaveFileManager* save_manager_; + AuthorizationState authorization_state_; + static const int kReadBufSize = 32768; // bytes DISALLOW_COPY_AND_ASSIGN(SaveFileResourceHandler); diff --git a/chromium/content/browser/download/save_item.cc b/chromium/content/browser/download/save_item.cc index 5fa262778e8..9bff4fa482f 100644 --- a/chromium/content/browser/download/save_item.cc +++ b/chromium/content/browser/download/save_item.cc @@ -28,23 +28,23 @@ SaveItem::SaveItem(const GURL& url, const Referrer& referrer, SavePackage* package, SaveFileCreateInfo::SaveFileSource save_source, - int frame_tree_node_id) + int frame_tree_node_id, + int container_frame_tree_node_id) : save_item_id_(GetNextSaveItemId()), url_(url), referrer_(referrer), frame_tree_node_id_(frame_tree_node_id), + container_frame_tree_node_id_(container_frame_tree_node_id), total_bytes_(0), received_bytes_(0), state_(WAIT_START), - has_final_name_(false), is_success_(false), save_source_(save_source), package_(package) { DCHECK(package); } -SaveItem::~SaveItem() { -} +SaveItem::~SaveItem() {} // Set start state for save item. void SaveItem::Start() { @@ -81,9 +81,8 @@ void SaveItem::Cancel() { // Small downloads might be complete before method has a chance to run. return; } - state_ = CANCELED; - is_success_ = false; Finish(received_bytes_, false); + state_ = CANCELED; package_->SaveCanceled(this); } @@ -95,33 +94,9 @@ void SaveItem::Finish(int64_t size, bool is_success) { UpdateSize(size); } -// Calculate the percentage of the save item -int SaveItem::PercentComplete() const { - switch (state_) { - case COMPLETE: - case CANCELED: - return 100; - case WAIT_START: - return 0; - case IN_PROGRESS: { - int percent = 0; - if (total_bytes_ > 0) - percent = static_cast<int>(received_bytes_ * 100.0 / total_bytes_); - return percent; - } - default: { - NOTREACHED(); - return -1; - } - } -} - -// Rename the save item with new path. -void SaveItem::Rename(const base::FilePath& full_path) { +void SaveItem::SetTargetPath(const base::FilePath& full_path) { DCHECK(!full_path.empty() && !has_final_name()); full_path_ = full_path; - file_name_ = full_path_.BaseName(); - has_final_name_ = true; } void SaveItem::SetTotalBytes(int64_t total_bytes) { diff --git a/chromium/content/browser/download/save_item.h b/chromium/content/browser/download/save_item.h index ef463953bf7..049c7f3936a 100644 --- a/chromium/content/browser/download/save_item.h +++ b/chromium/content/browser/download/save_item.h @@ -31,7 +31,8 @@ class SaveItem { const Referrer& referrer, SavePackage* package, SaveFileCreateInfo::SaveFileSource save_source, - int frame_tree_node_id); + int frame_tree_node_id, + int container_frame_tree_node_id); ~SaveItem(); @@ -46,12 +47,8 @@ class SaveItem { // Saving operation completed. void Finish(int64_t size, bool is_success); - // Rough percent complete, -1 means we don't know (since we didn't receive a - // total size). - int PercentComplete() const; - // Update path for SaveItem, the actual file is renamed on the file thread. - void Rename(const base::FilePath& full_path); + void SetTargetPath(const base::FilePath& full_path); void SetTotalBytes(int64_t total_bytes); @@ -59,18 +56,18 @@ class SaveItem { SaveItemId id() const { return save_item_id_; } SaveState state() const { return state_; } const base::FilePath& full_path() const { return full_path_; } - const base::FilePath& file_name() const { return file_name_; } const GURL& url() const { return url_; } const Referrer& referrer() const { return referrer_; } int frame_tree_node_id() const { return frame_tree_node_id_; } - int64_t total_bytes() const { return total_bytes_; } + int container_frame_tree_node_id() const { + return container_frame_tree_node_id_; + } int64_t received_bytes() const { return received_bytes_; } - bool has_final_name() const { return has_final_name_; } + bool has_final_name() const { return !full_path_.empty(); } bool success() const { return is_success_; } SaveFileCreateInfo::SaveFileSource save_source() const { return save_source_; } - SavePackage* package() const { return package_; } private: // Internal helper for maintaining consistent received and total sizes. @@ -82,17 +79,19 @@ class SaveItem { // Full path to the save item file. base::FilePath full_path_; - // Short display version of the file. - base::FilePath file_name_; - // The URL for this save item. GURL url_; Referrer referrer_; // Frame tree node id, if this save item represents a frame - // (otherwise FrameTreeNode::kFrameTreeNodeInvalidID). + // (otherwise FrameTreeNode::kFrameTreeNodeInvalidId). int frame_tree_node_id_; + // Frame tree node id of the frame containing this save item. + // (FrameTreeNode::kFrameTreeNodeInvalidId if this save item represents the + // main frame, which obviously doesn't have a containing/parent frame). + int container_frame_tree_node_id_; + // Total bytes expected. int64_t total_bytes_; @@ -102,9 +101,6 @@ class SaveItem { // The current state of this save item. SaveState state_; - // Specifies if this name is a final or not. - bool has_final_name_; - // Flag indicates whether SaveItem has error while in saving process. bool is_success_; diff --git a/chromium/content/browser/download/save_package.cc b/chromium/content/browser/download/save_package.cc index 7704eb4aae2..1a21d4125b1 100644 --- a/chromium/content/browser/download/save_package.cc +++ b/chromium/content/browser/download/save_package.cc @@ -90,21 +90,21 @@ const uint32_t kMaxFileOrdinalNumberPartLength = 6; // file names, i.e. those stripped of their extensions. // TODO(estade): improve this to not choke on alternate encodings. base::FilePath::StringType StripOrdinalNumber( - const base::FilePath::StringType& pure_file_name) { + const base::FilePath::StringType& base_name) { base::FilePath::StringType::size_type r_paren_index = - pure_file_name.rfind(FILE_PATH_LITERAL(')')); + base_name.rfind(FILE_PATH_LITERAL(')')); base::FilePath::StringType::size_type l_paren_index = - pure_file_name.rfind(FILE_PATH_LITERAL('(')); + base_name.rfind(FILE_PATH_LITERAL('(')); if (l_paren_index >= r_paren_index) - return pure_file_name; + return base_name; for (base::FilePath::StringType::size_type i = l_paren_index + 1; i != r_paren_index; ++i) { - if (!base::IsAsciiDigit(pure_file_name[i])) - return pure_file_name; + if (!base::IsAsciiDigit(base_name[i])) + return base_name; } - return pure_file_name.substr(0, l_paren_index); + return base_name.substr(0, l_paren_index); } // Check whether we can save page as complete-HTML for the contents which @@ -140,31 +140,32 @@ class SavePackageRequestHandle : public DownloadRequestHandleInterface { const base::FilePath::CharType SavePackage::kDefaultHtmlExtension[] = FILE_PATH_LITERAL("html"); +SavePackage::SavePackage(WebContents* web_contents) + : WebContentsObserver(web_contents), + page_url_(GetUrlToBeSaved(web_contents)), + title_(web_contents->GetTitle()), + start_tick_(base::TimeTicks::Now()), + file_name_set_(&base::FilePath::CompareLessIgnoreCase), + unique_id_(GetNextSavePackageId()) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + InternalInit(); +} + +// Used for tests. SavePackage::SavePackage(WebContents* web_contents, SavePageType save_type, const base::FilePath& file_full_path, const base::FilePath& directory_full_path) : WebContentsObserver(web_contents), - number_of_frames_pending_response_(0), - file_manager_(nullptr), - download_manager_(nullptr), - download_(nullptr), - page_url_(GetUrlToBeSaved()), + page_url_(GetUrlToBeSaved(web_contents)), saved_main_file_path_(file_full_path), saved_main_directory_path_(directory_full_path), title_(web_contents->GetTitle()), start_tick_(base::TimeTicks::Now()), - finished_(false), - user_canceled_(false), - disk_error_occurred_(false), save_type_(save_type), - all_save_items_count_(0), file_name_set_(&base::FilePath::CompareLessIgnoreCase), - wait_state_(INITIALIZE), - unique_id_(GetNextSavePackageId()), - wrote_to_completed_file_(false), - wrote_to_failed_file_(false) { - DCHECK(page_url_.is_valid()); + unique_id_(GetNextSavePackageId()) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK((save_type_ == SAVE_PAGE_TYPE_AS_ONLY_HTML) || (save_type_ == SAVE_PAGE_TYPE_AS_MHTML) || (save_type_ == SAVE_PAGE_TYPE_AS_COMPLETE_HTML)) @@ -176,55 +177,8 @@ SavePackage::SavePackage(WebContents* web_contents, InternalInit(); } -SavePackage::SavePackage(WebContents* web_contents) - : WebContentsObserver(web_contents), - number_of_frames_pending_response_(0), - file_manager_(nullptr), - download_manager_(nullptr), - download_(nullptr), - page_url_(GetUrlToBeSaved()), - title_(web_contents->GetTitle()), - start_tick_(base::TimeTicks::Now()), - finished_(false), - user_canceled_(false), - disk_error_occurred_(false), - save_type_(SAVE_PAGE_TYPE_UNKNOWN), - all_save_items_count_(0), - file_name_set_(&base::FilePath::CompareLessIgnoreCase), - wait_state_(INITIALIZE), - unique_id_(GetNextSavePackageId()), - wrote_to_completed_file_(false), - wrote_to_failed_file_(false) { - DCHECK(page_url_.is_valid()); - InternalInit(); -} - -// This is for testing use. Set |finished_| as true because we don't want -// method Cancel to be be called in destructor in test mode. -// We also don't call InternalInit(). -SavePackage::SavePackage(WebContents* web_contents, - const base::FilePath& file_full_path, - const base::FilePath& directory_full_path) - : WebContentsObserver(web_contents), - number_of_frames_pending_response_(0), - file_manager_(nullptr), - download_manager_(nullptr), - download_(nullptr), - saved_main_file_path_(file_full_path), - saved_main_directory_path_(directory_full_path), - start_tick_(base::TimeTicks::Now()), - finished_(true), - user_canceled_(false), - disk_error_occurred_(false), - save_type_(SAVE_PAGE_TYPE_UNKNOWN), - all_save_items_count_(0), - file_name_set_(&base::FilePath::CompareLessIgnoreCase), - wait_state_(INITIALIZE), - unique_id_(GetNextSavePackageId()), - wrote_to_completed_file_(false), - wrote_to_failed_file_(false) {} - SavePackage::~SavePackage() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); // Stop receiving saving job's updates if (!finished_ && !canceled()) { // Unexpected quit. @@ -252,17 +206,20 @@ SavePackage::~SavePackage() { file_manager_ = nullptr; } -GURL SavePackage::GetUrlToBeSaved() { - // Instead of using web_contents_.GetURL here, we use url() (which is the +// static +GURL SavePackage::GetUrlToBeSaved(WebContents* web_contents) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); + // Instead of using web_contents->GetURL here, we call GetURL() (which is the // "real" url of the page) from the NavigationEntry because it reflects its - // origin rather than the displayed one (returned by GetURL) which may be - // different (like having "view-source:" on the front). + // origin rather than the displayed one (returned by GetVirtualURL) which may + // be different (like having "view-source:" on the front). NavigationEntry* visible_entry = - web_contents()->GetController().GetVisibleEntry(); + web_contents->GetController().GetVisibleEntry(); return visible_entry ? visible_entry->GetURL() : GURL::EmptyGURL(); } void SavePackage::Cancel(bool user_action) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (!canceled()) { if (user_action) user_canceled_ = true; @@ -276,12 +233,9 @@ void SavePackage::Cancel(bool user_action) { // Init() can be called directly, or indirectly via GetSaveInfo(). In both // cases, we need file_manager_ to be initialized, so we do this first. void SavePackage::InternalInit() { - ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get(); - if (!rdh) { - NOTREACHED(); - return; - } + DCHECK_CURRENTLY_ON(BrowserThread::UI); + ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get(); file_manager_ = rdh->save_file_manager(); DCHECK(file_manager_); @@ -296,6 +250,7 @@ void SavePackage::InternalInit() { bool SavePackage::Init( const SavePackageDownloadCreatedCallback& download_created_callback) { DCHECK_CURRENTLY_ON(BrowserThread::UI); + DCHECK(page_url_.is_valid()); // Set proper running state. if (wait_state_ != INITIALIZE) return false; @@ -346,13 +301,11 @@ void SavePackage::InitWithDownloadItem( } else { DCHECK_EQ(SAVE_PAGE_TYPE_AS_ONLY_HTML, save_type_); wait_state_ = NET_FILES; - SaveFileCreateInfo::SaveFileSource save_source = page_url_.SchemeIsFile() ? - SaveFileCreateInfo::SAVE_FILE_FROM_FILE : - SaveFileCreateInfo::SAVE_FILE_FROM_NET; // Add this item to waiting list. - waiting_item_queue_.push_back( - new SaveItem(page_url_, Referrer(), this, save_source, - FrameTreeNode::kFrameTreeNodeInvalidId)); + waiting_item_queue_.push_back(new SaveItem( + page_url_, Referrer(), this, SaveFileCreateInfo::SAVE_FILE_FROM_NET, + FrameTreeNode::kFrameTreeNodeInvalidId, + web_contents()->GetMainFrame()->GetFrameTreeNodeId())); all_save_items_count_ = 1; download_->SetTotalBytes(1); @@ -361,6 +314,7 @@ void SavePackage::InitWithDownloadItem( } void SavePackage::OnMHTMLGenerated(int64_t size) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (size <= 0) { Cancel(false); return; @@ -388,7 +342,7 @@ void SavePackage::OnMHTMLGenerated(int64_t size) { } } -// On POSIX, the length of |pure_file_name| + |file_name_ext| is further +// On POSIX, the length of |base_name| + |file_name_ext| is further // restricted by NAME_MAX. The maximum allowed path looks like: // '/path/to/save_dir' + '/' + NAME_MAX. uint32_t SavePackage::GetMaxPathLengthForDirectory( @@ -402,29 +356,12 @@ uint32_t SavePackage::GetMaxPathLengthForDirectory( #endif } -// File name is considered being consist of pure file name, dot and file -// extension name. File name might has no dot and file extension, or has -// multiple dot inside file name. The dot, which separates the pure file -// name and file extension name, is last dot in the whole file name. -// This function is for making sure the length of specified file path is not -// great than the specified maximum length of file path and getting safe pure -// file name part if the input pure file name is too long. -// The parameter |dir_path| specifies directory part of the specified -// file path. The parameter |file_name_ext| specifies file extension -// name part of the specified file path (including start dot). The parameter -// |max_file_path_len| specifies maximum length of the specified file path. -// The parameter |pure_file_name| input pure file name part of the specified -// file path. If the length of specified file path is great than -// |max_file_path_len|, the |pure_file_name| will output new pure file name -// part for making sure the length of specified file path is less than -// specified maximum length of file path. Return false if the function can -// not get a safe pure file name, otherwise it returns true. -bool SavePackage::GetSafePureFileName( +bool SavePackage::TruncateBaseNameToFitPathConstraints( const base::FilePath& dir_path, const base::FilePath::StringType& file_name_ext, uint32_t max_file_path_len, - base::FilePath::StringType* pure_file_name) { - DCHECK(!pure_file_name->empty()); + base::FilePath::StringType* base_name) { + DCHECK(!base_name->empty()); int available_length = static_cast<int>(max_file_path_len - dir_path.value().length() - file_name_ext.length()); @@ -433,17 +370,17 @@ bool SavePackage::GetSafePureFileName( --available_length; // Plenty of room. - if (static_cast<int>(pure_file_name->length()) <= available_length) + if (static_cast<int>(base_name->length()) <= available_length) return true; - // Limited room. Truncate |pure_file_name| to fit. + // Limited room. Truncate |base_name| to fit. if (available_length > 0) { - *pure_file_name = pure_file_name->substr(0, available_length); + *base_name = base_name->substr(0, available_length); return true; } - // Not enough room to even use a shortened |pure_file_name|. - pure_file_name->clear(); + // Not enough room to even use a shortened |base_name|. + base_name->clear(); return false; } @@ -462,25 +399,29 @@ bool SavePackage::GenerateFileName(const std::string& disposition, kDefaultSaveName); DCHECK(!file_path.empty()); - base::FilePath::StringType pure_file_name = + if (need_html_ext) + file_path = file_path.ReplaceExtension(kDefaultHtmlExtension); + + DownloadManagerDelegate* delegate = download_manager_->GetDelegate(); + if (delegate) + delegate->SanitizeSavePackageResourceName(&file_path); + + DCHECK_EQ(file_path.value(), file_path.BaseName().value()) + << "SanitizeSavePackageResourceName should only return a basename."; + + base::FilePath::StringType base_name = file_path.RemoveExtension().BaseName().value(); base::FilePath::StringType file_name_ext = file_path.Extension(); - // If it is HTML resource, use ".html" as its extension. - if (need_html_ext) { - file_name_ext = FILE_PATH_LITERAL("."); - file_name_ext.append(kDefaultHtmlExtension); - } - // Need to make sure the suggested file name is not too long. uint32_t max_path = GetMaxPathLengthForDirectory(saved_main_directory_path_); // Get safe pure file name. - if (!GetSafePureFileName(saved_main_directory_path_, file_name_ext, - max_path, &pure_file_name)) + if (!TruncateBaseNameToFitPathConstraints( + saved_main_directory_path_, file_name_ext, max_path, &base_name)) return false; - base::FilePath::StringType file_name = pure_file_name + file_name_ext; + base::FilePath::StringType file_name = base_name + file_name_ext; // Check whether we already have same name in a case insensitive manner. FileNameSet::const_iterator iter = file_name_set_.find(file_name); @@ -488,15 +429,14 @@ bool SavePackage::GenerateFileName(const std::string& disposition, file_name_set_.insert(file_name); } else { // Found same name, increase the ordinal number for the file name. - pure_file_name = - base::FilePath(*iter).RemoveExtension().BaseName().value(); - base::FilePath::StringType base_file_name = - StripOrdinalNumber(pure_file_name); + base_name = base::FilePath(*iter).RemoveExtension().BaseName().value(); + base::FilePath::StringType base_file_name = StripOrdinalNumber(base_name); // We need to make sure the length of base file name plus maximum ordinal // number path will be less than or equal to kMaxFilePathLength. - if (!GetSafePureFileName(saved_main_directory_path_, file_name_ext, - max_path - kMaxFileOrdinalNumberPartLength, &base_file_name)) + if (!TruncateBaseNameToFitPathConstraints( + saved_main_directory_path_, file_name_ext, + max_path - kMaxFileOrdinalNumberPartLength, &base_file_name)) return false; // Prepare the new ordinal number. @@ -517,9 +457,9 @@ bool SavePackage::GenerateFileName(const std::string& disposition, base::CreateTemporaryFile(&temp_file); file_name = temp_file.RemoveExtension().BaseName().value(); // Get safe pure file name. - if (!GetSafePureFileName(saved_main_directory_path_, - base::FilePath::StringType(), - max_path, &file_name)) + if (!TruncateBaseNameToFitPathConstraints(saved_main_directory_path_, + base::FilePath::StringType(), + max_path, &file_name)) return false; } else { for (int i = ordinal_number; i < kMaxFileOrdinalNumber; ++i) { @@ -546,6 +486,7 @@ bool SavePackage::GenerateFileName(const std::string& disposition, // We have received a message from SaveFileManager about a new saving job. We // find a SaveItem and store it in our in_progress list. void SavePackage::StartSave(const SaveFileCreateInfo* info) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK(info); SaveItemIdMap::iterator it = in_progress_items_.find(info->save_item_id); @@ -597,20 +538,10 @@ void SavePackage::StartSave(const SaveFileCreateInfo* info) { // rename the SaveItem. base::FilePath final_name = saved_main_directory_path_.Append(generated_name); - save_item->Rename(final_name); + save_item->SetTargetPath(final_name); } else { // It is the main HTML file, use the name chosen by the user. - save_item->Rename(saved_main_file_path_); - } - - // If the save source is from file system, inform SaveFileManager to copy - // corresponding file to the file path which this SaveItem specifies. - if (info->save_source == SaveFileCreateInfo::SAVE_FILE_FROM_FILE) { - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&SaveFileManager::SaveLocalFile, file_manager_, - save_item->url(), save_item->id(), id())); - return; + save_item->SetTargetPath(saved_main_file_path_); } // Check whether we begin to require serialized HTML data. @@ -622,7 +553,8 @@ void SavePackage::StartSave(const SaveFileCreateInfo* info) { } } -SaveItem* SavePackage::LookupSaveItemInProcess(SaveItemId save_item_id) { +SaveItem* SavePackage::LookupInProgressSaveItem(SaveItemId save_item_id) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); auto it = in_progress_items_.find(save_item_id); if (it != in_progress_items_.end()) { SaveItem* save_item = it->second; @@ -633,6 +565,7 @@ SaveItem* SavePackage::LookupSaveItemInProcess(SaveItemId save_item_id) { } void SavePackage::PutInProgressItemToSavedMap(SaveItem* save_item) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); SaveItemIdMap::iterator it = in_progress_items_.find(save_item->id()); DCHECK(it != in_progress_items_.end()); DCHECK_EQ(save_item, it->second); @@ -648,9 +581,10 @@ void SavePackage::PutInProgressItemToSavedMap(SaveItem* save_item) { bool SavePackage::UpdateSaveProgress(SaveItemId save_item_id, int64_t size, bool write_success) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); // Because we might have canceled this saving job before, // so we might not find corresponding SaveItem. - SaveItem* save_item = LookupSaveItemInProcess(save_item_id); + SaveItem* save_item = LookupInProgressSaveItem(save_item_id); if (!save_item) return false; @@ -667,6 +601,7 @@ bool SavePackage::UpdateSaveProgress(SaveItemId save_item_id, // Stop all page saving jobs that are in progress and instruct the FILE thread // to delete all saved files. void SavePackage::Stop() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); // If we haven't moved out of the initial state, there's nothing to cancel and // there won't be valid pointers for |file_manager_| or |download_|. if (wait_state_ == INITIALIZE) @@ -711,6 +646,7 @@ void SavePackage::Stop() { } void SavePackage::CheckFinish() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (in_process_count() || finished_) return; @@ -735,6 +671,7 @@ void SavePackage::CheckFinish() { // Successfully finished all items of this SavePackage. void SavePackage::Finish() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); // User may cancel the job when we're moving files to the final directory. if (canceled()) return; @@ -785,9 +722,10 @@ void SavePackage::Finish() { void SavePackage::SaveFinished(SaveItemId save_item_id, int64_t size, bool is_success) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); // Because we might have canceled this saving job before, // so we might not find corresponding SaveItem. Just ignore it. - SaveItem* save_item = LookupSaveItemInProcess(save_item_id); + SaveItem* save_item = LookupInProgressSaveItem(save_item_id); if (!save_item) return; @@ -835,6 +773,7 @@ void SavePackage::SaveCanceled(const SaveItem* save_item) { } void SavePackage::SaveNextFile(bool process_all_remaining_items) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK(web_contents()); DCHECK(!waiting_item_queue_.empty()); @@ -847,16 +786,36 @@ void SavePackage::SaveNextFile(bool process_all_remaining_items) { DCHECK(!ContainsKey(in_progress_items_, save_item->id())); in_progress_items_[save_item->id()] = save_item; save_item->Start(); + + // Find the frame responsible for making the network request below - it will + // be used in security checks made later by ResourceDispatcherHostImpl. + int requester_frame_tree_node_id = + save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_NET + ? save_item->container_frame_tree_node_id() + : save_item->frame_tree_node_id(); + DCHECK_NE(FrameTreeNode::kFrameTreeNodeInvalidId, + requester_frame_tree_node_id); + FrameTreeNode* requester_frame_tree_node = + FrameTreeNode::GloballyFindByID(requester_frame_tree_node_id); + if (!requester_frame_tree_node) { + save_item->Finish(0, /* is_success = */ false); + continue; + } + RenderFrameHostImpl* requester_frame = + requester_frame_tree_node->current_frame_host(); + file_manager_->SaveURL( save_item->id(), save_item->url(), save_item->referrer(), - web_contents()->GetRenderProcessHost()->GetID(), routing_id(), - web_contents()->GetMainFrame()->GetRoutingID(), - save_item->save_source(), save_item->full_path(), + requester_frame->GetProcess()->GetID(), + requester_frame->render_view_host()->GetRoutingID(), + requester_frame->routing_id(), save_item->save_source(), + save_item->full_path(), web_contents()->GetBrowserContext()->GetResourceContext(), this); } while (process_all_remaining_items && !waiting_item_queue_.empty()); } int SavePackage::PercentComplete() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (!all_save_items_count_) return 0; if (!in_process_count()) @@ -865,12 +824,14 @@ int SavePackage::PercentComplete() { } int64_t SavePackage::CurrentSpeed() const { + DCHECK_CURRENTLY_ON(BrowserThread::UI); base::TimeDelta diff = base::TimeTicks::Now() - start_tick_; int64_t diff_ms = diff.InMilliseconds(); return diff_ms == 0 ? 0 : completed_count() * 1000 / diff_ms; } void SavePackage::DoSavingProcess() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (save_type_ == SAVE_PAGE_TYPE_AS_COMPLETE_HTML) { // We guarantee that images and JavaScripts must be downloaded first. // So when finishing all those sub-resources, we will know which @@ -931,6 +892,7 @@ bool SavePackage::OnMessageReceived(const IPC::Message& message, // map:(originalURL:currentLocalPath) to render process (backend). // Then render process will serialize DOM and send data to us. void SavePackage::GetSerializedHtmlWithLocalLinks() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (wait_state_ != HTML_DATA) return; @@ -979,6 +941,7 @@ void SavePackage::GetSerializedHtmlWithLocalLinks() { void SavePackage::GetSerializedHtmlWithLocalLinksForFrame( FrameTreeNode* target_tree_node) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK(target_tree_node); int target_frame_tree_node_id = target_tree_node->frame_tree_node_id(); RenderFrameHostImpl* target = target_tree_node->current_frame_host(); @@ -1005,7 +968,7 @@ void SavePackage::GetSerializedHtmlWithLocalLinksForFrame( if (target_tree_node->IsMainFrame()) { local_path = local_path.Append(saved_main_directory_path_.BaseName()); } - local_path = local_path.Append(save_item->file_name()); + local_path = local_path.Append(save_item->full_path().BaseName()); // Insert the link into |url_to_local_path| or |routing_id_to_local_path|. if (save_item->save_source() != SaveFileCreateInfo::SAVE_FILE_FROM_DOM) { @@ -1042,6 +1005,7 @@ void SavePackage::OnSerializedHtmlWithLocalLinksResponse( RenderFrameHostImpl* sender, const std::string& data, bool end_of_data) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); // Check current state. if (wait_state_ != HTML_DATA) return; @@ -1100,6 +1064,7 @@ void SavePackage::OnSerializedHtmlWithLocalLinksResponse( // Ask for all savable resource links from backend, include main frame and // sub-frame. void SavePackage::GetSavableResourceLinks() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (wait_state_ != START_PROCESS) return; @@ -1125,6 +1090,7 @@ void SavePackage::OnSavableResourceLinksResponse( const std::vector<GURL>& resources_list, const Referrer& referrer, const std::vector<SavableSubframe>& subframes) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (wait_state_ != RESOURCES_LIST) return; @@ -1165,9 +1131,11 @@ SaveItem* SavePackage::CreatePendingSaveItem( const GURL& url, const Referrer& referrer, SaveFileCreateInfo::SaveFileSource save_source) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); Referrer sanitized_referrer = Referrer::SanitizeForRequest(url, referrer); - SaveItem* save_item = new SaveItem(url, sanitized_referrer, this, save_source, - save_item_frame_tree_node_id); + SaveItem* save_item = + new SaveItem(url, sanitized_referrer, this, save_source, + save_item_frame_tree_node_id, container_frame_tree_node_id); waiting_item_queue_.push_back(save_item); frame_tree_node_id_to_contained_save_items_[container_frame_tree_node_id] @@ -1181,6 +1149,7 @@ SaveItem* SavePackage::CreatePendingSaveItemDeduplicatingByUrl( const GURL& url, const Referrer& referrer, SaveFileCreateInfo::SaveFileSource save_source) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK(url.is_valid()); // |url| should be validated by the callers. // Frames should not be deduplicated by URL. @@ -1205,20 +1174,19 @@ SaveItem* SavePackage::CreatePendingSaveItemDeduplicatingByUrl( void SavePackage::EnqueueSavableResource(int container_frame_tree_node_id, const GURL& url, const Referrer& referrer) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); if (!url.is_valid()) return; - SaveFileCreateInfo::SaveFileSource save_source = - url.SchemeIsFile() ? SaveFileCreateInfo::SAVE_FILE_FROM_FILE - : SaveFileCreateInfo::SAVE_FILE_FROM_NET; CreatePendingSaveItemDeduplicatingByUrl( container_frame_tree_node_id, FrameTreeNode::kFrameTreeNodeInvalidId, url, - referrer, save_source); + referrer, SaveFileCreateInfo::SAVE_FILE_FROM_NET); } void SavePackage::EnqueueFrame(int container_frame_tree_node_id, int frame_tree_node_id, const GURL& frame_original_url) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); SaveItem* save_item = CreatePendingSaveItem( container_frame_tree_node_id, frame_tree_node_id, frame_original_url, Referrer(), SaveFileCreateInfo::SAVE_FILE_FROM_DOM); @@ -1227,10 +1195,12 @@ void SavePackage::EnqueueFrame(int container_frame_tree_node_id, } void SavePackage::OnSavableResourceLinksError(RenderFrameHostImpl* sender) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); CompleteSavableResourceLinksResponse(); } void SavePackage::CompleteSavableResourceLinksResponse() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); --number_of_frames_pending_response_; DCHECK_LE(0, number_of_frames_pending_response_); if (number_of_frames_pending_response_ != 0) @@ -1272,6 +1242,7 @@ void SavePackage::CompleteSavableResourceLinksResponse() { base::FilePath SavePackage::GetSuggestedNameForSaveAs( bool can_save_as_complete, const std::string& contents_mime_type) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); base::FilePath name_with_proper_ext = base::FilePath::FromUTF16Unsafe(title_); // If the page's title matches its URL, use the URL. Try to use the last path @@ -1348,6 +1319,7 @@ base::FilePath SavePackage::EnsureMimeExtension(const base::FilePath& name, return name; } +// static const base::FilePath::CharType* SavePackage::ExtensionForMimeType( const std::string& contents_mime_type) { static const struct { @@ -1368,6 +1340,7 @@ const base::FilePath::CharType* SavePackage::ExtensionForMimeType( } void SavePackage::GetSaveInfo() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); // Can't use |web_contents_| in the FILE thread, so get the data that we need // before calling to it. base::FilePath website_save_dir; @@ -1380,17 +1353,23 @@ void SavePackage::GetSaveInfo() { &download_save_dir, &skip_dir_check); } std::string mime_type = web_contents()->GetContentsMimeType(); - BrowserThread::PostTask( + bool can_save_as_complete = CanSaveAsComplete(mime_type); + base::FilePath suggested_filename = + GetSuggestedNameForSaveAs(can_save_as_complete, mime_type); + BrowserThread::PostTaskAndReplyWithResult( BrowserThread::FILE, FROM_HERE, - base::Bind(&SavePackage::CreateDirectoryOnFileThread, this, - website_save_dir, download_save_dir, skip_dir_check, mime_type)); + base::Bind(&SavePackage::CreateDirectoryOnFileThread, website_save_dir, + download_save_dir, suggested_filename, skip_dir_check), + base::Bind(&SavePackage::ContinueGetSaveInfo, this, + can_save_as_complete)); } -void SavePackage::CreateDirectoryOnFileThread( +// static +base::FilePath SavePackage::CreateDirectoryOnFileThread( const base::FilePath& website_save_dir, const base::FilePath& download_save_dir, - bool skip_dir_check, - const std::string& mime_type) { + const base::FilePath& suggested_filename, + bool skip_dir_check) { DCHECK_CURRENTLY_ON(BrowserThread::FILE); base::FilePath save_dir; @@ -1408,34 +1387,26 @@ void SavePackage::CreateDirectoryOnFileThread( save_dir = website_save_dir; } - bool can_save_as_complete = CanSaveAsComplete(mime_type); - base::FilePath suggested_filename = GetSuggestedNameForSaveAs( - can_save_as_complete, mime_type); - base::FilePath::StringType pure_file_name = + base::FilePath::StringType base_name = suggested_filename.RemoveExtension().BaseName().value(); base::FilePath::StringType file_name_ext = suggested_filename.Extension(); // Need to make sure the suggested file name is not too long. uint32_t max_path = GetMaxPathLengthForDirectory(save_dir); - if (GetSafePureFileName(save_dir, file_name_ext, max_path, &pure_file_name)) { - save_dir = save_dir.Append(pure_file_name + file_name_ext); - } else { - // Cannot create a shorter filename. This will cause the save as operation - // to fail unless the user pick a shorter name. Continuing even though it - // will fail because returning means no save as popup for the user, which - // is even more confusing. This case should be rare though. - save_dir = save_dir.Append(suggested_filename); + if (TruncateBaseNameToFitPathConstraints(save_dir, file_name_ext, max_path, + &base_name)) { + return save_dir.Append(base_name + file_name_ext); } - - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&SavePackage::ContinueGetSaveInfo, this, save_dir, - can_save_as_complete)); + // Cannot create a shorter filename. This will cause the save as operation + // to fail unless the user pick a shorter name. Continuing even though it + // will fail because returning means no save as popup for the user, which + // is even more confusing. This case should be rare though. + return save_dir.Append(suggested_filename); } -void SavePackage::ContinueGetSaveInfo(const base::FilePath& suggested_path, - bool can_save_as_complete) { +void SavePackage::ContinueGetSaveInfo(bool can_save_as_complete, + const base::FilePath& suggested_path) { DCHECK_CURRENTLY_ON(BrowserThread::UI); // The WebContents which owns this SavePackage may have disappeared during @@ -1460,6 +1431,7 @@ void SavePackage::OnPathPicked( const base::FilePath& final_name, SavePageType type, const SavePackageDownloadCreatedCallback& download_created_callback) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK((type == SAVE_PAGE_TYPE_AS_ONLY_HTML) || (type == SAVE_PAGE_TYPE_AS_MHTML) || (type == SAVE_PAGE_TYPE_AS_COMPLETE_HTML)) << type; @@ -1482,7 +1454,8 @@ void SavePackage::OnPathPicked( Init(download_created_callback); } -void SavePackage::StopObservation() { +void SavePackage::RemoveObservers() { + DCHECK_CURRENTLY_ON(BrowserThread::UI); DCHECK(download_); DCHECK(download_manager_); @@ -1492,14 +1465,14 @@ void SavePackage::StopObservation() { } void SavePackage::OnDownloadDestroyed(DownloadItem* download) { - StopObservation(); + RemoveObservers(); } void SavePackage::FinalizeDownloadEntry() { DCHECK(download_); download_manager_->OnSavePackageSuccessfullyFinished(download_); - StopObservation(); + RemoveObservers(); } } // namespace content diff --git a/chromium/content/browser/download/save_package.h b/chromium/content/browser/download/save_package.h index d4219814c88..edc54841c8e 100644 --- a/chromium/content/browser/download/save_package.h +++ b/chromium/content/browser/download/save_package.h @@ -88,14 +88,6 @@ class CONTENT_EXPORT SavePackage // in the "Save As" dialog box. explicit SavePackage(WebContents* web_contents); - // This contructor is used only for testing. We can bypass the file and - // directory name generation / sanitization by providing well known paths - // better suited for tests. - SavePackage(WebContents* web_contents, - SavePageType save_type, - const base::FilePath& file_full_path, - const base::FilePath& directory_full_path); - // Initialize the SavePackage. Returns true if it initializes properly. Need // to make sure that this method must be called in the UI thread because using // g_browser_process on a non-UI thread can cause crashes during shutdown. @@ -133,6 +125,34 @@ class CONTENT_EXPORT SavePackage private: friend class base::RefCountedThreadSafe<SavePackage>; + // Friends for testing. Needed for accessing the test-only constructor below. + friend class SavePackageTest; + friend class WebContentsImpl; + FRIEND_TEST_ALL_PREFIXES(SavePackageTest, TestSuggestedSaveNames); + FRIEND_TEST_ALL_PREFIXES(SavePackageTest, TestLongSafePureFilename); + FRIEND_TEST_ALL_PREFIXES(SavePackageBrowserTest, ImplicitCancel); + FRIEND_TEST_ALL_PREFIXES(SavePackageBrowserTest, ExplicitCancel); + + // Map from SaveItem::id() (aka save_item_id) into a SaveItem. + using SaveItemIdMap = + std::unordered_map<SaveItemId, SaveItem*, SaveItemId::Hasher>; + + using SaveItemQueue = std::deque<SaveItem*>; + + using FileNameSet = std::set<base::FilePath::StringType, + bool (*)(base::FilePath::StringPieceType, + base::FilePath::StringPieceType)>; + + using FileNameCountMap = + std::unordered_map<base::FilePath::StringType, uint32_t>; + + // Used only for testing. Bypasses the file and directory name generation / + // sanitization by providing well known paths better suited for tests. + SavePackage(WebContents* web_contents, + SavePageType save_type, + const base::FilePath& file_full_path, + const base::FilePath& directory_full_path); + void InitWithDownloadItem( const SavePackageDownloadCreatedCallback& download_created_callback, DownloadItemImpl* item); @@ -140,11 +160,6 @@ class CONTENT_EXPORT SavePackage // Callback for WebContents::GenerateMHTML(). void OnMHTMLGenerated(int64_t size); - // For testing only. - SavePackage(WebContents* web_contents, - const base::FilePath& file_full_path, - const base::FilePath& directory_full_path); - ~SavePackage() override; // Notes from Init() above applies here as well. @@ -173,7 +188,7 @@ class CONTENT_EXPORT SavePackage void FinalizeDownloadEntry(); // Detach from DownloadManager. - void StopObservation(); + void RemoveObservers(); // Return max length of a path for a specific base directory. // This is needed on POSIX, which restrict the length of file names in @@ -181,11 +196,27 @@ class CONTENT_EXPORT SavePackage // |base_dir| is assumed to be a directory name with no trailing slash. static uint32_t GetMaxPathLengthForDirectory(const base::FilePath& base_dir); - static bool GetSafePureFileName( - const base::FilePath& dir_path, - const base::FilePath::StringType& file_name_ext, - uint32_t max_file_path_len, - base::FilePath::StringType* pure_file_name); + // Truncates a filename to fit length constraints. + // + // |directory| : Directory containing target file. + // |extension| : Extension. + // |max_path_len| : Maximum size allowed for |len(directory + base_name + + // extension|. + // |base_name| : Variable portion. The length of this component will be + // adjusted to fit the length constraints described at + // |max_path_len| above. + // + // Returns true if |base_name| could be successfully adjusted to fit the + // aforementioned constraints, or false otherwise. + // TODO(asanka): This function is wrong. |base_name| cannot be truncated + // without knowing its encoding and truncation has to be performed on + // character boundaries. Also the implementation doesn't look up the actual + // path constraints and instead uses hard coded constants. crbug.com/618737 + static bool TruncateBaseNameToFitPathConstraints( + const base::FilePath& directory, + const base::FilePath::StringType& extension, + uint32_t max_path_len, + base::FilePath::StringType* base_name); // Create a file name based on the response from the server. bool GenerateFileName(const std::string& disposition, @@ -263,33 +294,26 @@ class CONTENT_EXPORT SavePackage bool end_of_data); // Look up SaveItem by save item id from in progress map. - SaveItem* LookupSaveItemInProcess(SaveItemId save_item_id); + SaveItem* LookupInProgressSaveItem(SaveItemId save_item_id); // Remove SaveItem from in progress map and put it to saved map. void PutInProgressItemToSavedMap(SaveItem* save_item); // Retrieves the URL to be saved from the WebContents. - GURL GetUrlToBeSaved(); - - void CreateDirectoryOnFileThread(const base::FilePath& website_save_dir, - const base::FilePath& download_save_dir, - bool skip_dir_check, - const std::string& mime_type); - void ContinueGetSaveInfo(const base::FilePath& suggested_path, - bool can_save_as_complete); + static GURL GetUrlToBeSaved(WebContents* web_contents); + + static base::FilePath CreateDirectoryOnFileThread( + const base::FilePath& website_save_dir, + const base::FilePath& download_save_dir, + const base::FilePath& suggested_filename, + bool skip_dir_check); + void ContinueGetSaveInfo(bool can_save_as_complete, + const base::FilePath& suggested_path); void OnPathPicked( const base::FilePath& final_name, SavePageType type, const SavePackageDownloadCreatedCallback& cb); - // Map from SaveItem::id() (aka save_item_id) into a SaveItem. - using SaveItemIdMap = - std::unordered_map<SaveItemId, SaveItem*, SaveItemId::Hasher>; - // Map of all saving job in in-progress state. - SaveItemIdMap in_progress_items_; - // Map of all saving job which are failed. - SaveItemIdMap saved_failed_items_; - // The number of in process SaveItems. int in_process_count() const { return static_cast<int>(in_progress_items_.size()); @@ -329,10 +353,15 @@ class CONTENT_EXPORT SavePackage static const base::FilePath::CharType* ExtensionForMimeType( const std::string& contents_mime_type); - using SaveItemQueue = std::deque<SaveItem*>; // A queue for items we are about to start saving. SaveItemQueue waiting_item_queue_; + // Map of all saving job in in-progress state. + SaveItemIdMap in_progress_items_; + + // Map of all saving job which are failed. + SaveItemIdMap saved_failed_items_; + // Used to de-dupe urls that are being gathered into |waiting_item_queue_| // and also to find SaveItems to associate with a containing frame. // Note that |url_to_save_item_| does NOT own SaveItems - they @@ -354,17 +383,17 @@ class CONTENT_EXPORT SavePackage frame_tree_node_id_to_contained_save_items_; // Number of frames that we still need to get a response from. - int number_of_frames_pending_response_; + int number_of_frames_pending_response_ = 0; // Map of all saving job which are successfully saved. SaveItemIdMap saved_success_items_; // Non-owning pointer for handling file writing on the FILE thread. - SaveFileManager* file_manager_; + SaveFileManager* file_manager_ = nullptr; // DownloadManager owns the DownloadItem and handles history and UI. - DownloadManagerImpl* download_manager_; - DownloadItemImpl* download_; + DownloadManagerImpl* download_manager_ = nullptr; + DownloadItemImpl* download_ = nullptr; // The URL of the page the user wants to save. GURL page_url_; @@ -375,51 +404,41 @@ class CONTENT_EXPORT SavePackage base::string16 title_; // Used to calculate package download speed (in files per second). - base::TimeTicks start_tick_; + const base::TimeTicks start_tick_; // Indicates whether the actual saving job is finishing or not. - bool finished_; + bool finished_ = false; // Indicates whether user canceled the saving job. - bool user_canceled_; + bool user_canceled_ = false; // Indicates whether user get disk error. - bool disk_error_occurred_; + bool disk_error_occurred_ = false; + + // Variables to record errors that happened so we can record them via + // UMA statistics. + bool wrote_to_completed_file_ = false; + bool wrote_to_failed_file_ = false; // Type about saving page as only-html or complete-html. - SavePageType save_type_; + SavePageType save_type_ = SAVE_PAGE_TYPE_UNKNOWN; // Number of all need to be saved resources. - size_t all_save_items_count_; + size_t all_save_items_count_ = 0; - using FileNameSet = - std::set<base::FilePath::StringType, - bool (*)(base::FilePath::StringPieceType, - base::FilePath::StringPieceType)>; // This set is used to eliminate duplicated file names in saving directory. FileNameSet file_name_set_; - using FileNameCountMap = - std::unordered_map<base::FilePath::StringType, uint32_t>; // This map is used to track serial number for specified filename. FileNameCountMap file_name_count_map_; // Indicates current waiting state when SavePackage try to get something // from outside. - WaitState wait_state_; + WaitState wait_state_ = INITIALIZE; // Unique ID for this SavePackage. const SavePackageId unique_id_; - // Variables to record errors that happened so we can record them via - // UMA statistics. - bool wrote_to_completed_file_; - bool wrote_to_failed_file_; - - friend class SavePackageTest; - FRIEND_TEST_ALL_PREFIXES(SavePackageTest, TestSuggestedSaveNames); - FRIEND_TEST_ALL_PREFIXES(SavePackageTest, TestLongSafePureFilename); - DISALLOW_COPY_AND_ASSIGN(SavePackage); }; diff --git a/chromium/content/browser/download/save_package_unittest.cc b/chromium/content/browser/download/save_package_unittest.cc index c426772a820..c75a28bf6c7 100644 --- a/chromium/content/browser/download/save_package_unittest.cc +++ b/chromium/content/browser/download/save_package_unittest.cc @@ -10,10 +10,12 @@ #include "base/files/file_path.h" #include "base/files/scoped_temp_dir.h" #include "base/macros.h" +#include "base/run_loop.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" #include "build/build_config.h" #include "content/browser/download/save_package.h" +#include "content/browser/loader/resource_dispatcher_host_impl.h" #include "content/public/common/url_constants.h" #include "content/test/test_render_view_host.h" #include "content/test/test_web_contents.h" @@ -42,13 +44,6 @@ const uint32_t kMaxFilePathLength = PATH_MAX - 1; const uint32_t kMaxFileNameLength = NAME_MAX; #endif -// Used to make long filenames. -std::string long_file_name( - "EFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz01234567" - "89ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz012345" - "6789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz0123" - "456789ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789a"); - bool HasOrdinalNumber(const base::FilePath::StringType& filename) { base::FilePath::StringType::size_type r_paren_index = filename.rfind(FPL(')')); @@ -94,7 +89,7 @@ class SavePackageTest : public RenderViewHostImplTestHarness { } GURL GetUrlToBeSaved() { - return save_package_success_->GetUrlToBeSaved(); + return SavePackage::GetUrlToBeSaved(save_package_success_->web_contents()); } protected: @@ -105,20 +100,43 @@ class SavePackageTest : public RenderViewHostImplTestHarness { // RenderViewHostImplTestHarness::SetUp. ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); - save_package_success_ = new SavePackage(contents(), - temp_dir_.path().AppendASCII("testfile" HTML_EXTENSION), - temp_dir_.path().AppendASCII("testfile_files")); + save_package_success_ = + new SavePackage(contents(), SAVE_PAGE_TYPE_AS_COMPLETE_HTML, + temp_dir_.path().AppendASCII("testfile" HTML_EXTENSION), + temp_dir_.path().AppendASCII("testfile_files")); + + base::FilePath::StringType long_file_name = GetLongFileName(); + save_package_fail_ = new SavePackage( + contents(), SAVE_PAGE_TYPE_AS_COMPLETE_HTML, + temp_dir_.path().Append(long_file_name + FPL_HTML_EXTENSION), + temp_dir_.path().Append(long_file_name + FPL("_files"))); + } + + BrowserContext* CreateBrowserContext() override { + // This method is invoked after the browser threads have been created and + // obviously before the BrowserContext is created. This is the correct time + // to create a ResourceDispatcherHostImpl so that our SavePackage objects + // can initialize correctly. + rdh_.reset(new ResourceDispatcherHostImpl); + return RenderViewHostImplTestHarness::CreateBrowserContext(); + } + + void TearDown() override { + DeleteContents(); + base::RunLoop().RunUntilIdle(); + + save_package_success_ = nullptr; + save_package_fail_ = nullptr; + rdh_.reset(); - // We need to construct a path that is *almost* kMaxFilePathLength long - long_file_name.reserve(kMaxFilePathLength + long_file_name.length()); - while (long_file_name.length() < kMaxFilePathLength) - long_file_name += long_file_name; - long_file_name.resize( - kMaxFilePathLength - 9 - temp_dir_.path().value().length()); + RenderViewHostImplTestHarness::TearDown(); + } - save_package_fail_ = new SavePackage(contents(), - temp_dir_.path().AppendASCII(long_file_name + HTML_EXTENSION), - temp_dir_.path().AppendASCII(long_file_name + "_files")); + // Returns a path that is *almost* kMaxFilePathLength long + base::FilePath::StringType GetLongFileName() const { + size_t target_length = + kMaxFilePathLength - 9 - temp_dir_.path().value().length(); + return base::FilePath::StringType(target_length, FPL('a')); } private: @@ -128,6 +146,8 @@ class SavePackageTest : public RenderViewHostImplTestHarness { scoped_refptr<SavePackage> save_package_fail_; base::ScopedTempDir temp_dir_; + + std::unique_ptr<ResourceDispatcherHostImpl> rdh_; }; static const struct { @@ -211,19 +231,21 @@ TEST_F(SavePackageTest, TestUnSuccessfullyGenerateSavePackageFilename) { #endif TEST_F(SavePackageTest, MAYBE_TestLongSavePackageFilename) { const std::string base_url("http://www.google.com/"); - const std::string long_file = long_file_name + ".css"; - const std::string url = base_url + long_file; + const base::FilePath::StringType long_file_name = + GetLongFileName() + FPL(".css"); + const std::string url = + base_url + base::FilePath(long_file_name).AsUTF8Unsafe(); base::FilePath::StringType filename; // Test that the filename is successfully shortened to fit. ASSERT_TRUE(GetGeneratedFilename(true, std::string(), url, false, &filename)); - EXPECT_TRUE(filename.length() < long_file.length()); + EXPECT_TRUE(filename.length() < long_file_name.length()); EXPECT_FALSE(HasOrdinalNumber(filename)); // Test that the filename is successfully shortened to fit, and gets an // an ordinal appended. ASSERT_TRUE(GetGeneratedFilename(true, std::string(), url, false, &filename)); - EXPECT_TRUE(filename.length() < long_file.length()); + EXPECT_TRUE(filename.length() < long_file_name.length()); EXPECT_TRUE(HasOrdinalNumber(filename)); // Test that the filename is successfully shortened to fit, and gets a @@ -231,7 +253,7 @@ TEST_F(SavePackageTest, MAYBE_TestLongSavePackageFilename) { base::FilePath::StringType filename2; ASSERT_TRUE( GetGeneratedFilename(true, std::string(), url, false, &filename2)); - EXPECT_TRUE(filename2.length() < long_file.length()); + EXPECT_TRUE(filename2.length() < long_file_name.length()); EXPECT_TRUE(HasOrdinalNumber(filename2)); EXPECT_NE(filename, filename2); } @@ -245,17 +267,12 @@ TEST_F(SavePackageTest, MAYBE_TestLongSavePackageFilename) { TEST_F(SavePackageTest, MAYBE_TestLongSafePureFilename) { const base::FilePath save_dir(FPL("test_dir")); const base::FilePath::StringType ext(FPL_HTML_EXTENSION); - base::FilePath::StringType filename = -#if defined(OS_WIN) - base::ASCIIToUTF16(long_file_name); -#else - long_file_name; -#endif + base::FilePath::StringType filename = GetLongFileName(); // Test that the filename + extension doesn't exceed kMaxFileNameLength uint32_t max_path = SavePackage::GetMaxPathLengthForDirectory(save_dir); - ASSERT_TRUE(SavePackage::GetSafePureFileName(save_dir, ext, max_path, - &filename)); + ASSERT_TRUE(SavePackage::TruncateBaseNameToFitPathConstraints( + save_dir, ext, max_path, &filename)); EXPECT_TRUE(filename.length() <= kMaxFileNameLength-ext.length()); } @@ -399,9 +416,10 @@ static const struct SuggestedSaveNameTestCase { #define MAYBE_TestSuggestedSaveNames TestSuggestedSaveNames #endif TEST_F(SavePackageTest, MAYBE_TestSuggestedSaveNames) { + GURL url = net::URLRequestMockHTTPJob::GetMockUrl("save_page/a.htm"); + NavigateAndCommit(url); for (size_t i = 0; i < arraysize(kSuggestedSaveNames); ++i) { - scoped_refptr<SavePackage> save_package( - new SavePackage(contents(), base::FilePath(), base::FilePath())); + scoped_refptr<SavePackage> save_package(new SavePackage(contents())); save_package->page_url_ = GURL(kSuggestedSaveNames[i].page_url); save_package->title_ = kSuggestedSaveNames[i].page_title; diff --git a/chromium/content/browser/download/save_types.h b/chromium/content/browser/download/save_types.h index 9d61a9a70a0..e8076d27540 100644 --- a/chromium/content/browser/download/save_types.h +++ b/chromium/content/browser/download/save_types.h @@ -7,8 +7,8 @@ #include <stdint.h> -#include <map> #include <string> +#include <unordered_map> #include <utility> #include <vector> @@ -25,7 +25,8 @@ class SaveItem; using SaveItemId = gpu::IdType32<SaveItem>; // Map from save_item_id into final file path. -using FinalNamesMap = std::map<SaveItemId, base::FilePath>; +using FinalNamesMap = + std::unordered_map<SaveItemId, base::FilePath, SaveItemId::Hasher>; // This structure is used to handle and deliver some info // when processing each save item job. @@ -38,9 +39,6 @@ struct SaveFileCreateInfo { // This type indicates the save item needs to be retrieved from serializing // DOM. SAVE_FILE_FROM_DOM, - // This type indicates the save item needs to be retrieved from local file - // system. - SAVE_FILE_FROM_FILE }; // Constructor for SAVE_FILE_FROM_DOM and/or SAVE_FILE_FROM_FILE. |