From f32637e666d60dcb31153d775cb431596887b852 Mon Sep 17 00:00:00 2001
From: Shivani Sharma <shivanisha@chromium.org>
Date: Wed, 22 Jul 2026 07:57:17 -0700
Subject: [PATCH] [Connection-Allowlist] KeepAlive Retry issue fix

KeepAliveURLLoader allows a blocked keepalive fetch request to schedule
a retry after the renderer disconnects by dropping the original
Connection Allowlist's access-denied error. If this retry occurs after
the document's network restriction id is cleared (60 seconds after
unload), it permits the previously blocked request.

The fix is to make sure that the initial error is checked before the
request is retried. Since C-A's error code is not in either of the
error code lists checked in IsNetErrorEligibleForRetry() or
IsServerGuaranteedToBeNotReachedYet(), the function
KeepAliveURLLoader::IsEligibleForRetry() returns false.

Bug: 497338168

Change-Id: If67f7ada74c5aba497ccae101b2413245439a533
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8130778
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Shivani Sharma <shivanisha@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1666296}
---

diff --git a/content/browser/loader/keep_alive_url_loader.cc b/content/browser/loader/keep_alive_url_loader.cc
index 4028c34..4264fd7 100644
--- a/content/browser/loader/keep_alive_url_loader.cc
+++ b/content/browser/loader/keep_alive_url_loader.cc
@@ -987,6 +987,7 @@
   // received another error signal after this (e.g. OnComplete with error
   // happened, then the disconnection triggers CancelWithStatus).
   url_loader_.reset();
+  last_attempt_completion_status_ = std::nullopt;
 
   // Set a timer to delete self when the max age has been reached. Note that
   // we check if the timer is already set here, because it could've been set
@@ -1216,6 +1217,7 @@
     return false;
   }
 
+  last_attempt_completion_status_ = status;
   // Schedule retry if needed.
   if (MaybeScheduleRetry(status)) {
     return true;
@@ -1359,7 +1361,7 @@
   if (resource_request_.fetch_retry_options.has_value() &&
       resource_request_.fetch_retry_options->retry_after_unload &&
       (IsAttemptingRetry(/*include_failed_retry=*/false) ||
-       MaybeScheduleRetry(/*completion_status=*/std::nullopt))) {
+       MaybeScheduleRetry(last_attempt_completion_status_))) {
     // A retry is already pending or we just scheduled a retry. Don't delete
     // the loader, and instead keep it around for the retry.
     return;
diff --git a/content/browser/loader/keep_alive_url_loader.h b/content/browser/loader/keep_alive_url_loader.h
index 896289e..cdc171a5 100644
--- a/content/browser/loader/keep_alive_url_loader.h
+++ b/content/browser/loader/keep_alive_url_loader.h
@@ -29,6 +29,7 @@
 #include "net/url_request/url_request.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/url_loader_completion_status.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
 #include "services/network/public/mojom/url_response_head.mojom.h"
 #include "third_party/blink/public/common/features.h"
@@ -356,6 +357,10 @@
                            CookiesClearingWillDeleteRetryingLoader);
   FRIEND_TEST_ALL_PREFIXES(KeepAliveURLLoaderServiceRetryTest,
                            FailedMaxAttemptWillForwardLastError);
+  FRIEND_TEST_ALL_PREFIXES(KeepAliveURLLoaderServiceRetryTest,
+                           IneligibleErrorWillNotBeRetriedOnDisconnect);
+  FRIEND_TEST_ALL_PREFIXES(KeepAliveURLLoaderServiceRetryTest,
+                           NoResultWillBeRetriedOnDisconnect);
 
   // These values are persisted to logs. Entries should not be renumbered and
   // numeric values should never be reused.
@@ -506,6 +511,12 @@
   };
   RetryState retry_state_ = RetryState::kNotAttemptingRetry;
 
+  // Stores the completion status of the most recent attempt.
+  // Set in `RetryOrDelayErrorIfNeeded()` when an error occurs, and reset in
+  // `MaybeScheduleRetry()` when a retry is scheduled.
+  std::optional<network::URLLoaderCompletionStatus>
+      last_attempt_completion_status_;
+
   // The last delay used for `retry_timer_` to schedule a retry.
   base::TimeDelta last_retry_delay_;
 
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 350a1ec7..634c9f2 100644
--- a/content/browser/loader/keep_alive_url_loader_service_unittest.cc
+++ b/content/browser/loader/keep_alive_url_loader_service_unittest.cc
@@ -1384,10 +1384,15 @@
   static constexpr base::TimeDelta kMinRetryDeltaForTesting = base::Seconds(10);
   static constexpr double kMinRetryBackoffFactorForTesting = 10.0;
   static constexpr base::TimeDelta kMaxRetryAgeForTesting = base::Days(1);
+  static constexpr base::TimeDelta kDisconnectedLoaderTimeoutForTesting =
+      base::Seconds(30);
 
   void SetUp() override {
     feature_list().InitWithFeaturesAndParameters(
-        {{blink::features::kKeepAliveInBrowserMigration, {}},
+        {{blink::features::kKeepAliveInBrowserMigration,
+          {{"disconnected_loader_timeout_seconds",
+            base::NumberToString(
+                kDisconnectedLoaderTimeoutForTesting.InSeconds())}}},
          {blink::features::kFetchRetry,
           {
               {"max_retry_count",
@@ -1975,4 +1980,105 @@
   EXPECT_FALSE(loader.get());
 }
 
+// Test that when a request completes with an error ineligible for retry, and
+// the renderer subsequently disconnects, the loader will not attempt to retry.
+TEST_F(KeepAliveURLLoaderServiceRetryTest,
+       IneligibleErrorWillNotBeRetriedOnDisconnect) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 10;
+  options.max_age = base::Days(1);
+  options.retry_after_unload = true;
+  resource_request.fetch_retry_options = options;
+
+  // Loads keepalive request:
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote());
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 1);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 1u);
+
+  base::WeakPtr<KeepAliveURLLoader> loader =
+      loader_service().GetLoaderWithRequestIdForTesting(
+          FakeRemoteURLLoaderFactory::kRequestId);
+
+  // Complete with an ineligible error.
+  loader->OnComplete(
+      network::URLLoaderCompletionStatus(net::ERR_NETWORK_ACCESS_REVOKED));
+
+  // The request is not eligible for retry, but the loader is kept alive to
+  // delay sending the error until max_age is reached.
+  EXPECT_FALSE(loader->IsAttemptingRetry(/*include_failed_retry=*/false));
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 1u);
+
+  // Disconnects and unbinds the receiver client & remote loader.
+  renderer_loader_client.ResetReceiver();
+  renderer_loader_factory.reset_remote_url_loader();
+  {
+    base::RunLoop run_loop;
+    run_loop.QuitWhenIdle();
+    run_loop.Run();
+  }
+
+  // Fast forward by the disconnect timeout so OnDisconnectedLoaderTimerFired
+  // runs.
+  task_environment()->FastForwardBy(kDisconnectedLoaderTimeoutForTesting);
+
+  // Because the previous completion status was an ineligible error,
+  // OnDisconnectedLoaderTimerFired should not schedule a retry and should
+  // delete the loader.
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);
+  EXPECT_FALSE(loader.get());
+}
+
+// Test that when no completion result is received before renderer
+// disconnection, the loader will attempt a retry upon disconnect timer firing
+// if configured.
+TEST_F(KeepAliveURLLoaderServiceRetryTest, NoResultWillBeRetriedOnDisconnect) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 10;
+  options.max_age = base::Days(1);
+  options.retry_after_unload = true;
+  resource_request.fetch_retry_options = options;
+
+  // Loads keepalive request:
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote());
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 1);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 1u);
+
+  base::WeakPtr<KeepAliveURLLoader> loader =
+      loader_service().GetLoaderWithRequestIdForTesting(
+          FakeRemoteURLLoaderFactory::kRequestId);
+
+  // Disconnect without completing the request.
+  renderer_loader_client.ResetReceiver();
+  renderer_loader_factory.reset_remote_url_loader();
+  {
+    base::RunLoop run_loop;

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 350a1ec7..634c9f2 100644
--- a/content/browser/loader/keep_alive_url_loader_service_unittest.cc
+++ b/content/browser/loader/keep_alive_url_loader_service_unittest.cc
@@ -1384,10 +1384,15 @@
   static constexpr base::TimeDelta kMinRetryDeltaForTesting = base::Seconds(10);
   static constexpr double kMinRetryBackoffFactorForTesting = 10.0;
   static constexpr base::TimeDelta kMaxRetryAgeForTesting = base::Days(1);
+  static constexpr base::TimeDelta kDisconnectedLoaderTimeoutForTesting =
+      base::Seconds(30);
 
   void SetUp() override {
     feature_list().InitWithFeaturesAndParameters(
-        {{blink::features::kKeepAliveInBrowserMigration, {}},
+        {{blink::features::kKeepAliveInBrowserMigration,
+          {{"disconnected_loader_timeout_seconds",
+            base::NumberToString(
+                kDisconnectedLoaderTimeoutForTesting.InSeconds())}}},
          {blink::features::kFetchRetry,
           {
               {"max_retry_count",
@@ -1975,4 +1980,105 @@
   EXPECT_FALSE(loader.get());
 }
 
+// Test that when a request completes with an error ineligible for retry, and
+// the renderer subsequently disconnects, the loader will not attempt to retry.
+TEST_F(KeepAliveURLLoaderServiceRetryTest,
+       IneligibleErrorWillNotBeRetriedOnDisconnect) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 10;
+  options.max_age = base::Days(1);
+  options.retry_after_unload = true;
+  resource_request.fetch_retry_options = options;
+
+  // Loads keepalive request:
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote());
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 1);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 1u);
+
+  base::WeakPtr<KeepAliveURLLoader> loader =
+      loader_service().GetLoaderWithRequestIdForTesting(
+          FakeRemoteURLLoaderFactory::kRequestId);
+
+  // Complete with an ineligible error.
+  loader->OnComplete(
+      network::URLLoaderCompletionStatus(net::ERR_NETWORK_ACCESS_REVOKED));
+
+  // The request is not eligible for retry, but the loader is kept alive to
+  // delay sending the error until max_age is reached.
+  EXPECT_FALSE(loader->IsAttemptingRetry(/*include_failed_retry=*/false));
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 1u);
+
+  // Disconnects and unbinds the receiver client & remote loader.
+  renderer_loader_client.ResetReceiver();
+  renderer_loader_factory.reset_remote_url_loader();
+  {
+    base::RunLoop run_loop;
+    run_loop.QuitWhenIdle();
+    run_loop.Run();
+  }
+
+  // Fast forward by the disconnect timeout so OnDisconnectedLoaderTimerFired
+  // runs.
+  task_environment()->FastForwardBy(kDisconnectedLoaderTimeoutForTesting);
+
+  // Because the previous completion status was an ineligible error,
+  // OnDisconnectedLoaderTimerFired should not schedule a retry and should
+  // delete the loader.
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 0u);
+  EXPECT_FALSE(loader.get());
+}
+
+// Test that when no completion result is received before renderer
+// disconnection, the loader will attempt a retry upon disconnect timer firing
+// if configured.
+TEST_F(KeepAliveURLLoaderServiceRetryTest, NoResultWillBeRetriedOnDisconnect) {
+  FakeRemoteURLLoaderFactory renderer_loader_factory;
+  MockReceiverURLLoaderClient renderer_loader_client;
+  BindKeepAliveURLLoaderFactory(renderer_loader_factory);
+
+  auto resource_request = CreateResourceRequest(GURL(kTestRequestUrl));
+  network::FetchRetryOptions options;
+  options.max_attempts = 10;
+  options.max_age = base::Days(1);
+  options.retry_after_unload = true;
+  resource_request.fetch_retry_options = options;
+
+  // Loads keepalive request:
+  renderer_loader_factory.CreateLoaderAndStart(
+      resource_request, renderer_loader_client.BindNewPipeAndPassRemote());
+  EXPECT_EQ(network_url_loader_factory().NumPending(), 1);
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 1u);
+
+  base::WeakPtr<KeepAliveURLLoader> loader =
+      loader_service().GetLoaderWithRequestIdForTesting(
+          FakeRemoteURLLoaderFactory::kRequestId);
+
+  // Disconnect without completing the request.
+  renderer_loader_client.ResetReceiver();
+  renderer_loader_factory.reset_remote_url_loader();
+  {
+    base::RunLoop run_loop;
+    run_loop.QuitWhenIdle();
+    run_loop.Run();
+  }
+
+  // Fast forward by the disconnect timeout so OnDisconnectedLoaderTimerFired
+  // runs.
+  task_environment()->FastForwardBy(kDisconnectedLoaderTimeoutForTesting);
+
+  // Because there was no completion status yet, OnDisconnectedLoaderTimerFired
+  // should schedule a retry and keep the loader alive.
+  EXPECT_EQ(loader_service().NumLoadersForTesting(), 1u);
+  EXPECT_TRUE(loader.get());
+  if (loader) {
+    EXPECT_TRUE(loader->IsAttemptingRetry(/*include_failed_retry=*/false));
+  }
+}
+
 }  // namespace content