DnsProbeJob: Start asynchronously

DnsProbeJob can sometimes finish "instantly" if DNS transactions fail
synchronously.  DnsProbeService sometimes needs to delete a job right after
creating it.  By making the job start asynchronously using a weak pointer,
we let the service delete it without having it send a callback.

This reverses https://chromiumcodereview.appspot.com/11308244, which is no longer necessary now that we can create and delete a job within the same task and not receive a callback.

BUG=156415,162636

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

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

3 files changed, 87 insertions, 53 deletions

diff --git a/chrome/browser/net/dns_probe_job.cc b/chrome/browser/net/dns_probe_job.cc
index 6f57b5a..d638d0c 100644
--- a/chrome/browser/net/dns_probe_job.cc
+++ b/chrome/browser/net/dns_probe_job.cc
@@ -4,7 +4,9 @@
 
 #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"
 #include "base/time.h"
 #include "net/base/address_list.h"
@@ -43,6 +45,8 @@ class DnsProbeJobImpl : public DnsProbeJob {
     QUERY_NET_ERROR,
   };
 
+  void Start();
+
   scoped_ptr<DnsTransaction> CreateTransaction(
       const std::string& hostname);
   void StartTransaction(DnsTransaction* transaction);
@@ -55,7 +59,6 @@ class DnsProbeJobImpl : public DnsProbeJob {
   void OnTransactionComplete(DnsTransaction* transaction,
                              int net_error,
                              const DnsResponse* response);
-  void RunCallback(DnsProbeJob::Result result);
 
   BoundNetLog bound_net_log_;
   scoped_ptr<DnsClient> dns_client_;
@@ -66,6 +69,7 @@ class DnsProbeJobImpl : public DnsProbeJob {
   bool bad_running_;
   QueryResult good_result_;
   QueryResult bad_result_;
+  base::WeakPtrFactory<DnsProbeJobImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsProbeJobImpl);
 };
@@ -80,10 +84,21 @@ DnsProbeJobImpl::DnsProbeJobImpl(scoped_ptr<DnsClient> dns_client,
       good_running_(false),
       bad_running_(false),
       good_result_(QUERY_UNKNOWN),
-      bad_result_(QUERY_UNKNOWN) {
+      bad_result_(QUERY_UNKNOWN),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
   DCHECK(dns_client_.get());
   DCHECK(dns_client_->GetConfig());
 
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&DnsProbeJobImpl::Start,
+                 weak_factory_.GetWeakPtr()));
+}
+
+DnsProbeJobImpl::~DnsProbeJobImpl() {
+}
+
+void DnsProbeJobImpl::Start() {
   // TODO(ttuttle): Pick a good random hostname for the bad case.
   //                Consider running transactions in series?
   good_transaction_ = CreateTransaction("google.com");
@@ -100,9 +115,6 @@ DnsProbeJobImpl::DnsProbeJobImpl(scoped_ptr<DnsClient> dns_client,
   StartTransaction(bad_transaction_.get());
 }
 
-DnsProbeJobImpl::~DnsProbeJobImpl() {
-}
-
 scoped_ptr<DnsTransaction> DnsProbeJobImpl::CreateTransaction(
     const std::string& hostname) {
   return dns_client_->GetTransactionFactory()->CreateTransaction(
@@ -192,20 +204,11 @@ void DnsProbeJobImpl::OnTransactionComplete(DnsTransaction* transaction,
   if (good_running_ || bad_running_)
     return;
 
-  RunCallback(EvaluateQueryResults());
+  callback_.Run(this, EvaluateQueryResults());
 
   // TODO(ttuttle): Log probe finished.
 }
 
-void DnsProbeJobImpl::RunCallback(DnsProbeJob::Result result) {
-  // Make sure we're not running the callback in the constructor.
-  // This avoids a race where our owner tries to destroy us while we're still
-  // being created, then ends up with a dangling pointer to us.
-  MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(callback_, base::Unretained(this), result));
-}
-
 }  // namespace
 
 scoped_ptr<DnsProbeJob> DnsProbeJob::CreateJob(
diff --git a/chrome/browser/net/dns_probe_service.cc b/chrome/browser/net/dns_probe_service.cc
index 6fa36d6..e8d5c24 100644
--- a/chrome/browser/net/dns_probe_service.cc
+++ b/chrome/browser/net/dns_probe_service.cc
@@ -109,8 +109,7 @@ void DnsProbeService::StartProbes() {
   public_result_ = DnsProbeJob::SERVERS_UNKNOWN;
 
   system_job_ = CreateSystemProbeJob(job_callback);
-  if (system_job_.get())
-    public_job_ = CreatePublicProbeJob(job_callback);
+  public_job_ = CreatePublicProbeJob(job_callback);
 
   // If we can't create one or both jobs, fail the probe immediately.
   if (!system_job_.get() || !public_job_.get()) {
diff --git a/chrome/browser/net/dns_probe_service_unittest.cc b/chrome/browser/net/dns_probe_service_unittest.cc
index 06aec2c..62e886e 100644
--- a/chrome/browser/net/dns_probe_service_unittest.cc
+++ b/chrome/browser/net/dns_probe_service_unittest.cc
@@ -5,6 +5,8 @@
 #include "chrome/browser/net/dns_probe_service.h"
 
 #include "base/bind.h"
+#include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop.h"
 #include "base/run_loop.h"
 #include "chrome/browser/net/dns_probe_job.h"
@@ -16,14 +18,24 @@ namespace {
 
 class MockDnsProbeJob : public DnsProbeJob {
  public:
-  MockDnsProbeJob(const CallbackType& callback,
-                  Result result) {
+  MockDnsProbeJob(const CallbackType& callback, Result result)
+      : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
     MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(callback, base::Unretained(this), result));
+        base::Bind(&MockDnsProbeJob::CallCallback,
+                   weak_factory_.GetWeakPtr(),
+                   callback,
+                   result));
   }
 
   virtual ~MockDnsProbeJob() { }
+
+ private:
+  void CallCallback(const CallbackType& callback, Result result) {
+    callback.Run(this, result);
+  }
+
+  base::WeakPtrFactory<MockDnsProbeJob> weak_factory_;
 };
 
 class TestDnsProbeService : public DnsProbeService {
@@ -33,18 +45,32 @@ class TestDnsProbeService : public DnsProbeService {
         system_job_created_(false),
         public_job_created_(false),
         mock_system_result_(DnsProbeJob::SERVERS_UNKNOWN),
-        mock_public_result_(DnsProbeJob::SERVERS_UNKNOWN) {
+        mock_public_result_(DnsProbeJob::SERVERS_UNKNOWN),
+        mock_system_fail_(false) {
   }
 
   virtual ~TestDnsProbeService() { }
 
-  void SetMockJobResults(
+  void set_mock_results(
       DnsProbeJob::Result mock_system_result,
       DnsProbeJob::Result mock_public_result) {
     mock_system_result_ = mock_system_result;
     mock_public_result_ = mock_public_result;
   }
 
+  void set_mock_system_fail(bool mock_system_fail) {
+    mock_system_fail_ = mock_system_fail;
+  }
+
+  bool jobs_created(void) {
+    return system_job_created_ && public_job_created_;
+  }
+
+  void ResetJobsCreated() {
+    system_job_created_ = false;
+    public_job_created_ = false;
+  }
+
   void MockExpireResults() {
     ExpireResults();
   }
@@ -57,6 +83,9 @@ class TestDnsProbeService : public DnsProbeService {
 
   virtual scoped_ptr<DnsProbeJob> CreateSystemProbeJob(
       const DnsProbeJob::CallbackType& job_callback) OVERRIDE {
+    if (mock_system_fail_)
+      return scoped_ptr<DnsProbeJob>(NULL);
+
     system_job_created_ = true;
     return scoped_ptr<DnsProbeJob>(
         new MockDnsProbeJob(job_callback,
@@ -73,6 +102,7 @@ class TestDnsProbeService : public DnsProbeService {
 
   DnsProbeJob::Result mock_system_result_;
   DnsProbeJob::Result mock_public_result_;
+  bool mock_system_fail_;
 };
 
 class DnsProbeServiceTest : public testing::Test {
@@ -82,14 +112,9 @@ class DnsProbeServiceTest : public testing::Test {
         callback_result_(DnsProbeService::PROBE_UNKNOWN) {
   }
 
-  void SetMockJobResults(DnsProbeJob::Result mock_system_result,
-                         DnsProbeJob::Result mock_public_result) {
-    service_.SetMockJobResults(mock_system_result, mock_public_result);
-  }
-
   void Probe() {
     service_.ProbeDns(base::Bind(&DnsProbeServiceTest::ProbeCallback,
-                                  base::Unretained(this)));
+                                 base::Unretained(this)));
   }
 
   void RunUntilIdle() {
@@ -97,8 +122,9 @@ class DnsProbeServiceTest : public testing::Test {
     run_loop.RunUntilIdle();
   }
 
-  void ExpireResults() {
-    service_.MockExpireResults();
+  void Reset() {
+    service_.ResetJobsCreated();
+    callback_called_ = false;
   }
 
   MessageLoopForIO message_loop_;
@@ -117,11 +143,11 @@ TEST_F(DnsProbeServiceTest, Null) {
 }
 
 TEST_F(DnsProbeServiceTest, Probe) {
-  SetMockJobResults(DnsProbeJob::SERVERS_CORRECT, DnsProbeJob::SERVERS_CORRECT);
+  service_.set_mock_results(DnsProbeJob::SERVERS_CORRECT,
+                            DnsProbeJob::SERVERS_CORRECT);
 
   Probe();
-  EXPECT_TRUE(service_.system_job_created_);
-  EXPECT_TRUE(service_.public_job_created_);
+  EXPECT_TRUE(service_.jobs_created());
   EXPECT_FALSE(callback_called_);
 
   RunUntilIdle();
@@ -130,23 +156,17 @@ TEST_F(DnsProbeServiceTest, Probe) {
 }
 
 TEST_F(DnsProbeServiceTest, Cache) {
-  SetMockJobResults(DnsProbeJob::SERVERS_CORRECT, DnsProbeJob::SERVERS_CORRECT);
+  service_.set_mock_results(DnsProbeJob::SERVERS_CORRECT,
+                            DnsProbeJob::SERVERS_CORRECT);
 
   Probe();
-  EXPECT_TRUE(service_.system_job_created_);
-  EXPECT_TRUE(service_.public_job_created_);
-
   RunUntilIdle();
-  EXPECT_TRUE(callback_called_);
-  EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
+  Reset();
 
-  callback_called_ = false;
-  service_.system_job_created_ = false;
-  service_.public_job_created_ = false;
+  // Cached NXDOMAIN result should persist.
 
   Probe();
-  EXPECT_FALSE(service_.system_job_created_);
-  EXPECT_FALSE(service_.public_job_created_);
+  EXPECT_FALSE(service_.jobs_created());
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
@@ -154,31 +174,43 @@ TEST_F(DnsProbeServiceTest, Cache) {
 }
 
 TEST_F(DnsProbeServiceTest, Expired) {
-  SetMockJobResults(DnsProbeJob::SERVERS_CORRECT, DnsProbeJob::SERVERS_CORRECT);
+  service_.set_mock_results(DnsProbeJob::SERVERS_CORRECT,
+                            DnsProbeJob::SERVERS_CORRECT);
 
   Probe();
-  EXPECT_TRUE(service_.system_job_created_);
-  EXPECT_TRUE(service_.public_job_created_);
+  EXPECT_TRUE(service_.jobs_created());
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
   EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
 
-  callback_called_ = false;
-  service_.system_job_created_ = false;
-  service_.public_job_created_ = false;
+  Reset();
 
-  ExpireResults();
+  service_.MockExpireResults();
 
   Probe();
-  EXPECT_TRUE(service_.system_job_created_);
-  EXPECT_TRUE(service_.public_job_created_);
+  EXPECT_TRUE(service_.jobs_created());
 
   RunUntilIdle();
   EXPECT_TRUE(callback_called_);
   EXPECT_EQ(DnsProbeService::PROBE_NXDOMAIN, callback_result_);
 }
 
+TEST_F(DnsProbeServiceTest, SystemFail) {
+  service_.set_mock_results(DnsProbeJob::SERVERS_CORRECT,
+                            DnsProbeJob::SERVERS_CORRECT);
+  service_.set_mock_system_fail(true);
+
+  Probe();
+  EXPECT_TRUE(callback_called_);
+  EXPECT_EQ(DnsProbeService::PROBE_UNKNOWN, callback_result_);
+
+  Reset();
+
+  RunUntilIdle();
+  EXPECT_FALSE(callback_called_);
+}
+
 }  // namespace
 
 }  // namespace chrome_browser_net