Make PrefetchService receive PrefetchRequest, not PrefetchContainer Before this CL: 1. A `PrefetchContainer` is created by the users of `PrefetchService` and then passed to `PrefetchService`. 2. The `PrefetchContainer` is either added to `PrefetchService::owned_prefetches_`, or destroyed immediately when it is merged into an existing `PrefetchContainer`. After this CL: 1. A `PrefetchRequest` is created by the users of `PrefetchService` and then passed to `PrefetchService`. 2. When it is not merged into an existing `PrefetchContainer`, a new `PrefetchContainer` is created and added to `PrefetchService::owned_prefetches_`. In order to: - Avoid transient `PrefetchContainer` construction followed by its immediate destruction. - Make `PrefetchContainer` always owned by `PrefetchService::owned_prefetches_` in non-test code. - Create `PrefetchContainer`s only from `PrefetchService` in non-test code. - Clarify that `PrefetchRequest`s are merged into an `PrefetchContainer`. Bug: 437631382 Change-Id: I0c29d7f99eff21a090bd636eaf805392ddafb592 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6897472 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Reviewed-by: Taiyo Mizuhashi <taiyo@chromium.org> Reviewed-by: Ken Okada <kenoss@chromium.org> Cr-Commit-Position: refs/heads/main@{#1512056}
diff --git a/content/browser/browser_context.cc b/content/browser/browser_context.cc index f89b24f..99cf7ba 100644 --- a/content/browser/browser_context.cc +++ b/content/browser/browser_context.cc
@@ -226,8 +226,7 @@ /*attempt=*/nullptr, additional_headers, std::move(request_status_listener), ttl, should_append_variations_header, should_disable_block_until_head_timeout); - return prefetch_service->AddPrefetchContainerWithHandle( - PrefetchContainer::Create(std::move(request))); + return prefetch_service->AddPrefetchRequestWithHandle(std::move(request)); } void BrowserContext::UpdatePrefetchServiceDelegateAcceptLanguageHeader(
diff --git a/content/browser/preloading/prefetch/prefetch_container.cc b/content/browser/preloading/prefetch/prefetch_container.cc index fee9a19..1c7fd99 100644 --- a/content/browser/preloading/prefetch/prefetch_container.cc +++ b/content/browser/preloading/prefetch/prefetch_container.cc
@@ -233,6 +233,7 @@ // static std::unique_ptr<PrefetchContainer> PrefetchContainer::Create( + base::PassKey<PrefetchService>, std::unique_ptr<const PrefetchRequest> request) { return std::make_unique<PrefetchContainer>(base::PassKey<PrefetchContainer>(), std::move(request)); @@ -1555,13 +1556,9 @@ } } -void PrefetchContainer::MigrateNewlyAdded( - std::unique_ptr<PrefetchContainer> added) { - // `inherited_preload_pipeline_infos_` increases only if it is managed under - // `PrefetchService`. - CHECK(added->inherited_preload_pipeline_infos_.empty()); - - // Propagate eligibility (and status) to `added`. +void PrefetchContainer::MergeNewPrefetchRequest( + std::unique_ptr<const PrefetchRequest> prefetch_request) { + // Propagate eligibility (and status) to `prefetch_request`. // // Assume we don't. (*) case is problematic. // @@ -1569,7 +1566,7 @@ // the following `OnEligibilityCheckComplete()` and // `SetPrefetchStatusWithoutUpdatingTriggeringOutcome()`. // - If eligibility is got and ineligible, this `PrefetchContainer` is - // `kNotServed` and `MigrateNewlyAdded()` is not called. + // `kNotServed` and `MergeNewPrefetchRequest()` is not called. // - If eligibility is got and `kEligible`: // - If status is not got, status will be propagated by the following // `SetPrefetchStatusWithoutUpdatingTriggeringOutcome()`. @@ -1582,7 +1579,7 @@ // `kPrefetchResponseUsed` will be propagated at the prefetch matching // end. // - If status is got and failure, this `PrefetchContainer` is `kNotServed` - // and `MigrateNewlyAdded()` is not called. + // and `MergeNewPrefetchRequest()` is not called. // // In (*), `PrerenderHost` have to cancel prerender with eligibility // `kUnspecified` and status failure. It's relatively complicated condition. @@ -1591,7 +1588,7 @@ // // To make things simple, we propagate both eligibility and status. scoped_refptr<PreloadPipelineInfoImpl> added_preload_pipeline_info = - base::WrapRefCounted(&added->request().preload_pipeline_info()); + base::WrapRefCounted(&prefetch_request->preload_pipeline_info()); added_preload_pipeline_info->SetPrefetchEligibility( request().preload_pipeline_info().prefetch_eligibility()); @@ -1603,7 +1600,8 @@ inherited_preload_pipeline_infos_.push_back( std::move(added_preload_pipeline_info)); - is_likely_ahead_of_prerender_ |= added->is_likely_ahead_of_prerender_; + is_likely_ahead_of_prerender_ |= CalculateIsLikelyAheadOfPrerender( + prefetch_request->preload_pipeline_info()); } void PrefetchContainer::NotifyPrefetchRequestWillBeSent(
diff --git a/content/browser/preloading/prefetch/prefetch_container.h b/content/browser/preloading/prefetch/prefetch_container.h index 6d342da..462b7d94 100644 --- a/content/browser/preloading/prefetch/prefetch_container.h +++ b/content/browser/preloading/prefetch/prefetch_container.h
@@ -41,6 +41,7 @@ class PrefetchNetworkContext; class PrefetchRequest; class PrefetchResponseReader; +class PrefetchService; class PrefetchServingHandle; class PrefetchServingPageMetricsContainer; class PrefetchSingleRedirectHop; @@ -99,9 +100,10 @@ // `PrefetchService::MakePrefetchRequest()`. class CONTENT_EXPORT PrefetchContainer { public: - // TODO(https://crbug.com/437631382): add `base::PassKey<PrefetchService>` for - // the `Create()` for non-tests. + // In non-test, `PrefetchContainer` should be only created and owned by + // `PrefetchService`. static std::unique_ptr<PrefetchContainer> Create( + base::PassKey<PrefetchService>, std::unique_ptr<const PrefetchRequest> request); static std::unique_ptr<PrefetchContainer> CreateForTesting( std::unique_ptr<const PrefetchRequest> request); @@ -509,12 +511,12 @@ bool IsLikelyAheadOfPrerender() const { return is_likely_ahead_of_prerender_; } - // TODO(crbug.com/372186548): Revisit for right naming. + + // Merge a new `prefetch_request` into this `PrefetchContainer`. // - // Migrate newly added `PrefetchContainer` into this as keys are conflicted. - // - // See also `PrefetchService::AddPrefetchContainerWithoutStartingPrefetch()`. - void MigrateNewlyAdded(std::unique_ptr<PrefetchContainer> added); + // See also `PrefetchService::AddPrefetchContainerInternal()`. + void MergeNewPrefetchRequest( + std::unique_ptr<const PrefetchRequest> prefetch_request); // Handles loader related events. Currently used for DevTools and metrics. void NotifyPrefetchRequestWillBeSent(
diff --git a/content/browser/preloading/prefetch/prefetch_document_manager.cc b/content/browser/preloading/prefetch/prefetch_document_manager.cc index 707fba9..53a4084f 100644 --- a/content/browser/preloading/prefetch/prefetch_document_manager.cc +++ b/content/browser/preloading/prefetch/prefetch_document_manager.cc
@@ -271,8 +271,7 @@ referring_page_metrics_.prefetch_attempted_count++; all_prefetches_[all_prefetches_key] = - prefetch_service->AddPrefetchContainerWithHandle( - PrefetchContainer::Create(std::move(request))); + prefetch_service->AddPrefetchRequestWithHandle(std::move(request)); } bool PrefetchDocumentManager::IsPrefetchAttemptFailedOrDiscarded(
diff --git a/content/browser/preloading/prefetch/prefetch_service.cc b/content/browser/preloading/prefetch/prefetch_service.cc index 2ba4999..aa744a1 100644 --- a/content/browser/preloading/prefetch/prefetch_service.cc +++ b/content/browser/preloading/prefetch/prefetch_service.cc
@@ -371,8 +371,8 @@ return origin_prober_.get(); } -void PrefetchService::AddPrefetchContainerWithoutStartingPrefetch( - std::unique_ptr<PrefetchContainer> owned_prefetch_container) { +base::WeakPtr<PrefetchContainer> PrefetchService::AddPrefetchRequestInternal( + std::unique_ptr<const PrefetchRequest> prefetch_request) { enum class Action { kTakeOldWithMigration, kReplaceOldWithNew, @@ -402,7 +402,7 @@ // // TODO(crbug.com/372186548): Revisit the merging process and comments here // and below. - auto prefetch_iter = owned_prefetches().find(owned_prefetch_container->key()); + auto prefetch_iter = owned_prefetches().find(prefetch_request->key()); Action action = [&]() { if (prefetch_iter == owned_prefetches().end()) { return Action::kTakeNew; @@ -435,7 +435,7 @@ // With `kPrerender2FallbackPrefetchSpecRules`, B' triggers prefetch ahead // of prerender B for URL X. Sites use SpecRules A+B' with expectation // "prefetch X then prerender X", but the order of - // `PrefetchService::AddPrefetchContainer*()` for A and B is unstable in + // `PrefetchService::AddPrefetchRequest*()` for A and B is unstable in // general. // // `PrerenderHost` of B' needs to know eligibility and status of B. We use @@ -453,28 +453,26 @@ switch (action) { case Action::kTakeOldWithMigration: - prefetch_iter->second->MigrateNewlyAdded( - std::move(owned_prefetch_container)); + prefetch_iter->second->MergeNewPrefetchRequest( + std::move(prefetch_request)); if (UsePrefetchScheduler()) { scheduler_->NotifyAttributeMightChangedAndProgressAsync( *prefetch_iter->second, /*should_progress=*/false); } - break; + return nullptr; case Action::kReplaceOldWithNew: ResetPrefetchContainer(prefetch_iter->second->GetWeakPtr(), /*should_progress=*/false); - AddPrefetchContainerToOwnedPrefetches( - std::move(owned_prefetch_container)); - break; + return CreatePrefetchContainer(std::move(prefetch_request)); case Action::kTakeNew: - AddPrefetchContainerToOwnedPrefetches( - std::move(owned_prefetch_container)); - break; + return CreatePrefetchContainer(std::move(prefetch_request)); } } -void PrefetchService::AddPrefetchContainerToOwnedPrefetches( - std::unique_ptr<PrefetchContainer> owned_prefetch_container) { +base::WeakPtr<PrefetchContainer> PrefetchService::CreatePrefetchContainer( + std::unique_ptr<const PrefetchRequest> prefetch_request) { + auto owned_prefetch_container = PrefetchContainer::Create( + base::PassKey<PrefetchService>(), std::move(prefetch_request)); const base::WeakPtr<PrefetchContainer> prefetch_container = owned_prefetch_container->GetWeakPtr(); @@ -487,6 +485,8 @@ prefetch_container->OnAddedToPrefetchService(); prefetch_container->AddObserver(this); + + return prefetch_container; } bool PrefetchService::IsPrefetchDuplicate( @@ -641,12 +641,10 @@ callback; }; -std::unique_ptr<PrefetchHandle> PrefetchService::AddPrefetchContainerWithHandle( - std::unique_ptr<PrefetchContainer> owned_prefetch_container) { +std::unique_ptr<PrefetchHandle> PrefetchService::AddPrefetchRequestWithHandle( + std::unique_ptr<const PrefetchRequest> prefetch_request) { base::WeakPtr<PrefetchContainer> prefetch_container = - owned_prefetch_container->GetWeakPtr(); - AddPrefetchContainerWithoutStartingPrefetch( - std::move(owned_prefetch_container)); + AddPrefetchRequestInternal(std::move(prefetch_request)); if (prefetch_container) { PrefetchUrl(prefetch_container); @@ -656,12 +654,9 @@ } base::WeakPtr<PrefetchContainer> -PrefetchService::AddPrefetchContainerWithoutStartingPrefetchForTesting( - std::unique_ptr<PrefetchContainer> prefetch_container) { - base::WeakPtr<PrefetchContainer> weak_prefetch_container = - prefetch_container->GetWeakPtr(); - AddPrefetchContainerWithoutStartingPrefetch(std::move(prefetch_container)); - return weak_prefetch_container; +PrefetchService::AddPrefetchRequestWithoutStartingPrefetchForTesting( + std::unique_ptr<const PrefetchRequest> prefetch_request) { + return AddPrefetchRequestInternal(std::move(prefetch_request)); } void PrefetchService::PrefetchUrl(
diff --git a/content/browser/preloading/prefetch/prefetch_service.h b/content/browser/preloading/prefetch/prefetch_service.h index a418aa0..e34a4610 100644 --- a/content/browser/preloading/prefetch/prefetch_service.h +++ b/content/browser/preloading/prefetch/prefetch_service.h
@@ -108,18 +108,22 @@ // |prefetch_container| to the default network context. virtual void CopyIsolatedCookies(const PrefetchServingHandle& serving_handle); - // Adds `PrefetchContainer` under control of `PrefetchService` and returns - // PrefetchHandle so that the caller can control prefetch resources associated - // with this. + // Adds a `PrefetchContainer` created from the `PrefetchRequest` under control + // of `PrefetchService` and returns `PrefetchHandle` so that the caller can + // control prefetch resources associated with this. // - // `AddPrefetchContainer*()` synchronously destruct `prefetch_container` if - // the key conflicted to the one already added with migration of some - // attributes. See also `MigrateNewlyaAdded()`. - [[nodiscard]] std::unique_ptr<PrefetchHandle> AddPrefetchContainerWithHandle( - std::unique_ptr<PrefetchContainer> prefetch_container); + // If the request is merged into an existing `PrefetchContainer`, some of + // `prefetch_request` attributes are migrated to the `PrefetchContainer` and + // this returns a `PrefetchHandle` with null `PrefetchContainer`. + // TODO(https://crbug.com/390329781): In the merging case, we should ideally + // return a `PrefetchHandle` that points to the existing `PrefetchContainer` + // to which `prefetch_request` is merged into. + [[nodiscard]] std::unique_ptr<PrefetchHandle> AddPrefetchRequestWithHandle( + std::unique_ptr<const PrefetchRequest> prefetch_request); + [[nodiscard]] base::WeakPtr<PrefetchContainer> - AddPrefetchContainerWithoutStartingPrefetchForTesting( - std::unique_ptr<PrefetchContainer> prefetch_container); + AddPrefetchRequestWithoutStartingPrefetchForTesting( + std::unique_ptr<const PrefetchRequest> prefetch_request); // Returns `true` if a new prefetch request with `url` and // `no_vary_search_hint` has a duplicate in the prefetch cache and thus the @@ -285,14 +289,21 @@ CheckEligibilityParams params, PreloadingEligibility eligibility); - // Adds `prefetch_container` to the cache but doesn't initiate prefetching. - // Use `AddPrefetchContainerWithHandle()` for non-test cases. - void AddPrefetchContainerWithoutStartingPrefetch( - std::unique_ptr<PrefetchContainer> prefetch_container); + // The core method to add a prefetch request. + // + // Returns non-null `PrefetchContainer`, if the `prefetch_request` creates a + // new `PrefetchContainer`. + // + // This doesn't initiate prefetching, so the caller should call + // `PrefetchUrl()` if needed. + // + // Use `AddPrefetchRequestWithHandle()` for non-test cases. + base::WeakPtr<PrefetchContainer> AddPrefetchRequestInternal( + std::unique_ptr<const PrefetchRequest> prefetch_request); - // Adds to `owned_prefetches_`. - void AddPrefetchContainerToOwnedPrefetches( - std::unique_ptr<PrefetchContainer> prefetch_container); + // Creates a new `PrefetchContainer` and adds it to `owned_prefetches_`. + base::WeakPtr<PrefetchContainer> CreatePrefetchContainer( + std::unique_ptr<const PrefetchRequest> prefetch_request); // Starts the network requests for as many prefetches in |prefetch_queue_| as // possible. @@ -493,11 +504,10 @@ // TODO(crbug.com/406754449): Remove it. std::optional<PrefetchKey> active_prefetch_; - // Prefetches owned by `this`. All `PrefetchContainer`s added by - // `AddPrefetchContainer*` will be stored here. + // Prefetches owned by `this`. All `PrefetchContainer`s will be stored here. // // `PrefetchContainer`s in `owned_prefetches_` must be always: - // - Added by `AddPrefetchContainerToOwnedPrefetches()`. + // - Added by `CreatePrefetchContainer()`. // - Destructed either by: // - `ResetPrefetchContainer()` or // - `~PrefetchService()` dtor.
diff --git a/content/browser/preloading/prefetch/prefetch_service_unittest.cc b/content/browser/preloading/prefetch/prefetch_service_unittest.cc index 2719bc0..00ede65 100644 --- a/content/browser/preloading/prefetch/prefetch_service_unittest.cc +++ b/content/browser/preloading/prefetch/prefetch_service_unittest.cc
@@ -470,8 +470,8 @@ PreloadPipelineInfo::Create( /*planned_max_preloading_type=*/PreloadingType::kPrefetch), /*attempt=*/nullptr); - return prefetch_service().AddPrefetchContainerWithHandle( - PrefetchContainer::CreateForTesting(std::move(prefetch_request))); + return prefetch_service().AddPrefetchRequestWithHandle( + std::move(prefetch_request)); } [[nodiscard]] std::unique_ptr<content::PrefetchHandle> @@ -7235,8 +7235,8 @@ PreloadPipelineInfo::Create(planned_max_preloading_type), attempt->GetWeakPtr()); return prefetch_service() - .AddPrefetchContainerWithoutStartingPrefetchForTesting( - PrefetchContainer::CreateForTesting(std::move(prefetch_request))); + .AddPrefetchRequestWithoutStartingPrefetchForTesting( + std::move(prefetch_request)); } private:
diff --git a/content/browser/preloading/prefetch/prefetch_url_loader_interceptor_unittest.cc b/content/browser/preloading/prefetch/prefetch_url_loader_interceptor_unittest.cc index d991cb1..e178c560 100644 --- a/content/browser/preloading/prefetch/prefetch_url_loader_interceptor_unittest.cc +++ b/content/browser/preloading/prefetch/prefetch_url_loader_interceptor_unittest.cc
@@ -395,8 +395,8 @@ /*planned_max_preloading_type=*/PreloadingType::kPrefetch), attempt->GetWeakPtr()); return GetPrefetchService() - ->AddPrefetchContainerWithoutStartingPrefetchForTesting( - PrefetchContainer::CreateForTesting(std::move(prefetch_request))); + ->AddPrefetchRequestWithoutStartingPrefetchForTesting( + std::move(prefetch_request)); } base::WeakPtr<PrefetchContainer> CreateSpeculationRulesPrefetchContainer( @@ -420,8 +420,8 @@ /*planned_max_preloading_type=*/PreloadingType::kPrefetch), /*attempt=*/nullptr); return GetPrefetchService() - ->AddPrefetchContainerWithoutStartingPrefetchForTesting( - PrefetchContainer::CreateForTesting(std::move(prefetch_request))); + ->AddPrefetchRequestWithoutStartingPrefetchForTesting( + std::move(prefetch_request)); } void SimulateCookieCopyProcess(PrefetchContainer& prefetch_container) {
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index a5f02a9..1544df8d 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc
@@ -12009,8 +12009,7 @@ std::move(preload_pipeline_info), std::move(attempt), holdback_status_override, std::move(ttl)); - return prefetch_service->AddPrefetchContainerWithHandle( - PrefetchContainer::Create(std::move(request))); + return prefetch_service->AddPrefetchRequestWithHandle(std::move(request)); } std::unique_ptr<PrerenderHandle> WebContentsImpl::StartPrerendering(