Support auto-reload on errors.

chrome/renderer/net/net_error_helper_core.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/net/net_error_helper_core.h"
 
 #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"
 #include "net/base/net_errors.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebURLError.h"
 #include "url/gurl.h"
 
 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) {
   return error.domain.utf8() == net::kErrorDomain &&
          (error.reason == net::ERR_NAME_NOT_RESOLVED ||
           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.
 bool GetErrorPageURL(const blink::WebURLError& error,
                      const GURL& alt_error_page_url,
                      GURL* error_page_url) {
   if (!alt_error_page_url.is_valid())
     return false;
 
   // Parameter to send to the error page indicating the error type.
@@ skipping 74 matching lines @@
   // for tests to wait for.
   GURL alternate_error_page_url;
 
   // True if a page has completed loading, at which point it can receive
   // updates.
   bool is_finished_loading;
 };
 
 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() {
 }
 
 void NetErrorHelperCore::OnStop() {
   // On stop, cancel loading the alternate error page, and prevent any pending
   // error page load from starting a new error page load.  Swapping in the error
   // page when it's finished loading could abort the navigation, otherwise.
   if (committed_error_page_info_)
     committed_error_page_info_->alternate_error_page_url = GURL();
   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) {
   if (frame_type != MAIN_FRAME)
     return;
 
   // If there's no pending error page information associated with the page load,
   // or the new page is not an error page, then reset pending error page state.
   if (!pending_error_page_info_ || page_type != ERROR_PAGE) {
     OnStop();
   }
 }
 
 void NetErrorHelperCore::OnCommitLoad(FrameType frame_type) {
   if (frame_type != MAIN_FRAME)
     return;
 
   committed_error_page_info_.reset(pending_error_page_info_.release());
 }
 
 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;
 
   if (committed_error_page_info_->alternate_error_page_url.is_valid()) {
     // If there is another pending error page load,
     // |replace_with_alternate_error_page| should have been set to false.
     DCHECK(!pending_error_page_info_);
     DCHECK(!committed_error_page_info_->needs_dns_updates);
     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 ||
       last_probe_status_ == chrome_common_net::DNS_PROBE_POSSIBLE) {
     return;
   }
   DVLOG(1) << "Error page finished loading; sending saved status.";
   UpdateErrorPage();
 }
 
@@ skipping 129 matching lines @@
   }
 
   blink::WebURLError updated_error;
   updated_error.domain = blink::WebString::fromUTF8(
       chrome_common_net::kDnsProbeErrorDomain);
   updated_error.reason = last_probe_status_;
   updated_error.unreachableURL = error.unreachableURL;
 
   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();
+  }
+}