From 279e99c17920f2fefb34755efb23cda12f6c92d1 Mon Sep 17 00:00:00 2001
From: Ming-Ying Chung <mych@chromium.org>
Date: Wed, 12 Aug 2026 19:32:39 -0700
Subject: [PATCH] [fetch-retry]] Validate fetch_retry_options against feature

The renderer only sets ResourceRequest::fetch_retry_options when the
FetchRetry runtime feature is enabled. However, the browser-side
KeepAliveURLLoaderFactoriesBase::CreateKeepAliveURLLoader did not
re-check the feature or its Origin Trial state before honoring it.

This CL adds the missing browser-side check to validate both the global
feature gate (blink::features::kFetchRetry) and the document-specific
Origin Trial state. If fetch_retry_options is set but the feature is
disabled, the request is rejected via mojo::ReportBadMessage.

Additionally, this CL adds a feature check in
KeepAliveURLLoader::IsEligibleForRetry to prevent retries if the feature
is disabled.

Bug: 513049445
Change-Id: I7401949a8f791b074360ef02204a9c02c0101c13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8181647
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Commit-Queue: Ming-Ying Chung <mych@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1678546}
---

diff --git a/content/browser/loader/keep_alive_url_loader.cc b/content/browser/loader/keep_alive_url_loader.cc
index 64f8c99..9d6535e 100644
--- a/content/browser/loader/keep_alive_url_loader.cc
+++ b/content/browser/loader/keep_alive_url_loader.cc
@@ -914,8 +914,9 @@
 bool KeepAliveURLLoader::IsEligibleForRetry(
     std::optional<network::URLLoaderCompletionStatus> completion_status) const {
   auto retry_options = resource_request_.fetch_retry_options;
-  if (!retry_options.has_value()) {
-    // The fetch must opt-in to retry.
+  if (!retry_options.has_value() ||
+      !base::FeatureList::IsEnabled(blink::features::kFetchRetry)) {
+    // The fetch must opt-in to retry and the feature must be enabled.
     return false;
   }
 
diff --git a/content/browser/loader/keep_alive_url_loader_service.cc b/content/browser/loader/keep_alive_url_loader_service.cc
index 83768c3..6206e1f 100644
--- a/content/browser/loader/keep_alive_url_loader_service.cc
+++ b/content/browser/loader/keep_alive_url_loader_service.cc
@@ -19,6 +19,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/runtime_feature_state/runtime_feature_state_document_data.h"
 #include "content/public/browser/url_loader_throttles.h"
 #include "content/public/browser/web_contents.h"
 #include "mojo/public/cpp/bindings/associated_receiver_set.h"
@@ -73,6 +74,13 @@
   ukm_source_id = navigation_handle->GetNextPageUkmSourceId();
   policy_container_host = rfh->policy_container_host();
   CHECK(policy_container_host);
+
+  cached_is_fetch_retry_enabled.reset();
+  // Cache the feature flag for the newly committed document while
+  // `RenderFrameHostImpl` is guaranteed valid. This preserves feature
+  // state for requests dispatched during document unload whose IPCs arrive
+  // post-destruction.
+  std::ignore = IsFetchRetryEnabled();
 }
 
 bool KeepAliveURLLoaderService::FactoryContext::WasInitiatorDocumentDestroyed()
@@ -95,6 +103,30 @@
   factory = new_factory;
 }
 
+bool KeepAliveURLLoaderService::FactoryContext::IsFetchRetryEnabled() const {
+  if (!base::FeatureList::IsEnabled(blink::features::kFetchRetry)) {
+    return false;
+  }
+  std::optional<bool> overridden_state =
+      base::FeatureList::GetStateIfOverridden(blink::features::kFetchRetry);
+  if (overridden_state == std::make_optional(true)) {
+    return true;
+  }
+  if (cached_is_fetch_retry_enabled.has_value()) {
+    return *cached_is_fetch_retry_enabled;
+  }
+  if (auto* rfh = static_cast<RenderFrameHostImpl*>(
+          weak_document_ptr.AsRenderFrameHostIfValid())) {
+    if (auto* document_data =
+            RuntimeFeatureStateDocumentData::GetForCurrentDocument(rfh)) {
+      cached_is_fetch_retry_enabled =
+          document_data->runtime_feature_state_read_context()
+              .IsFetchRetryEnabled();
+    }
+  }
+  return cached_is_fetch_retry_enabled.value_or(false);
+}
+
 // KeepAliveURLLoaderFactoriesBase is an abstract base class for creating and
 // managing all the KeepAliveURLLoader instances created by multiple factories
 // of the same `Interface`.
@@ -224,6 +256,15 @@
           "resource_request.trusted_params must not be set");
       return nullptr;
     }
+    if (resource_request.fetch_retry_options.has_value() &&
+        !context->IsFetchRetryEnabled()) {
+      mojo::ReportBadMessage(
+          "Unexpected `resource_request` in "
+          "KeepAliveURLLoaderFactoriesBase::CreateLoaderAndStart(): "
+          "resource_request.fetch_retry_options must not be set when "
+          "FetchRetry is disabled");
+      return nullptr;
+    }
 
     // Notifies RenderFrameHostImpl (if any) that a fetch keepalive request is
     // created.
diff --git a/content/browser/loader/keep_alive_url_loader_service.h b/content/browser/loader/keep_alive_url_loader_service.h
index e6f7010..3729af4 100644
--- a/content/browser/loader/keep_alive_url_loader_service.h
+++ b/content/browser/loader/keep_alive_url_loader_service.h
@@ -100,6 +100,12 @@
     void UpdateFactory(
         scoped_refptr<network::SharedURLLoaderFactory> new_factory);
 
+    // Returns true if the `FetchRetry` feature is enabled for the document
+    // associated with this context (or was enabled before the document
+    // unloaded) and the global feature flag is enabled. Lazily computes and
+    // caches the result.
+    bool IsFetchRetryEnabled() const;
+
     // The factory to use for the requests initiated from this context.
     scoped_refptr<network::SharedURLLoaderFactory> factory;
 
@@ -141,6 +147,12 @@
     // been invalidated.
     bool did_commit_navigation = false;
 
+    // Caches whether the FetchRetry feature is enabled for the initiator
+    // document. Lazily computed by `IsFetchRetryEnabled()` while the document's
+    // `RenderFrameHostImpl` is active, preserving feature state if the document
+    // unloads before keepalive request IPCs arrive.
+    mutable std::optional<bool> cached_is_fetch_retry_enabled;
+
     // This must be the last member.
     base::WeakPtrFactory<FactoryContext> weak_ptr_factory{this};
 
diff --git a/content/browser/loader/keep_alive_url_loader_service_unittest.cc b/content/browser/loader/keep_alive_url_loader_service_unittest.cc
index c74d69a..db0f051 100644
--- a/content/browser/loader/keep_alive_url_loader_service_unittest.cc
+++ b/content/browser/loader/keep_alive_url_loader_service_unittest.cc
@@ -565,6 +565,59 @@
       "resource_request.trusted_params must not be set");
 }
 
+TEST_F(KeepAliveURLLoaderServiceTest,
+       LoadRequestWithRetryOptionsWhenFeatureDisabledAndTerminate) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  base::test::ScopedFeatureList overwritten_feature_list;
+  overwritten_feature_list.InitAndDisableFeature(blink::features::kFetchRetry);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 1;
+  resource_request.fetch_retry_options = options;
+
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote(),
+      /*expect_success=*/false);
+
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 0);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);
+  EXPECT_FALSE(renderer_loader_factory.is_remote_url_loader_connected());
+  ExpectMojoBadMessage(
+      "Unexpected `resource_request` in "
+      "KeepAliveURLLoaderFactoriesBase::CreateLoaderAndStart(): "
+      "resource_request.fetch_retry_options must not be set when "
+      "FetchRetry is disabled");
+}
+
+TEST_F(KeepAliveURLLoaderServiceTest,
+       LoadRequestWithRetryOptionsWithoutOriginTrialAndTerminate) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 1;
+  resource_request.fetch_retry_options = options;
+
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote(),
+      /*expect_success=*/false);
+
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 0);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);

diff --git a/content/browser/loader/keep_alive_url_loader_service_unittest.cc b/content/browser/loader/keep_alive_url_loader_service_unittest.cc
index c74d69a..db0f051 100644
--- a/content/browser/loader/keep_alive_url_loader_service_unittest.cc
+++ b/content/browser/loader/keep_alive_url_loader_service_unittest.cc
@@ -565,6 +565,59 @@
       "resource_request.trusted_params must not be set");
 }
 
+TEST_F(KeepAliveURLLoaderServiceTest,
+       LoadRequestWithRetryOptionsWhenFeatureDisabledAndTerminate) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  base::test::ScopedFeatureList overwritten_feature_list;
+  overwritten_feature_list.InitAndDisableFeature(blink::features::kFetchRetry);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 1;
+  resource_request.fetch_retry_options = options;
+
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote(),
+      /*expect_success=*/false);
+
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 0);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);
+  EXPECT_FALSE(renderer_loader_factory.is_remote_url_loader_connected());
+  ExpectMojoBadMessage(
+      "Unexpected `resource_request` in "
+      "KeepAliveURLLoaderFactoriesBase::CreateLoaderAndStart(): "
+      "resource_request.fetch_retry_options must not be set when "
+      "FetchRetry is disabled");
+}
+
+TEST_F(KeepAliveURLLoaderServiceTest,
+       LoadRequestWithRetryOptionsWithoutOriginTrialAndTerminate) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 1;
+  resource_request.fetch_retry_options = options;
+
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote(),
+      /*expect_success=*/false);
+
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 0);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);
+  EXPECT_FALSE(renderer_loader_factory.is_remote_url_loader_connected());
+  ExpectMojoBadMessage(
+      "Unexpected `resource_request` in "
+      "KeepAliveURLLoaderFactoriesBase::CreateLoaderAndStart(): "
+      "resource_request.fetch_retry_options must not be set when "
+      "FetchRetry is disabled");
+}
+
 TEST_F(KeepAliveURLLoaderServiceTest, LoadRequestAfterPageIsUnloaded) {
   FakeRemoteURLLoaderFactory renderer_loader_factory;
   MockReceiverURLLoaderClient renderer_loader_client;
@@ -1418,6 +1471,57 @@
 }
 
 TEST_F(FetchLaterKeepAliveURLLoaderServiceTest,
+       LoadFetchLaterRequestWithRetryOptionsWhenFeatureDisabledAndTerminate) {
+  FakeRemoteFetchLaterLoaderFactory renderer_loader_factory;
+  BindFetchLaterLoaderFactory(renderer_loader_factory);
+
+  base::test::ScopedFeatureList overwritten_feature_list;
+  overwritten_feature_list.InitAndDisableFeature(blink::features::kFetchRetry);
+
+  auto resource_request =
+      CreateFetchLaterResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 1;
+  resource_request.fetch_retry_options = options;
+  renderer_loader_factory.CreateLoader(resource_request,
+                                       /*expect_success=*/false);
+
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 0);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);
+  EXPECT_FALSE(
+      renderer_loader_factory.is_remote_fetch_later_loader_connected());
+  ExpectMojoBadMessage(
+      "Unexpected `resource_request` in "
+      "KeepAliveURLLoaderFactoriesBase::CreateLoaderAndStart(): "
+      "resource_request.fetch_retry_options must not be set when "
+      "FetchRetry is disabled");
+}
+
+TEST_F(FetchLaterKeepAliveURLLoaderServiceTest,
+       LoadFetchLaterRequestWithRetryOptionsWithoutOriginTrialAndTerminate) {
+  FakeRemoteFetchLaterLoaderFactory renderer_loader_factory;
+  BindFetchLaterLoaderFactory(renderer_loader_factory);
+
+  auto resource_request =
+      CreateFetchLaterResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 1;
+  resource_request.fetch_retry_options = options;
+  renderer_loader_factory.CreateLoader(resource_request,
+                                       /*expect_success=*/false);
+
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 0);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);
+  EXPECT_FALSE(
+      renderer_loader_factory.is_remote_fetch_later_loader_connected());
+  ExpectMojoBadMessage(
+      "Unexpected `resource_request` in "
+      "KeepAliveURLLoaderFactoriesBase::CreateLoaderAndStart(): "
+      "resource_request.fetch_retry_options must not be set when "
+      "FetchRetry is disabled");
+}
+
+TEST_F(FetchLaterKeepAliveURLLoaderServiceTest,
        LoadFetchLaterRequestAndDeferred) {
   FakeRemoteFetchLaterLoaderFactory renderer_loader_factory;
   BindFetchLaterLoaderFactory(renderer_loader_factory);