DnsProbeJob: Differentiate network and DNS errors

chrome/browser/net/dns_probe_job.cc

 // Copyright (c) 2012 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/browser/net/dns_probe_job.h"
 
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
@@ skipping 11 matching lines @@
 using net::BoundNetLog;
 using net::DnsClient;
 using net::DnsResponse;
 using net::DnsTransaction;
 using net::NetLog;
 
 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,
                   const DnsProbeJob::CallbackType& callback,
                   NetLog* net_log);
   virtual ~DnsProbeJobImpl();
 
  private:
   enum QueryResult {
     QUERY_UNKNOWN,
@@ skipping 90 matching lines @@
     OnTransactionComplete(transaction, rv, NULL);
   }
 
   // TODO(ttuttle): Log transaction started.
 }
 
 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;
   DnsResponse::Result result = response->ParseToAddressList(&addr_list, &ttl);
 
   if (result != DnsResponse::DNS_PARSE_OK)
     return QUERY_DNS_ERROR;
 
   if (addr_list.empty())
     return QUERY_INCORRECT;
 
   return QUERY_CORRECT;
 }
 
 DnsProbeJobImpl::QueryResult DnsProbeJobImpl::EvaluateBadResponse(
     int net_error,
     const DnsResponse* response) {
   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;
 }
 
 DnsProbeJob::Result DnsProbeJobImpl::EvaluateQueryResults() {
   if (good_result_ == QUERY_NET_ERROR || bad_result_ == QUERY_NET_ERROR)
     return SERVERS_UNREACHABLE;
 
   if (good_result_ == QUERY_DNS_ERROR || bad_result_ == QUERY_DNS_ERROR)
     return SERVERS_FAILING;
@@ skipping 36 matching lines @@
 
 scoped_ptr<DnsProbeJob> DnsProbeJob::CreateJob(
     scoped_ptr<DnsClient> dns_client,
     const CallbackType& callback,
     NetLog* net_log) {
   return scoped_ptr<DnsProbeJob>(
       new DnsProbeJobImpl(dns_client.Pass(), callback, net_log));
 }
 
 }  // namespace chrome_browser_net