Commit d259d8fc authored by Daniel Murphy's avatar Daniel Murphy Committed by Commit Bot

[BlobStorage] Adding WriteBlobToFile to the BlobStorageContext mojo API

This change adds the API method WriteBlobToFile to
blob_storage_context.mojom and adds test for that method. It also adds
some basic tests to the mojo version of BlobStorageContext.

This call tries to optimize the file copy in the case of the blob being
a single file. In that case, it will do a base::CopyFile call, which is
optimized on Windows and should be faster. Otherwise, the API uses the
blob.mojom ReadAll API and stream the data to a file.

R=mek@chromium.org
TBR=ksakamoto@chromium.org

Bug: 1024966, 1036415
Change-Id: I5bf49c39780bd8e25c2ad0f68bb8de7979925284
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1974700Reviewed-by: default avatarChris Palmer <palmer@chromium.org>
Reviewed-by: default avatarMarijn Kruisselbrink <mek@chromium.org>
Reviewed-by: default avatarenne <enne@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731694}
parent 5f1264ad
......@@ -93,12 +93,12 @@ ChromeBlobStorageContext* ChromeBlobStorageContext::GetFor(
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (!context->GetUserData(kBlobStorageContextKeyName)) {
scoped_refptr<ChromeBlobStorageContext> blob =
scoped_refptr<ChromeBlobStorageContext> blob_storage_context =
new ChromeBlobStorageContext();
context->SetUserData(
kBlobStorageContextKeyName,
std::make_unique<UserDataAdapter<ChromeBlobStorageContext>>(
blob.get()));
blob_storage_context.get()));
// Check first to avoid memory leak in unittests.
bool io_thread_valid =
......@@ -131,7 +131,8 @@ ChromeBlobStorageContext* ChromeBlobStorageContext::GetFor(
if (io_thread_valid) {
base::PostTask(
FROM_HERE, {BrowserThread::IO},
base::BindOnce(&ChromeBlobStorageContext::InitializeOnIOThread, blob,
base::BindOnce(&ChromeBlobStorageContext::InitializeOnIOThread,
blob_storage_context, context->GetPath(),
std::move(blob_storage_dir),
std::move(file_task_runner)));
}
......@@ -161,10 +162,11 @@ ChromeBlobStorageContext::GetRemoteFor(BrowserContext* browser_context) {
}
void ChromeBlobStorageContext::InitializeOnIOThread(
FilePath blob_storage_dir,
const FilePath& profile_dir,
const FilePath& blob_storage_dir,
scoped_refptr<base::TaskRunner> file_task_runner) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
context_.reset(new BlobStorageContext(std::move(blob_storage_dir),
context_.reset(new BlobStorageContext(profile_dir, blob_storage_dir,
std::move(file_task_runner)));
// Signal the BlobMemoryController when it's appropriate to calculate its
// storage limits.
......
......@@ -57,7 +57,8 @@ class CONTENT_EXPORT ChromeBlobStorageContext
static mojo::PendingRemote<storage::mojom::BlobStorageContext> GetRemoteFor(
BrowserContext* browser_context);
void InitializeOnIOThread(base::FilePath blob_storage_dir,
void InitializeOnIOThread(const base::FilePath& profile_dir,
const base::FilePath& blob_storage_dir,
scoped_refptr<base::TaskRunner> file_task_runner);
storage::BlobStorageContext* context() const;
......
......@@ -55,7 +55,8 @@ class NativeFileSystemFileHandleImplTest : public testing::Test {
test_file_url_));
chrome_blob_context_ = base::MakeRefCounted<ChromeBlobStorageContext>();
chrome_blob_context_->InitializeOnIOThread(base::FilePath(), nullptr);
chrome_blob_context_->InitializeOnIOThread(base::FilePath(),
base::FilePath(), nullptr);
manager_ = base::MakeRefCounted<NativeFileSystemManagerImpl>(
file_system_context_, chrome_blob_context_,
......
......@@ -89,7 +89,8 @@ class NativeFileSystemFileWriterImplTest : public testing::Test {
test_swap_url_));
chrome_blob_context_ = base::MakeRefCounted<ChromeBlobStorageContext>();
chrome_blob_context_->InitializeOnIOThread(base::FilePath(), nullptr);
chrome_blob_context_->InitializeOnIOThread(base::FilePath(),
base::FilePath(), nullptr);
blob_context_ = chrome_blob_context_->context();
manager_ = base::MakeRefCounted<NativeFileSystemManagerImpl>(
......
......@@ -55,7 +55,8 @@ class NativeFileSystemHandleBaseTest : public testing::Test {
/*quota_manager_proxy=*/nullptr, dir_.GetPath());
chrome_blob_context_ = base::MakeRefCounted<ChromeBlobStorageContext>();
chrome_blob_context_->InitializeOnIOThread(base::FilePath(), nullptr);
chrome_blob_context_->InitializeOnIOThread(base::FilePath(),
base::FilePath(), nullptr);
manager_ = base::MakeRefCounted<NativeFileSystemManagerImpl>(
file_system_context_, chrome_blob_context_,
......
......@@ -42,7 +42,8 @@ class NativeFileSystemManagerImplTest : public testing::Test {
/*quota_manager_proxy=*/nullptr, dir_.GetPath());
chrome_blob_context_ = base::MakeRefCounted<ChromeBlobStorageContext>();
chrome_blob_context_->InitializeOnIOThread(base::FilePath(), nullptr);
chrome_blob_context_->InitializeOnIOThread(base::FilePath(),
base::FilePath(), nullptr);
manager_ = base::MakeRefCounted<NativeFileSystemManagerImpl>(
file_system_context_, chrome_blob_context_, &permission_context_,
......
......@@ -31,7 +31,7 @@ class WebBundleBlobDataSourceTest : public testing::Test {
void SetUp() override {
ASSERT_TRUE(data_dir_.CreateUniqueTempDir());
context_ = std::make_unique<storage::BlobStorageContext>(
data_dir_.GetPath(),
data_dir_.GetPath(), data_dir_.GetPath(),
base::CreateTaskRunner({base::ThreadPool(), base::MayBlock()}));
storage::BlobStorageLimits limits;
limits.max_ipc_memory_size = kTestBlobStorageMaxBytesDataItemSize;
......
......@@ -70,6 +70,8 @@ jumbo_component("browser") {
"blob/shareable_file_reference.h",
"blob/view_blob_internals_job.cc",
"blob/view_blob_internals_job.h",
"blob/write_blob_to_file.cc",
"blob/write_blob_to_file.h",
"database/database_quota_client.cc",
"database/database_quota_client.h",
"database/database_tracker.cc",
......@@ -273,6 +275,7 @@ source_set("unittests") {
"blob/blob_reader_unittest.cc",
"blob/blob_registry_impl_unittest.cc",
"blob/blob_slice_unittest.cc",
"blob/blob_storage_context_mojo_unittest.cc",
"blob/blob_storage_context_unittest.cc",
"blob/blob_storage_registry_unittest.cc",
"blob/blob_transport_strategy_unittest.cc",
......
......@@ -52,7 +52,7 @@ class BlobBuilderFromStreamTestWithDelayedLimits
void SetUp() override {
ASSERT_TRUE(data_dir_.CreateUniqueTempDir());
context_ = std::make_unique<BlobStorageContext>(
data_dir_.GetPath(),
data_dir_.GetPath(), data_dir_.GetPath(),
base::CreateTaskRunner({base::ThreadPool(), base::MayBlock()}));
limits_.max_ipc_memory_size = kTestBlobStorageMaxBytesDataItemSize;
......
......@@ -179,8 +179,8 @@ TEST_F(BlobFlattenerTest, BlobWithSlices) {
// * full data blob,
// * pending data,
context_ =
std::make_unique<BlobStorageContext>(temp_dir_.GetPath(), file_runner_);
context_ = std::make_unique<BlobStorageContext>(
temp_dir_.GetPath(), temp_dir_.GetPath(), file_runner_);
SetTestMemoryLimits();
std::unique_ptr<BlobDataHandle> data_blob;
......
......@@ -62,7 +62,7 @@ class BlobRegistryImplTest : public testing::Test {
void SetUp() override {
ASSERT_TRUE(data_dir_.CreateUniqueTempDir());
context_ = std::make_unique<BlobStorageContext>(
data_dir_.GetPath(),
data_dir_.GetPath(), data_dir_.GetPath(),
base::CreateTaskRunner({base::ThreadPool(), base::MayBlock()}));
auto storage_policy =
base::MakeRefCounted<content::MockSpecialStoragePolicy>();
......
......@@ -32,6 +32,7 @@
#include "storage/browser/blob/blob_data_snapshot.h"
#include "storage/browser/blob/blob_impl.h"
#include "storage/browser/blob/shareable_blob_data_item.h"
#include "storage/browser/blob/write_blob_to_file.h"
#include "third_party/blink/public/common/blob/blob_utils.h"
#include "third_party/blink/public/mojom/blob/data_element.mojom.h"
#include "url/gurl.h"
......@@ -43,15 +44,18 @@ using QuotaAllocationTask = BlobMemoryController::QuotaAllocationTask;
} // namespace
BlobStorageContext::BlobStorageContext()
: memory_controller_(base::FilePath(), scoped_refptr<base::TaskRunner>()) {
: profile_directory_(base::FilePath()),
memory_controller_(base::FilePath(), scoped_refptr<base::TaskRunner>()) {
base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
this, "BlobStorageContext", base::ThreadTaskRunnerHandle::Get());
}
BlobStorageContext::BlobStorageContext(
base::FilePath storage_directory,
const base::FilePath& profile_directory,
const base::FilePath& storage_directory,
scoped_refptr<base::TaskRunner> file_runner)
: memory_controller_(std::move(storage_directory), std::move(file_runner)) {
: profile_directory_(profile_directory),
memory_controller_(storage_directory, std::move(file_runner)) {
base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
this, "BlobStorageContext", base::ThreadTaskRunnerHandle::Get());
}
......@@ -734,4 +738,45 @@ void BlobStorageContext::RegisterFromMemory(
BlobImpl::Create(std::move(handle), std::move(blob));
}
void BlobStorageContext::WriteBlobToFile(
mojo::PendingRemote<::blink::mojom::Blob> pending_blob,
const base::FilePath& file_path,
bool flush_on_write,
base::Optional<base::Time> last_modified,
BlobStorageContext::WriteBlobToFileCallback callback) {
DCHECK(!last_modified || !last_modified.value().is_null());
if (profile_directory_.empty()) {
std::move(callback).Run(mojom::WriteBlobToFileResult::kBadPath);
return;
}
if (file_path.ReferencesParent()) {
std::move(callback).Run(mojom::WriteBlobToFileResult::kBadPath);
return;
}
if (!profile_directory_.IsParent(file_path)) {
std::move(callback).Run(mojom::WriteBlobToFileResult::kBadPath);
return;
}
GetBlobDataFromBlobRemote(
std::move(pending_blob),
base::BindOnce(
[](base::WeakPtr<BlobStorageContext> blob_context,
const base::FilePath& file_path, bool flush_on_write,
base::Optional<base::Time> last_modified,
BlobStorageContext::WriteBlobToFileCallback callback,
std::unique_ptr<BlobDataHandle> handle) {
if (!handle || !blob_context) {
std::move(callback).Run(
mojom::WriteBlobToFileResult::kInvalidBlob);
return;
}
::storage::WriteBlobToFile(std::move(handle), file_path,
flush_on_write, last_modified,
std::move(callback));
},
AsWeakPtr(), file_path, flush_on_write, last_modified,
std::move(callback)));
}
} // namespace storage
......@@ -15,6 +15,7 @@
#include "base/callback_forward.h"
#include "base/component_export.h"
#include "base/files/file_path.h"
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
......@@ -58,7 +59,8 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) BlobStorageContext
// Initializes the context without disk support.
BlobStorageContext();
// Disk support is enabled if |file_runner| isn't null.
BlobStorageContext(base::FilePath storage_directory,
BlobStorageContext(const base::FilePath& profile_directory,
const base::FilePath& blob_storage_directory,
scoped_refptr<base::TaskRunner> file_runner);
~BlobStorageContext() override;
......@@ -249,7 +251,13 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) BlobStorageContext
void RegisterFromMemory(mojo::PendingReceiver<::blink::mojom::Blob> blob,
const std::string& uuid,
mojo_base::BigBuffer data) override;
void WriteBlobToFile(mojo::PendingRemote<::blink::mojom::Blob> blob,
const base::FilePath& path,
bool flush_on_write,
base::Optional<base::Time> last_modified,
WriteBlobToFileCallback callback) override;
base::FilePath profile_directory_;
BlobStorageRegistry registry_;
BlobMemoryController memory_controller_;
mojo::ReceiverSet<mojom::BlobStorageContext> receivers_;
......
This diff is collapsed.
......@@ -20,6 +20,7 @@
#include "base/strings/string_number_conversions.h"
#include "base/test/task_environment.h"
#include "base/test/test_simple_task_runner.h"
#include "base/threading/thread_restrictions.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "net/base/io_buffer.h"
......@@ -76,12 +77,14 @@ class BlobStorageContextTest : public testing::Test {
void SetUp() override {
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
base::ThreadRestrictions::SetIOAllowed(false);
context_ = std::make_unique<BlobStorageContext>();
}
void TearDown() override {
base::RunLoop().RunUntilIdle();
file_runner_->RunPendingTasks();
RunFileTasks();
base::ThreadRestrictions::SetIOAllowed(true);
ASSERT_TRUE(temp_dir_.Delete());
}
......@@ -126,6 +129,11 @@ class BlobStorageContextTest : public testing::Test {
return received_uuid;
}
void RunFileTasks() {
base::ScopedAllowBlockingForTesting allow_blocking;
file_runner_->RunPendingTasks();
}
std::vector<FileCreationInfo> files_;
base::ScopedTempDir temp_dir_;
scoped_refptr<TestSimpleTaskRunner> file_runner_ = new TestSimpleTaskRunner();
......@@ -500,8 +508,8 @@ TEST_F(BlobStorageContextTest, BuildReadableDataHandleBlob) {
TEST_F(BlobStorageContextTest, BuildFutureFileOnlyBlob) {
const std::string kId1("id1");
context_ =
std::make_unique<BlobStorageContext>(temp_dir_.GetPath(), file_runner_);
context_ = std::make_unique<BlobStorageContext>(
temp_dir_.GetPath(), temp_dir_.GetPath(), file_runner_);
SetTestMemoryLimits();
auto builder = std::make_unique<BlobDataBuilder>(kId1);
......@@ -521,7 +529,7 @@ TEST_F(BlobStorageContextTest, BuildFutureFileOnlyBlob) {
EXPECT_EQ(0u, blobs_finished);
EXPECT_TRUE(file_runner_->HasPendingTask());
file_runner_->RunPendingTasks();
RunFileTasks();
EXPECT_EQ(0u, blobs_finished);
EXPECT_EQ(BlobStatus::ERR_INVALID_CONSTRUCTION_ARGUMENTS, status);
EXPECT_EQ(BlobStatus::PENDING_QUOTA, handle->GetBlobStatus());
......@@ -546,7 +554,7 @@ TEST_F(BlobStorageContextTest, BuildFutureFileOnlyBlob) {
base::RunLoop().RunUntilIdle();
// We should have file cleanup tasks.
EXPECT_TRUE(file_runner_->HasPendingTask());
file_runner_->RunPendingTasks();
RunFileTasks();
base::RunLoop().RunUntilIdle();
EXPECT_EQ(0lu, context_->memory_controller().memory_usage());
EXPECT_EQ(0lu, context_->memory_controller().disk_usage());
......@@ -770,8 +778,8 @@ void PopulateDataInBuilder(
TEST_F(BlobStorageContextTest, BuildBlobCombinations) {
const std::string kId("id");
context_ =
std::make_unique<BlobStorageContext>(temp_dir_.GetPath(), file_runner_);
context_ = std::make_unique<BlobStorageContext>(
temp_dir_.GetPath(), temp_dir_.GetPath(), file_runner_);
SetTestMemoryLimits();
auto data_handle = base::MakeRefCounted<storage::FakeBlobDataHandle>(
......@@ -837,7 +845,7 @@ TEST_F(BlobStorageContextTest, BuildBlobCombinations) {
// We should be needing to send a page or two to disk.
EXPECT_TRUE(file_runner_->HasPendingTask());
do {
file_runner_->RunPendingTasks();
RunFileTasks();
base::RunLoop().RunUntilIdle();
// Continue populating data for items that can fit.
for (size_t i = 0; i < kTotalRawBlobs; i++) {
......@@ -871,7 +879,7 @@ TEST_F(BlobStorageContextTest, BuildBlobCombinations) {
files_.clear();
// We should have file cleanup tasks.
EXPECT_TRUE(file_runner_->HasPendingTask());
file_runner_->RunPendingTasks();
RunFileTasks();
base::RunLoop().RunUntilIdle();
EXPECT_EQ(0lu, context_->memory_controller().memory_usage());
EXPECT_EQ(0lu, context_->memory_controller().disk_usage());
......
......@@ -5,6 +5,8 @@
module storage.mojom;
import "mojo/public/mojom/base/big_buffer.mojom";
import "mojo/public/mojom/base/file_path.mojom";
import "mojo/public/mojom/base/time.mojom";
import "third_party/blink/public/mojom/blob/blob.mojom";
// A reader for the data and side data in a cache storage entry.
......@@ -44,10 +46,23 @@ struct BlobDataItem {
pending_remote<BlobDataItemReader> reader;
};
// The result of writing a blob to disk.
enum WriteBlobToFileResult {
kError, // There was an error writing the blob to a file.
kBadPath, // The path given is not accessible or has references.
kInvalidBlob, // The blob is invalid and cannot be read.
kIOError, // Error writing bytes on disk.
kTimestampError, // Error writing the last modified timestamp.
kSuccess,
};
// This interface is the primary access point to the browser's blob system
// for chrome internals. This is a simplified version of the
// blink.mojom.BlobRegistry interface. To avoid giving the renderer
// different capabilities, this is a separate interface.
// for chrome internals. This interface lives in the browser process. This is a
// simplified version of the blink.mojom.BlobRegistry interface.
//
// This interface has enhanced capabilities that should NOT be accessible to a
// renderer, which is why it is a separate interface. For example,
// WriteBlobToFile writes a blob to an arbitrary file path.
interface BlobStorageContext {
// Create a blob with a particular uuid and consisting of a single
// BlobDataItem::DataHandle constructed from |item|.
......@@ -57,4 +72,15 @@ interface BlobStorageContext {
// in |data|.
RegisterFromMemory(pending_receiver<blink.mojom.Blob> blob, string uuid,
mojo_base.mojom.BigBuffer data);
// Writes the given blob to the given file path. If the given |path| is not
// under the blob storage context's profile directory or if it has references
// (like "..") then the implementation returns kBadPath. If a file already
// exists at |path| then it is overwritten. If |flush_on_write| is true, then
// Flush will be called on the new file before it is closed.
WriteBlobToFile(pending_remote<blink.mojom.Blob> blob,
mojo_base.mojom.FilePath path,
bool flush_on_write,
mojo_base.mojom.Time? last_modified)
=> (WriteBlobToFileResult result);
};
This diff is collapsed.
// Copyright 2019 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 STORAGE_BROWSER_BLOB_WRITE_BLOB_TO_FILE_H_
#define STORAGE_BROWSER_BLOB_WRITE_BLOB_TO_FILE_H_
#include "base/files/file_path.h"
#include "base/optional.h"
#include "base/time/time.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "storage/browser/blob/blob_data_handle.h"
#include "storage/browser/blob/blob_entry.h"
#include "storage/browser/blob/mojom/blob_storage_context.mojom.h"
namespace storage {
// Writes the blob at |blob_handle| to the file at |file_path|. If a file
// already exists, then it is overwritten. If |flush_on_write| is true, then the
// Flush will be called on the file before it is closed. If |last_modified| is
// populated, then the file's last modified & last accessed time will be set to
// |last_modified|.
// If successful, |callback| is called with the resulting file size. If not,
// then a net error code is used ( < 0).
void WriteBlobToFile(
std::unique_ptr<BlobDataHandle> blob_handle,
const base::FilePath& file_path,
bool flush_on_write,
base::Optional<base::Time> last_modified,
mojom::BlobStorageContext::WriteBlobToFileCallback callback);
} // namespace storage
#endif // STORAGE_BROWSER_BLOB_WRITE_BLOB_TO_FILE_H_
......@@ -31,7 +31,11 @@ namespace storage {
// A generic interface for writing to a file-like object.
class FileStreamWriter {
public:
enum OpenOrCreate { OPEN_EXISTING_FILE, CREATE_NEW_FILE };
enum OpenOrCreate {
OPEN_EXISTING_FILE,
CREATE_NEW_FILE,
CREATE_NEW_FILE_ALWAYS
};
// Creates a writer for the existing file in the path |file_path| starting
// from |initial_offset|. Uses |task_runner| for async file operations.
......@@ -44,6 +48,8 @@ class FileStreamWriter {
// Creates a writer for the existing memory file in the path |file_path|
// starting from |initial_offset|.
// TODO(mek): Remove or use |open_or_create| field here, as it is not
// currently used. https://crbug.com/1041048
COMPONENT_EXPORT(STORAGE_BROWSER)
static std::unique_ptr<FileStreamWriter> CreateForMemoryFile(
base::WeakPtr<ObfuscatedFileUtilMemoryDelegate> memory_file_util,
......
......@@ -20,6 +20,9 @@ const int kOpenFlagsForWrite =
base::File::FLAG_OPEN | base::File::FLAG_WRITE | base::File::FLAG_ASYNC;
const int kCreateFlagsForWrite =
base::File::FLAG_CREATE | base::File::FLAG_WRITE | base::File::FLAG_ASYNC;
const int kCreateFlagsForWriteAlways = base::File::FLAG_CREATE_ALWAYS |
base::File::FLAG_WRITE |
base::File::FLAG_ASYNC;
} // namespace
......@@ -110,6 +113,9 @@ int LocalFileStreamWriter::InitiateOpen(base::OnceClosure main_operation) {
case CREATE_NEW_FILE:
open_flags = kCreateFlagsForWrite;
break;
case CREATE_NEW_FILE_ALWAYS:
open_flags = kCreateFlagsForWriteAlways;
break;
}
return stream_impl_->Open(
......
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