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 89c6b63b7555109d18c80f146ffcf6504fb8c1b1..781fc582abd46820d5e0c789a7b3092c3c3e377f 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/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/common/localized_error.h"
 #include "chrome/common/render_messages.h"
@@ -18,13 +22,16 @@
 #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/Source/WebKit/chromium/public/WebDataSource.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 
 using base::DictionaryValue;
-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;
@@ -32,75 +39,30 @@ 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_status_(chrome_common_net::DNS_PROBE_POSSIBLE),
+      dns_error_active_(false),
+      page_loaded_(false),
       is_failed_post_(false) {
 }
 
@@ -108,24 +70,66 @@ 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");
+  }
 }
 
 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));
+}
+
+void NetErrorHelper::OnStartLoad(bool is_main_frame, bool is_error_page) {
+  if (!is_main_frame)
+    return;
+
+  is_error_page_ = is_error_page;
+}
+
+void NetErrorHelper::OnFailLoad(bool is_main_frame, bool is_dns_error) {
+  if (!is_main_frame || !is_dns_error)
+    return;
+
+  dns_error_active_ = true;

[mmenke, 2013/06/13 20:04:55]

I think the state machine here is too confusing.

I suggest just three boolean variables:

is_loading_dns_error_page_:  Set to true here, set to false in OnStartLoad if
not an error page, and in OnCommitLoad.

is_non_updated_dns_error_page_:  Set to is_loading_dns_error_page_ in
OnCommitLoad.  Set to false after the page is updated with a final probe.

page_loaded_ is fine as-is.

Advantage with these is there's no ambiguity.  dns_error_active_ won't be true
for non-error pages while they're loading, for example.

[Deprecated (see juliatuttle), 2013/06/18 19:36:37]

Done.

I had to clear page_loaded in OnFailLoad to make sure that, if we have two DNS
errors in a row, we don't consider the STARTED for the new page (which can come
as soon as we've failed a second time) intended for the old page.

+  last_status_ = chrome_common_net::DNS_PROBE_POSSIBLE;
+}
+
+void NetErrorHelper::OnCommitLoad(bool is_main_frame) {
+  if (!is_main_frame)
+    return;
+
+  if (!is_error_page_)
+    dns_error_active_ = false;
+
+  page_loaded_ = false;
+}
+
+void NetErrorHelper::OnFinishLoad(bool is_main_frame) {

[mmenke, 2013/06/13 20:04:55]

This should check if it's a main frame, presumably.

[Deprecated (see juliatuttle), 2013/06/18 19:36:37]

Done.

+  page_loaded_ = true;
+
+  if (dns_error_active_ && is_error_page_) {
+    DVLOG(1) << "Error page finished loading; updating.";
+    UpdateErrorPage();
+  }
 }
 
 bool NetErrorHelper::OnMessageReceived(const IPC::Message& message) {
@@ -139,53 +143,95 @@ bool NetErrorHelper::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void NetErrorHelper::OnNetErrorInfo(int dns_probe_result) {
-  DVLOG(1) << "Received DNS probe result " << dns_probe_result;
-
-  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;
+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 (!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::OnNetErrorInfo(int status_num) {
+  if (status_num < 0 ||
+      status_num >= chrome_common_net::DNS_PROBE_MAX) {

[mmenke, 2013/06/13 20:04:55]

Think we can just DCHECK on this, with the branch - generally, if something can
only happen in the case of a bug or random memory corruption, we shouldn't worry
about handling it.

[Deprecated (see juliatuttle), 2013/06/18 19:36:37]

Done.

+    LOG(WARNING) << "Ignoring NetErrorInfo: invalid status " << status_num;
     NOTREACHED();
     return;
   }
 
-  if (dns_error_page_state_ != NetErrorTracker::DNS_ERROR_PAGE_LOADED) {
-    DVLOG(1) << "Ignoring DNS probe result: not on DNS error page.";
+  DVLOG(1) << "Received status "
+           << DnsProbeStatusToString(status_num);
+
+  DnsProbeStatus status = static_cast<DnsProbeStatus>(status_num);
+  if (status == chrome_common_net::DNS_PROBE_POSSIBLE) {

[mmenke, 2013/06/13 20:04:55]

Same as above comment - I think a DCHECK is sufficient.

[Deprecated (see juliatuttle), 2013/06/18 19:36:37]

Done.

+    NOTREACHED();
     return;
   }
 
-  if (updated_error_page_) {
-    DVLOG(1) << "Ignoring DNS probe result: already updated error page.";
+  if (!dns_error_active_) {
+    DVLOG(1) << "Ignoring NetErrorInfo: no active DNS error";
     return;
   }
 
-  UpdateErrorPage(static_cast<DnsProbeResult>(dns_probe_result));
-  updated_error_page_ = true;
-}
+  last_status_ = status;
 
-void NetErrorHelper::TrackerCallback(
-    NetErrorTracker::DnsErrorPageState state) {
-  dns_error_page_state_ = state;
-
-  if (state == NetErrorTracker::DNS_ERROR_PAGE_LOADED)
-    updated_error_page_ = false;
+  if (page_loaded_)
+    UpdateErrorPage();

[mmenke, 2013/06/13 20:04:55]

Can this happen:

page_loaded_ starts as true (after a previous navigation).
OnStartLoad
OnFailLoad
OnNetErrorInfo (Before the error page starts to load)?
OnCommitLoad
...

If OnNetErrorInfo can occur before OnCommitLoad, then we're calling into the
wrong page.

If OnCommitLoad occurs asynchronously, this seems possible, if not likely.

[Deprecated (see juliatuttle), 2013/06/18 19:36:37]

...ha, that is the exact race condition I just found.  Fixed (see above).

 }
 
-void NetErrorHelper::UpdateErrorPage(DnsProbeResult dns_probe_result) {
-  DVLOG(1) << "Updating error page with result " << dns_probe_result;
-
-  int net_error = DnsProbeResultToNetError(dns_probe_result);
-  if (net_error == net::OK)
-    return;
-
-  DVLOG(1) << "net error code is " << net_error;
-
+void NetErrorHelper::UpdateErrorPage() {
   DictionaryValue error_strings;
-  LocalizedError::GetStrings(NetErrorToWebURLError(net_error),
+  LocalizedError::GetStrings(GetUpdatedError(),
                              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_status_);
+  DVLOG(2) << "New strings: " << js;
+
+  string16 frame_xpath;
+  render_view()->EvaluateScript(frame_xpath, js16, 0, false);

[mmenke, 2013/06/13 20:04:55]

Should we do something to prevent too many updates?  I believe it may be
possible get a probe result from a previously displayed error page, and then get
two more results as we "run" a probe again in response to a new DNS error.

[Deprecated (see juliatuttle), 2013/06/18 19:36:37]

We could?  I don't think it'd be a huge deal if it occasionally updated twice.

+}
+
+WebKit::WebURLError NetErrorHelper::GetUpdatedError() const {
+  // If a probe didn't run or wasn't conclusive, restore the original error.
+  if (last_status_ == chrome_common_net::DNS_PROBE_NOT_RUN ||
+      last_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_status_;
+  error.unreachableURL = last_error_.unreachableURL;
+
+  return error;
 }