Skip to content

Commit

Permalink
Revert "Introduce kServiceWorkerScopeCache feature"
Browse files Browse the repository at this point in the history 
This reverts commit 821732c.

Reason for revert: https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests%20(dbg)(1)/111151/test-results?sortby=&groupby=

Original change's description:
> Introduce kServiceWorkerScopeCache feature
>
> 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}

Bug: 1411197
Bug: 1375174
Change-Id: Ibee2b5645239e946664d38642ccf686912f219a7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4221235
Auto-Submit: Andreea Costinas <acostinas@google.com>
Owners-Override: Andreea Costinas <acostinas@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Andreea Costinas <acostinas@google.com>
Commit-Queue: Andreea Costinas <acostinas@google.com>
Cr-Commit-Position: refs/heads/main@{#1100932}
  • Loading branch information
Andreea Costinas authored and Chromium LUCI CQ committed Feb 3, 2023
1 parent e58c21d commit 84f727d
Show file tree
Hide file tree
Showing 10 changed files with 31 additions and 453 deletions.
15 changes: 2 additions & 13 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,22 +153,11 @@ 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`. `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.
// Reads a stored registration for `client_url` that is associated with `key`.
FindRegistrationForClientUrl(url.mojom.Url client_url,
blink.mojom.StorageKey key) =>
(ServiceWorkerDatabaseStatus status,
ServiceWorkerFindRegistrationResult? result,
array<url.mojom.Url>? scopes);
ServiceWorkerFindRegistrationResult? result);
// 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: 7 additions & 43 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,7 +11,6 @@
#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 @@ -58,17 +57,6 @@ 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 @@ -126,13 +114,12 @@ void ServiceWorkerStorage::GetRegisteredStorageKeys(
void ServiceWorkerStorage::FindRegistrationForClientUrl(
const GURL& client_url,
const blink::StorageKey& key,
FindRegistrationForClientUrlDataCallback callback) {
FindRegistrationDataCallback 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 @@ -152,12 +139,8 @@ 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, /*scopes=*/scopes,
/*data=*/nullptr, /*resources=*/nullptr,
ServiceWorkerDatabase::Status::kErrorNotFound);
return;
}
Expand Down Expand Up @@ -1721,7 +1704,7 @@ void ServiceWorkerStorage::FindForClientUrlInDB(
scoped_refptr<base::SequencedTaskRunner> original_task_runner,
const GURL& client_url,
const blink::StorageKey& key,
FindForClientUrlInDBCallback callback) {
FindInDBCallback callback) {
base::TimeTicks now = base::TimeTicks::Now();
TRACE_EVENT1("ServiceWorker", "ServiceWorkerStorage::FindForClientUrlInDB",
"url", client_url);
Expand All @@ -1734,8 +1717,7 @@ void ServiceWorkerStorage::FindForClientUrlInDB(
original_task_runner->PostTask(
FROM_HERE, base::BindOnce(std::move(callback),
/*data=*/nullptr,
/*resources=*/nullptr,
/*scopes=*/absl::nullopt, status));
/*resources=*/nullptr, status));
return;
}

Expand All @@ -1746,33 +1728,15 @@ void ServiceWorkerStorage::FindForClientUrlInDB(
// Find one with a scope match.
blink::ServiceWorkerLongestScopeMatcher matcher(client_url);
int64_t match = blink::mojom::kInvalidServiceWorkerRegistrationId;
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)) {
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), scopes, status));
std::move(resources), status));

base::UmaHistogramMediumTimes(
"ServiceWorker.Storage.FindForClientUrlInDB.Time",
Expand Down
24 changes: 4 additions & 20 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,12 +16,10 @@

#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 @@ -49,9 +47,6 @@ 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 @@ -64,11 +59,6 @@ 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 @@ -126,10 +116,9 @@ 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,
FindRegistrationForClientUrlDataCallback callback);
void FindRegistrationForClientUrl(const GURL& client_url,
const blink::StorageKey& key,
FindRegistrationDataCallback callback);
void FindRegistrationForScope(const GURL& scope,
const blink::StorageKey& key,
FindRegistrationDataCallback callback);
Expand Down Expand Up @@ -356,11 +345,6 @@ 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 @@ -458,7 +442,7 @@ class ServiceWorkerStorage {
scoped_refptr<base::SequencedTaskRunner> original_task_runner,
const GURL& client_url,
const blink::StorageKey& key,
FindForClientUrlInDBCallback callback);
FindInDBCallback callback);
static void FindForScopeInDB(
ServiceWorkerDatabase* database,
scoped_refptr<base::SequencedTaskRunner> original_task_runner,
Expand Down
38 changes: 7 additions & 31 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,9 +158,8 @@ void ServiceWorkerStorageControlImpl::FindRegistrationForClientUrl(
FindRegistrationForClientUrlCallback callback) {
storage_->FindRegistrationForClientUrl(
client_url, key,
base::BindOnce(
&ServiceWorkerStorageControlImpl::DidFindRegistrationForClientUrl,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
base::BindOnce(&ServiceWorkerStorageControlImpl::DidFindRegistration,
weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
}

void ServiceWorkerStorageControlImpl::FindRegistrationForScope(
Expand Down Expand Up @@ -438,30 +437,6 @@ 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 @@ -477,13 +452,14 @@ 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(*resources)));
std::move(callback).Run(status,
mojom::ServiceWorkerFindRegistrationResult::New(
std::move(remote_reference), std::move(data),
std::move(resource_list)));
}

void ServiceWorkerStorageControlImpl::DidGetRegistrationsForStorageKey(
Expand Down
6 changes: 0 additions & 6 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,12 +190,6 @@ 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: 1 addition & 2 deletions 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,8 +222,7 @@ class ServiceWorkerStorageControlImplTest : public testing::Test {
client_url, key,
base::BindLambdaForTesting(
[&](DatabaseStatus status,
mojom::ServiceWorkerFindRegistrationResultPtr entry,
const absl::optional<std::vector<GURL>>& scopes) {
mojom::ServiceWorkerFindRegistrationResultPtr entry) {
return_value.status = status;
return_value.entry = std::move(entry);
loop.Quit();
Expand Down
14 changes: 6 additions & 8 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,14 +335,12 @@ class ServiceWorkerStorageTest : public testing::Test {
base::RunLoop loop;
storage()->FindRegistrationForClientUrl(
document_url, key,
base::BindLambdaForTesting(
[&](mojom::ServiceWorkerRegistrationDataPtr,
std::unique_ptr<ResourceList>,
const absl::optional<std::vector<GURL>>& scopes,
ServiceWorkerDatabase::Status status) {
result = status;
loop.Quit();
}));
base::BindLambdaForTesting([&](mojom::ServiceWorkerRegistrationDataPtr,
std::unique_ptr<ResourceList>,
ServiceWorkerDatabase::Status status) {
result = status;
loop.Quit();
}));
loop.Run();
return result;
}
Expand Down

0 comments on commit 84f727d

Please sign in to comment.