Fix auto-reload histograms.

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/i18n/rtl.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #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"
 #include "net/base/net_util.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebURLError.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
 namespace {
@@ skipping 210 matching lines @@
     }
   }
 
   if (params->override_suggestions->empty() &&
       !params->search_url.is_valid()) {
     return NULL;
   }
   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 {
   ErrorPageInfo(blink::WebURLError error, bool was_failed_post)
       : error(error),
         was_failed_post(was_failed_post),
         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.
   blink::WebURLError error;
   bool was_failed_post;
 
   // Information about the status of the error page.
 
   // True if a page is a DNS error page and has not yet received a final DNS
   // probe status.
@@ skipping 11 matching lines @@
   //                the blank page is loading, to get rid of these.
   std::string navigation_correction_request_body;
 
   // Track if specific buttons are included in an error page, for statistics.
   bool reload_button_in_page;
   bool load_stale_button_in_page;
 
   // 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(
     const NetErrorHelperCore::ErrorPageInfo& info) {
   return info.error.domain.utf8() == net::kErrorDomain &&
          info.error.reason != net::ERR_ABORTED &&
          !info.was_failed_post;
 }
 
 NetErrorHelperCore::NetErrorHelperCore(Delegate* delegate)
     : 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_);
   }
 }
 
 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();
   }
   if (pending_error_page_info_) {
     pending_error_page_info_->navigation_correction_url = GURL();
     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;
 }
 
 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
   // the browser reload button, this code will still believe the
   // result is from the page reload button.
   if (committed_error_page_info_ && pending_error_page_info_ &&
       navigation_from_button_ != NO_BUTTON &&
       committed_error_page_info_->error.unreachableURL ==
           pending_error_page_info_->error.unreachableURL) {
     DCHECK(navigation_from_button_ == RELOAD_BUTTON ||
            navigation_from_button_ == LOAD_STALE_BUTTON);
     chrome_common_net::RecordEvent(
         navigation_from_button_ == RELOAD_BUTTON ?
             chrome_common_net::NETWORK_ERROR_PAGE_RELOAD_BUTTON_ERROR :
             chrome_common_net::NETWORK_ERROR_PAGE_LOAD_STALE_BUTTON_ERROR);
   }
   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());
 }
 
 void NetErrorHelperCore::OnFinishLoad(FrameType frame_type) {
   if (frame_type != MAIN_FRAME)
     return;
 
   if (!committed_error_page_info_) {
@@ skipping 214 matching lines @@
 void NetErrorHelperCore::Reload() {
   if (!committed_error_page_info_) {
     return;
   }
   delegate_->ReloadPage();
 }
 
 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;
 }
 
 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,
       base::Bind(&NetErrorHelperCore::AutoReloadTimerFired,
                  base::Unretained(this)));
 }
 
 void NetErrorHelperCore::AutoReloadTimerFired() {
   auto_reload_count_++;
   Reload();
 }
 
 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,
                                                  const GURL& url) {
   // Don't suppress child frame errors.
   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;
   // TODO(ellyjones): also plumb the error code down to CCRC and check that
   if (error_url != url)
     return 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.
   MaybeStartAutoReloadTimer();
@@ skipping 19 matching lines @@
       // Visual effects on page are handled in Javascript code.
       chrome_common_net::RecordEvent(
           chrome_common_net::NETWORK_ERROR_PAGE_MORE_BUTTON_CLICKED);
       return;
     case NO_BUTTON:
       NOTREACHED();
       return;
   }
 }