Display DNS probe results.

chrome/renderer/net/net_error_helper.cc

Index: chrome/renderer/net/net_error_helper.cc
diff --git a/chrome/renderer/net/net_error_helper.cc b/chrome/renderer/net/net_error_helper.cc
index fb86dfcc3d30fef11d63aba6dd6dad9b56516da2..afaa880ad3b82a1c3f38a1cb62a61d7a0ea868f8 100644
--- a/chrome/renderer/net/net_error_helper.cc
+++ b/chrome/renderer/net/net_error_helper.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/renderer/net/net_error_helper.h"
 
+#include <string>
+
+#include "base/json/json_writer.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/common/localized_error.h"
 #include "chrome/common/render_messages.h"
@@ -18,12 +22,15 @@
 #include "ipc/ipc_message_macros.h"
 #include "net/base/net_errors.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
-#include "third_party/WebKit/public/platform/WebURLError.h"
 #include "third_party/WebKit/public/platform/WebURLRequest.h"
 #include "third_party/WebKit/public/web/WebDataSource.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 
-using chrome_common_net::DnsProbeResult;
+using base::JSONWriter;
+using chrome_common_net::DnsProbeStatus;
+using chrome_common_net::DnsProbeStatusIsFinished;
+using chrome_common_net::DnsProbeStatusToString;
+using chrome_common_net::DnsProbesEnabledByFieldTrial;
 using content::RenderThread;
 using content::RenderView;
 using content::RenderViewObserver;
@@ -31,75 +38,31 @@ using content::kUnreachableWebDataURL;
 
 namespace {
 
-GURL GetProvisionallyLoadingURLFromWebFrame(WebKit::WebFrame* frame) {
-  return frame->provisionalDataSource()->request().url();
+bool IsLoadingErrorPage(WebKit::WebFrame* frame) {
+  GURL url = frame->provisionalDataSource()->request().url();
+  return url.spec() == kUnreachableWebDataURL;
 }
 
-bool IsErrorPage(const GURL& url) {
-  return (url.spec() == kUnreachableWebDataURL);
+bool IsMainFrame(const WebKit::WebFrame* frame) {
+  return !frame->parent();
 }
 
 // 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(int net_error) {
-  return net_error == net::ERR_NAME_NOT_RESOLVED ||
-         net_error == net::ERR_NAME_RESOLUTION_FAILED;
-}
-
-NetErrorTracker::FrameType GetFrameType(WebKit::WebFrame* frame) {
-  return frame->parent() ? NetErrorTracker::FRAME_SUB
-                         : NetErrorTracker::FRAME_MAIN;
-}
-
-NetErrorTracker::PageType GetPageType(WebKit::WebFrame* frame) {
-  bool error_page = IsErrorPage(GetProvisionallyLoadingURLFromWebFrame(frame));
-  return error_page ? NetErrorTracker::PAGE_ERROR
-                    : NetErrorTracker::PAGE_NORMAL;
-}
-
-NetErrorTracker::ErrorType GetErrorType(const WebKit::WebURLError& error) {
-  return IsDnsError(error.reason) ? NetErrorTracker::ERROR_DNS
-                                  : NetErrorTracker::ERROR_OTHER;
-}
-
-// Converts a DNS probe result into a net error.  Returns OK if the error page
-// should not be changed from the original DNS error.
-int DnsProbeResultToNetError(DnsProbeResult result) {
-  switch (result) {
-  case chrome_common_net::DNS_PROBE_UNKNOWN:
-    return net::OK;
-  case chrome_common_net::DNS_PROBE_NO_INTERNET:
-    // TODO(ttuttle): This is not the same error as when NCN returns this;
-    // ideally we should have two separate error codes for "no network" and
-    // "network with no internet".
-    return net::ERR_INTERNET_DISCONNECTED;
-  case chrome_common_net::DNS_PROBE_BAD_CONFIG:
-    // This is unspecific enough that we should still show the full DNS error
-    // page.
-    return net::OK;
-  case chrome_common_net::DNS_PROBE_NXDOMAIN:
-    return net::ERR_NAME_NOT_RESOLVED;
-  default:
-    NOTREACHED();
-    return net::OK;
-  }
-}
-
-WebKit::WebURLError NetErrorToWebURLError(int net_error) {
-  WebKit::WebURLError error;
-  error.domain = WebKit::WebString::fromUTF8(net::kErrorDomain);
-  error.reason = net_error;
-  return error;
+bool IsDnsError(const WebKit::WebURLError& error) {
+  return std::string(error.domain.utf8()) == net::kErrorDomain &&
+         (error.reason == net::ERR_NAME_NOT_RESOLVED ||
+          error.reason == net::ERR_NAME_RESOLUTION_FAILED);
 }
 
 }  // namespace
 
 NetErrorHelper::NetErrorHelper(RenderView* render_view)
     : RenderViewObserver(render_view),
-      tracker_(base::Bind(&NetErrorHelper::TrackerCallback,
-                          base::Unretained(this))),
-      dns_error_page_state_(NetErrorTracker::DNS_ERROR_PAGE_NONE),
-      updated_error_page_(false),
+      last_probe_status_(chrome_common_net::DNS_PROBE_POSSIBLE),
+      last_start_was_error_page_(false),
+      last_fail_was_dns_error_(false),
+      forwarding_probe_results_(false),
       is_failed_post_(false) {
 }
 
@@ -107,84 +70,185 @@ NetErrorHelper::~NetErrorHelper() {
 }
 
 void NetErrorHelper::DidStartProvisionalLoad(WebKit::WebFrame* frame) {
-  tracker_.OnStartProvisionalLoad(GetFrameType(frame), GetPageType(frame));
+  OnStartLoad(IsMainFrame(frame), IsLoadingErrorPage(frame));
 }
 
 void NetErrorHelper::DidFailProvisionalLoad(WebKit::WebFrame* frame,
                                             const WebKit::WebURLError& error) {
-  WebKit::WebDataSource* data_source = frame->provisionalDataSource();
-  const WebKit::WebURLRequest& failed_request = data_source->request();
-  is_failed_post_ = EqualsASCII(failed_request.httpMethod(), "POST");
-  tracker_.OnFailProvisionalLoad(GetFrameType(frame), GetErrorType(error));
+  const bool main_frame = IsMainFrame(frame);
+  const bool dns_error = IsDnsError(error);
+
+  OnFailLoad(main_frame, dns_error);
+
+  if (main_frame && dns_error) {
+    last_error_ = error;
+
+    WebKit::WebDataSource* data_source = frame->provisionalDataSource();
+    const WebKit::WebURLRequest& failed_request = data_source->request();
+    is_failed_post_ = EqualsASCII(failed_request.httpMethod(), "POST");
+  }

[mmenke, 2013/07/12 16:14:48]

Hmm...we're not testing either this or the stuff we use to generate the new
page's contents...  If is_failed_post_ broke, for instance, we wouldn't catch
it.  Worth testing?

I'm also no a big fan of splitting up with failed load updating between here and
OnFailLoad.

[Deprecated (see juliatuttle), 2013/07/12 17:42:49]

I'm not sure what a good way to test this would be; I guess we could expose
last_error_ and is_failed_post_ somehow and check them in the browser test?
I'm not either, but I'm not sure of another way we can unittest the state
machine without having some way to create an entire mock WebFrame and simulate
it loading.

[mmenke, 2013/07/12 18:02:12]

We could just pass error and is_failed_post into OnFailLoad, and then pass them
in for the tests.  UpdateErrorPage could take the updated error and
is_failed_post as inputs.

May make things too ugly, I haven't spent much time thinking about it.

[Deprecated (see juliatuttle), 2013/07/12 20:55:55]

That seems ugly, and it still wouldn't check the actual logic that goes from a
WebFrame to is_failed_post.  I'm not as worried that we'd overwrite
is_failed_post or fail to record it properly.

[mmenke, 2013/07/15 16:33:22]

My concern isn't so much that we'd overwrite it, but that we could end up not
setting it in some future update.

 }
 
 void NetErrorHelper::DidCommitProvisionalLoad(WebKit::WebFrame* frame,
                                               bool is_new_navigation) {
-  tracker_.OnCommitProvisionalLoad(GetFrameType(frame));
+  OnCommitLoad(IsMainFrame(frame));
 }
 
 void NetErrorHelper::DidFinishLoad(WebKit::WebFrame* frame) {
-  tracker_.OnFinishLoad(GetFrameType(frame));
+  OnFinishLoad(IsMainFrame(frame));
 }
 
-bool NetErrorHelper::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
-
-  IPC_BEGIN_MESSAGE_MAP(NetErrorHelper, message)
-    IPC_MESSAGE_HANDLER(ChromeViewMsg_NetErrorInfo, OnNetErrorInfo)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
+void NetErrorHelper::OnStartLoad(bool is_main_frame, bool is_error_page) {
+  DVLOG(1) << "OnStartLoad(is_main_frame=" << is_main_frame
+           << ", is_error_page=" << is_error_page << ")";
+  if (!is_main_frame)
+    return;
 
-  return handled;
+  last_start_was_error_page_ = is_error_page;
 }
 
-void NetErrorHelper::OnNetErrorInfo(int dns_probe_result) {
-  DVLOG(1) << "Received DNS probe result " << dns_probe_result;
+void NetErrorHelper::OnFailLoad(bool is_main_frame, bool is_dns_error) {
+  DVLOG(1) << "OnFailLoad(is_main_frame=" << is_main_frame
+           << ", is_dns_error=" << is_dns_error << ")";
 
-  if (dns_probe_result < 0 ||
-      dns_probe_result >= chrome_common_net::DNS_PROBE_MAX) {
-    DLOG(WARNING) << "Ignoring DNS probe result: invalid result "
-                  << dns_probe_result;
-    NOTREACHED();
+  if (!is_main_frame)
     return;
+
+  last_fail_was_dns_error_ = is_dns_error;
+
+  if (is_dns_error) {
+    last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE;
+    // If the helper was forwarding probe results and another DNS error has
+    // occurred, stop forwarding probe results until the corresponding (new)
+    // error page loads.
+    forwarding_probe_results_ = false;
   }
+}
 
-  if (dns_error_page_state_ != NetErrorTracker::DNS_ERROR_PAGE_LOADED) {
-    DVLOG(1) << "Ignoring DNS probe result: not on DNS error page.";
+void NetErrorHelper::OnCommitLoad(bool is_main_frame) {
+  DVLOG(1) << "OnCommitLoad(is_main_frame=" << is_main_frame << ")";
+
+  if (!is_main_frame)
     return;
-  }
 
-  if (updated_error_page_) {
-    DVLOG(1) << "Ignoring DNS probe result: already updated error page.";
+  // If the helper was forwarding probe results and a normal page (or a non-DNS
+  // error page) has  committed, stop forwarding probe results, since the DNS
+  // error page is gone.
+  if (!last_start_was_error_page_ || !last_fail_was_dns_error_)
+    forwarding_probe_results_ = false;

[mmenke, 2013/07/12 16:14:48]

Any reason not to just set forwarding to false here?

[mmenke, 2013/07/12 16:14:48]

BUG:  Need "last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE"
somewhere, other than constructor.  Maybe here?

Should have a unit test for this.

[Deprecated (see juliatuttle), 2013/07/12 17:42:49]

We do that in OnFailLoad, when a DNS error occurs.
For which?

[Deprecated (see juliatuttle), 2013/07/12 17:42:49]

Hmm, I guess not.  If we have two DNS error pages in a row, we don't actually
want to forward results while the second page is committed but not loaded.

[mmenke, 2013/07/12 18:02:12]

Hmm...I even searched the page for that.  How did I miss it?  :(
A test that depends on resetting the probe status.  Your current two load test
gets the probe start status before the load stops, so it would pass even if you
weren't resetting the status.

[Deprecated (see juliatuttle), 2013/07/12 20:55:55]

Added another test that repeats the "DNS_PROBE_FINISHED after FinishLoad" test
case twice.

+}
+
+void NetErrorHelper::OnFinishLoad(bool is_main_frame) {
+  DVLOG(1) << "OnFinishLoad(is_main_frame=" << is_main_frame << ")";
+
+  if (!is_main_frame)
     return;
+
+  // If a DNS error page just finished loading, start forwarding probe results
+  // to it.
+  forwarding_probe_results_ =
+      last_fail_was_dns_error_ && last_start_was_error_page_;

[mmenke, 2013/07/12 16:14:48]

No known way to set this to false for the link doctor, right?

[Deprecated (see juliatuttle), 2013/07/12 17:42:49]

Nope.  As far as I can tell, the loads are identical for the blank link doctor
page and the real error page.

+
+  if (forwarding_probe_results_ &&
+      last_probe_status_ != chrome_common_net::DNS_PROBE_POSSIBLE) {
+    DVLOG(1) << "Error page finished loading; sending saved status.";
+    UpdateErrorPage();
   }
+}
+
+bool NetErrorHelper::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
 
-  UpdateErrorPage(static_cast<DnsProbeResult>(dns_probe_result));
-  updated_error_page_ = true;
+  IPC_BEGIN_MESSAGE_MAP(NetErrorHelper, message)
+    IPC_MESSAGE_HANDLER(ChromeViewMsg_NetErrorInfo, OnNetErrorInfo)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  return handled;
 }
 
-void NetErrorHelper::TrackerCallback(
-    NetErrorTracker::DnsErrorPageState state) {
-  dns_error_page_state_ = state;
+bool NetErrorHelper::GetErrorStringsForDnsProbe(
+    WebKit::WebFrame* frame,
+    const WebKit::WebURLError& error,
+    bool is_failed_post,
+    const std::string& locale,
+    base::DictionaryValue* error_strings) {
+  if (!IsMainFrame(frame))
+    return false;
 
-  if (state == NetErrorTracker::DNS_ERROR_PAGE_LOADED)
-    updated_error_page_ = false;
+  if (!IsDnsError(error))
+    return false;
+
+  // Get the strings for a fake "DNS probe possible" error.
+  WebKit::WebURLError fake_error;
+  fake_error.domain = WebKit::WebString::fromUTF8(
+      chrome_common_net::kDnsProbeErrorDomain);
+  fake_error.reason = chrome_common_net::DNS_PROBE_POSSIBLE;
+  fake_error.unreachableURL = error.unreachableURL;
+  LocalizedError::GetStrings(
+      fake_error, is_failed_post, locale, error_strings);
+  return true;
 }
 
-void NetErrorHelper::UpdateErrorPage(DnsProbeResult dns_probe_result) {
-  DVLOG(1) << "Updating error page with result " << dns_probe_result;
+void NetErrorHelper::OnNetErrorInfo(int status_num) {
+  DCHECK(status_num >= 0 && status_num < chrome_common_net::DNS_PROBE_MAX);
+
+  DVLOG(1) << "Received status " << DnsProbeStatusToString(status_num);
 
-  int net_error = DnsProbeResultToNetError(dns_probe_result);
-  if (net_error == net::OK)
+  DnsProbeStatus status = static_cast<DnsProbeStatus>(status_num);
+  DCHECK_NE(chrome_common_net::DNS_PROBE_POSSIBLE, status);
+
+  if (!(last_fail_was_dns_error_ || forwarding_probe_results_)) {
+    DVLOG(1) << "Ignoring NetErrorInfo: no DNS error";
     return;
+  }
+
+  last_probe_status_ = status;
 
-  DVLOG(1) << "net error code is " << net_error;
+  if (forwarding_probe_results_)
+    UpdateErrorPage();
+}
+
+void NetErrorHelper::UpdateErrorPage() {
+  DCHECK(forwarding_probe_results_);
 
   base::DictionaryValue error_strings;
-  LocalizedError::GetStrings(NetErrorToWebURLError(net_error),
+  LocalizedError::GetStrings(GetUpdatedError(),

[mmenke, 2013/07/12 16:14:48]

Hmm...between fail and commit, we'd actually display the wrong URL and
last_probe_status_ values.  Doesn't look like there's a simple way to fix it,
without either adding significantly more complexity.  Probably not worth
worrying about, unless there's a quick fix I'm missing, that can't end up with
us getting stuck at a sending probe page.

[Deprecated (see juliatuttle), 2013/07/12 17:42:49]

I don't think so?

Until the error page commits, we're still showing the previous,
successfully-loaded page.
After an error page commits but before it finishes loading, we'll be showing the
error page (either a blank pre-link-doctor page or the actual error page), with
DNS_PROBE_POSSIBLE strings.
After the error page finishes loading, we'll update it to either
DNS_PROBE_RUNNING or _NOT_RUN or _FINISHED_*.

This sounds no different than any other error page, except for the update?

[mmenke, 2013/07/15 16:33:22]

A page fails with a DNS issue, we commit the page.  User navigates again in the
same frame and we use the same RenderView.  New navigation fails, also with a
DNS error, DidFailProvisionalLoad updates is_failed_post_ and the URL, and we
then get the probe result.

We update the original, committed, page for the failed post using the URL and
post information for the first page load, not the second.

When the second page commits, we update it with the correct information.

                              is_failed_post_,
                              RenderThread::Get()->GetLocale(),
                              &error_strings);
 
-  // TODO(ttuttle): Update error page with error_strings.
+  std::string json;
+  JSONWriter::Write(&error_strings, &json);
+
+  std::string js = "if (window.updateForDnsProbe) "
+                   "updateForDnsProbe(" + json + ");";
+  string16 js16;
+  if (!UTF8ToUTF16(js.c_str(), js.length(), &js16)) {
+    NOTREACHED();
+    return;
+  }
+
+  DVLOG(1) << "Updating error page with status "
+           << chrome_common_net::DnsProbeStatusToString(last_probe_status_);
+  DVLOG(2) << "New strings: " << js;
+
+  string16 frame_xpath;
+  render_view()->EvaluateScript(frame_xpath, js16, 0, false);
+}
+
+WebKit::WebURLError NetErrorHelper::GetUpdatedError() const {
+  // If a probe didn't run or wasn't conclusive, restore the original error.
+  if (last_probe_status_ == chrome_common_net::DNS_PROBE_NOT_RUN ||
+      last_probe_status_ == chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN) {
+    return last_error_;
+  }
+
+  WebKit::WebURLError error;
+  error.domain = WebKit::WebString::fromUTF8(
+      chrome_common_net::kDnsProbeErrorDomain);
+  error.reason = last_probe_status_;
+  error.unreachableURL = last_error_.unreachableURL;
+
+  return error;
 }