DnsProbeJob: Start fleshing things out.

BUG=156415
TEST=updates accompanying unittests; they still pass


Review URL: https://chromiumcodereview.appspot.com/11264044

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@165419 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 149 insertions, 91 deletions

diff --git a/chrome/browser/net/dns_probe_job.cc b/chrome/browser/net/dns_probe_job.cc
index cb2555f..3e5328f 100644
--- a/chrome/browser/net/dns_probe_job.cc
+++ b/chrome/browser/net/dns_probe_job.cc
@@ -6,12 +6,17 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/time.h"
+#include "net/base/address_list.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/dns/dns_client.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::DnsResponse;
@@ -30,18 +35,23 @@ class DnsProbeJobImpl : public DnsProbeJob {
   virtual ~DnsProbeJobImpl();
 
  private:
-  enum QueryStatus {
+  enum QueryResult {
     QUERY_UNKNOWN,
     QUERY_CORRECT,
     QUERY_INCORRECT,
-    QUERY_ERROR,
-    QUERY_RUNNING,
+    QUERY_DNS_ERROR,
+    QUERY_NET_ERROR,
   };
 
-  void MaybeFinishProbe();
   scoped_ptr<DnsTransaction> CreateTransaction(
       const std::string& hostname);
   void StartTransaction(DnsTransaction* transaction);
+  // Checks that |net_error| is OK, |response| parses, and has at least one
+  // address.
+  QueryResult EvaluateGoodResponse(int net_error, const DnsResponse* response);
+  // Checks that |net_error| is OK, |response| parses but has no addresses.
+  QueryResult EvaluateBadResponse(int net_error, const DnsResponse* response);
+  DnsProbeJob::Result EvaluateQueryResults();
   void OnTransactionComplete(DnsTransaction* transaction,
                              int net_error,
                              const DnsResponse* response);
@@ -50,11 +60,12 @@ class DnsProbeJobImpl : public DnsProbeJob {
   BoundNetLog bound_net_log_;
   scoped_ptr<DnsClient> dns_client_;
   const DnsProbeJob::CallbackType callback_;
-  bool probe_running_;
   scoped_ptr<DnsTransaction> good_transaction_;
   scoped_ptr<DnsTransaction> bad_transaction_;
-  QueryStatus good_status_;
-  QueryStatus bad_status_;
+  bool good_running_;
+  bool bad_running_;
+  QueryResult good_result_;
+  QueryResult bad_result_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsProbeJobImpl);
 };
@@ -66,22 +77,22 @@ DnsProbeJobImpl::DnsProbeJobImpl(scoped_ptr<DnsClient> dns_client,
           BoundNetLog::Make(net_log, NetLog::SOURCE_DNS_PROBER)),
       dns_client_(dns_client.Pass()),
       callback_(callback),
-      probe_running_(false),
-      good_status_(QUERY_UNKNOWN),
-      bad_status_(QUERY_UNKNOWN) {
+      good_running_(false),
+      bad_running_(false),
+      good_result_(QUERY_UNKNOWN),
+      bad_result_(QUERY_UNKNOWN) {
   DCHECK(dns_client_.get());
   DCHECK(dns_client_->GetConfig());
 
   // TODO(ttuttle): Pick a good random hostname for the bad case.
   //                Consider running transactions in series?
   good_transaction_ = CreateTransaction("google.com");
-  bad_transaction_  = CreateTransaction("thishostname.doesnotresolve");
+  bad_transaction_ = CreateTransaction("thishostname.doesnotresolve");
 
   // StartTransaction may call the callback synchrononously, so set these
   // before we call it.
-  probe_running_ = true;
-  good_status_ = QUERY_RUNNING;
-  bad_status_ = QUERY_RUNNING;
+  good_running_ = true;
+  bad_running_ = true;
 
   // TODO(ttuttle): Log probe started.
 
@@ -93,23 +104,6 @@ DnsProbeJobImpl::~DnsProbeJobImpl() {
   // TODO(ttuttle): Cleanup?  (Transactions stop themselves on destruction.)
 }
 
-void DnsProbeJobImpl::MaybeFinishProbe(void) {
-  DCHECK(probe_running_);
-
-  if (good_status_ == QUERY_RUNNING || bad_status_ == QUERY_RUNNING)
-    return;
-
-  probe_running_ = false;
-
-  // TODO(ttuttle): Flesh out logic.
-  if (good_status_ == QUERY_ERROR || bad_status_ == QUERY_ERROR)
-    RunCallback(DNS_BROKEN);
-  else
-    RunCallback(DNS_WORKING);
-
-  // TODO(ttuttle): Log probe finished.
-}
-
 scoped_ptr<DnsTransaction> DnsProbeJobImpl::CreateTransaction(
     const std::string& hostname) {
   return dns_client_->GetTransactionFactory()->CreateTransaction(
@@ -130,34 +124,83 @@ void DnsProbeJobImpl::StartTransaction(DnsTransaction* transaction) {
   // TODO(ttuttle): Log transaction started.
 }
 
+DnsProbeJobImpl::QueryResult DnsProbeJobImpl::EvaluateGoodResponse(
+    int net_error,
+    const DnsResponse* response) {
+  if (net_error != net::OK)
+    return 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::OK)
+    return 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_CORRECT;
+
+  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;
+
+  // Ignore incorrect responses to known-bad query to avoid flagging domain
+  // helpers.
+  if (good_result_ == QUERY_INCORRECT)
+    return SERVERS_INCORRECT;
+
+  return SERVERS_CORRECT;
+}
+
 void DnsProbeJobImpl::OnTransactionComplete(DnsTransaction* transaction,
                                             int net_error,
                                             const DnsResponse* response) {
-  QueryStatus* status;
-
   if (transaction == good_transaction_.get()) {
-    status = &good_status_;
-    good_transaction_.reset();
+    DCHECK(good_running_);
+    DCHECK_EQ(QUERY_UNKNOWN, good_result_);
+    good_result_ = EvaluateGoodResponse(net_error, response);
+    good_running_ = false;
   } else if (transaction == bad_transaction_.get()) {
-    status = &bad_status_;
-    bad_transaction_.reset();
+    DCHECK(bad_running_);
+    DCHECK_EQ(QUERY_UNKNOWN, bad_result_);
+    bad_result_ = EvaluateBadResponse(net_error, response);
+    bad_running_ = false;
   } else {
     NOTREACHED();
     return;
   }
 
-  DCHECK_EQ(QUERY_RUNNING, *status);
-
-  if (net_error == net::OK) {
-    // TODO(ttuttle): Examine DNS response and make sure it's correct.
-    *status = QUERY_CORRECT;
-  } else {
-    *status = QUERY_ERROR;
-  }
+  if (good_running_ || bad_running_)
+    return;
 
-  // TODO(ttuttle): Log transaction completed.
+  RunCallback(EvaluateQueryResults());
 
-  MaybeFinishProbe();
+  // TODO(ttuttle): Log probe finished.
 }
 
 void DnsProbeJobImpl::RunCallback(DnsProbeJob::Result result) {
diff --git a/chrome/browser/net/dns_probe_job.h b/chrome/browser/net/dns_probe_job.h
index 462ec46..cbed207 100644
--- a/chrome/browser/net/dns_probe_job.h
+++ b/chrome/browser/net/dns_probe_job.h
@@ -20,12 +20,13 @@ namespace chrome_browser_net {
 class DnsProbeJob {
  public:
   enum Result {
-    DNS_UNKNOWN,
-    DNS_WORKING,  // Server responds with correct answers.
+    SERVERS_UNKNOWN,
+    SERVERS_CORRECT,  // Server responds with correct answers.
+    SERVERS_INCORRECT,  // Server responds with success but incorrect answer.
     // TODO(ttuttle): Do we want an "unreliable" result, for e.g. servers that
     // lie about NXDOMAIN?
-    DNS_BROKEN,  // Server responds with errors.
-    DNS_UNREACHABLE,  // Server doesn't respond (or never got our packets)
+    SERVERS_FAILING,  // Server responds with errors.
+    SERVERS_UNREACHABLE,  // Server doesn't respond (or never got our packets)
   };
   typedef base::Callback<void(DnsProbeJob* job, Result result)> CallbackType;
 
diff --git a/chrome/browser/net/dns_probe_job_unittest.cc b/chrome/browser/net/dns_probe_job_unittest.cc
index 10c2946..ce2bf85 100644
--- a/chrome/browser/net/dns_probe_job_unittest.cc
+++ b/chrome/browser/net/dns_probe_job_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/net/dns_probe_job.h"
 
+#include "base/basictypes.h"
 #include "base/message_loop.h"
 #include "base/run_loop.h"
 #include "net/base/net_log.h"
@@ -66,7 +67,7 @@ void DnsProbeJobTest::RunProbe(MockDnsClientRule::Result good_result,
   // Need to set these before creating job, because it can call the callback
   // synchronously in the constructor if both transactions fail to start.
   callback_called_ = false;
-  callback_result_ = DnsProbeJob::DNS_UNKNOWN;
+  callback_result_ = DnsProbeJob::SERVERS_UNKNOWN;
 
   // DnsProbeJob needs somewhere to post the callback.
   scoped_ptr<MessageLoop> message_loop_(new MessageLoopForIO());
@@ -87,22 +88,35 @@ void DnsProbeJobTest::OnProbeFinished(DnsProbeJob* job,
   callback_result_ = result;
 }
 
-TEST_F(DnsProbeJobTest, Reliable) {
-  RunProbe(MockDnsClientRule::OK, MockDnsClientRule::EMPTY);
-  EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeJob::DNS_WORKING, callback_result_);
-}
-
-TEST_F(DnsProbeJobTest, BothFailAsync) {
-  RunProbe(MockDnsClientRule::FAIL_ASYNC, MockDnsClientRule::FAIL_ASYNC);
-  EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeJob::DNS_BROKEN, callback_result_);
-}
+struct TestCase {
+  MockDnsClientRule::Result good_result;
+  MockDnsClientRule::Result bad_result;
+  DnsProbeJob::Result expected_probe_result;
+};
 
-TEST_F(DnsProbeJobTest, BothFailSync) {
-  RunProbe(MockDnsClientRule::FAIL_SYNC, MockDnsClientRule::FAIL_SYNC);
-  EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeJob::DNS_BROKEN, callback_result_);
+TEST_F(DnsProbeJobTest, Test) {
+  static const TestCase kTestCases[] = {
+    { MockDnsClientRule::OK,
+      MockDnsClientRule::EMPTY,
+      DnsProbeJob::SERVERS_CORRECT },
+    { MockDnsClientRule::EMPTY,
+      MockDnsClientRule::EMPTY,
+      DnsProbeJob::SERVERS_INCORRECT },
+    // TODO(ttuttle): Test that triggers QUERY_DNS_ERROR.
+    //                (Need to add another mock behavior to MockDnsClient.)
+    { MockDnsClientRule::FAIL_ASYNC,
+      MockDnsClientRule::FAIL_ASYNC,
+      DnsProbeJob::SERVERS_UNREACHABLE },
+    { MockDnsClientRule::FAIL_SYNC,
+      MockDnsClientRule::FAIL_SYNC,
+      DnsProbeJob::SERVERS_UNREACHABLE },
+  };
+  for (size_t i = 0; i < arraysize(kTestCases); i++) {
+    const TestCase* test_case = &kTestCases[i];
+    RunProbe(test_case->good_result, test_case->bad_result);
+    EXPECT_TRUE(callback_called_);
+    EXPECT_EQ(test_case->expected_probe_result, callback_result_);
+  }
 }
 
 }  // namespace
diff --git a/chrome/browser/net/dns_probe_service.cc b/chrome/browser/net/dns_probe_service.cc
index 6280b0c..0d006c3 100644
--- a/chrome/browser/net/dns_probe_service.cc
+++ b/chrome/browser/net/dns_probe_service.cc
@@ -36,10 +36,10 @@ IPEndPoint MakeDnsEndPoint(const std::string& dns_ip_literal) {
 }
 
 DnsProbeService::DnsProbeService()
-    : system_result_(DnsProbeJob::DNS_UNKNOWN),
-      public_result_(DnsProbeJob::DNS_UNKNOWN),
+    : system_result_(DnsProbeJob::SERVERS_UNKNOWN),
+      public_result_(DnsProbeJob::SERVERS_UNKNOWN),
       state_(STATE_NO_RESULTS),
-      result_(DNS_PROBE_UNKNOWN) {
+      result_(PROBE_UNKNOWN) {
 }
 
 DnsProbeService::~DnsProbeService() {
@@ -81,7 +81,7 @@ void DnsProbeService::ExpireResults() {
   DCHECK_EQ(STATE_RESULTS_CACHED, state_);
 
   state_ = STATE_NO_RESULTS;
-  result_ = DNS_PROBE_UNKNOWN;
+  result_ = PROBE_UNKNOWN;
 }
 
 void DnsProbeService::StartProbes() {
@@ -95,8 +95,8 @@ void DnsProbeService::StartProbes() {
 
   // TODO(ttuttle): Do we want to keep explicit flags for "job done"?
   //                Or maybe DnsProbeJob should have a "finished" flag?
-  system_result_ = DnsProbeJob::DNS_UNKNOWN;
-  public_result_ = DnsProbeJob::DNS_UNKNOWN;
+  system_result_ = DnsProbeJob::SERVERS_UNKNOWN;
+  public_result_ = DnsProbeJob::SERVERS_UNKNOWN;
 
   system_job_ = CreateSystemProbeJob(job_callback);
   public_job_ = CreatePublicProbeJob(job_callback);
@@ -109,7 +109,7 @@ void DnsProbeService::OnProbesComplete() {
 
   state_ = STATE_RESULTS_CACHED;
   // TODO(ttuttle): Calculate result.
-  result_ = DNS_PROBE_NXDOMAIN;
+  result_ = PROBE_NXDOMAIN;
 
   CallCallbacks();
 }
@@ -150,8 +150,8 @@ void DnsProbeService::OnProbeJobComplete(DnsProbeJob* job,
     return;
   }
 
-  if (system_result_ != DnsProbeJob::DNS_UNKNOWN &&
-      public_result_ != DnsProbeJob::DNS_UNKNOWN) {
+  if (system_result_ != DnsProbeJob::SERVERS_UNKNOWN &&
+      public_result_ != DnsProbeJob::SERVERS_UNKNOWN) {
     OnProbesComplete();
   }
 }
diff --git a/chrome/browser/net/dns_probe_service.h b/chrome/browser/net/dns_probe_service.h
index dfcc921..22430b2 100644
--- a/chrome/browser/net/dns_probe_service.h
+++ b/chrome/browser/net/dns_probe_service.h
@@ -20,10 +20,10 @@ namespace chrome_browser_net {
 class DnsProbeService {
  public:
   enum Result {
-    DNS_PROBE_UNKNOWN,
-    DNS_PROBE_NO_INTERNET,
-    DNS_PROBE_BAD_CONFIG,
-    DNS_PROBE_NXDOMAIN
+    PROBE_UNKNOWN,
+    PROBE_NO_INTERNET,
+    PROBE_BAD_CONFIG,
+    PROBE_NXDOMAIN
   };
   typedef base::Callback<void(Result result)> CallbackType;
 
diff --git a/chrome/browser/net/dns_probe_service_unittest.cc b/chrome/browser/net/dns_probe_service_unittest.cc
index 3d8ac03..06aec2c 100644
--- a/chrome/browser/net/dns_probe_service_unittest.cc
+++ b/chrome/browser/net/dns_probe_service_unittest.cc
@@ -32,8 +32,8 @@ class TestDnsProbeService : public DnsProbeService {
       : DnsProbeService(),
         system_job_created_(false),
         public_job_created_(false),
-        mock_system_result_(DnsProbeJob::DNS_UNKNOWN),
-        mock_public_result_(DnsProbeJob::DNS_UNKNOWN) {
+        mock_system_result_(DnsProbeJob::SERVERS_UNKNOWN),
+        mock_public_result_(DnsProbeJob::SERVERS_UNKNOWN) {
   }
 
   virtual ~TestDnsProbeService() { }
@@ -79,7 +79,7 @@ class DnsProbeServiceTest : public testing::Test {
  public:
   DnsProbeServiceTest()
       : callback_called_(false),
-        callback_result_(DnsProbeService::DNS_PROBE_UNKNOWN) {
+        callback_result_(DnsProbeService::PROBE_UNKNOWN) {
   }
 
   void SetMockJobResults(DnsProbeJob::Result mock_system_result,
@@ -117,7 +117,7 @@ TEST_F(DnsProbeServiceTest, Null) {
 }
 
 TEST_F(DnsProbeServiceTest, Probe) {
-  SetMockJobResults(DnsProbeJob::DNS_WORKING, DnsProbeJob::DNS_WORKING);
+  SetMockJobResults(DnsProbeJob::SERVERS_CORRECT, DnsProbeJob::SERVERS_CORRECT);
 
   Probe();
   EXPECT_TRUE(service_.system_job_created_);
@@ -126,11 +126,11 @@ TEST_F(DnsProbeServiceTest, Probe) {
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeService::DNS_PROBE_NXDOMAIN, callback_result_);
+  EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
 }
 
 TEST_F(DnsProbeServiceTest, Cache) {
-  SetMockJobResults(DnsProbeJob::DNS_WORKING, DnsProbeJob::DNS_WORKING);
+  SetMockJobResults(DnsProbeJob::SERVERS_CORRECT, DnsProbeJob::SERVERS_CORRECT);
 
   Probe();
   EXPECT_TRUE(service_.system_job_created_);
@@ -138,7 +138,7 @@ TEST_F(DnsProbeServiceTest, Cache) {
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeService::DNS_PROBE_NXDOMAIN, callback_result_);
+  EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
 
   callback_called_ = false;
   service_.system_job_created_ = false;
@@ -150,11 +150,11 @@ TEST_F(DnsProbeServiceTest, Cache) {
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeService::DNS_PROBE_NXDOMAIN, callback_result_);
+  EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
 }
 
 TEST_F(DnsProbeServiceTest, Expired) {
-  SetMockJobResults(DnsProbeJob::DNS_WORKING, DnsProbeJob::DNS_WORKING);
+  SetMockJobResults(DnsProbeJob::SERVERS_CORRECT, DnsProbeJob::SERVERS_CORRECT);
 
   Probe();
   EXPECT_TRUE(service_.system_job_created_);
@@ -162,7 +162,7 @@ TEST_F(DnsProbeServiceTest, Expired) {
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeService::DNS_PROBE_NXDOMAIN, callback_result_);
+  EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
 
   callback_called_ = false;
   service_.system_job_created_ = false;
@@ -176,7 +176,7 @@ TEST_F(DnsProbeServiceTest, Expired) {
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeService::DNS_PROBE_NXDOMAIN, callback_result_);
+  EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
 }
 
 }  // namespace