DnsProbeJob: Differentiate network and DNS errors

chrome/browser/net/dns_probe_job.cc

Index: chrome/browser/net/dns_probe_job.cc
diff --git a/chrome/browser/net/dns_probe_job.cc b/chrome/browser/net/dns_probe_job.cc
index d638d0c99772827eaac14ebe31a37a7883761c8e..f0140d1c7a739472cf069344c3e5144412c6ca04 100644
--- a/chrome/browser/net/dns_probe_job.cc
+++ b/chrome/browser/net/dns_probe_job.cc
@@ -29,6 +29,22 @@ namespace chrome_browser_net {
 
 namespace {
 
+// Returns true if the given net_error indicates that we received a response
+// from the DNS server containing an error, or false if the given net_error
+// indicates that we never received a response.
+static bool IsDnsServerError(int net_error) {

[szym, 2012/12/11 21:05:31]

no need for static

[szym, 2012/12/11 21:05:31]

The name is a bit misleading. Some of those errors are issues with the
(incomplete) client.

[Deprecated (see juliatuttle), 2012/12/11 23:54:03]

Done.

[Deprecated (see juliatuttle), 2012/12/11 23:54:03]

What's a better name?  DoesErrorMeanServerResponded seems a little heavyweight.

[szym, 2012/12/12 01:13:14]

DidReceiveDnsResponse()

This is a local name so does not have to be globally meaningful.

[mmenke, 2012/12/12 17:01:19]

Alternatively, you could name this DnsErrorToQueryResult, and have it returning
QUERY_WHATEVER.  Don't think we'll need different results for the known good and
known bad addresses here, though could handle that, too, by taking in a boolean,
if we really wanted.

+  switch (net_error) {
+  case net::ERR_NAME_NOT_RESOLVED:  // NXDOMAIN maps to this.
+  case net::ERR_DNS_MALFORMED_RESPONSE:

[szym, 2012/12/11 21:05:31]

This one is tricky. If DnsTransaction receives a response that does not match
the query (might indicate that the presumed server does not speak DNS), it makes
another attempt, but if no more attempts are allowed, it fails with
ERR_DNS_MALFORMED_RESPONSE.

That said, it does indicate that we got some packet back.

[Deprecated (see juliatuttle), 2012/12/11 23:54:03]

This is mostly fine.  Our primary goal is to differentiate between "can't reach
the server" and "the server is there but doing something wrong".  If the server
isn't a DNS server, we'll end up telling the user something is wrong with their
DNS server/config, which I think is an appropriate outcome.

+  case net::ERR_DNS_SERVER_REQUIRES_TCP:
+  case net::ERR_DNS_SERVER_FAILED:
+  case net::ERR_DNS_SORT_ERROR:  // Can only happen if the server responds.
+    return true;
+  default:
+    return false;

[szym, 2012/12/11 21:05:31]

One of the issues with retransmissions within DnsTransaction is that if the
first attempt returns a valid NXDOMAIN response and the subsequent attempt fails
with ERR_UNREACHABLE or similar, you won't know that the server responded the
first time.

Food for thought.

But this reminds me that in DnsProbeService::GetSystemDnsConfig you should
definitely suffix.clear() to disable suffix search.

[Deprecated (see juliatuttle), 2012/12/11 23:54:03]

Why do we retry on NXDOMAIN?

[szym, 2012/12/12 01:13:14]

Misspoke. We don't retry NXDOMAIN, but we suffix search. Your other CL will
address this case.

+  }
+}
+
 class DnsProbeJobImpl : public DnsProbeJob {
  public:
   DnsProbeJobImpl(scoped_ptr<DnsClient> dns_client,
@@ -139,7 +155,7 @@ DnsProbeJobImpl::QueryResult DnsProbeJobImpl::EvaluateGoodResponse(
     int net_error,
     const DnsResponse* response) {
   if (net_error != net::OK)
-    return QUERY_NET_ERROR;
+    return IsDnsServerError(net_error) ? QUERY_DNS_ERROR : QUERY_NET_ERROR;
 
   AddressList addr_list;
   TimeDelta ttl;
@@ -160,10 +176,8 @@ DnsProbeJobImpl::QueryResult DnsProbeJobImpl::EvaluateBadResponse(
   if (net_error == net::ERR_NAME_NOT_RESOLVED)  // NXDOMAIN maps to this
     return QUERY_CORRECT;
 
-  // TODO(ttuttle): Examine net_error a little more closely to see whether
-  // it's a DNS error or a network error.
   if (net_error != net::OK)
-    return QUERY_NET_ERROR;
+    return IsDnsServerError(net_error) ? QUERY_DNS_ERROR : QUERY_NET_ERROR;
 
   return QUERY_INCORRECT;
 }