diff options
-rw-r--r-- | chrome/browser/net/dns_probe_job.cc | 137 | ||||
-rw-r--r-- | chrome/browser/net/dns_probe_job.h | 9 | ||||
-rw-r--r-- | chrome/browser/net/dns_probe_job_unittest.cc | 46 | ||||
-rw-r--r-- | chrome/browser/net/dns_probe_service.cc | 18 | ||||
-rw-r--r-- | chrome/browser/net/dns_probe_service.h | 8 | ||||
-rw-r--r-- | chrome/browser/net/dns_probe_service_unittest.cc | 22 |
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 |