Skip to content

Commit

Permalink
FileSystem: Remove DeleteDirectoryForStorageKeyAndType
Browse files Browse the repository at this point in the history 
This change removes DeleteDirectoryForStorageKeyAndType
which makes sync calls to retrieve the BucketLocator
and updates its callers to do async calls to retrieve
the BucketLocator first, then call
DeleteDirectoryForBucketAndType instead. This should reduce
sync calls to retrieve BucketLocator as well as remove
duplicated code.

Bug: 1444138
Change-Id: I7d3434e25e756988888c305938a51eefba547db7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4702092
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1177309}
  • Loading branch information
@ayuishii
ayuishii authored and Chromium LUCI CQ committed Jul 31, 2023
1 parent 03910c7 commit c1773c8
Show file tree
Hide file tree
Showing 10 changed files with 78 additions and 355 deletions.
1 change: 1 addition & 0 deletions components/browsing_data/content/file_system_helper_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ TEST_F(FileSystemHelperTest, DeleteData) {

helper_->DeleteFileSystemOrigin(origin1);
helper_->DeleteFileSystemOrigin(origin2);
content::RunAllTasksUntilIdle();

FetchFileSystems();

Expand Down
79 changes: 56 additions & 23 deletions storage/browser/file_system/file_system_context.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,39 +271,54 @@ void FileSystemContext::Initialize() {
base::RetainedRef(this), std::move(quota_client_receiver)));
}

bool FileSystemContext::DeleteDataForStorageKeyOnFileTaskRunner(
void FileSystemContext::DeleteDataForStorageKeyOnFileTaskRunner(
const blink::StorageKey& storage_key) {
DCHECK(default_file_task_runner()->RunsTasksInCurrentSequence());
DCHECK(!storage_key.origin().opaque());

bool success = true;
bool delete_default_cache = false;
// Different FileSystemTypes may map to the same BucketLocator. Retrieve the
// bucket once for those FileSystemTypes.
base::flat_map<blink::mojom::StorageType, std::vector<FileSystemType>>
quota_to_fs_type_map;
for (auto& type_backend_pair : backend_map_) {
FileSystemBackend* backend = type_backend_pair.second;
if (!backend->GetQuotaUtil())
if (!type_backend_pair.second->GetQuotaUtil()) {
continue;
if (backend->GetQuotaUtil()->DeleteStorageKeyDataOnFileTaskRunner(
this, quota_manager_proxy().get(), storage_key,
type_backend_pair.first) != base::File::FILE_OK) {
// Continue the loop, but record the failure.
success = false;
}
auto quota_type = FileSystemTypeToQuotaStorageType(type_backend_pair.first);
quota_to_fs_type_map[quota_type].push_back(type_backend_pair.first);
}

if (FileSystemTypeToQuotaStorageType(type_backend_pair.first) ==
blink::mojom::StorageType::kTemporary) {
delete_default_cache = true;
}
for (auto& type_pair : quota_to_fs_type_map) {
quota_manager_proxy()->GetOrCreateBucketDeprecated(
BucketInitParams::ForDefaultBucket(storage_key), type_pair.first,
default_file_task_runner_.get(),
base::BindOnce(&FileSystemContext::OnGetBucketForStorageKeyDeletion,
weak_factory_.GetWeakPtr(), type_pair.second));
}
}

void FileSystemContext::OnGetBucketForStorageKeyDeletion(
std::vector<FileSystemType> types,
QuotaErrorOr<BucketInfo> result) {
if (!result.has_value()) {
return;
}

auto bucket = result->ToBucketLocator();
for (auto& type : types) {
FileSystemBackend* backend = GetFileSystemBackend(type);
backend->GetQuotaUtil()->DeleteBucketDataOnFileTaskRunner(
this, quota_manager_proxy().get(), bucket, type);
}

// Trigger cache deletion for the default bucket once. This is done after
// `storage_key` data deletion so deletion doesn't trigger twice for
// kFileSystemTypeTemporary and kFileSystemTypePersistent.
if (delete_default_cache) {
if (auto* quota_util = GetQuotaUtil(kFileSystemTypeTemporary))
quota_util->DeleteCachedDefaultBucket(storage_key);
if (bucket.type == blink::mojom::StorageType::kTemporary) {
if (auto* quota_util = GetQuotaUtil(kFileSystemTypeTemporary)) {
quota_util->DeleteCachedDefaultBucket(bucket.storage_key);
}
}

return success;
}

scoped_refptr<QuotaReservation>
Expand Down Expand Up @@ -582,13 +597,31 @@ void FileSystemContext::DeleteFileSystem(const blink::StorageKey& storage_key,
return;
}

quota_manager_proxy()->GetOrCreateBucketDeprecated(
BucketInitParams::ForDefaultBucket(storage_key),
FileSystemTypeToQuotaStorageType(type), io_task_runner_.get(),
base::BindOnce(&FileSystemContext::OnGetBucketForDeleteFileSystem,
weak_factory_.GetWeakPtr(), type, std::move(callback)));
}

void FileSystemContext::OnGetBucketForDeleteFileSystem(
FileSystemType type,
StatusCallback callback,
QuotaErrorOr<BucketInfo> result) {
if (!result.has_value()) {
std::move(callback).Run(base::File::FILE_ERROR_FAILED);
return;
}

FileSystemBackend* backend = GetFileSystemBackend(type);
default_file_task_runner()->PostTaskAndReplyWithResult(
FROM_HERE,
// It is safe to pass Unretained(quota_util) since context owns it.
base::BindOnce(
&FileSystemQuotaUtil::DeleteStorageKeyDataOnFileTaskRunner,
base::Unretained(backend->GetQuotaUtil()), base::RetainedRef(this),
base::Unretained(quota_manager_proxy().get()), storage_key, type),
base::BindOnce(&FileSystemQuotaUtil::DeleteBucketDataOnFileTaskRunner,
base::Unretained(backend->GetQuotaUtil()),
base::RetainedRef(this),
base::Unretained(quota_manager_proxy().get()),
result->ToBucketLocator(), type),
std::move(callback));
}

Expand Down
9 changes: 7 additions & 2 deletions storage/browser/file_system/file_system_context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) FileSystemContext
const FileSystemOptions& options,
base::PassKey<FileSystemContext>);

bool DeleteDataForStorageKeyOnFileTaskRunner(
// Called by CookiesTreeModel, to be removed in crbug.com/1304449.
void DeleteDataForStorageKeyOnFileTaskRunner(
const blink::StorageKey& storage_key);

// Creates a new QuotaReservation for the given `storage_key` and `type`.
Expand Down Expand Up @@ -400,7 +401,11 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) FileSystemContext
const GURL& filesystem_root,
const std::string& filesystem_name,
base::File::Error error);

void OnGetBucketForDeleteFileSystem(FileSystemType type,
StatusCallback callback,
QuotaErrorOr<BucketInfo> result);
void OnGetBucketForStorageKeyDeletion(std::vector<FileSystemType> type,
QuotaErrorOr<BucketInfo> result);
// OnGetOrCreateBucket is the callback for calling
// QuotaManagerProxy::GetOrCreateDefault.
void OnGetOrCreateBucket(const blink::StorageKey& storage_key,
Expand Down
8 changes: 0 additions & 8 deletions storage/browser/file_system/file_system_quota_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) FileSystemQuotaUtil {
public:
virtual ~FileSystemQuotaUtil() = default;

// Deletes the data on the StorageKey and reports the amount of deleted data
// to the quota manager via |proxy|.
virtual base::File::Error DeleteStorageKeyDataOnFileTaskRunner(
FileSystemContext* context,
QuotaManagerProxy* proxy,
const blink::StorageKey& storage_key,
FileSystemType type) = 0;

// Deletes the cached default bucket for `storage_key` of type
// StorageType::kTemporary. Called when the default bucket is deleted from
// Quota service.
Expand Down
88 changes: 4 additions & 84 deletions storage/browser/file_system/obfuscated_file_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -961,59 +961,6 @@ ObfuscatedFileUtil::GetDirectoryForStorageKeyAndType(
return path;
}

bool ObfuscatedFileUtil::DeleteDirectoryForStorageKeyAndType(
const blink::StorageKey& storage_key,
const absl::optional<FileSystemType>& type) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DestroyDirectoryDatabaseForStorageKey(storage_key, type);

base::FileErrorOr<base::FilePath> origin_path =
GetDirectoryForStorageKey(storage_key, false);
if (!origin_path.has_value() || origin_path->empty())
return true;

if (type) {
// Delete the filesystem type directory.
const base::FileErrorOr<base::FilePath> origin_type_path =
GetDirectoryForStorageKeyAndType(storage_key, type.value(), false);
if (!origin_type_path.has_value() &&
origin_type_path.error() == base::File::FILE_ERROR_FAILED) {
return false;
}
if (origin_type_path.has_value() && !origin_type_path->empty() &&
!delegate_->DeleteFileOrDirectory(origin_type_path.value(),
true /* recursive */)) {
return false;
}

// At this point we are sure we had successfully deleted the origin/type
// directory (i.e. we're ready to just return true).
// See if we have other directories in this origin directory.
const std::string type_string =
SandboxFileSystemBackendDelegate::GetTypeString(type.value());
for (const std::string& known_type : known_type_strings_) {
if (known_type == type_string)
continue;
if (delegate_->DirectoryExists(origin_path->AppendASCII(known_type))) {
// Other type's directory exists; just return true here.
return true;
}
}
}

// No other directories seem exist. If we have a first-party StorageKey,
// try deleting the entire origin directory.
if (storage_key.IsFirstPartyContext()) {
InitOriginDatabase(storage_key.origin(), false);
if (origin_database_) {
origin_database_->RemovePathForOrigin(
GetIdentifierFromOrigin(storage_key.origin()));
}
}
return delegate_->DeleteFileOrDirectory(origin_path.value(),
true /* recursive */);
}

bool ObfuscatedFileUtil::DeleteDirectoryForBucketAndType(
const BucketLocator& bucket_locator,
const absl::optional<FileSystemType>& type) {
Expand Down Expand Up @@ -1089,23 +1036,9 @@ ObfuscatedFileUtil::CreateStorageKeyEnumerator() {
origin_database_.get(), file_util_delegate, file_system_directory_);
}

void ObfuscatedFileUtil::DestroyDirectoryDatabaseForStorageKey(
const blink::StorageKey& storage_key,
const absl::optional<FileSystemType>& type) {
DestroyDirectoryDatabaseHelper(absl::nullopt, storage_key, type);
}

void ObfuscatedFileUtil::DestroyDirectoryDatabaseForBucket(
const BucketLocator& bucket_locator,
const absl::optional<FileSystemType>& type) {
DestroyDirectoryDatabaseHelper(bucket_locator, bucket_locator.storage_key,
type);
}

void ObfuscatedFileUtil::DestroyDirectoryDatabaseHelper(
const absl::optional<BucketLocator>& bucket_locator,
const blink::StorageKey& storage_key,
const absl::optional<FileSystemType>& type) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

DatabaseKey key_prefix;
Expand All @@ -1115,24 +1048,11 @@ void ObfuscatedFileUtil::DestroyDirectoryDatabaseHelper(
// `key.bucket()` is absl::nullopt for all non-kTemporary types.
if (type && (FileSystemTypeToQuotaStorageType(type.value()) ==
::blink::mojom::StorageType::kTemporary)) {
if (bucket_locator.has_value()) {
key_prefix =
DatabaseKey(bucket_locator->storage_key, bucket_locator, type_string);
} else {
// If we are not provided a custom bucket value we must find the default
// bucket corresponding to the StorageKey.
QuotaErrorOr<BucketLocator> default_bucket =
GetOrCreateDefaultBucket(storage_key);
// If looking up the default bucket for a given StorageKey fails, things
// are pretty broken, and there isn't anything this method can do.
if (!default_bucket.has_value()) {
return;
}
key_prefix =
DatabaseKey(storage_key, default_bucket.value(), type_string);
}
key_prefix =
DatabaseKey(bucket_locator.storage_key, bucket_locator, type_string);
} else { // All other storage types.
key_prefix = DatabaseKey(storage_key, absl::nullopt, type_string);
key_prefix =
DatabaseKey(bucket_locator.storage_key, absl::nullopt, type_string);
}

// If `type` is empty, delete all filesystem types under `storage_key`.
Expand Down
22 changes: 1 addition & 21 deletions storage/browser/file_system/obfuscated_file_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) ObfuscatedFileUtil
// filesystem directory. `known_type_strings` are known type string names that
// this file system should care about. This info is used to determine whether
// we could delete the entire origin directory or not in
// DeleteDirectoryForStorageKeyAndType. If no directory for any known type
// DeleteDirectoryForBucketAndType. If no directory for any known type
// exists the origin directory may get deleted when one StorageKey/type pair
// is deleted. NOTE: type strings are not mapped 1-to-1 with FileSystemType,
// and as a result, directories should only be directly compared using type
Expand Down Expand Up @@ -234,13 +234,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) ObfuscatedFileUtil
const absl::optional<FileSystemType>& type,
bool create);

// Deletes the topmost directory specific to this StorageKey and type. This
// will delete its directory database. Deletes the topmost StorageKey
// directory if `type` is absl::nullopt.
bool DeleteDirectoryForStorageKeyAndType(
const blink::StorageKey& storage_key,
const absl::optional<FileSystemType>& type);

// Deletes the topmost directory specific to this BucketLocator and type. This
// will delete its directory database. Deletes the topmost bucket
// directory if `type` is absl::nullopt.
Expand All @@ -253,12 +246,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) ObfuscatedFileUtil
// object.
std::unique_ptr<AbstractStorageKeyEnumerator> CreateStorageKeyEnumerator();

// Deletes a directory database from the database list and destroys the
// database on the disk corresponding to the provided StorageKey and type.
void DestroyDirectoryDatabaseForStorageKey(
const blink::StorageKey& storage_key,
const absl::optional<FileSystemType>& type);

// Deletes a directory database from the database list and destroys the
// database on the disk corresponding to the provided bucket locator and type.
void DestroyDirectoryDatabaseForBucket(
Expand Down Expand Up @@ -351,13 +338,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) ObfuscatedFileUtil
base::FilePath DataPathToLocalPath(const FileSystemURL& url,
const base::FilePath& data_file_path);

// Deletes a directory database from the database list and destroys the
// database on the disk.
void DestroyDirectoryDatabaseHelper(
const absl::optional<BucketLocator>& bucket_locator,
const blink::StorageKey& storage_key,
const absl::optional<FileSystemType>& type);

// This returns nullptr if `create` flag is false and a filesystem does not
// exist for the given `url`.
// For read operations `create` should be false.
Expand Down
Loading

0 comments on commit c1773c8

Please sign in to comment.