Skip to content

Commit

Permalink
Introduce kServiceWorkerScopeCache feature
Browse files Browse the repository at this point in the history 
Before this CL, there was the following problem.

When a service worker is registered in https://a.com/scope, the origin
of this URL (https://a.com/ in this case) is used for StorageKey. This
means that, even when the user navigates to https://a.com/ (which is
out-of-scope of https://a.com/scope service worker), StorageKey becomes
the same as https://a.com/scope, and FindRegistrationForClientUrl mojo
needs to be called. FindRegistrationForClientUrl delays navigation,
hence we want to avoid calling it when possible.

This CL introduces kServiceWorkerScopeCache feature which creates a
ServiceWorker's scope URL cache on the UI thread. By using this cache,
we can avoid calling the FindRegistrationForClientUrl mojo function when
it's not needed.

[doc (google internal)] https://docs.google.com/document/d/1746-NLoUY5Pr3oimcVC_frNqOgkF3jOsdBsDSQlE-RA

Bug: 1411197
Bug: 1375174
Change-Id: I113036c11d2c29b902577220243001b818bc940f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4173488
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Shunya Shishido <sisidovski@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Minoru Chikamune <chikamune@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1100878}
  • Loading branch information
@chikamune-cr
chikamune-cr authored and Chromium LUCI CQ committed Feb 3, 2023
1 parent 9cf99e7 commit 821732c
Show file tree
Hide file tree
Showing 10 changed files with 453 additions and 31 deletions.
15 changes: 13 additions & 2 deletions components/services/storage/public/mojom/service_worker_storage_control.mojom
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,22 @@ interface ServiceWorkerStorageControl {
// Returns all StorageKeys which have service worker registrations.
GetRegisteredStorageKeys() => (array<blink.mojom.StorageKey> keys);

// Reads a stored registration for `client_url` that is associated with `key`.
// Reads a stored registration for `client_url` that is associated
// with `key`. `scopes` contains all of the service worker's
// registration scopes that are relevant to the `key` so that we can
// cache scope URLs in the UI thread. The 'scopes' is valid only when
// the status is `kOk` or `kErrorNotFound`. The `scopes` must be
// optional because `scopes` can be empty if the scope count exceeds
// the threshold even when there are scopes in DB so that we can avoid
// sending too large information via mojo API. When this API returns
// null for `scopes`, the caller of this API can't cache the
// scopes. This means that the caller needs to fall back to the slow
// code path that queries the registration from the DB.
FindRegistrationForClientUrl(url.mojom.Url client_url,
blink.mojom.StorageKey key) =>
(ServiceWorkerDatabaseStatus status,
ServiceWorkerFindRegistrationResult? result);
ServiceWorkerFindRegistrationResult? result,
array<url.mojom.Url>? scopes);
// Reads a stored registration for `scope` that is associated with `key`.
FindRegistrationForScope(url.mojom.Url scope, blink.mojom.StorageKey key) =>
(ServiceWorkerDatabaseStatus status,
Expand Down
50 changes: 43 additions & 7 deletions components/services/storage/service_worker/service_worker_storage.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <utility>

#include "base/containers/contains.h"
#include "base/feature_list.h"
#include "base/files/file_util.h"
#include "base/functional/callback_helpers.h"
#include "base/memory/ptr_util.h"
Expand Down Expand Up @@ -57,6 +58,17 @@ void RecordDeleteAndStartOverResult(DeleteAndStartOverResult result) {

} // namespace

// When this is enabled, The ServiceWorker's scope URLs are cached on the UI
// thread, and stops calling FindRegistrationForClientUrl mojo function if
// possible.
BASE_FEATURE(kServiceWorkerScopeCache,
"ServiceWorkerScopeCache",
base::FEATURE_DISABLED_BY_DEFAULT);

// The scope URL count limit per the storage key.
const base::FeatureParam<int> kServiceWorkerScopeCacheLimitPerKey{
&kServiceWorkerScopeCache, "ServiceWorkerScopeCacheLimitPerKey", 100};

ServiceWorkerStorage::InitialData::InitialData()
: next_registration_id(blink::mojom::kInvalidServiceWorkerRegistrationId),
next_version_id(blink::mojom::kInvalidServiceWorkerVersionId),
Expand Down Expand Up @@ -114,12 +126,13 @@ void ServiceWorkerStorage::GetRegisteredStorageKeys(
void ServiceWorkerStorage::FindRegistrationForClientUrl(
const GURL& client_url,
const blink::StorageKey& key,
FindRegistrationDataCallback callback) {
FindRegistrationForClientUrlDataCallback callback) {
DCHECK(!client_url.has_ref());
switch (state_) {
case STORAGE_STATE_DISABLED:
std::move(callback).Run(
/*data=*/nullptr, /*resources=*/nullptr,
/*scopes=*/absl::nullopt,
ServiceWorkerDatabase::Status::kErrorDisabled);
return;
case STORAGE_STATE_INITIALIZING:
Expand All @@ -139,8 +152,12 @@ void ServiceWorkerStorage::FindRegistrationForClientUrl(

// Bypass database lookup when there is no stored registration.
if (!base::Contains(registered_keys_, key)) {
absl::optional<std::vector<GURL>> scopes;
if (base::FeatureList::IsEnabled(storage::kServiceWorkerScopeCache)) {
scopes = std::vector<GURL>();
}
std::move(callback).Run(
/*data=*/nullptr, /*resources=*/nullptr,
/*data=*/nullptr, /*resources=*/nullptr, /*scopes=*/scopes,
ServiceWorkerDatabase::Status::kErrorNotFound);
return;
}
Expand Down Expand Up @@ -1704,7 +1721,7 @@ void ServiceWorkerStorage::FindForClientUrlInDB(
scoped_refptr<base::SequencedTaskRunner> original_task_runner,
const GURL& client_url,
const blink::StorageKey& key,
FindInDBCallback callback) {
FindForClientUrlInDBCallback callback) {
base::TimeTicks now = base::TimeTicks::Now();
TRACE_EVENT1("ServiceWorker", "ServiceWorkerStorage::FindForClientUrlInDB",
"url", client_url);
Expand All @@ -1717,7 +1734,8 @@ void ServiceWorkerStorage::FindForClientUrlInDB(
original_task_runner->PostTask(
FROM_HERE, base::BindOnce(std::move(callback),
/*data=*/nullptr,
/*resources=*/nullptr, status));
/*resources=*/nullptr,
/*scopes=*/absl::nullopt, status));
return;
}

Expand All @@ -1728,15 +1746,33 @@ void ServiceWorkerStorage::FindForClientUrlInDB(
// Find one with a scope match.
blink::ServiceWorkerLongestScopeMatcher matcher(client_url);
int64_t match = blink::mojom::kInvalidServiceWorkerRegistrationId;
for (const auto& registration_data : registration_data_list)
if (matcher.MatchLongest(registration_data->scope))
bool enable_scope_cache =
base::FeatureList::IsEnabled(kServiceWorkerScopeCache) &&
(registration_data_list.size() <=
static_cast<size_t>(kServiceWorkerScopeCacheLimitPerKey.Get()));
// `scopes` should contain all of the service worker's registration
// scopes that are relevant to the `key` so that we can cache scope
// URLs in the UI thread. The 'scopes' is valid only when the status
// is `kOk` or `kErrorNotFound`.
absl::optional<std::vector<GURL>> scopes;
if (enable_scope_cache) {
scopes = std::vector<GURL>();
scopes->reserve(registration_data_list.size());
}
for (const auto& registration_data : registration_data_list) {
if (matcher.MatchLongest(registration_data->scope)) {
match = registration_data->registration_id;
}
if (enable_scope_cache) {
scopes->push_back(std::move(registration_data->scope));
}
}
if (match != blink::mojom::kInvalidServiceWorkerRegistrationId)
status = database->ReadRegistration(match, key, &data, resources.get());

original_task_runner->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), std::move(data),
std::move(resources), status));
std::move(resources), scopes, status));

base::UmaHistogramMediumTimes(
"ServiceWorker.Storage.FindForClientUrlInDB.Time",
Expand Down
24 changes: 20 additions & 4 deletions components/services/storage/service_worker/service_worker_storage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@

#include "base/containers/circular_deque.h"
#include "base/containers/flat_map.h"
#include "base/feature_list.h"
#include "base/files/file_path.h"
#include "base/functional/bind.h"
#include "base/gtest_prod_util.h"
#include "base/memory/weak_ptr.h"
#include "base/metrics/field_trial_params.h"
#include "components/services/storage/public/mojom/local_storage_control.mojom.h"
#include "components/services/storage/public/mojom/service_worker_storage_control.mojom.h"
#include "components/services/storage/public/mojom/storage_policy_update.mojom.h"
Expand Down Expand Up @@ -47,6 +49,9 @@ FORWARD_DECLARE_TEST(ServiceWorkerStorageDiskTest,
FORWARD_DECLARE_TEST(ServiceWorkerStorageTest, DisabledStorage);
} // namespace service_worker_storage_unittest

BASE_DECLARE_FEATURE(kServiceWorkerScopeCache);
extern const base::FeatureParam<int> kServiceWorkerScopeCacheLimitPerKey;

// This class provides an interface to store and retrieve ServiceWorker
// registration data. The lifetime is equal to ServiceWorkerRegistry that is
// an owner of this class. When a storage operation fails, this is marked as
Expand All @@ -59,6 +64,11 @@ class ServiceWorkerStorage {
using ResourceList = std::vector<mojom::ServiceWorkerResourceRecordPtr>;
using GetRegisteredStorageKeysCallback =
base::OnceCallback<void(const std::vector<blink::StorageKey>& keys)>;
using FindRegistrationForClientUrlDataCallback =
base::OnceCallback<void(mojom::ServiceWorkerRegistrationDataPtr data,
std::unique_ptr<ResourceList> resources,
const absl::optional<std::vector<GURL>>& scopes,
ServiceWorkerDatabase::Status status)>;
using FindRegistrationDataCallback =
base::OnceCallback<void(mojom::ServiceWorkerRegistrationDataPtr data,
std::unique_ptr<ResourceList> resources,
Expand Down Expand Up @@ -116,9 +126,10 @@ class ServiceWorkerStorage {
// ResourceList if registration is found, or returns
// ServiceWorkerDatabase::Status::kErrorNotFound if no matching registration
// is found.
void FindRegistrationForClientUrl(const GURL& client_url,
const blink::StorageKey& key,
FindRegistrationDataCallback callback);
void FindRegistrationForClientUrl(
const GURL& client_url,
const blink::StorageKey& key,
FindRegistrationForClientUrlDataCallback callback);
void FindRegistrationForScope(const GURL& scope,
const blink::StorageKey& key,
FindRegistrationDataCallback callback);
Expand Down Expand Up @@ -345,6 +356,11 @@ class ServiceWorkerStorage {
StorageKeyState storage_key_state,
const ServiceWorkerDatabase::DeletedVersion& deleted_version_data,
ServiceWorkerDatabase::Status status)>;
using FindForClientUrlInDBCallback =
base::OnceCallback<void(mojom::ServiceWorkerRegistrationDataPtr data,
std::unique_ptr<ResourceList> resources,
const absl::optional<std::vector<GURL>>& scopes,
ServiceWorkerDatabase::Status status)>;
using FindInDBCallback =
base::OnceCallback<void(mojom::ServiceWorkerRegistrationDataPtr data,
std::unique_ptr<ResourceList> resources,
Expand Down Expand Up @@ -442,7 +458,7 @@ class ServiceWorkerStorage {
scoped_refptr<base::SequencedTaskRunner> original_task_runner,
const GURL& client_url,
const blink::StorageKey& key,
FindInDBCallback callback);
FindForClientUrlInDBCallback callback);
static void FindForScopeInDB(
ServiceWorkerDatabase* database,
scoped_refptr<base::SequencedTaskRunner> original_task_runner,
Expand Down
38 changes: 31 additions & 7 deletions components/services/storage/service_worker/service_worker_storage_control_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,9 @@ void ServiceWorkerStorageControlImpl::FindRegistrationForClientUrl(
FindRegistrationForClientUrlCallback callback) {
storage_->FindRegistrationForClientUrl(
client_url, key,
base::BindOnce(&ServiceWorkerStorageControlImpl::DidFindRegistration,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
base::BindOnce(
&ServiceWorkerStorageControlImpl::DidFindRegistrationForClientUrl,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
}

void ServiceWorkerStorageControlImpl::FindRegistrationForScope(
Expand Down Expand Up @@ -437,6 +438,30 @@ void ServiceWorkerStorageControlImpl::SetPurgingCompleteCallbackForTest(
storage_->SetPurgingCompleteCallbackForTest(std::move(callback)); // IN-TEST
}

void ServiceWorkerStorageControlImpl::DidFindRegistrationForClientUrl(
FindRegistrationForClientUrlCallback callback,
mojom::ServiceWorkerRegistrationDataPtr data,
std::unique_ptr<ResourceList> resources,
const absl::optional<std::vector<GURL>>& scopes,
mojom::ServiceWorkerDatabaseStatus status) {
if (status != mojom::ServiceWorkerDatabaseStatus::kOk) {
std::move(callback).Run(status, /*result=*/nullptr, scopes);
return;
}

DCHECK(resources);
DCHECK(data);

mojo::PendingRemote<mojom::ServiceWorkerLiveVersionRef> remote_reference =
CreateLiveVersionReferenceRemote(data->version_id);

std::move(callback).Run(
status,
mojom::ServiceWorkerFindRegistrationResult::New(
std::move(remote_reference), std::move(data), std::move(*resources)),
scopes);
}

void ServiceWorkerStorageControlImpl::DidFindRegistration(
base::OnceCallback<void(mojom::ServiceWorkerDatabaseStatus status,
mojom::ServiceWorkerFindRegistrationResultPtr)>
Expand All @@ -452,14 +477,13 @@ void ServiceWorkerStorageControlImpl::DidFindRegistration(
DCHECK(resources);
DCHECK(data);

ResourceList resource_list = std::move(*resources);
mojo::PendingRemote<mojom::ServiceWorkerLiveVersionRef> remote_reference =
CreateLiveVersionReferenceRemote(data->version_id);

std::move(callback).Run(status,
mojom::ServiceWorkerFindRegistrationResult::New(
std::move(remote_reference), std::move(data),
std::move(resource_list)));
std::move(callback).Run(
status,
mojom::ServiceWorkerFindRegistrationResult::New(
std::move(remote_reference), std::move(data), std::move(*resources)));
}

void ServiceWorkerStorageControlImpl::DidGetRegistrationsForStorageKey(
Expand Down
6 changes: 6 additions & 0 deletions components/services/storage/service_worker/service_worker_storage_control_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,12 @@ class ServiceWorkerStorageControlImpl
using ResourceList = std::vector<mojom::ServiceWorkerResourceRecordPtr>;

// Callbacks for ServiceWorkerStorage methods.
void DidFindRegistrationForClientUrl(
FindRegistrationForClientUrlCallback callback,
mojom::ServiceWorkerRegistrationDataPtr data,
std::unique_ptr<ResourceList> resources,
const absl::optional<std::vector<GURL>>& scopes,
mojom::ServiceWorkerDatabaseStatus status);
void DidFindRegistration(
base::OnceCallback<void(mojom::ServiceWorkerDatabaseStatus status,
mojom::ServiceWorkerFindRegistrationResultPtr)>
Expand Down
3 changes: 2 additions & 1 deletion components/services/storage/service_worker/service_worker_storage_control_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,8 @@ class ServiceWorkerStorageControlImplTest : public testing::Test {
client_url, key,
base::BindLambdaForTesting(
[&](DatabaseStatus status,
mojom::ServiceWorkerFindRegistrationResultPtr entry) {
mojom::ServiceWorkerFindRegistrationResultPtr entry,
const absl::optional<std::vector<GURL>>& scopes) {
return_value.status = status;
return_value.entry = std::move(entry);
loop.Quit();
Expand Down
14 changes: 8 additions & 6 deletions components/services/storage/service_worker/service_worker_storage_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,14 @@ class ServiceWorkerStorageTest : public testing::Test {
base::RunLoop loop;
storage()->FindRegistrationForClientUrl(
document_url, key,
base::BindLambdaForTesting([&](mojom::ServiceWorkerRegistrationDataPtr,
std::unique_ptr<ResourceList>,
ServiceWorkerDatabase::Status status) {
result = status;
loop.Quit();
}));
base::BindLambdaForTesting(
[&](mojom::ServiceWorkerRegistrationDataPtr,
std::unique_ptr<ResourceList>,
const absl::optional<std::vector<GURL>>& scopes,
ServiceWorkerDatabase::Status status) {
result = status;
loop.Quit();
}));
loop.Run();
return result;
}
Expand Down

0 comments on commit 821732c

Please sign in to comment.