Support auto-reload on errors.

chrome/renderer/net/net_error_helper_core.cc

Index: chrome/renderer/net/net_error_helper_core.cc
diff --git a/chrome/renderer/net/net_error_helper_core.cc b/chrome/renderer/net/net_error_helper_core.cc
index 55cb771c9e50b103835ff0cf013367e5c7c39bff..0469b4c0ccc4a7be79c647673ed90650d836d867 100644
--- a/chrome/renderer/net/net_error_helper_core.cc
+++ b/chrome/renderer/net/net_error_helper_core.cc
@@ -6,6 +6,9 @@
 
 #include <string>
 
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
 #include "base/metrics/histogram.h"
 #include "chrome/common/localized_error.h"
 #include "net/base/escape.h"
@@ -16,6 +19,15 @@
 
 namespace {
 
+base::TimeDelta GetAutoReloadBackoff(size_t reloads) {

[mmenke, 2014/02/05 23:31:43]

I suggest reload_count or num_reloads.  I generally think using plurals for
counters can be confusing.  reload_count also matches the caller a bit more
closely.

[mmenke, 2014/02/05 23:31:43]

I suggest "GetAutoReloadTime" or "GetAutoReloadTimeWithBackoff", since it
returns a time, not a "backoff".

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+  static const int kDelaysMs[] = {
+    5000, 30000, 60000, 300000, 600000, 1800000

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

I'd like to have some UMA as to when we succeed on this.  We're almost certainly
going to want to be tweaking these constants from data pulled in from the field,
so let's plan for that.

+  };
+  if (reloads >= arraysize(kDelaysMs))
+    reloads = arraysize(kDelaysMs) - 1;
+  return base::TimeDelta::FromMilliseconds(kDelaysMs[reloads]);
+}
+
 // Returns whether |net_error| is a DNS-related error (and therefore whether
 // the tab helper should start a DNS probe after receiving it.)
 bool IsDnsError(const blink::WebURLError& error) {
@@ -24,6 +36,17 @@ bool IsDnsError(const blink::WebURLError& error) {
           error.reason == net::ERR_NAME_RESOLUTION_FAILED);
 }
 
+bool IsReloadableError(const blink::WebURLError& error) {

[mmenke, 2014/02/05 23:31:43]

You should check the error domain, too, rather than assume it's net.  It maybe
also be "http" (There's also a DNS domain, but I don't think we ever pass a DNS
error here, we always just pass those errors directly to the code to generate an
error page)

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+  return error.reason != net::ERR_ABORTED;
+/*

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Why commented out?

[Elly Fong-Jones, 2014/02/10 21:42:06]

leftover from earlier, oops

Done.

+  return error.reason == net::ERR_NAME_NOT_RESOLVED ||
+         error.reason == net::ERR_INTERNET_DISCONNECTED ||
+         error.reason == net::ERR_ADDRESS_UNREACHABLE ||
+         error.reason == net::ERR_CONNECTION_TIMED_OUT ||
+         error.reason == net::ERR_NETWORK_CHANGED;
+*/
+}
+
 // If an alternate error page should be retrieved remotely for a main frame load
 // that failed with |error|, returns true and sets |error_page_url| to the URL
 // of the remote error page.
@@ -118,7 +141,9 @@ struct NetErrorHelperCore::ErrorPageInfo {
 
 NetErrorHelperCore::NetErrorHelperCore(Delegate* delegate)
     : delegate_(delegate),
-      last_probe_status_(chrome_common_net::DNS_PROBE_POSSIBLE) {
+      last_probe_status_(chrome_common_net::DNS_PROBE_POSSIBLE),
+      auto_reload_enabled_(false),
+      auto_reload_count_(0) {
 }
 
 NetErrorHelperCore::~NetErrorHelperCore() {
@@ -133,6 +158,8 @@ void NetErrorHelperCore::OnStop() {
   if (pending_error_page_info_)
     pending_error_page_info_->alternate_error_page_url = GURL();
   delegate_->CancelFetchErrorPage();
+  if (auto_reload_timer_.get())
+    auto_reload_timer_->Stop();

[mmenke, 2014/02/05 23:31:43]

Can't these two lines just be replaced with "auto_reload_timer_.reset()"?

[mmenke, 2014/02/05 23:31:43]

auto_reload_count_ = 0;?

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

 }
 
 void NetErrorHelperCore::OnStartLoad(FrameType frame_type, PageType page_type) {
@@ -154,8 +181,14 @@ void NetErrorHelperCore::OnCommitLoad(FrameType frame_type) {
 }
 
 void NetErrorHelperCore::OnFinishLoad(FrameType frame_type) {
-  if (frame_type != MAIN_FRAME || !committed_error_page_info_)
+  if (frame_type != MAIN_FRAME || !committed_error_page_info_) {
+    if (frame_type == MAIN_FRAME && !committed_error_page_info_) {

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

I find this cascade of conditionals confusing.  Maybe separate out the two cases
for clarity?  (I.e. 

if (frame_type != MAIN_FRAME)
  return;


if (!committed_error_page_info_) {
  auto_reload_count_ = 0;
  return;
}

[Elly Fong-Jones, 2014/02/10 21:42:06]

Aha, good catch.

+      // Just finished a non-error page load. Clear the auto-reload backoff
+      // count.
+      auto_reload_count_ = 0;
+    }
     return;
+  }
 
   committed_error_page_info_->is_finished_loading = true;
 
@@ -167,6 +200,13 @@ void NetErrorHelperCore::OnFinishLoad(FrameType frame_type) {
     GURL error_page_url;
     delegate_->FetchErrorPage(
         committed_error_page_info_->alternate_error_page_url);
+  } else if (auto_reload_enabled_) {
+    blink::WebURLError error = committed_error_page_info_->error;
+    bool was_failed_post = committed_error_page_info_->was_failed_post;
+    if (IsReloadableError(error) && !was_failed_post)
+      StartAutoReloadTimer();

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Hmmm.  My preference would be to only start the timer if we're online.  (Also,
if we're offline when we get the error and then go online, I'd rather not wait
the five seconds to start the time.)

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+    else
+      auto_reload_count_ = 0;
   }
 
   if (!committed_error_page_info_->needs_dns_updates ||
@@ -316,3 +356,32 @@ blink::WebURLError NetErrorHelperCore::GetUpdatedError(
 
   return updated_error;
 }
+
+void NetErrorHelperCore::AutoReload() {
+  if (!committed_error_page_info_) {
+    return;
+  }
+  delegate_->ReloadPage();

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Confirming, just because I'm not sure which of the CLs I've seen fly by have
actually landed/are scheduled for landing: this reload will evaporate (i.e. not
replace the error page, with the possible user annoyance involved there) if it
produces another error page?

+}
+
+void NetErrorHelperCore::StartAutoReloadTimer() {
+  base::TimeDelta delay = GetAutoReloadBackoff(auto_reload_count_);
+  auto_reload_count_++;
+  auto_reload_timer_.reset(delegate_->NewMockableOneShotTimer());

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

It looks to me as if OneShotTimer can be re-used (Stop() followed by Start()). 
Given that, I don't think the NEHC needs more than one OneShotTimer.  Given
that, I think it would be a cleaner interface to pass the timer into the
constructor, and get rid of the extra delegate interface/method on NEH.  WDYT?

[Elly Fong-Jones, 2014/02/10 21:42:06]

Good idea, I like it.

Done.

+  auto_reload_timer_->Start(FROM_HERE, delay,
+      base::Bind(&NetErrorHelperCore::AutoReload,
+          base::Unretained(this)));
+}
+
+bool NetErrorHelperCore::IsAutoReloading() const {
+  return auto_reload_count_ != 0;
+}
+
+void NetErrorHelperCore::NetworkStateChanged(bool online) {
+  if (online && auto_reload_timer_->IsRunning()) {

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Should we stop the timer if the change was to offline?

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+    // start over

[mmenke, 2014/02/05 23:31:43]

nit:  Start over. (Per style guide, comments should be complete sentences)

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+    auto_reload_count_ = 0;
+    auto_reload_timer_->Stop();
+    StartAutoReloadTimer();
+  }
+}