You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
229 lines
11 KiB
229 lines
11 KiB
9 months ago
|
commit 57526b8dc45b2e6c67bba7306f1dde73b1f2910c
|
||
|
Author: sisidovski <sisidovski@chromium.org>
|
||
|
Date: Tue Oct 24 09:32:49 2023 +0000
|
||
|
|
||
|
Remove unused items from the RaceNetworkRequest hashmap
|
||
|
|
||
|
When the AutoPreload or the race-network-and-fetch-handler option in the
|
||
|
static routing API is enabled, network requests are dispatched and
|
||
|
URLLoaderFactories are held in a hashmap in ServiceWorkerGlobalScope.
|
||
|
Those are consumed inside the fetch handler when fetch(e.request) is
|
||
|
called. But if the fetch handler doesn't call fetch() e.g. fallback,
|
||
|
those hashmap items does not have a chance to be removed.
|
||
|
|
||
|
This CL changes the hashmap items to be removed when the fetch event
|
||
|
finishes, and the URLLoaderFactory is still not consumed at that time.
|
||
|
This may loose the dedupe capability if fetch() is called later e.g.
|
||
|
setTimeout(() => fetch()), but it makes sense to prioritize keeping the
|
||
|
hashmap small.
|
||
|
|
||
|
Change-Id: I51bdc9d5eb5185f2b5b4df6ee785715b1180c848
|
||
|
Bug: 1492640
|
||
|
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4964840
|
||
|
Reviewed-by: Minoru Chikamune <chikamune@chromium.org>
|
||
|
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
|
||
|
Reviewed-by: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
|
||
|
Cr-Commit-Position: refs/heads/main@{#1214064}
|
||
|
|
||
|
diff -up chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.cc.me chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.cc
|
||
|
--- chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.cc.me 2024-03-18 10:34:27.604707632 +0100
|
||
|
+++ chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.cc 2024-03-18 11:52:14.309983505 +0100
|
||
|
@@ -46,7 +46,6 @@
|
||
|
#include "services/network/public/cpp/cross_origin_embedder_policy.h"
|
||
|
#include "services/network/public/mojom/cookie_manager.mojom-blink.h"
|
||
|
#include "services/network/public/mojom/cross_origin_embedder_policy.mojom.h"
|
||
|
-#include "services/network/public/mojom/url_loader_factory.mojom-blink.h"
|
||
|
#include "third_party/blink/public/common/features.h"
|
||
|
#include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom-blink.h"
|
||
|
#include "third_party/blink/public/mojom/notifications/notification.mojom-blink.h"
|
||
|
@@ -1097,10 +1096,6 @@ void ServiceWorkerGlobalScope::DidHandle
|
||
|
TRACE_ID_WITH_SCOPE(kServiceWorkerGlobalScopeTraceScope,
|
||
|
TRACE_ID_LOCAL(event_id)),
|
||
|
TRACE_EVENT_FLAG_FLOW_IN, "status", MojoEnumToString(status));
|
||
|
-
|
||
|
- // Delete the URLLoaderFactory for the RaceNetworkRequest if it's not used.
|
||
|
- RemoveItemFromRaceNetworkRequests(event_id);
|
||
|
-
|
||
|
if (!RunEventCallback(&fetch_event_callbacks_, event_queue_.get(), event_id,
|
||
|
status)) {
|
||
|
// The event may have been aborted. Its response callback also needs to be
|
||
|
@@ -1500,7 +1495,6 @@ void ServiceWorkerGlobalScope::AbortCall
|
||
|
response_callback_iter->value->TakeValue().reset();
|
||
|
fetch_response_callbacks_.erase(response_callback_iter);
|
||
|
}
|
||
|
- RemoveItemFromRaceNetworkRequests(event_id);
|
||
|
|
||
|
// Run the event callback with the error code.
|
||
|
auto event_callback_iter = fetch_event_callbacks_.find(event_id);
|
||
|
@@ -1588,11 +1582,52 @@ void ServiceWorkerGlobalScope::StartFetc
|
||
|
|
||
|
if (params->race_network_request_loader_factory &&
|
||
|
params->request->service_worker_race_network_request_token) {
|
||
|
- InsertNewItemToRaceNetworkRequests(
|
||
|
- event_id,
|
||
|
- params->request->service_worker_race_network_request_token.value(),
|
||
|
- std::move(params->race_network_request_loader_factory),
|
||
|
- params->request->url);
|
||
|
+ auto insert_result = race_network_request_loader_factories_.insert(
|
||
|
+ String(params->request->service_worker_race_network_request_token
|
||
|
+ ->ToString()),
|
||
|
+ std::move(params->race_network_request_loader_factory));
|
||
|
+
|
||
|
+ // DumpWithoutCrashing if the token is empty, or not inserted as a new entry
|
||
|
+ // to |race_network_request_loader_factories_|.
|
||
|
+ // TODO(crbug.com/1492640) Remove DumpWithoutCrashing once we collect data
|
||
|
+ // and identify the cause.
|
||
|
+ static bool has_dumped_without_crashing_for_empty_token = false;
|
||
|
+ static bool has_dumped_without_crashing_for_not_new_entry = false;
|
||
|
+ if (!has_dumped_without_crashing_for_empty_token &&
|
||
|
+ params->request->service_worker_race_network_request_token
|
||
|
+ ->is_empty()) {
|
||
|
+ has_dumped_without_crashing_for_empty_token = true;
|
||
|
+ SCOPED_CRASH_KEY_BOOL(
|
||
|
+ "SWGlobalScope", "empty_race_token",
|
||
|
+ params->request->service_worker_race_network_request_token
|
||
|
+ ->is_empty());
|
||
|
+ SCOPED_CRASH_KEY_STRING64(
|
||
|
+ "SWGlobalScope", "race_token_string",
|
||
|
+ params->request->service_worker_race_network_request_token
|
||
|
+ ->ToString());
|
||
|
+ SCOPED_CRASH_KEY_BOOL("SWGlobalScope", "race_insert_new_entry",
|
||
|
+ insert_result.is_new_entry);
|
||
|
+ SCOPED_CRASH_KEY_STRING256("SWGlobalScope", "race_request_url",
|
||
|
+ params->request->url.GetString().Utf8());
|
||
|
+ base::debug::DumpWithoutCrashing();
|
||
|
+ }
|
||
|
+ if (!has_dumped_without_crashing_for_not_new_entry &&
|
||
|
+ !insert_result.is_new_entry) {
|
||
|
+ has_dumped_without_crashing_for_not_new_entry = true;
|
||
|
+ SCOPED_CRASH_KEY_BOOL(
|
||
|
+ "SWGlobalScope", "empty_race_token",
|
||
|
+ params->request->service_worker_race_network_request_token
|
||
|
+ ->is_empty());
|
||
|
+ SCOPED_CRASH_KEY_STRING64(
|
||
|
+ "SWGlobalScope", "race_token_string",
|
||
|
+ params->request->service_worker_race_network_request_token
|
||
|
+ ->ToString());
|
||
|
+ SCOPED_CRASH_KEY_BOOL("SWGlobalScope", "race_insert_new_entry",
|
||
|
+ insert_result.is_new_entry);
|
||
|
+ SCOPED_CRASH_KEY_STRING256("SWGlobalScope", "race_request_url",
|
||
|
+ params->request->url.GetString().Utf8());
|
||
|
+ base::debug::DumpWithoutCrashing();
|
||
|
+ }
|
||
|
}
|
||
|
|
||
|
Request* request = Request::Create(
|
||
|
@@ -2805,71 +2840,12 @@ bool ServiceWorkerGlobalScope::SetAttrib
|
||
|
std::optional<mojo::PendingRemote<network::mojom::blink::URLLoaderFactory>>
|
||
|
ServiceWorkerGlobalScope::FindRaceNetworkRequestURLLoaderFactory(
|
||
|
const base::UnguessableToken& token) {
|
||
|
- std::unique_ptr<RaceNetworkRequestInfo> result =
|
||
|
- race_network_requests_.Take(String(token.ToString()));
|
||
|
+ mojo::PendingRemote<network::mojom::blink::URLLoaderFactory> result =
|
||
|
+ race_network_request_loader_factories_.Take(String(token.ToString()));
|
||
|
if (result) {
|
||
|
- race_network_request_fetch_event_ids_.erase(result->fetch_event_id);
|
||
|
- return std::optional<
|
||
|
- mojo::PendingRemote<network::mojom::blink::URLLoaderFactory>>(
|
||
|
- std::move(result->url_loader_factory));
|
||
|
+ return result;
|
||
|
}
|
||
|
return std::nullopt;
|
||
|
}
|
||
|
|
||
|
-void ServiceWorkerGlobalScope::InsertNewItemToRaceNetworkRequests(
|
||
|
- int fetch_event_id,
|
||
|
- const base::UnguessableToken& token,
|
||
|
- mojo::PendingRemote<network::mojom::blink::URLLoaderFactory>
|
||
|
- url_loader_factory,
|
||
|
- const KURL& request_url) {
|
||
|
- auto race_network_request_token = String(token.ToString());
|
||
|
- auto info = std::make_unique<RaceNetworkRequestInfo>(
|
||
|
- fetch_event_id, race_network_request_token,
|
||
|
- std::move(url_loader_factory));
|
||
|
- race_network_request_fetch_event_ids_.insert(fetch_event_id, info.get());
|
||
|
- auto insert_result = race_network_requests_.insert(race_network_request_token,
|
||
|
- std::move(info));
|
||
|
-
|
||
|
- // DumpWithoutCrashing if the token is empty, or not inserted as a new entry
|
||
|
- // to |race_network_request_loader_factories_|.
|
||
|
- // TODO(crbug.com/1492640) Remove DumpWithoutCrashing once we collect data
|
||
|
- // and identify the cause.
|
||
|
- static bool has_dumped_without_crashing_for_empty_token = false;
|
||
|
- static bool has_dumped_without_crashing_for_not_new_entry = false;
|
||
|
- if (!has_dumped_without_crashing_for_empty_token && token.is_empty()) {
|
||
|
- has_dumped_without_crashing_for_empty_token = true;
|
||
|
- SCOPED_CRASH_KEY_BOOL("SWGlobalScope", "empty_race_token",
|
||
|
- token.is_empty());
|
||
|
- SCOPED_CRASH_KEY_STRING64("SWGlobalScope", "race_token_string",
|
||
|
- token.ToString());
|
||
|
- SCOPED_CRASH_KEY_BOOL("SWGlobalScope", "race_insert_new_entry",
|
||
|
- insert_result.is_new_entry);
|
||
|
- SCOPED_CRASH_KEY_STRING256("SWGlobalScope", "race_request_url",
|
||
|
- request_url.GetString().Utf8());
|
||
|
- base::debug::DumpWithoutCrashing();
|
||
|
- }
|
||
|
- if (!has_dumped_without_crashing_for_not_new_entry &&
|
||
|
- !insert_result.is_new_entry) {
|
||
|
- has_dumped_without_crashing_for_not_new_entry = true;
|
||
|
- SCOPED_CRASH_KEY_BOOL("SWGlobalScope", "empty_race_token",
|
||
|
- token.is_empty());
|
||
|
- SCOPED_CRASH_KEY_STRING64("SWGlobalScope", "race_token_string",
|
||
|
- token.ToString());
|
||
|
- SCOPED_CRASH_KEY_BOOL("SWGlobalScope", "race_insert_new_entry",
|
||
|
- insert_result.is_new_entry);
|
||
|
- SCOPED_CRASH_KEY_STRING256("SWGlobalScope", "race_request_url",
|
||
|
- request_url.GetString().Utf8());
|
||
|
- base::debug::DumpWithoutCrashing();
|
||
|
- }
|
||
|
-}
|
||
|
-
|
||
|
-void ServiceWorkerGlobalScope::RemoveItemFromRaceNetworkRequests(
|
||
|
- int fetch_event_id) {
|
||
|
- RaceNetworkRequestInfo* info =
|
||
|
- race_network_request_fetch_event_ids_.Take(fetch_event_id);
|
||
|
- if (info) {
|
||
|
- race_network_requests_.erase(info->token);
|
||
|
- }
|
||
|
-}
|
||
|
-
|
||
|
} // namespace blink
|
||
|
diff -up chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.h.me chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.h
|
||
|
--- chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.h.me 2024-03-18 10:26:14.905817501 +0100
|
||
|
+++ chromium-123.0.6312.46/third_party/blink/renderer/modules/service_worker/service_worker_global_scope.h 2024-03-18 11:47:58.202198028 +0100
|
||
|
@@ -623,14 +623,6 @@ class MODULES_EXPORT ServiceWorkerGlobal
|
||
|
// ServiceWorker.FetchEvent.QueuingTime histogram.
|
||
|
void RecordQueuingTime(base::TimeTicks created_time);
|
||
|
|
||
|
- void InsertNewItemToRaceNetworkRequests(
|
||
|
- int fetch_event_id,
|
||
|
- const base::UnguessableToken& token,
|
||
|
- mojo::PendingRemote<network::mojom::blink::URLLoaderFactory>
|
||
|
- url_loader_factory,
|
||
|
- const KURL& request_url);
|
||
|
- void RemoveItemFromRaceNetworkRequests(int fetch_event_id);
|
||
|
-
|
||
|
Member<ServiceWorkerClients> clients_;
|
||
|
Member<ServiceWorkerRegistration> registration_;
|
||
|
Member<::blink::ServiceWorker> service_worker_;
|
||
|
@@ -776,17 +768,10 @@ class MODULES_EXPORT ServiceWorkerGlobal
|
||
|
|
||
|
blink::BlinkStorageKey storage_key_;
|
||
|
|
||
|
- struct RaceNetworkRequestInfo {
|
||
|
- int fetch_event_id;
|
||
|
- String token;
|
||
|
- mojo::PendingRemote<network::mojom::blink::URLLoaderFactory>
|
||
|
- url_loader_factory;
|
||
|
- };
|
||
|
// TODO(crbug.com/918702) WTF::HashMap cannot use base::UnguessableToken as a
|
||
|
// key. As a workaround uses WTF::String as a key instead.
|
||
|
- HashMap<String, std::unique_ptr<RaceNetworkRequestInfo>>
|
||
|
- race_network_requests_;
|
||
|
- HashMap<int, RaceNetworkRequestInfo*> race_network_request_fetch_event_ids_;
|
||
|
+ HashMap<String, mojo::PendingRemote<network::mojom::blink::URLLoaderFactory>>
|
||
|
+ race_network_request_loader_factories_;
|
||
|
|
||
|
HeapMojoAssociatedRemote<mojom::blink::AssociatedInterfaceProvider>
|
||
|
remote_associated_interfaces_{this};
|