Display DNS probe results.

chrome/browser/net/dns_probe_runner.cc

+// Copyright (c) 2013 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_runner.h"
+
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"

[mmenke, 2013/06/11 16:15:35]

This is included in header (As is bind, though I gave another suggestion there).

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

+#include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_log.h"
+#include "net/base/net_util.h"
+#include "net/base/network_change_notifier.h"
+#include "net/dns/dns_client.h"
+#include "net/dns/dns_config_service.h"
+#include "net/dns/dns_protocol.h"
+#include "net/dns/dns_response.h"
+#include "net/dns/dns_transaction.h"
+
+using base::TimeDelta;
+using net::AddressList;
+using net::BoundNetLog;
+using net::DnsClient;
+using net::DnsConfig;
+using net::DnsResponse;
+using net::DnsTransaction;
+using net::IPAddressNumber;
+using net::IPEndPoint;
+using net::NetLog;
+using net::NetworkChangeNotifier;
+using net::ParseIPLiteralToNumber;
+
+namespace {
+
+// The public DNS servers used by the DnsProbeService to verify internet
+// connectivity.
+const char kPublicDnsPrimary[]   = "8.8.8.8";
+const char kPublicDnsSecondary[] = "8.8.4.4";
+
+IPEndPoint MakeDnsEndPoint(const std::string& dns_ip_literal) {
+  IPAddressNumber dns_ip_number;
+  bool rv = ParseIPLiteralToNumber(dns_ip_literal, &dns_ip_number);
+  DCHECK(rv);
+  return IPEndPoint(dns_ip_number, net::dns_protocol::kDefaultPort);
+}
+
+// Returns true if the given net_error indicates that we received a response

[mmenke, 2013/06/11 16:15:35]

nit:  |net_error| (References to arguments should be in ||'s, per Google style
guide).  Same for the next line.

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

+// from the DNS server containing an error, or false if the given net_error
+// indicates that we never received a response.
+bool DidReceiveDnsResponse(int net_error) {
+  switch (net_error) {
+  case net::ERR_NAME_NOT_RESOLVED:  // NXDOMAIN maps to this.
+  case net::ERR_DNS_MALFORMED_RESPONSE:
+  case net::ERR_DNS_SERVER_REQUIRES_TCP:

[mmenke, 2013/06/11 16:15:35]

Think it's worth noting that this shouldn't happen, since the DnsTransaction
supports TCP fallback.

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

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

[mmenke, 2013/06/11 16:15:35]

completely optional nit:  Just to make the behavior match the name, may want to
make OK return true, too (And update comment).  Or can just rename the function
name to "DidReceiveDnsError" or DidReceiveDnsFailingResponse" or somesuch.

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

+  default:
+    return false;
+  }
+}
+
+}  // namespace
+
+namespace chrome_browser_net {
+
+DnsProbeRunner::DnsProbeRunner()
+    : weak_factory_(this),
+      bound_net_log_(BoundNetLog::Make(NULL, NetLog::SOURCE_DNS_PROBER)) {

[mmenke, 2013/06/11 16:15:35]

Per other comment, not needed.

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

+}
+
+DnsProbeRunner::~DnsProbeRunner() {}
+
+void DnsProbeRunner::RunProbe(const ProbeCallback& callback) {
+  DCHECK(client_.get());
+  DCHECK(!is_running());
+  DCHECK(!callback.is_null());
+
+  callback_ = callback;
+  transaction_ = client_->GetTransactionFactory()->CreateTransaction(
+      "google.com",

[mmenke, 2013/06/11 16:15:35]

Suggest putting this up top, with the public DNS IPs, just to make it more
obvious.

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

+      net::dns_protocol::kTypeA,
+      base::Bind(&DnsProbeRunner::OnTransactionComplete,
+                 weak_factory_.GetWeakPtr(),
+                 true /* async */),
+      bound_net_log_);

[mmenke, 2013/06/11 16:15:35]

BoundNetLog()

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

+
+  int rv = transaction_->Start();
+  if (rv != net::ERR_IO_PENDING) {
+    OnTransactionComplete(false /* not async */,
+                          transaction_.get(),
+                          rv,
+                          NULL);
+  }
+}
+
+void DnsProbeRunner::set_client(scoped_ptr<net::DnsClient> client) {
+  client_ = client.Pass();
+}
+
+bool DnsProbeRunner::is_running() const {
+  return transaction_.get();
+}
+
+void DnsProbeRunner::OnTransactionComplete(
+    bool async,
+    DnsTransaction* transaction,
+    int net_error,
+    const DnsResponse* response) {
+  DCHECK(is_running());
+  DCHECK_EQ(transaction_.get(), transaction);
+
+  const Result result = EvaluateResponse(async, net_error, response);
+  transaction_.reset();
+  callback_.Run(result);
+  callback_ = ProbeCallback();

[mmenke, 2013/06/11 16:15:35]

To avoid the possibility of any re-entrancy issues, I suggest clearing callback_
and storing it in the local before calling it.  This admittedly isn't fool proof
- could still get infinite recursion and run out of stack space with a bunch of
sync failures in a row - but still feel better with at least some protection in
place.

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

Done.

+}
+
+// static
+DnsProbeRunner::Result DnsProbeRunner::EvaluateResponse(
+    bool async,
+    int net_error,
+    const DnsResponse* response) {
+  if (!async) {

[mmenke, 2013/06/11 16:15:35]

Do we need this case / parameter?  Seems like we'd just be getting a net_error
in this case, anyways, and the next code block would handle it.

[Deprecated (see juliatuttle), 2013/06/13 14:37:04]

I like having it explicit, but I'll drop it if you want.

[mmenke, 2013/06/13 15:00:12]

I believe it's technically possible to get a sync success...  Just extremely
unlikely.

[szym, 2013/06/13 15:10:18]

Actually, DnsTransaction can never complete synchronously:
https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/dns_transa...

[mmenke, 2013/06/13 15:42:12]

Thanks!  Hmm...That was added to fix a crasher when it actually did succeed
synchronously, wasn't it?

[szym, 2013/06/13 15:44:44]

I'd like to say that it was by design, but yes, it was a crasher
http://crbug.com/120976

+    return UNREACHABLE;
+  }
+
+  if (net_error != net::OK) {
+    if (DidReceiveDnsResponse(net_error)) {
+      return FAILING;
+    } else {
+      return UNREACHABLE;
+    }
+  }
+
+  AddressList addr_list;
+  TimeDelta ttl;
+  DnsResponse::Result result = response->ParseToAddressList(&addr_list, &ttl);
+
+  if (result != DnsResponse::DNS_PARSE_OK) {
+    return FAILING;
+  }
+
+  if (addr_list.empty()) {
+    return INCORRECT;
+  }
+
+  return CORRECT;
+}
+
+}  // namespace chrome_browser_net