From 69fd5caaf52d636159a13ee893e63de0fd249495 Mon Sep 17 00:00:00 2001
From: Matt Menke <mmenke@chromium.org>
Date: Mon, 09 Mar 2026 12:05:58 -0700
Subject: [PATCH] Make NetErrorAutoReloader less aggressive.

On certain main frame network errors, NetErrorAutoReloader will
periodically reload the page, and keep on doing so until a new page is
committed, checking on every failed commit if the last commit was an
error page that needed reloading, and starting a reload timer if so,
as well as cancelling commits of an identical new error page. It does
not check the reason that a commit failed, so a commit being cancelled
because of a 204, or a download, will not stop the reloaded from
trying to reload the page.

This CL instead only starts the autoreload time on failed commits if
that failed commit was due to the AutoReloader itself cancelling the
previous commit, due to it being the same error page.

So now, e.g., cancelling an auto reload, triggering a download (which
looks like a cancelled navigation), or starting a new load and then
cancelling it before commit will all stop the auto reload timer.
This is a bit more user friendly, though going offline and then online
again will restart the reload timer (as will another navigation
resulting in a network error, whether it's the same error or a new
one).

Fixed: 475265304
Change-Id: If12a2256c33bf5f91c7ad544c0ad3e2f682df855
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7629083
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: mmenke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1596504}
---

diff --git a/components/error_page/content/browser/net_error_auto_reloader.cc b/components/error_page/content/browser/net_error_auto_reloader.cc
index 5eb7c6c9..5808aef 100644
--- a/components/error_page/content/browser/net_error_auto_reloader.cc
+++ b/components/error_page/content/browser/net_error_auto_reloader.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <array>
+#include <map>
 
 #include "base/functional/callback.h"
 #include "base/logging.h"
@@ -164,7 +165,7 @@
 
   // Suppress automatic reload as long as any navigations are pending.
   PauseAutoReloadTimerIfRunning();
-  pending_navigations_.insert(handle);
+  pending_navigations_.emplace(handle, IsSuppressedErrorPage(false));
 }
 
 void NetErrorAutoReloader::DidFinishNavigation(
@@ -173,7 +174,22 @@
     return;
   }
 
-  pending_navigations_.erase(handle);
+  auto pending_navigation = pending_navigations_.find(handle);
+  bool is_suppressed_error_page = false;
+  // Per comments above MaybeCreateAndAddNavigationThrottle(), the
+  // NetErrorAutoReloader is only created for a WebContents as needed, when
+  // creating throttles, so it never receives the navigation start event for a
+  // tab's initial navigation, and as a result, the first navigation is never
+  // added to `pending_navigations_`. This isn't a problem, as the
+  // NetErrorAutoReloader doesn't do anything until after an error page commits,
+  // but it does mean that we have to handle the case where the committed
+  // navigation isn't listed in `pending_navigations_`.
+  if (pending_navigation != pending_navigations_.end()) {
+    is_suppressed_error_page =
+        pending_navigation->second == IsSuppressedErrorPage(true);
+    pending_navigations_.erase(pending_navigation);
+  }
+
   if (!handle->HasCommitted()) {
     // This navigation was cancelled and not committed. If there are still other
     // pending navigations, or we aren't sitting on a error page which allows
@@ -182,11 +198,14 @@
       return;
     }
 
-    // The last pending navigation was just cancelled and we're sitting on an
-    // error page which allows auto-reload. Schedule the next auto-reload
-    // attempt.
+    // There are no pending navigations, so there's no auto-reload in progress.
     is_auto_reload_in_progress_ = false;
-    ScheduleNextAutoReload();
+
+    // The last pending navigation was just cancelled due to resulting in an
+    // identical error page as before. Schedule the next auto-reload attempt.
+    if (is_suppressed_error_page) {
+      ScheduleNextAutoReload();
+    }
     return;
   }
 
@@ -334,6 +353,8 @@
 
 bool NetErrorAutoReloader::ShouldSuppressErrorPage(
     content::NavigationHandle* handle) {
+  DCHECK(pending_navigations_.contains(handle));
+
   // We already verified these conditions when the throttle was created, but now
   // that the throttle is about to fail its navigation, we double-check in case
   // another navigation has committed in the interim.
@@ -343,6 +364,7 @@
     return false;
   }
 
+  pending_navigations_[handle] = IsSuppressedErrorPage(true);
   return true;
 }
 
diff --git a/components/error_page/content/browser/net_error_auto_reloader.h b/components/error_page/content/browser/net_error_auto_reloader.h
index 8bced091..9057190 100644
--- a/components/error_page/content/browser/net_error_auto_reloader.h
+++ b/components/error_page/content/browser/net_error_auto_reloader.h
@@ -7,14 +7,15 @@
 
 #include <stddef.h>
 
+#include <map>
 #include <memory>
 #include <optional>
-#include <set>
 
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "base/types/strong_alias.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "net/base/net_errors.h"
@@ -46,7 +47,7 @@
   NetErrorAutoReloader& operator=(const NetErrorAutoReloader&) = delete;
   ~NetErrorAutoReloader() override;
 
-  // Maybe installs a throttle for the given navigation, lazily initializing the
+  // Maybe install a throttle for the given navigation, lazily initializing the
   // appropriate WebContents' NetErrorAutoReloader instance if necessary. For
   // embedders wanting to use NetErrorAutoReload's behavior, it's sufficient to
   // call this from ContentBrowserClient::CreateThrottlesForNavigation for each
@@ -103,9 +104,15 @@
     net::Error error;
   };
 
+  // True if a NavigationHandle corresponds to a load that was suppressed due to
+  // being a redundant error page load.
+  using IsSuppressedErrorPage =
+      base::StrongAlias<struct IsSuppressedErrorPageTag, bool>;
+
   raw_ptr<network::NetworkConnectionTracker> connection_tracker_;
   bool is_online_ = true;
-  std::set<raw_ptr<content::NavigationHandle, SetExperimental>>
+  std::map<raw_ptr<content::NavigationHandle, SetExperimental>,
+           IsSuppressedErrorPage>
       pending_navigations_;
   std::optional<base::OneShotTimer> next_reload_timer_;
   std::optional<ErrorPageInfo> current_reloadable_error_page_info_;
diff --git a/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc b/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc
index 0e777fb3..d883833d 100644
--- a/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc
+++ b/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc
@@ -10,6 +10,9 @@
 
 #include "base/memory/raw_ptr.h"
 #include "base/test/bind.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/download_manager.h"
+#include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_throttle_registry.h"
 #include "content/public/browser/web_contents.h"
@@ -479,9 +482,74 @@
   EXPECT_EQ(std::nullopt, GetCurrentAutoReloadDelay());
 
   // Now cancel the deferred navigation and observe that auto-reload for the
-  // error page is rescheduled.
+  // error page is cancelled.
   deferrer.CancelAndWaitForNavigationToFinish();
+  EXPECT_EQ(std::nullopt, GetCurrentAutoReloadDelay());
+}
+
+// A DownloadManagerDelegate that claims it handles all downloads, while
+// silently dropping them instead. This prevents any save dialog, or trying to
+// save anything to disk.
+class TestShellDownloadManagerDelegate
+    : public content::DownloadManagerDelegate {
+ public:
+  TestShellDownloadManagerDelegate() = default;
+  ~TestShellDownloadManagerDelegate() override = default;
+
+  // DownloadManagerDelegate:
+  bool InterceptDownloadIfApplicable(
+      const GURL& url,
+      const std::string& user_agent,
+      const std::string& content_disposition,

diff --git a/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc b/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc
index 0e777fb3..d883833d 100644
--- a/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc
+++ b/components/error_page/content/browser/net_error_auto_reloader_browsertest.cc
@@ -10,6 +10,9 @@
 
 #include "base/memory/raw_ptr.h"
 #include "base/test/bind.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/download_manager.h"
+#include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_throttle_registry.h"
 #include "content/public/browser/web_contents.h"
@@ -479,9 +482,74 @@
   EXPECT_EQ(std::nullopt, GetCurrentAutoReloadDelay());
 
   // Now cancel the deferred navigation and observe that auto-reload for the
-  // error page is rescheduled.
+  // error page is cancelled.
   deferrer.CancelAndWaitForNavigationToFinish();
+  EXPECT_EQ(std::nullopt, GetCurrentAutoReloadDelay());
+}
+
+// A DownloadManagerDelegate that claims it handles all downloads, while
+// silently dropping them instead. This prevents any save dialog, or trying to
+// save anything to disk.
+class TestShellDownloadManagerDelegate
+    : public content::DownloadManagerDelegate {
+ public:
+  TestShellDownloadManagerDelegate() = default;
+  ~TestShellDownloadManagerDelegate() override = default;
+
+  // DownloadManagerDelegate:
+  bool InterceptDownloadIfApplicable(
+      const GURL& url,
+      const std::string& user_agent,
+      const std::string& content_disposition,
+      const std::string& mime_type,
+      const std::string& request_origin,
+      int64_t content_length,
+      bool is_transient,
+      bool is_content_initiated,
+      content::WebContents* web_contents) override {
+    run_loop_.Quit();
+    return true;
+  }
+
+  void WaitForDownload() { run_loop_.Run(); }
+
+ private:
+  base::RunLoop run_loop_;
+};
+
+// When an autoreload results in a download, and thus cancels the navigation,
+// auto-reloading is stopped.
+IN_PROC_BROWSER_TEST_F(NetErrorAutoReloaderBrowserTest,
+                       AutoReloadDownloadStopsAutoReload) {
+  GURL download_url = embedded_test_server()->GetURL("/download-test1.lib");
+
+  // Force an error to initiate auto-reload.
+  auto interceptor = std::make_unique<NetErrorUrlInterceptor>(
+      download_url, net::ERR_CONNECTION_RESET);
+
+  EXPECT_FALSE(NavigateMainFrame(download_url));
   EXPECT_EQ(GetDelayForReloadCount(0), GetCurrentAutoReloadDelay());
+  interceptor.reset();
+
+  TestShellDownloadManagerDelegate test_delegate;
+  content::DownloadManager* manager =
+      shell()->web_contents()->GetBrowserContext()->GetDownloadManager();
+  manager->GetDelegate()->Shutdown();
+  manager->SetDelegate(&test_delegate);
+
+  content::TestNavigationManager navigation(web_contents(), download_url);
+  ForceScheduledAutoReloadNow();
+  // This is considered a successful navigation by the WebContents, despite the
+  // navigation being cancelled, from the perspective of NavigationObservers.
+  EXPECT_TRUE(navigation.WaitForNavigationFinished());
+  test_delegate.WaitForDownload();
+
+  // The error page should still be showing.
+  EXPECT_TRUE(web_contents()->GetPrimaryMainFrame()->IsErrorDocument());
+  // There should be no new auto reload pending.
+  EXPECT_EQ(std::nullopt, GetCurrentAutoReloadDelay());
+
+  manager->SetDelegate(nullptr);
 }
 
 // An error page while offline does not trigger auto-reload.