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..020c37cca8eb8863993526606e71abfadac6c605 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"
@@ -24,6 +27,10 @@ bool IsDnsError(const blink::WebURLError& error) {
           error.reason == net::ERR_NAME_RESOLUTION_FAILED);
 }
 
+bool IsReloadableError(const blink::WebURLError& error) {
+  return error.reason != net::ERR_ABORTED;

[Randy Smith (Not in Mondays), 2014/01/30 20:14:47]

I believe we just want to reload on "offline like" errors, which I currently
believe are:

  return (error == net::ERR_NAME_NOT_RESOLVED ||
          error == net::ERR_INTERNET_DISCONNECTED ||
          error == net::ERR_ADDRESS_UNREACHABLE ||
          error == net::ERR_CONNECTION_TIMED_OUT);

and based on the discussion with Paul J, I think we should add
ERR_NETWORK_CHANGED.

[mmenke, 2014/01/30 20:47:31]

I'm not sure about those...  ERR_CONNECTION_TIMED_OUT and
ERR_ADDRESS_UNREACHABLE in particular.  The latter you often see when upstream
network devices are down, like your cable modem rebooting.  Timed out you often
see on random connection hiccups, or site hiccups.  Also, if things are timing
out, at least you're not really hammering the network trying to reload.

[Randy Smith (Not in Mondays), 2014/01/30 20:57:30]

I'm confused; isn't modem rebooting and random connection hiccups exactly the
situations in which we want to auto-reload with an exponential backoff?

+}
+
 // 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 +125,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),

[Randy Smith (Not in Mondays), 2014/01/30 20:14:47]

Why not attach this to the flag setup for this work in
https://codereview.chromium.org/113133004/?

[Elly Fong-Jones, 2014/02/05 21:12:37]

Done.

+      auto_reload_count_(0) {
 }
 
 NetErrorHelperCore::~NetErrorHelperCore() {
@@ -133,6 +142,9 @@ 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();
+  delegate_->CancelFetchAutoReloadPage();
 }
 
 void NetErrorHelperCore::OnStartLoad(FrameType frame_type, PageType page_type) {
@@ -154,8 +166,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_) {
+      // 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;
 
@@ -202,6 +220,13 @@ void NetErrorHelperCore::GetErrorHTML(
       pending_error_page_info_->alternate_error_page_url = error_page_url;
       return;
     }
+
+    if (auto_reload_enabled_) {
+      if (IsReloadableError(error))

[mmenke, 2014/01/30 20:45:07]

Don't want to auto reload POSTs.

[mmenke, 2014/01/30 20:45:07]

May make more sense to trigger this when the error page finishes loading (That
is, OnDidFinishLoading, when there's a committed_error_page_info_, no link
doctor load to start, and no pending_error_page_info_).

[mmenke, 2014/01/30 20:45:07]

We just try reloading eternally?

Suppose shouldn't be too bad, but I wonder about the 1, 2, 4, 8, and 16 seconds
reloads...Seems a bit too many.

[Elly Fong-Jones, 2014/02/05 21:12:37]

Done.

[Elly Fong-Jones, 2014/02/05 21:12:37]

Done.

+        StartAutoReload();
+      else
+        auto_reload_count_ = 0;
+    }
   }
 
   GenerateLocalErrorPage(frame_type, error, is_failed_post, error_html);
@@ -316,3 +341,24 @@ blink::WebURLError NetErrorHelperCore::GetUpdatedError(
 
   return updated_error;
 }
+
+void NetErrorHelperCore::AutoReload() {
+  if (!committed_error_page_info_) {

[mmenke, 2014/01/30 20:45:07]

Is it possible for this to trigger because it's taking the first error page too
long to load?

[Elly Fong-Jones, 2014/02/05 21:12:37]

No. The timer is only restarted in GetErrorHTML.

+    return;
+  }
+  GURL url = committed_error_page_info_->error.unreachableURL;
+  delegate_->FetchAutoReloadPage(url);
+}
+
+void NetErrorHelperCore::StartAutoReload() {

[Randy Smith (Not in Mondays), 2014/01/30 20:14:47]

We want the NCN indicating we're offline to override this; we should only do
exponential backoff when the NCN says we're online.

[mmenke, 2014/01/30 20:45:07]

StartAutoReloadTimer?

[Elly Fong-Jones, 2014/02/05 21:12:37]

Done.

+  int delay = 1 << auto_reload_count_;

[mmenke, 2014/01/30 20:45:07]

I suggest making the 1 a constant at the top of the file, and make make it in
milliseconds.

[Elly Fong-Jones, 2014/02/05 21:12:37]

Moved it into a helper.

+  auto_reload_count_++;
+  auto_reload_timer_.reset(delegate_->NewMockableOneShotTimer());
+  auto_reload_timer_->Start(FROM_HERE, base::TimeDelta::FromSeconds(delay),
+      base::Bind(&NetErrorHelperCore::AutoReload,
+          base::Unretained(this)));
+}
+
+bool NetErrorHelperCore::IsAutoReloading() const {
+  return auto_reload_count_ != 0;

[mmenke, 2014/01/30 20:45:07]

This isn't correct in the OnStop() case.

[Elly Fong-Jones, 2014/02/05 21:12:37]

Yeah. Hm.

+}