Surface button for loading stale cache copy on net error page.

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"
@@ skipping 235 matching lines @@
   return params.release();
 }
 
 }  // 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) {
   }
 
   // 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.
   bool needs_dns_updates;
 
   // Navigation correction service url, which will be used in response to
   // certain types of network errors.  This is also stored by the
   // NetErrorHelperCore itself, but it stored here as well in case its modified
   // in the middle of an error page load.  Empty when no error page should be
   // fetched, or if there's already a fetch in progress.
   GURL navigation_correction_url;
 
   // Request body to use when requesting corrections from a web service.
   // TODO(mmenke):  Investigate loading the error page at the same time as
   //                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;
 };
 
 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)),
       // TODO(ellyjones): Make online_ accurate at object creation.
       online_(true),
       auto_reload_count_(0),
-      can_auto_reload_page_(false) {
+      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_);
   }
 }
@@ skipping 37 matching lines @@
   } else {
     // 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) {
   if (frame_type != MAIN_FRAME)
     return;
 
+  // Track if an error occurred due to a page button press.
+  if (committed_error_page_info_ && pending_error_page_info_ &&
+      navigation_from_button_ != NO_BUTTON) {
+    DCHECK(navigation_from_button_ == RELOAD_BUTTON ||
+           navigation_from_button_ == LOAD_STALE_BUTTON);
+    chrome_common_net::RecordEvent(
+        navigation_from_button_ == RELOAD_BUTTON ?
+        chrome_common_net::RELOAD_BUTTON_ERROR_EVENT :
+        chrome_common_net::LOAD_STALE_BUTTON_ERROR_EVENT);
+  }
+  navigation_from_button_ = NO_BUTTON;

[mmenke, 2014/04/09 15:59:26]

Problem:  Navigate to foo.com/, get an error.  Click reload.  Reload takes a
while.  Click back button.  We end up at another error page, but with the same
RenderView, resulting in recording the wrong thing (Some omnibox loads may also
reuse the RenderView).

[Randy Smith (Not in Mondays), 2014/04/10 21:51:01]

Good point.  I added in a test to make sure it was the same URL; do you think
that solves the problem?  (I tested manually by hijacking Reload ->
www.google.com, and that didn't increment the reload->error count.)

[mmenke, 2014/04/11 19:42:32]

This still isn't perfect, but it's probably good enough, if we can't do better
without adding significant complexity.  One simple case where this doesn't work
is user presses the reload button button on the page, it hangs, and then the
user presses the browser's reload button, or F5, and the second load succeeds.

I do think this funkiness is worth a comment.

[Randy Smith (Not in Mondays), 2014/04/14 22:02:13]

I've added a comment, but I'd like to get a design in my head for doing the
right thing before deciding that we can't do better without adding significant
complexity.  While this isn't currently true (:-{), to my mind the page reload
and the browser reload button should be functionally doing the same thing.  If
they (eventually) do, we're not going to be able to distinguish between them
unless we get an "about to load" signal on the renderer.  It looks to me as if
DidStartProvisionalLoad() is such a signal.  If we null out
navigation_from_button_ on the second call to DidStartProvisionalLoad() after we
do either Reload or Load Stale (and turned the url check into a DCHECK), do you
think that would do the job?  I think the only way that could fail is if we had
a race between a browser initiated navigation and our renderer initiated one,
and (in the current implementation) I'm not sure that can happen since the
renderer initiated one is (I think) synchronous to DidStartProvisionalLoad(). 
WDYT?  (I'm also going to cc David in, since I think he knows this code pretty
well.)

[davidben, 2014/04/15 22:04:38]

Hrm. So I don't entirely follow what's going on here, but
pending_error_page_info_ is the error info corresponding to the error page
that's busy loading, right? And the problem is that sometimes a load commits but
it's a different load because the error page load got canceled first?

So, in that case, I think you want to listen for didFailProvisionalLoad. That
should reliably get emitted if your load gets aborted for whatever reason?

There's another complexity in that pending_error_page_info_ might not actually
correspond to a load it looks like? RenderFrameImpl::didFailProvisionalLoad
calls GetNavigationErrorStrings on the renderer client which seems to eventually
call GetErrorHTML and set pending_error_page_info_. (Is this what the comment
means by "pre-provisional"?) Looks like there are a number of codepaths in
RFI::didFailProvisionalLoad which abort before LoadNavigationErrorPage is
called, so you're not guaranteed your pre-provisional load actually happens. I'm
not sure how you'd blow that one away if your "pre-provisional" error page never
actually materializes.

Maybe separate pre-provisional from provisional error info and only upgrade
pre-provisional to provisional when you see a didStartProvisionalLoad for
kUnreachableWebDataURL, which is what error pages use. Or reliably signal on all
the early returns?

Or am I misunderstanding what's going on?

+
   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_.reset(pending_error_page_info_.release());
 }
 
 void NetErrorHelperCore::OnFinishLoad(FrameType frame_type) {
   if (frame_type != MAIN_FRAME)
     return;
 
   if (!committed_error_page_info_) {
     auto_reload_count_ = 0;
     return;
   }
 
   committed_error_page_info_->is_finished_loading = true;
 
-  // Only enable stale cache JS bindings if this wasn't a post.
-  if (!committed_error_page_info_->was_failed_post) {
-    delegate_->EnableStaleLoadBindings(
-        committed_error_page_info_->error.unreachableURL);
-  }
+  chrome_common_net::RecordEvent(chrome_common_net::PAGE_SHOWN_EVENT);
+  if (committed_error_page_info_->reload_button_in_page)
+    chrome_common_net::RecordEvent(
+        chrome_common_net::RELOAD_BUTTON_SHOWN_EVENT);
+  if (committed_error_page_info_->load_stale_button_in_page)
+    chrome_common_net::RecordEvent(
+        chrome_common_net::LOAD_STALE_BUTTON_SHOWN_EVENT);

[mmenke, 2014/04/09 15:59:26]

nit:  Use braces on multi-line if's (x2).

[Randy Smith (Not in Mondays), 2014/04/10 21:51:01]

Done.

+
+  delegate_->EnablePageHelperFunctions(
+      committed_error_page_info_->error.unreachableURL);
 
   if (committed_error_page_info_->navigation_correction_url.is_valid()) {
     // If there is another pending error page load, |fix_url| should have been
     // cleared.
     DCHECK(!pending_error_page_info_);
     DCHECK(!committed_error_page_info_->needs_dns_updates);
     delegate_->FetchNavigationCorrections(
         committed_error_page_info_->navigation_correction_url,
         committed_error_page_info_->navigation_correction_request_body);
   } else if (auto_reload_enabled_ &&
@@ skipping 57 matching lines @@
     pending_error_page_info_.reset(new ErrorPageInfo(error, is_failed_post));
     // Skip DNS logic if suggestions were received from a remote server.
     if (IsDnsError(error) && !params) {
       // This is not strictly necessary, but waiting for a new status to be
       // sent as a result of the DidFinishLoading call keeps the histograms
       // consistent with older versions of the code, at no real cost.
       last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE;
 
       delegate_->GenerateLocalizedErrorPage(
           GetUpdatedError(error), is_failed_post, params.Pass(),
+          &pending_error_page_info_->reload_button_in_page,
+          &pending_error_page_info_->load_stale_button_in_page,
           error_html);
       pending_error_page_info_->needs_dns_updates = true;
       return;
     }
   }
 
-  delegate_->GenerateLocalizedErrorPage(error, is_failed_post,
-                                        params.Pass(), error_html);
+  bool reload_button_in_page = false;
+  bool load_stale_button_in_page = false;
+  delegate_->GenerateLocalizedErrorPage(
+      error, is_failed_post, params.Pass(),
+      &reload_button_in_page, &load_stale_button_in_page, error_html);
+  if (pending_error_page_info_.get()) {

[mmenke, 2014/04/09 15:59:26]

nit:  .get() not needed.

[Randy Smith (Not in Mondays), 2014/04/10 21:51:01]

Done.

+    pending_error_page_info_->reload_button_in_page = reload_button_in_page;
+    pending_error_page_info_->load_stale_button_in_page =
+        load_stale_button_in_page;
+  }
 }
 
 void NetErrorHelperCore::OnNetErrorInfo(
     chrome_common_net::DnsProbeStatus status) {
   DCHECK_NE(chrome_common_net::DNS_PROBE_POSSIBLE, status);
 
   last_probe_status_ = status;
 
   if (!committed_error_page_info_ ||
       !committed_error_page_info_->needs_dns_updates ||
@@ skipping 24 matching lines @@
 
   UMA_HISTOGRAM_ENUMERATION("DnsProbe.ErrorPageUpdateStatus",
                             last_probe_status_,
                             chrome_common_net::DNS_PROBE_MAX);
   // Every status other than DNS_PROBE_POSSIBLE and DNS_PROBE_STARTED is a
   // final status code.  Once one is reached, the page does not need further
   // updates.
   if (last_probe_status_ != chrome_common_net::DNS_PROBE_STARTED)
     committed_error_page_info_->needs_dns_updates = false;
 
+  // We don't worry about the button display statistics here because
+  // the presentation of the reload and load stale buttons can't be
+  // changed by a DNS error update.

[mmenke, 2014/04/09 15:59:26]

nit: Don't use we in comments.

[Randy Smith (Not in Mondays), 2014/04/10 21:51:01]

Done.

   delegate_->UpdateErrorPage(
       GetUpdatedError(committed_error_page_info_->error),
       committed_error_page_info_->was_failed_post);
 }
 
 void NetErrorHelperCore::OnNavigationCorrectionsFetched(
     const std::string& corrections,
     const std::string& accept_languages,
     bool is_rtl) {
   // Loading suggestions only starts when a blank error page finishes loading,
@@ skipping 112 matching lines @@
   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();
   return true;
 }
+
+void NetErrorHelperCore::ExecuteButtonPress(Button button) {
+  switch (button) {
+    case RELOAD_BUTTON:
+      chrome_common_net::RecordEvent(
+          chrome_common_net::RELOAD_BUTTON_CLICKED_EVENT);
+      navigation_from_button_ = RELOAD_BUTTON;
+      Reload();
+      return;
+    case LOAD_STALE_BUTTON:
+      chrome_common_net::RecordEvent(
+          chrome_common_net::LOAD_STALE_BUTTON_CLICKED_EVENT);
+      navigation_from_button_ = LOAD_STALE_BUTTON;
+      delegate_->LoadPageFromCache(
+          committed_error_page_info_->error.unreachableURL);
+      return;
+    case MORE_BUTTON:
+      // Visual effects on page are handled in Javascript code.
+      chrome_common_net::RecordEvent(
+          chrome_common_net::MORE_BUTTON_CLICKED_EVENT);
+      return;
+    case NO_BUTTON:
+      NOTREACHED();
+      return;
+  }
+}
+