Commit 47a9f9bc authored by tzik's avatar tzik Committed by Commit bot

Fix and refactor downloaded file handling in the loading stack

In download_to_file mode of resource loading, the resource handler needs
to call ResourceDispatcherHostImpl::RegisterDownloadedTempFile when the
file is created, and needs to call UnregisterDownloadedTempFile after the
client handles the file. Otherwise, the file is deleted before the client
uses it.
However, MojoAsyncResourceHandler doesn't call them, and that causes
layout test failure around XHR with responseType = 'blob'. This CL fixes
that by adding the RDTF and UDTF pair, and refactors similar code in
AsyncResourceHandler to share the same lifetime management of the
downloaded file in ResourceDispatcherHostImpl.

BUG=603396, 659917

Review-Url: https://codereview.chromium.org/2503813002
Cr-Commit-Position: refs/heads/master@{#435551}
parent 79e61212
......@@ -752,6 +752,8 @@ source_set("browser") {
"loader/async_revalidation_manager.h",
"loader/detachable_resource_handler.cc",
"loader/detachable_resource_handler.h",
"loader/downloaded_temp_file_impl.cc",
"loader/downloaded_temp_file_impl.h",
"loader/global_routing_id.h",
"loader/intercepting_resource_handler.cc",
"loader/intercepting_resource_handler.h",
......
......@@ -11,6 +11,7 @@ specific_include_rules = {
"async_resource_handler\.(cc|h)": [
"-content",
"+content/browser/loader/async_resource_handler.h",
"+content/browser/loader/downloaded_temp_file_impl.h",
"+content/browser/loader/netlog_observer.h",
"+content/browser/loader/resource_buffer.h",
"+content/browser/loader/resource_dispatcher_host_impl.h",
......@@ -62,6 +63,13 @@ specific_include_rules = {
"+content/common/resource_messages.h",
"+content/common/resource_request.h",
],
"downloaded_temp_file_impl\.(cc|h)": [
"-content",
"+content/browser/loader/downloaded_temp_file_impl.h",
"+content/browser/loader/resource_dispatcher_host_impl.h",
"+content/common/content_export.h",
"+content/common/url_loader_factory.mojom.h"
],
"resource_buffer.*\.(cc|h)": [
"-content",
"+content/browser/loader/resource_buffer.h",
......@@ -79,6 +87,7 @@ specific_include_rules = {
"mojo_async_resource_handler\.(cc|h)": [
"-content",
"+content/browser/loader/async_resource_handler.h",
"+content/browser/loader/downloaded_temp_file_impl.h",
"+content/browser/loader/mojo_async_resource_handler.h",
"+content/browser/loader/test_url_loader_client.h",
"+content/browser/loader/netlog_observer.h",
......
// Copyright 2016 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "content/browser/loader/downloaded_temp_file_impl.h"
#include "content/browser/loader/resource_dispatcher_host_impl.h"
#include "mojo/public/cpp/bindings/strong_binding.h"
namespace content {
// static
mojo::InterfacePtr<mojom::DownloadedTempFile> DownloadedTempFileImpl::Create(
int child_id,
int request_id) {
mojo::InterfacePtr<mojom::DownloadedTempFile> ptr;
auto binding = mojo::MakeStrongBinding(
base::MakeUnique<DownloadedTempFileImpl>(child_id, request_id),
mojo::GetProxy(&ptr));
return ptr;
}
DownloadedTempFileImpl::~DownloadedTempFileImpl() {
ResourceDispatcherHostImpl::Get()->UnregisterDownloadedTempFile(child_id_,
request_id_);
}
DownloadedTempFileImpl::DownloadedTempFileImpl(int child_id, int request_id)
: child_id_(child_id), request_id_(request_id) {}
} // namespace content
// Copyright 2016 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CONTENT_BROWSER_LOADER_DOWNLOADED_TEMP_FILE_IMPL_H_
#define CONTENT_BROWSER_LOADER_DOWNLOADED_TEMP_FILE_IMPL_H_
#include "base/macros.h"
#include "content/common/content_export.h"
#include "content/common/url_loader_factory.mojom.h"
namespace content {
// DownloadedTempFileImpl is created on the download_to_file mode of resource
// loading. The instance is held by the client and lives until the client
// destroys the interface, slightly after the URLLoader is destroyed.
class CONTENT_EXPORT DownloadedTempFileImpl final
: public mojom::DownloadedTempFile {
public:
static mojo::InterfacePtr<mojom::DownloadedTempFile> Create(int child_id,
int request_id);
DownloadedTempFileImpl(int child_id, int request_id);
~DownloadedTempFileImpl() override;
private:
int child_id_;
int request_id_;
DISALLOW_COPY_AND_ASSIGN(DownloadedTempFileImpl);
};
} // namespace content
#endif // CONTENT_BROWSER_LOADER_DOWNLOADED_TEMP_FILE_IMPL_H_
......@@ -12,6 +12,7 @@
#include "base/macros.h"
#include "base/strings/string_number_conversions.h"
#include "base/time/time.h"
#include "content/browser/loader/downloaded_temp_file_impl.h"
#include "content/browser/loader/netlog_observer.h"
#include "content/browser/loader/resource_dispatcher_host_impl.h"
#include "content/browser/loader/resource_request_info_impl.h"
......@@ -167,7 +168,17 @@ bool MojoAsyncResourceHandler::OnResponseStarted(ResourceResponse* response,
response->head.request_start = request()->creation_time();
response->head.response_start = base::TimeTicks::Now();
sent_received_response_message_ = true;
url_loader_client_->OnReceiveResponse(response->head);
mojom::DownloadedTempFilePtr downloaded_file_ptr;
if (!response->head.download_file_path.empty()) {
downloaded_file_ptr = DownloadedTempFileImpl::Create(info->GetChildID(),
info->GetRequestID());
rdh_->RegisterDownloadedTempFile(info->GetChildID(), info->GetRequestID(),
response->head.download_file_path);
}
url_loader_client_->OnReceiveResponse(response->head,
std::move(downloaded_file_ptr));
return true;
}
......
......@@ -28,6 +28,7 @@
#include "content/browser/download/download_resource_handler.h"
#include "content/browser/frame_host/navigation_request_info.h"
#include "content/browser/loader/detachable_resource_handler.h"
#include "content/browser/loader/downloaded_temp_file_impl.h"
#include "content/browser/loader/navigation_resource_throttle.h"
#include "content/browser/loader/navigation_url_loader.h"
#include "content/browser/loader/resource_dispatcher_host_impl.h"
......@@ -3366,6 +3367,7 @@ TEST_P(ResourceDispatcherHostTest, RegisterDownloadedTempFile) {
// Create a temporary file.
base::FilePath file_path;
ASSERT_TRUE(base::CreateTemporaryFile(&file_path));
EXPECT_TRUE(base::PathExists(file_path));
scoped_refptr<ShareableFileReference> deletable_file =
ShareableFileReference::GetOrCreate(
file_path, ShareableFileReference::DELETE_ON_FINAL_RELEASE,
......@@ -3394,7 +3396,57 @@ TEST_P(ResourceDispatcherHostTest, RegisterDownloadedTempFile) {
// Release extra references and wait for the file to be deleted. (This relies
// on the delete happening on the FILE thread which is mapped to main thread
// in this test.)
deletable_file = NULL;
deletable_file = nullptr;
base::RunLoop().RunUntilIdle();
// The file is no longer readable to the child and has been deleted.
EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
filter_->child_id(), file_path));
EXPECT_FALSE(base::PathExists(file_path));
}
// Tests the dispatcher host's temporary file management in the mojo-enabled
// loading.
TEST_P(ResourceDispatcherHostTest, RegisterDownloadedTempFileWithMojo) {
const int kRequestID = 1;
// Create a temporary file.
base::FilePath file_path;
ASSERT_TRUE(base::CreateTemporaryFile(&file_path));
EXPECT_TRUE(base::PathExists(file_path));
scoped_refptr<ShareableFileReference> deletable_file =
ShareableFileReference::GetOrCreate(
file_path, ShareableFileReference::DELETE_ON_FINAL_RELEASE,
BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE).get());
// Not readable.
EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
filter_->child_id(), file_path));
// Register it for a resource request.
auto downloaded_file =
DownloadedTempFileImpl::Create(filter_->child_id(), kRequestID);
mojom::DownloadedTempFilePtr downloaded_file_ptr =
DownloadedTempFileImpl::Create(filter_->child_id(), kRequestID);
host_.RegisterDownloadedTempFile(filter_->child_id(), kRequestID, file_path);
// Should be readable now.
EXPECT_TRUE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
filter_->child_id(), file_path));
// The child releases from the request.
downloaded_file_ptr = nullptr;
base::RunLoop().RunUntilIdle();
// Still readable because there is another reference to the file. (The child
// may take additional blob references.)
EXPECT_TRUE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
filter_->child_id(), file_path));
// Release extra references and wait for the file to be deleted. (This relies
// on the delete happening on the FILE thread which is mapped to main thread
// in this test.)
deletable_file = nullptr;
base::RunLoop().RunUntilIdle();
// The file is no longer readable to the child and has been deleted.
......
......@@ -14,7 +14,8 @@ TestURLLoaderClient::TestURLLoaderClient() : binding_(this) {}
TestURLLoaderClient::~TestURLLoaderClient() {}
void TestURLLoaderClient::OnReceiveResponse(
const ResourceResponseHead& response_head) {
const ResourceResponseHead& response_head,
mojom::DownloadedTempFilePtr downloaded_file) {
has_received_response_ = true;
response_head_ = response_head;
if (quit_closure_for_on_receive_response_)
......
......@@ -28,7 +28,8 @@ class TestURLLoaderClient final : public mojom::URLLoaderClient {
TestURLLoaderClient();
~TestURLLoaderClient() override;
void OnReceiveResponse(const ResourceResponseHead& response_head) override;
void OnReceiveResponse(const ResourceResponseHead& response_head,
mojom::DownloadedTempFilePtr downloaded_file) override;
void OnReceiveRedirect(const net::RedirectInfo& redirect_info,
const ResourceResponseHead& response_head) override;
void OnDataDownloaded(int64_t data_length, int64_t encoded_length) override;
......
......@@ -81,8 +81,10 @@ class DelegatingURLLoaderClient final : public mojom::URLLoaderClient {
void OnDataDownloaded(int64_t data_length, int64_t encoded_length) override {
client_->OnDataDownloaded(data_length, encoded_length);
}
void OnReceiveResponse(const ResourceResponseHead& head) override {
client_->OnReceiveResponse(head);
void OnReceiveResponse(
const ResourceResponseHead& head,
mojom::DownloadedTempFilePtr downloaded_file) override {
client_->OnReceiveResponse(head, std::move(downloaded_file));
}
void OnReceiveRedirect(const net::RedirectInfo& redirect_info,
const ResourceResponseHead& head) override {
......
......@@ -90,10 +90,13 @@ class URLLoaderClientImpl final : public mojom::URLLoaderClient {
body_consumer_->Cancel();
}
void OnReceiveResponse(const ResourceResponseHead& response_head) override {
void OnReceiveResponse(
const ResourceResponseHead& response_head,
mojom::DownloadedTempFilePtr downloaded_file) override {
has_received_response_ = true;
if (body_consumer_)
body_consumer_->Start(task_runner_.get());
downloaded_file_ = std::move(downloaded_file);
resource_dispatcher_->OnMessageReceived(
ResourceMsg_ReceivedResponse(request_id_, response_head));
}
......@@ -137,6 +140,7 @@ class URLLoaderClientImpl final : public mojom::URLLoaderClient {
private:
mojo::AssociatedBinding<mojom::URLLoaderClient> binding_;
scoped_refptr<URLResponseBodyConsumer> body_consumer_;
mojom::DownloadedTempFilePtr downloaded_file_;
const int request_id_;
bool has_received_response_ = false;
ResourceDispatcher* const resource_dispatcher_;
......
......@@ -24,9 +24,18 @@ interface URLLoader {
FollowRedirect();
};
// Opaque handle passed from the browser process to a child process to manage
// the lifetime of temporary files used for download_to_file resource loading.
// When the message pipe for this interface is closed, the browser process will
// clean up the corresponding temporary file.
interface DownloadedTempFile {
};
interface URLLoaderClient {
// Called when the response head is received.
OnReceiveResponse(URLResponseHead head);
// Called when the response head is received. |downloaded_file| is non-null in
// the 'download_to_file' case.
OnReceiveResponse(URLResponseHead head, DownloadedTempFile? downloaded_file);
// Called when the request has been redirected. The receiver is expected to
// call FollowRedirect or cancel the request.
......
......@@ -247,8 +247,11 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
"respondWith or waitUntil.")));
}
void OnReceiveResponse(const ResourceResponseHead& response_head) override {
void OnReceiveResponse(
const ResourceResponseHead& response_head,
mojom::DownloadedTempFilePtr downloaded_file) override {
DCHECK(!response_);
DCHECK(!downloaded_file);
response_ = base::MakeUnique<blink::WebServiceWorkerResponse>();
response_->setURL(url_);
DCHECK(response_head.headers);
......
......@@ -1563,20 +1563,11 @@ crbug.com/602693 imported/wpt/service-workers/service-worker/referer.https.html
# mojo-loading: This is an experimental feature. failing virtual tests are
# listed below.
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/blob-response-type-warm-cache.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/cross-origin-preflight-get-response-type-blob.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/response-document.html [ Failure ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/response-text.html [ Failure ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/upload-onload-event.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/upload-onloadend-event-after-load.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/upload-onprogress-event.html [ Failure ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/upload-progress-events.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob-sync.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/workers/shared-worker-response-type-blob.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/workers/upload-onprogress-event.html [ Failure ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob-sync.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/workers/xmlhttprequest-response-type-blob.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/xmlhttprequest-response-type-blob.html [ Failure Timeout ]
crbug.com/659917 virtual/mojo-loading/http/tests/xmlhttprequest/upload-onloadend-event-after-abort.html [ Timeout ]
# TODO(yhirano): There is an entry for the base test, but this test is flakier
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment