Fix auto-reload histograms.

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 cd681a593f0944629446aab6962d20ba6cdf51a1..813bd9c31edd4a1676788e1e5f2d4afe8e56a90e 100644
--- a/chrome/renderer/net/net_error_helper_core.cc
+++ b/chrome/renderer/net/net_error_helper_core.cc
@@ -17,6 +17,7 @@
 #include "base/strings/string16.h"
 #include "base/values.h"
 #include "chrome/common/localized_error.h"
+#include "content/public/common/url_constants.h"
 #include "grit/generated_resources.h"
 #include "net/base/escape.h"
 #include "net/base/net_errors.h"
@@ -247,6 +248,29 @@ LocalizedError::ErrorPageParams* ParseAdditionalSuggestions(
   return params.release();
 }
 
+void ReportAutoReloadSuccess(const blink::WebURLError& error, size_t count) {
+  if (error.domain.utf8() != net::kErrorDomain)
+    return;
+  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtSuccess",
+                                   -error.reason,
+                                   net::GetAllErrorCodesForUma());
+  UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtSuccess", count);
+  if (count == 1) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtFirstSuccess",
+                                     -error.reason,
+                                     net::GetAllErrorCodesForUma());
+  }
+}
+
+void ReportAutoReloadFailure(const blink::WebURLError& error, size_t count) {
+  if (error.domain.utf8() != net::kErrorDomain)
+    return;
+  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtStop",
+                                   -error.reason,
+                                   net::GetAllErrorCodesForUma());
+  UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", count);
+}
+
 }  // namespace
 
 struct NetErrorHelperCore::ErrorPageInfo {
@@ -256,7 +280,8 @@ struct NetErrorHelperCore::ErrorPageInfo {
         needs_dns_updates(false),
         reload_button_in_page(false),
         load_stale_button_in_page(false),
-        is_finished_loading(false) {
+        is_finished_loading(false),
+        auto_reload_triggered(false) {
   }
 
   // Information about the failed page load.
@@ -288,6 +313,10 @@ struct NetErrorHelperCore::ErrorPageInfo {
   // True if a page has completed loading, at which point it can receive
   // updates.
   bool is_finished_loading;
+
+  // True if the auto-reload timer has fired and a reload is or has been in
+  // flight.
+  bool auto_reload_triggered;
 };
 
 bool NetErrorHelperCore::IsReloadableError(
@@ -302,19 +331,19 @@ NetErrorHelperCore::NetErrorHelperCore(Delegate* delegate)
       last_probe_status_(chrome_common_net::DNS_PROBE_POSSIBLE),
       auto_reload_enabled_(false),
       auto_reload_timer_(new base::Timer(false, false)),
+      auto_reload_timer_paused_(false),
+      uncommitted_load_started_(false),
       // TODO(ellyjones): Make online_ accurate at object creation.

[Randy Smith (Not in Mondays), 2014/04/30 18:22:16]

Right, I had forgotten about this.  Doesn't this mean that if you do a
navigation while you're offline, you'll cycle through all the backoff intervals
(because this class will think it's online), and when you finally do come back
online, you'll wait for the largest interval we have before reloading?  That
seems like a common enough use case that I'd like to handle it somewhat better
than this.

It also makes me confused about why I've found this feature so useful so
quickly; I'm often in the position of starting a navigation while offline and
coming online almost immediately after that (as the network initializes).  I
would think I would have reloaded at 0, and then started backing off, and I'd
have to wait a few seconds (which I'd really like to avoid doing; I think a
*lot* of the value of this feature is doing the initial attempt at auto-reload
when the network state changes).  Do you understand what's going on?

This isn't a question for this CL (obviously; you didn't change this code) but I
think it's fairly high priority for the user experience of
navigation-while-offline, come online to be a reload fairly quickly after you
come online. 

[Elly Fong-Jones, 2014/05/01 18:33:03]

http://crbug.com/368796 tracks this.

       online_(true),
       auto_reload_count_(0),
-      can_auto_reload_page_(false),
       navigation_from_button_(NO_BUTTON) {
 }
 
 NetErrorHelperCore::~NetErrorHelperCore() {
-  if (committed_error_page_info_ && can_auto_reload_page_) {
-    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtStop",
-                                     -committed_error_page_info_->error.reason,
-                                     net::GetAllErrorCodesForUma());
-    UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_);
+  if (committed_error_page_info_ &&
+      committed_error_page_info_->auto_reload_triggered) {
+    ReportAutoReloadFailure(committed_error_page_info_->error,
+                            auto_reload_count_);
   }
 }
 
@@ -322,12 +351,6 @@ void NetErrorHelperCore::CancelPendingFetches() {
   // 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_ && can_auto_reload_page_) {
-    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtStop",
-                                     -committed_error_page_info_->error.reason,
-                                     net::GetAllErrorCodesForUma());
-    UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtStop", auto_reload_count_);
-  }
   if (committed_error_page_info_) {
     committed_error_page_info_->navigation_correction_url = GURL();
     committed_error_page_info_->navigation_correction_request_body.clear();
@@ -337,12 +360,20 @@ void NetErrorHelperCore::CancelPendingFetches() {
     pending_error_page_info_->navigation_correction_request_body.clear();
   }
   delegate_->CancelFetchNavigationCorrections();
-  auto_reload_timer_->Stop();
-  can_auto_reload_page_ = false;
+  if (auto_reload_timer_->IsRunning())
+    auto_reload_timer_->Stop();
+  if (auto_reload_timer_paused_)

[mmenke, 2014/04/30 18:06:58]

I don't think either of these if's are needed.  Just running their bodies has no
negative side effects if false.

[Elly Fong-Jones, 2014/05/01 18:33:03]

Done.

+    auto_reload_timer_paused_ = false;
 }
 
 void NetErrorHelperCore::OnStop() {
+  if (committed_error_page_info_ &&
+      committed_error_page_info_->auto_reload_triggered) {
+    ReportAutoReloadFailure(committed_error_page_info_->error,
+                            auto_reload_count_);
+  }
   CancelPendingFetches();
+  uncommitted_load_started_ = false;
   auto_reload_count_ = 0;
 }
 
@@ -350,20 +381,21 @@ void NetErrorHelperCore::OnStartLoad(FrameType frame_type, PageType page_type) {
   if (frame_type != MAIN_FRAME)
     return;
 
+  uncommitted_load_started_ = true;
+
   // 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) {
+  if (!pending_error_page_info_ || page_type != ERROR_PAGE)
     CancelPendingFetches();
-  } else if (auto_reload_enabled_) {
-    // If an error load is starting, the resulting error page is autoreloadable.
-    can_auto_reload_page_ = IsReloadableError(*pending_error_page_info_);
-  }
 }
 
-void NetErrorHelperCore::OnCommitLoad(FrameType frame_type) {
+void NetErrorHelperCore::OnCommitLoad(FrameType frame_type, const GURL& url) {
   if (frame_type != MAIN_FRAME)
     return;
 
+  DCHECK(uncommitted_load_started_);
+  uncommitted_load_started_ = false;
+
   // Track if an error occurred due to a page button press.
   // This isn't perfect; if (for instance), the server is slow responding
   // to a request generated from the page reload button, and the user hits
@@ -383,17 +415,13 @@ void NetErrorHelperCore::OnCommitLoad(FrameType frame_type) {
   navigation_from_button_ = NO_BUTTON;
 
   if (committed_error_page_info_ && !pending_error_page_info_ &&
-      can_auto_reload_page_) {
-    int reason = committed_error_page_info_->error.reason;
-    UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtSuccess",
-                                     -reason,
-                                     net::GetAllErrorCodesForUma());
-    UMA_HISTOGRAM_COUNTS("Net.AutoReload.CountAtSuccess", auto_reload_count_);
-    if (auto_reload_count_ == 1) {
-      UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.AutoReload.ErrorAtFirstSuccess",
-                                       -reason,
-                                       net::GetAllErrorCodesForUma());
-    }
+      committed_error_page_info_->auto_reload_triggered) {
+    const blink::WebURLError& error = committed_error_page_info_->error;
+    const GURL& error_url = error.unreachableURL;
+    if (url == error_url)
+      ReportAutoReloadSuccess(error, auto_reload_count_);
+    else if (url != GURL(content::kUnreachableWebDataURL))

[Randy Smith (Not in Mondays), 2014/04/30 18:22:16]

Just confirming: This will *not* trip if the user initiated a navigation that
errored out?  I think we want to record an auto-reload failure in that case, and
I don't see that happening on OnStartLoad().

[Elly Fong-Jones, 2014/05/01 18:33:03]

In that case, committed_error_page_info_->auto_reload_triggered is false, so
this whole block is not run.

+      ReportAutoReloadFailure(error, auto_reload_count_);
   }
 
   committed_error_page_info_.reset(pending_error_page_info_.release());
@@ -628,15 +656,14 @@ void NetErrorHelperCore::Reload() {
 bool NetErrorHelperCore::MaybeStartAutoReloadTimer() {
   if (!committed_error_page_info_ ||
       !committed_error_page_info_->is_finished_loading ||
-      !can_auto_reload_page_ ||
-      pending_error_page_info_) {
+      pending_error_page_info_ ||
+      uncommitted_load_started_) {
     return false;
   }
 
   DCHECK(IsReloadableError(*committed_error_page_info_));

[mmenke, 2014/04/30 18:06:58]

Suggest moving this into StartAutoReloadTimer.

[Elly Fong-Jones, 2014/05/01 18:33:03]

Done.

 
-  if (!online_)
-    return false;
+  committed_error_page_info_->auto_reload_triggered = true;

[mmenke, 2014/04/30 18:06:58]

Suggest either DCHECKING this is true in STartAutoReloadTimer, or moving this
line there.

[Elly Fong-Jones, 2014/05/01 18:33:03]

Done.

 
   StartAutoReloadTimer();
   return true;
@@ -644,7 +671,13 @@ bool NetErrorHelperCore::MaybeStartAutoReloadTimer() {
 
 void NetErrorHelperCore::StartAutoReloadTimer() {
   DCHECK(committed_error_page_info_);
-  DCHECK(can_auto_reload_page_);
+
+  if (!online_) {
+    auto_reload_timer_paused_ = true;

[mmenke, 2014/04/30 18:06:58]

Should expand the CL description to include behavior changes.

[Elly Fong-Jones, 2014/05/01 18:33:03]

argh gerrit eats changed commit messages. Will fix.

+    return;
+  }
+
+  auto_reload_timer_paused_ = false;
   base::TimeDelta delay = GetAutoReloadTime(auto_reload_count_);
   auto_reload_timer_->Stop();
   auto_reload_timer_->Start(FROM_HERE, delay,
@@ -658,17 +691,21 @@ void NetErrorHelperCore::AutoReloadTimerFired() {
 }
 
 void NetErrorHelperCore::NetworkStateChanged(bool online) {
+  bool was_online = online_;
   online_ = online;
-  if (auto_reload_timer_->IsRunning()) {
-    DCHECK(committed_error_page_info_);
-    // If there's an existing timer running, stop it and reset the retry count.
-    auto_reload_timer_->Stop();
-    auto_reload_count_ = 0;
+  // Transitioning offline -> online

[mmenke, 2014/04/30 18:06:58]

I believe comments for else/ifs generally go inside the block.  Think it's
confusing when the else clause has a comment before it (Your indentation below
is also wrong).

[Elly Fong-Jones, 2014/05/01 18:33:03]

Moved the comment. I do not understand your remark about indentation.

+  if (!was_online && online) {
+    if (auto_reload_timer_paused_)

[mmenke, 2014/04/30 18:06:58]

I don't think we need to check was_online here.  Is there any case where
auto_reload_timer_paused_ is true, online is true, but was_online is also true?

[Elly Fong-Jones, 2014/05/01 18:33:03]

I wrote the code this way to make it very clear what state transitions are being
looked for, even though the checks aren't strictly needed.

+      MaybeStartAutoReloadTimer();
+  // Transitioning online -> offline
+  } else if (was_online && !online) {

[mmenke, 2014/04/30 18:06:58]

Again, do we get anything out of checking was_online here?  Seems like we could
just remove it, or even DCHECK it in the inner if body.

[Elly Fong-Jones, 2014/05/01 18:33:03]

No. It is just there for clarity.

+    if (auto_reload_timer_->IsRunning()) {
+      DCHECK(committed_error_page_info_);

[mmenke, 2014/04/30 18:06:58]

Maybe:

DCHECK(!auto_reload_timer_paused_)?
DCHECK(committed_error_page_info_->auto_reload_triggered)?

[Elly Fong-Jones, 2014/05/01 18:33:03]

Done.

+      auto_reload_timer_->Stop();
+      auto_reload_count_ = 0;
+      auto_reload_timer_paused_ = true;
+    }
   }
-
-  // If the network state changed to online, maybe start auto-reloading again.
-  if (online)
-    MaybeStartAutoReloadTimer();
 }
 
 bool NetErrorHelperCore::ShouldSuppressErrorPage(FrameType frame_type,
@@ -677,14 +714,14 @@ bool NetErrorHelperCore::ShouldSuppressErrorPage(FrameType frame_type,
   if (frame_type != MAIN_FRAME)
     return false;
 
-  // If |auto_reload_timer_| is still running, this error page isn't from an
-  // auto reload.
-  if (auto_reload_timer_->IsRunning())
+  // 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_timer_paused_)
     return false;
 
   // If there's no committed error page, this error page wasn't from an auto
   // reload.
-  if (!committed_error_page_info_ || !can_auto_reload_page_)
+  if (!committed_error_page_info_)
     return false;

[mmenke, 2014/04/30 18:06:58]

optional:  I think it may make more sense to put this first.  The other checks
are kinda weird when this is NULL.

[Elly Fong-Jones, 2014/05/01 18:33:03]

Done.

[mmenke, 2014/04/30 18:06:58]

Optional:  Could throw in a
DCHECK(committed_error_page_info_->is_auto_reloading)

[Elly Fong-Jones, 2014/05/01 18:33:03]

Done.

   GURL error_url = committed_error_page_info_->error.unreachableURL;