From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Fri, 1 Dec 2023 16:40:02 -0600 Subject: fix font flooding in dev tools Added in this CL: https://chromium-review.googlesource.com/c/chromium/src/+/5033885 This patch resolves an issue that has been fixed in chromium involving font requests being sent multiple times to DevTools for a single request. This patch can be removed when chromium reaches version 121.0.6157.0. diff --git a/AUTHORS b/AUTHORS index a31f741d472830ec17291e3fad935902e042bca0..0f710af9690e54e2509a9a3abff020322047dbe0 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1151,6 +1151,7 @@ Rulong Chen Russell Davis Ryan Ackley Ryan Gonzalez +Ryan Manuel Ryan Norton Ryan Sleevi Ryan Yoakum diff --git a/third_party/blink/common/features.cc b/third_party/blink/common/features.cc index 619ccba17514b22ab9def18715047f2cefcde7a2..e0ac0cd89fc5c39cbb001b68c99392d80aff3c51 100644 --- a/third_party/blink/common/features.cc +++ b/third_party/blink/common/features.cc @@ -2010,6 +2010,14 @@ BASE_FEATURE(kUACHOverrideBlank, "UACHOverrideBlank", base::FEATURE_DISABLED_BY_DEFAULT); +// If enabled, the body of `EmulateLoadStartedForInspector` is executed only +// once per Resource per ResourceFetcher, and thus duplicated network load +// entries in DevTools caused by `EmulateLoadStartedForInspector` are removed. +// https://crbug.com/1502591 +BASE_FEATURE(kEmulateLoadStartedForInspectorOncePerResource, + "kEmulateLoadStartedForInspectorOncePerResource", + base::FEATURE_ENABLED_BY_DEFAULT); + BASE_FEATURE(kUseBlinkSchedulerTaskRunnerWithCustomDeleter, "UseBlinkSchedulerTaskRunnerWithCustomDeleter", base::FEATURE_ENABLED_BY_DEFAULT); diff --git a/third_party/blink/public/common/features.h b/third_party/blink/public/common/features.h index ad716edf68ec5beb50cf833af28bb1b69099af78..76917f9b8ecc9c015477f576894ceae134e8b9b8 100644 --- a/third_party/blink/public/common/features.h +++ b/third_party/blink/public/common/features.h @@ -1348,6 +1348,9 @@ BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kTimedHTMLParserBudget); BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kUACHOverrideBlank); +BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE( + kEmulateLoadStartedForInspectorOncePerResource); + // Kill switch for using a custom task runner in the blink scheduler that makes // DeleteSoon/ReleaseSoon less prone to memory leaks. BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE( diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc index 3d889e448fe706e69eac40e61035616b2be3f74c..d8d5b0e0379c4fc65debc4e25766997e5502191e 100644 --- a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc +++ b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc @@ -681,6 +681,19 @@ Resource* ResourceFetcher::CachedResource(const KURL& resource_url) const { return it->value.Get(); } +bool ResourceFetcher::ResourceHasBeenEmulatedLoadStartedForInspector( + const KURL& resource_url) const { + if (resource_url.IsEmpty()) { + return false; + } + KURL url = MemoryCache::RemoveFragmentIdentifierIfNeeded(resource_url); + const auto it = emulated_load_started_for_inspector_resources_map_.find(url); + if (it == emulated_load_started_for_inspector_resources_map_.end()) { + return false; + } + return true; +} + const HeapHashSet> ResourceFetcher::MoveResourceStrongReferences() { document_resource_strong_refs_total_size_ = 0; @@ -2463,11 +2476,25 @@ void ResourceFetcher::EmulateLoadStartedForInspector( if (CachedResource(url)) { return; } + + if (ResourceHasBeenEmulatedLoadStartedForInspector(url)) { + return; + } + if (resource->ErrorOccurred()) { // We should ideally replay the error steps, but we cannot. return; } + if (base::FeatureList::IsEnabled( + features::kEmulateLoadStartedForInspectorOncePerResource)) { + // Update the emulated load started for inspector resources map with the + // resource so that future emulations of the same resource won't happen. + String resource_url = MemoryCache::RemoveFragmentIdentifierIfNeeded(url); + emulated_load_started_for_inspector_resources_map_.Set(resource_url, + resource); + } + ResourceRequest resource_request(url); resource_request.SetRequestContext(request_context); resource_request.SetRequestDestination(request_destination); @@ -2767,6 +2794,7 @@ void ResourceFetcher::Trace(Visitor* visitor) const { visitor->Trace(loaders_); visitor->Trace(non_blocking_loaders_); visitor->Trace(cached_resources_map_); + visitor->Trace(emulated_load_started_for_inspector_resources_map_); visitor->Trace(image_resources_); visitor->Trace(not_loaded_image_resources_); visitor->Trace(preloads_); diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h index b0775b4d44bf800159d0c8ef07a2258df8674b94..af96732a045c35ab443b492bd5b34868eb1ba73e 100644 --- a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h +++ b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h @@ -178,6 +178,7 @@ class PLATFORM_EXPORT ResourceFetcher CodeCacheHost* GetCodeCacheHost(); Resource* CachedResource(const KURL&) const; + bool ResourceHasBeenEmulatedLoadStartedForInspector(const KURL&) const; // Registers an callback to be called with the resource priority of the fetch // made to the specified URL. When `new_load_only` is set to false, @@ -564,6 +565,15 @@ class PLATFORM_EXPORT ResourceFetcher // Weak reference to all the fetched resources. DocumentResourceMap cached_resources_map_; + // When a resource is in the global memory cache but not in the + // cached_resources_map_ and it is referenced (e.g. when the StyleEngine + // processes a @font-face rule), the resource will be emulated via + // `EmulateLoadStartedForInspector` so that it shows up in DevTools. + // In order to ensure that this only occurs once per resource, we keep + // a weak reference to all emulated resources and only emulate resources + // that have not been previously emulated. + DocumentResourceMap emulated_load_started_for_inspector_resources_map_; + // document_resource_strong_refs_ keeps strong references for fonts, images, // scripts and stylesheets within their freshness lifetime. HeapHashSet> document_resource_strong_refs_; diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc index d1947b5070cd537b29fdf4cb1a9e6780e803111f..6f8ef7af61abd84519543ee7628fb1e88ab9b5de 100644 --- a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc +++ b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc @@ -161,6 +161,8 @@ class ResourceFetcherTest : public testing::Test { return request_; } + void ClearLastRequest() { request_ = absl::nullopt; } + private: absl::optional request_; }; @@ -1627,4 +1629,123 @@ TEST_F(ResourceFetcherTest, StrongReferenceThreshold) { ASSERT_FALSE(perform_fetch.Run(KURL("http://127.0.0.1:8000/baz.png"))); } +TEST_F(ResourceFetcherTest, + EmulateLoadStartedForInspectorOncePerResourceDisabled) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndDisableFeature( + features::kEmulateLoadStartedForInspectorOncePerResource); + auto* observer = MakeGarbageCollected(); + + // Set up the initial fetcher and mark the resource as cached. + auto* fetcher = CreateFetcher(); + KURL url("http://127.0.0.1:8000/foo.woff2"); + RegisterMockedURLLoad(url); + FetchParameters fetch_params = + FetchParameters::CreateForTest(ResourceRequest(url)); + Resource* resource = MockResource::Fetch(fetch_params, fetcher, nullptr); + resource->SetStatus(ResourceStatus::kCached); + + ASSERT_NE(fetcher->CachedResource(url), nullptr); + ASSERT_FALSE(fetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + + // Set up the second fetcher. + auto* otherContextFetcher = CreateFetcher(); + otherContextFetcher->SetResourceLoadObserver(observer); + + // Ensure that the url is initially not marked as cached or + // emulated and the observer's last request is empty. + ASSERT_EQ(otherContextFetcher->CachedResource(url), nullptr); + ASSERT_FALSE( + otherContextFetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + ASSERT_EQ(observer->GetLastRequest(), absl::nullopt); + + otherContextFetcher->EmulateLoadStartedForInspector( + resource, url, mojom::blink::RequestContextType::FONT, + network::mojom::RequestDestination::kFont, + fetch_initiator_type_names::kCSS); + + // After the first emulation, ensure that the url is not cached, + // is not marked as emulated and the observer's last + // request is not empty with the feature disabled. + ASSERT_EQ(otherContextFetcher->CachedResource(url), nullptr); + ASSERT_FALSE( + otherContextFetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + ASSERT_NE(observer->GetLastRequest(), absl::nullopt); + + // Clear out the last request to start fresh + observer->ClearLastRequest(); + + otherContextFetcher->EmulateLoadStartedForInspector( + resource, url, mojom::blink::RequestContextType::FONT, + network::mojom::RequestDestination::kFont, + fetch_initiator_type_names::kCSS); + + // After the second emulation, ensure that the url is not cached, + // the resource is not marked as emulated, and the observer's last + // request is not empty with the feature disabled. This means that + // the observer was notified with this emulation. + ASSERT_EQ(otherContextFetcher->CachedResource(url), nullptr); + ASSERT_FALSE( + otherContextFetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + ASSERT_NE(observer->GetLastRequest(), absl::nullopt); +} + +TEST_F(ResourceFetcherTest, + EmulateLoadStartedForInspectorOncePerResourceEnabled) { + auto* observer = MakeGarbageCollected(); + + // Set up the initial fetcher and mark the resource as cached. + auto* fetcher = CreateFetcher(); + KURL url("http://127.0.0.1:8000/foo.woff2"); + RegisterMockedURLLoad(url); + FetchParameters fetch_params = + FetchParameters::CreateForTest(ResourceRequest(url)); + Resource* resource = MockResource::Fetch(fetch_params, fetcher, nullptr); + resource->SetStatus(ResourceStatus::kCached); + + ASSERT_NE(fetcher->CachedResource(url), nullptr); + ASSERT_FALSE(fetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + + // Set up the second fetcher. + auto* otherContextFetcher = CreateFetcher(); + otherContextFetcher->SetResourceLoadObserver(observer); + + // Ensure that the url is initially not cached, not marked as emulated, + // and the observer's last request is empty. + ASSERT_EQ(otherContextFetcher->CachedResource(url), nullptr); + ASSERT_FALSE( + otherContextFetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + ASSERT_EQ(observer->GetLastRequest(), absl::nullopt); + + otherContextFetcher->EmulateLoadStartedForInspector( + resource, url, mojom::blink::RequestContextType::FONT, + network::mojom::RequestDestination::kFont, + fetch_initiator_type_names::kCSS); + + // After the first emulation, ensure that the url is not cached, + // marked as emulated, and the observer's last request is not empty with + // the feature enabled. + ASSERT_EQ(otherContextFetcher->CachedResource(url), nullptr); + ASSERT_TRUE( + otherContextFetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + ASSERT_NE(observer->GetLastRequest(), absl::nullopt); + + // Clear out the last request to start fresh + observer->ClearLastRequest(); + + otherContextFetcher->EmulateLoadStartedForInspector( + resource, url, mojom::blink::RequestContextType::FONT, + network::mojom::RequestDestination::kFont, + fetch_initiator_type_names::kCSS); + + // After the first emulation, ensure that the url is not cached, + // marked as emulated, and the observer's last request is empty with + // the feature enabled. This means that the observer was not + // notified with this emulation. + ASSERT_EQ(otherContextFetcher->CachedResource(url), nullptr); + ASSERT_TRUE( + otherContextFetcher->ResourceHasBeenEmulatedLoadStartedForInspector(url)); + ASSERT_EQ(observer->GetLastRequest(), absl::nullopt); +} + } // namespace blink