DnsProbeJob: Differentiate network and DNS errors

DnsProbeJob: Differentiate network and DNS errors

DnsTransaction uses net_error to return errors returned by the remote DNS
server as well as local errors encountered trying to talk to the server (like
timeouts).  Make note of which net errors mean that we received a response
from the DNS server, and mark those as "failing" instead of "unreachable".

Also, add another behavior to mock DNS transactions so we can test cases where
the server is unreachable.

BUG=156415
TEST=updated DnsProbeJobTest with a new case


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172780

[Deprecated (see juliatuttle), 2012-12-11 20:43:12]

PTAL, mmenke and szym.

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

LGTM with some nits and comments.

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
File chrome/browser/net/dns_probe_job.cc (right):

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:35: static bool IsDnsServerError(int
net_error) {
no need for static

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:35: static bool IsDnsServerError(int
net_error) {
The name is a bit misleading. Some of those errors are issues with the
(incomplete) client.

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:38: case net::ERR_DNS_MALFORMED_RESPONSE:
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.

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:44: return false;
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]

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
File chrome/browser/net/dns_probe_job.cc (right):

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:35: static bool IsDnsServerError(int
net_error) {
On 2012/12/11 21:05:31, szym wrote:
> no need for static

Done.

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:35: static bool IsDnsServerError(int
net_error) {
On 2012/12/11 21:05:31, szym wrote:
> The name is a bit misleading. Some of those errors are issues with the
> (incomplete) client.

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

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:38: case net::ERR_DNS_MALFORMED_RESPONSE:
On 2012/12/11 21:05:31, szym wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:44: return false;
On 2012/12/11 21:05:31, szym wrote:
> 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.

Why do we retry on NXDOMAIN?

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

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
File chrome/browser/net/dns_probe_job.cc (right):

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:35: static bool IsDnsServerError(int
net_error) {
On 2012/12/11 23:54:03, ttuttle wrote:
> On 2012/12/11 21:05:31, szym wrote:
> > The name is a bit misleading. Some of those errors are issues with the
> > (incomplete) client.
> 
> What's a better name?  DoesErrorMeanServerResponded seems a little
heavyweight.

DidReceiveDnsResponse()

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

https://chromiumcodereview.appspot.com/11530018/diff/1/chrome/browser/net/dns...
chrome/browser/net/dns_probe_job.cc:44: return false;
On 2012/12/11 23:54:03, ttuttle wrote:
> On 2012/12/11 21:05:31, szym wrote:
> > 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.
> 
> Why do we retry on NXDOMAIN?

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

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

LGTM

https://codereview.chromium.org/11530018/diff/1/chrome/browser/net/dns_probe_...
File chrome/browser/net/dns_probe_job.cc (right):

https://codereview.chromium.org/11530018/diff/1/chrome/browser/net/dns_probe_...
chrome/browser/net/dns_probe_job.cc:35: static bool IsDnsServerError(int
net_error) {
On 2012/12/12 01:13:14, szym wrote:
> On 2012/12/11 23:54:03, ttuttle wrote:
> > On 2012/12/11 21:05:31, szym wrote:
> > > The name is a bit misleading. Some of those errors are issues with the
> > > (incomplete) client.
> > 
> > What's a better name?  DoesErrorMeanServerResponded seems a little
> heavyweight.
> 
> DidReceiveDnsResponse()
> 
> This is a local name so does not have to be globally meaningful.

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.

[commit-bot: I haz the power, 2012-12-12 23:10:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/11530018/8001

[commit-bot: I haz the power, 2012-12-13 02:22:01]

Change committed as 172780