autoreload: don't suppress manual error pages

Simplify the ShouldSuppressErrorPage logic to *know* whether to suppress an
error page instead of guessing with heuristics. This avoids the situation where
a manual reload causes an OnStartLoading(NON_ERROR), which causes a
CancelPendingRequests(), which causes SuppressErrorPage() to falsely think
autoreload caused this load. The new unit test in
NetErrorHelperCoreAutoReloadTest demonstrates this sequence of events.

BUG=413606

Review URL: https://codereview.chromium.org/582243002

Cr-Commit-Position: refs/heads/master@{#306230}

4 files changed, 61 insertions, 35 deletions

diff --git a/chrome/browser/errorpage_browsertest.cc b/chrome/browser/errorpage_browsertest.cc
index dcab17d..e2f66df 100644
--- a/chrome/browser/errorpage_browsertest.cc
+++ b/chrome/browser/errorpage_browsertest.cc
@@ -932,6 +932,28 @@ IN_PROC_BROWSER_TEST_F(ErrorPageAutoReloadTest, AutoReload) {
   EXPECT_EQ(kRequestsToFail + 1, interceptor()->requests());
 }
 
+IN_PROC_BROWSER_TEST_F(ErrorPageAutoReloadTest, ManualReloadNotSuppressed) {
+  GURL test_url("http://error.page.auto.reload");
+  const int kRequestsToFail = 3;
+  InstallInterceptor(test_url, kRequestsToFail);
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      browser(), test_url, 2);
+
+  EXPECT_EQ(2, interceptor()->failures());
+  EXPECT_EQ(2, interceptor()->requests());
+
+  ToggleHelpBox(browser());
+  EXPECT_TRUE(IsDisplayingNetError(browser(), net::ERR_CONNECTION_RESET));
+
+  content::WebContents* web_contents =
+    browser()->tab_strip_model()->GetActiveWebContents();
+  content::TestNavigationObserver nav_observer(web_contents, 1);
+  web_contents->GetMainFrame()->ExecuteJavaScript(
+      base::ASCIIToUTF16("document.getElementById('reload-button').click();"));
+  nav_observer.Wait();
+  EXPECT_FALSE(IsDisplayingNetError(browser(), net::ERR_CONNECTION_RESET));
+}
+
 // Interceptor that fails all requests with net::ERR_ADDRESS_UNREACHABLE.
 class AddressUnreachableInterceptor : public net::URLRequestInterceptor {
  public:
diff --git a/components/error_page/renderer/net_error_helper_core.cc b/components/error_page/renderer/net_error_helper_core.cc
index 94abe60..1cc3005 100644
--- a/components/error_page/renderer/net_error_helper_core.cc
+++ b/components/error_page/renderer/net_error_helper_core.cc
@@ -454,6 +454,7 @@ NetErrorHelperCore::NetErrorHelperCore(Delegate* delegate,
       auto_reload_visible_only_(auto_reload_visible_only),
       auto_reload_timer_(new base::Timer(false, false)),
       auto_reload_paused_(false),
+      auto_reload_in_flight_(false),
       uncommitted_load_started_(false),
       // TODO(ellyjones): Make online_ accurate at object creation.
       online_(true),
@@ -492,6 +493,7 @@ void NetErrorHelperCore::OnStop() {
   CancelPendingFetches();
   uncommitted_load_started_ = false;
   auto_reload_count_ = 0;
+  auto_reload_in_flight_ = false;
 }
 
 void NetErrorHelperCore::OnWasShown() {
@@ -525,6 +527,11 @@ void NetErrorHelperCore::OnCommitLoad(FrameType frame_type, const GURL& url) {
   if (frame_type != MAIN_FRAME)
     return;
 
+  // If a page is committing, either it's an error page and autoreload will be
+  // started again below, or it's a success page and we need to clear autoreload
+  // state.
+  auto_reload_in_flight_ = false;
+
   // uncommitted_load_started_ could already be false, since RenderFrameImpl
   // calls OnCommitLoad once for each in-page navigation (like a fragment
   // change) with no corresponding OnStartLoad.
@@ -825,7 +832,15 @@ void NetErrorHelperCore::StartAutoReloadTimer() {
 }
 
 void NetErrorHelperCore::AutoReloadTimerFired() {
+  // AutoReloadTimerFired only runs if:
+  // 1. StartAutoReloadTimer was previously called, which requires that
+  //    committed_error_page_info_ is populated;
+  // 2. No other page load has started since (1), since OnStartLoad stops the
+  //    auto-reload timer.
+  DCHECK(committed_error_page_info_);
+
   auto_reload_count_++;
+  auto_reload_in_flight_ = true;
   Reload();
 }
 
@@ -860,40 +875,16 @@ bool NetErrorHelperCore::ShouldSuppressErrorPage(FrameType frame_type,
   if (frame_type != MAIN_FRAME)
     return false;
 
-  if (!auto_reload_enabled_)
-    return false;
-
-  // If there's no committed error page, this error page wasn't from an auto
-  // reload.
-  if (!committed_error_page_info_)
-    return false;
-
-  // If the error page wasn't reloadable, display it.
-  if (!IsReloadableError(*committed_error_page_info_))
+  // If there's no auto reload attempt in flight, this error page didn't come
+  // from auto reload, so don't suppress it.
+  if (!auto_reload_in_flight_)
     return false;
 
-  // If |auto_reload_timer_| is still running or is paused, this error page
-  // isn't from an auto reload.
-  if (auto_reload_timer_->IsRunning() || auto_reload_paused_)
-    return false;
-
-  // If the error page was reloadable, and the timer isn't running or paused, an
-  // auto-reload has already been triggered.
-  DCHECK(committed_error_page_info_->auto_reload_triggered);
-
-  GURL error_url = committed_error_page_info_->error.unreachableURL;
-  // TODO(ellyjones): also plumb the error code down to CCRC and check that
-  if (error_url != url)
-    return false;
-
-  // Suppressed an error-page load; the previous uncommitted load was the error
-  // page load starting, so forget about it.
   uncommitted_load_started_ = false;
-
-  // The first iteration of the timer is started by OnFinishLoad calling
-  // MaybeStartAutoReloadTimer, but since error pages for subsequent loads are
-  // suppressed in this function, subsequent iterations of the timer have to be
-  // started here.
+  // This serves to terminate the auto-reload in flight attempt. If
+  // ShouldSuppressErrorPage is called, the auto-reload yielded an error, which
+  // means the request was already sent.
+  auto_reload_in_flight_ = false;
   MaybeStartAutoReloadTimer();
   return true;
 }
diff --git a/components/error_page/renderer/net_error_helper_core.h b/components/error_page/renderer/net_error_helper_core.h
index ec00d33..4c16e25 100644
--- a/components/error_page/renderer/net_error_helper_core.h
+++ b/components/error_page/renderer/net_error_helper_core.h
@@ -243,6 +243,9 @@ class NetErrorHelperCore {
   // offline->online network transition.
   bool auto_reload_paused_;
 
+  // Whether an auto-reload-initiated Reload() attempt is in flight.
+  bool auto_reload_in_flight_;
+
   // True if there is an uncommitted-but-started load, error page or not. This
   // is used to inhibit starting auto-reload when an error page finishes, in
   // case this happens:
diff --git a/components/error_page/renderer/net_error_helper_core_unittest.cc b/components/error_page/renderer/net_error_helper_core_unittest.cc
index 5015f83..b6a389f 100644
--- a/components/error_page/renderer/net_error_helper_core_unittest.cc
+++ b/components/error_page/renderer/net_error_helper_core_unittest.cc
@@ -2196,12 +2196,14 @@ TEST_F(NetErrorHelperCoreAutoReloadTest, ShouldSuppressErrorPage) {
   DoErrorLoad(net::ERR_CONNECTION_RESET);
   timer()->Fire();
 
+  // Sub-frame load.
   EXPECT_FALSE(core()->ShouldSuppressErrorPage(NetErrorHelperCore::SUB_FRAME,
                                               GURL(kFailedUrl)));
-  EXPECT_FALSE(core()->ShouldSuppressErrorPage(NetErrorHelperCore::MAIN_FRAME,
-                                              GURL("http://some.other.url")));
   EXPECT_TRUE(core()->ShouldSuppressErrorPage(NetErrorHelperCore::MAIN_FRAME,
-                                             GURL(kFailedUrl)));
+                                              GURL(kFailedUrl)));
+  // No auto-reload attempt in flight.
+  EXPECT_FALSE(core()->ShouldSuppressErrorPage(NetErrorHelperCore::MAIN_FRAME,
+                                               GURL(kFailedUrl)));
 }
 
 TEST_F(NetErrorHelperCoreAutoReloadTest, HiddenAndShown) {
@@ -2244,6 +2246,15 @@ TEST_F(NetErrorHelperCoreAutoReloadTest, ShownWhileNotReloading) {
   EXPECT_TRUE(timer()->IsRunning());
 }
 
+TEST_F(NetErrorHelperCoreAutoReloadTest, ManualReloadShowsError) {
+  SetUpCore(true, true, true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  core()->OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                      NetErrorHelperCore::ERROR_PAGE);
+  EXPECT_FALSE(core()->ShouldSuppressErrorPage(NetErrorHelperCore::MAIN_FRAME,
+                                               GURL(kFailedUrl)));
+}
+
 class NetErrorHelperCoreHistogramTest
     : public NetErrorHelperCoreAutoReloadTest {
  public:
@@ -2294,7 +2305,6 @@ TEST_F(NetErrorHelperCoreHistogramTest, SuccessAtSecondAttempt) {
   timer()->Fire();
   EXPECT_TRUE(core()->ShouldSuppressErrorPage(NetErrorHelperCore::MAIN_FRAME,
                                              default_url()));
-//  DoErrorLoad(net::ERR_CONNECTION_RESET);
   timer()->Fire();
   DoSuccessLoad();