Make DnsTransaction.Start() never complete synchronously.

This is an uncommon case, currently only done on certain
errors, and complicates logic elsewhere.  Since
HostResolverImpl::Jobs have to complete asynchronously
anyways, even in those rare cases, there was no performance
gain.

This is a prerequsite for at least one potential
implementation of simultaneous A/AAAA queries.

BUG=174992

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

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

7 files changed, 48 insertions, 99 deletions

diff --git a/net/dns/dns_test_util.cc b/net/dns/dns_test_util.cc
index 0482bc5d..37bf855 100644
--- a/net/dns/dns_test_util.cc
+++ b/net/dns/dns_test_util.cc
@@ -34,7 +34,7 @@ class MockTransaction : public DnsTransaction,
                   const std::string& hostname,
                   uint16 qtype,
                   const DnsTransactionFactory::CallbackType& callback)
-      : result_(MockDnsClientRule::FAIL_SYNC),
+      : result_(MockDnsClientRule::FAIL),
         hostname_(hostname),
         qtype_(qtype),
         callback_(callback),
@@ -59,15 +59,12 @@ class MockTransaction : public DnsTransaction,
     return qtype_;
   }
 
-  virtual int Start() OVERRIDE {
+  virtual void Start() OVERRIDE {
     EXPECT_FALSE(started_);
     started_ = true;
-    if (MockDnsClientRule::FAIL_SYNC == result_)
-      return ERR_NAME_NOT_RESOLVED;
     // Using WeakPtr to cleanly cancel when transaction is destroyed.
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(&MockTransaction::Finish, AsWeakPtr()));
-    return ERR_IO_PENDING;
   }
 
  private:
@@ -122,7 +119,7 @@ class MockTransaction : public DnsTransaction,
         EXPECT_TRUE(response.InitParse(nbytes, query));
         callback_.Run(this, OK, &response);
       } break;
-      case MockDnsClientRule::FAIL_ASYNC:
+      case MockDnsClientRule::FAIL:
         callback_.Run(this, ERR_NAME_NOT_RESOLVED, NULL);
         break;
       case MockDnsClientRule::TIMEOUT:
diff --git a/net/dns/dns_test_util.h b/net/dns/dns_test_util.h
index 1b67697..d447b29 100644
--- a/net/dns/dns_test_util.h
+++ b/net/dns/dns_test_util.h
@@ -178,11 +178,10 @@ class DnsClient;
 
 struct MockDnsClientRule {
   enum Result {
-    FAIL_SYNC,   // Fail synchronously with ERR_NAME_NOT_RESOLVED.
-    FAIL_ASYNC,  // Fail asynchronously with ERR_NAME_NOT_RESOLVED.
-    TIMEOUT,     // Fail asynchronously with ERR_DNS_TIMEOUT.
-    EMPTY,       // Return an empty response.
-    OK,          // Return a response with loopback address.
+    FAIL,     // Fail asynchronously with ERR_NAME_NOT_RESOLVED.
+    TIMEOUT,  // Fail asynchronously with ERR_DNS_TIMEOUT.
+    EMPTY,    // Return an empty response.
+    OK,       // Return a response with loopback address.
   };
 
   MockDnsClientRule(const std::string& prefix_arg,
diff --git a/net/dns/dns_transaction.cc b/net/dns/dns_transaction.cc
index b316dd7..e5afdf1 100644
--- a/net/dns/dns_transaction.cc
+++ b/net/dns/dns_transaction.cc
@@ -553,32 +553,25 @@ class DnsTransactionImpl : public DnsTransaction,
     return qtype_;
   }
 
-  virtual int Start() OVERRIDE {
+  virtual void Start() OVERRIDE {
     DCHECK(!callback_.is_null());
     DCHECK(attempts_.empty());
     net_log_.BeginEvent(NetLog::TYPE_DNS_TRANSACTION,
                         base::Bind(&NetLogStartCallback, &hostname_, qtype_));
-    int rv = PrepareSearch();
-    if (rv == OK) {
+    AttemptResult result(PrepareSearch(), NULL);
+    if (result.rv == OK) {
       qnames_initial_size_ = qnames_.size();
       if (qtype_ == dns_protocol::kTypeA)
         UMA_HISTOGRAM_COUNTS("AsyncDNS.SuffixSearchStart", qnames_.size());
-      AttemptResult result = ProcessAttemptResult(StartQuery());
-      if (result.rv == OK) {
-        // DnsTransaction must never succeed synchronously.
-        base::MessageLoop::current()->PostTask(
-            FROM_HERE,
-            base::Bind(&DnsTransactionImpl::DoCallback, AsWeakPtr(), result));
-        return ERR_IO_PENDING;
-      }
-      rv = result.rv;
+      result = ProcessAttemptResult(StartQuery());
     }
-    if (rv != ERR_IO_PENDING) {
-      callback_.Reset();
-      net_log_.EndEventWithNetErrorCode(NetLog::TYPE_DNS_TRANSACTION, rv);
+
+    // Must always return result asynchronously, to avoid reentrancy.
+    if (result.rv != ERR_IO_PENDING) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&DnsTransactionImpl::DoCallback, AsWeakPtr(), result));
     }
-    DCHECK_NE(OK, rv);
-    return rv;
   }
 
  private:
diff --git a/net/dns/dns_transaction.h b/net/dns/dns_transaction.h
index edbb7a7..faf4f64 100644
--- a/net/dns/dns_transaction.h
+++ b/net/dns/dns_transaction.h
@@ -34,10 +34,8 @@ class NET_EXPORT_PRIVATE DnsTransaction {
   // Returns the |qtype|.
   virtual uint16 GetType() const = 0;
 
-  // Starts the transaction. Returns the net error on synchronous failure or
-  // ERR_IO_PENDING in which case the result will be passed via the callback.
-  // Can be called at most once.
-  virtual int Start() = 0;
+  // Starts the transaction.  Always completes asynchronously.
+  virtual void Start() = 0;
 };
 
 // Creates DnsTransaction which performs asynchronous DNS search.
diff --git a/net/dns/dns_transaction_unittest.cc b/net/dns/dns_transaction_unittest.cc
index ee79005..80473b5 100644
--- a/net/dns/dns_transaction_unittest.cc
+++ b/net/dns/dns_transaction_unittest.cc
@@ -250,12 +250,7 @@ class TransactionHelper {
         BoundNetLog());
     EXPECT_EQ(hostname_, transaction_->GetHostname());
     EXPECT_EQ(qtype_, transaction_->GetType());
-    int rv = transaction_->Start();
-    if (rv != ERR_IO_PENDING) {
-      EXPECT_NE(OK, rv);
-      EXPECT_EQ(expected_answer_count_, rv);
-      completed_ = true;
-    }
+    transaction_->Start();
   }
 
   void Cancel() {
@@ -748,7 +743,7 @@ TEST_F(DnsTransactionTest, EmptySuffixSearch) {
   TransactionHelper helper1("singlelabel", dns_protocol::kTypeA,
                             ERR_DNS_SEARCH_EMPTY);
 
-  helper1.StartTransaction(transaction_factory_.get());
+  helper1.Run(transaction_factory_.get());
   EXPECT_TRUE(helper1.has_completed());
 }
 
diff --git a/net/dns/host_resolver_impl.cc b/net/dns/host_resolver_impl.cc
index f4dbe6b..755e490 100644
--- a/net/dns/host_resolver_impl.cc
+++ b/net/dns/host_resolver_impl.cc
@@ -993,9 +993,9 @@ class HostResolverImpl::DnsTask : public base::SupportsWeakPtr<DnsTask> {
         net_log_);
   }
 
-  int Start() {
+  void Start() {
     net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK);
-    return transaction_->Start();
+    transaction_->Start();
   }
 
  private:
@@ -1051,9 +1051,7 @@ class HostResolverImpl::DnsTask : public base::SupportsWeakPtr<DnsTask> {
             base::Bind(&DnsTask::OnTransactionComplete, base::Unretained(this),
                        false /* first_query */, base::TimeTicks::Now()),
             net_log_);
-        net_error = transaction_->Start();
-        if (net_error != ERR_IO_PENDING)
-          OnFailure(net_error, DnsResponse::DNS_PARSE_OK);
+        transaction_->Start();
         return;
       }
     } else {
@@ -1448,27 +1446,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
         base::Bind(&Job::OnDnsTaskComplete, base::Unretained(this), start_time),
         net_log_));
 
-    int rv = dns_task_->Start();
-    if (rv == ERR_IO_PENDING)
-      return;  // Complete asynchronously.
-    DCHECK_NE(OK, rv);
-    base::TimeDelta duration = base::TimeTicks::Now() - start_time;
-    if (resolver_->fallback_to_proctask_) {
-      DNS_HISTOGRAM("AsyncDNS.ResolveFail", duration);
-      dns_task_error_ = rv;
-      dns_task_.reset();
-      StartProcTask();
-    } else {
-      // We could be running within Resolve(), so make sure to complete
-      // asynchronously.
-      base::MessageLoopProxy::current()->PostTask(
-          FROM_HERE,
-          base::Bind(&Job::OnDnsTaskFailure,
-                     base::Unretained(this),
-                     dns_task_->AsWeakPtr(),
-                     duration,
-                     rv));
-    }
+    dns_task_->Start();
   }
 
   // Called if DnsTask fails. It is posted from StartDnsTask, so Job may be
diff --git a/net/dns/host_resolver_impl_unittest.cc b/net/dns/host_resolver_impl_unittest.cc
index 60c3df2..f6b7f69 100644
--- a/net/dns/host_resolver_impl_unittest.cc
+++ b/net/dns/host_resolver_impl_unittest.cc
@@ -1257,10 +1257,8 @@ DnsConfig CreateValidDnsConfig() {
 class HostResolverImplDnsTest : public HostResolverImplTest {
  protected:
   virtual void SetUp() OVERRIDE {
-    AddDnsRule("er", dns_protocol::kTypeA, MockDnsClientRule::FAIL_SYNC);
-    AddDnsRule("er", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL_SYNC);
-    AddDnsRule("nx", dns_protocol::kTypeA, MockDnsClientRule::FAIL_ASYNC);
-    AddDnsRule("nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL_ASYNC);
+    AddDnsRule("nx", dns_protocol::kTypeA, MockDnsClientRule::FAIL);
+    AddDnsRule("nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL);
     AddDnsRule("ok", dns_protocol::kTypeA, MockDnsClientRule::OK);
     AddDnsRule("ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK);
     AddDnsRule("4ok", dns_protocol::kTypeA, MockDnsClientRule::OK);
@@ -1268,7 +1266,7 @@ class HostResolverImplDnsTest : public HostResolverImplTest {
     AddDnsRule("6ok", dns_protocol::kTypeA, MockDnsClientRule::EMPTY);
     AddDnsRule("6ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK);
     AddDnsRule("4nx", dns_protocol::kTypeA, MockDnsClientRule::OK);
-    AddDnsRule("4nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL_ASYNC);
+    AddDnsRule("4nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL);
     CreateResolver();
   }
 
@@ -1306,7 +1304,6 @@ class HostResolverImplDnsTest : public HostResolverImplTest {
 TEST_F(HostResolverImplDnsTest, DnsTask) {
   resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
 
-  proc_->AddRuleForAllFamilies("er_succeed", "192.168.1.101");
   proc_->AddRuleForAllFamilies("nx_succeed", "192.168.1.102");
   // All other hostnames will fail in proc_.
 
@@ -1319,9 +1316,7 @@ TEST_F(HostResolverImplDnsTest, DnsTask) {
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_fail", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_fail", 80)->Resolve());
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_fail", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_succeed", 80)->Resolve());
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_succeed", 80)->Resolve());
 
   proc_->SignalMultiple(requests_.size());
@@ -1334,11 +1329,8 @@ TEST_F(HostResolverImplDnsTest, DnsTask) {
   EXPECT_TRUE(requests_[1]->HasOneAddress("127.0.0.1", 80));
   // Fallback to ProcTask.
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, requests_[2]->result());
-  EXPECT_EQ(ERR_NAME_NOT_RESOLVED, requests_[3]->result());
-  EXPECT_EQ(OK, requests_[4]->result());
-  EXPECT_TRUE(requests_[4]->HasOneAddress("192.168.1.101", 80));
-  EXPECT_EQ(OK, requests_[5]->result());
-  EXPECT_TRUE(requests_[5]->HasOneAddress("192.168.1.102", 80));
+  EXPECT_EQ(OK, requests_[3]->result());
+  EXPECT_TRUE(requests_[3]->HasOneAddress("192.168.1.102", 80));
 }
 
 // Test successful and failing resolutions in HostResolverImpl::DnsTask when
@@ -1347,7 +1339,6 @@ TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
   resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
   set_fallback_to_proctask(false);
 
-  proc_->AddRuleForAllFamilies("er_succeed", "192.168.1.101");
   proc_->AddRuleForAllFamilies("nx_succeed", "192.168.1.102");
   // All other hostnames will fail in proc_.
 
@@ -1356,17 +1347,17 @@ TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
   // Initially there is no config, so client should not be invoked.
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_fail", 80)->Resolve());
   // There is no config, so fallback to ProcTask must work.
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_succeed", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_succeed", 80)->Resolve());
   proc_->SignalMultiple(requests_.size());
 
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, requests_[0]->WaitForResult());
   EXPECT_EQ(OK, requests_[1]->WaitForResult());
-  EXPECT_TRUE(requests_[1]->HasOneAddress("192.168.1.101", 80));
+  EXPECT_TRUE(requests_[1]->HasOneAddress("192.168.1.102", 80));
 
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_abort", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_abort", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_abort", 80)->Resolve());
 
   // Simulate the case when the preference or policy has disabled the DNS client
   // causing AbortDnsTasks.
@@ -1376,7 +1367,6 @@ TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
   // First request is resolved by MockDnsClient, others should fail due to
   // disabled fallback to ProcTask.
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_fail", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_fail", 80)->Resolve());
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_fail", 80)->Resolve());
   proc_->SignalMultiple(requests_.size());
 
@@ -1388,14 +1378,13 @@ TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
   EXPECT_TRUE(requests_[4]->HasOneAddress("127.0.0.1", 80));
   // Fallback to ProcTask is disabled.
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, requests_[5]->WaitForResult());
-  EXPECT_EQ(ERR_NAME_NOT_RESOLVED, requests_[6]->WaitForResult());
 }
 
 // Test behavior of OnDnsTaskFailure when Job is aborted.
 TEST_F(HostResolverImplDnsTest, OnDnsTaskFailureAbortedJob) {
   resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
   ChangeDnsConfig(CreateValidDnsConfig());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_abort", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_abort", 80)->Resolve());
   // Abort all jobs here.
   CreateResolver();
   proc_->SignalMultiple(requests_.size());
@@ -1408,7 +1397,7 @@ TEST_F(HostResolverImplDnsTest, OnDnsTaskFailureAbortedJob) {
   resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
   set_fallback_to_proctask(false);
   ChangeDnsConfig(CreateValidDnsConfig());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_abort", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_abort", 80)->Resolve());
   // Abort all jobs here.
   CreateResolver();
   // Run to completion.
@@ -1453,7 +1442,7 @@ TEST_F(HostResolverImplDnsTest, ServeFromHosts) {
                                std::string());  // Default to failures.
   proc_->SignalMultiple(1u);  // For the first request which misses.
 
-  Request* req0 = CreateRequest("er_ipv4", 80);
+  Request* req0 = CreateRequest("nx_ipv4", 80);
   EXPECT_EQ(ERR_IO_PENDING, req0->Resolve());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, req0->WaitForResult());
 
@@ -1462,39 +1451,39 @@ TEST_F(HostResolverImplDnsTest, ServeFromHosts) {
   ASSERT_TRUE(ParseIPLiteralToNumber("::1", &local_ipv6));
 
   DnsHosts hosts;
-  hosts[DnsHostsKey("er_ipv4", ADDRESS_FAMILY_IPV4)] = local_ipv4;
-  hosts[DnsHostsKey("er_ipv6", ADDRESS_FAMILY_IPV6)] = local_ipv6;
-  hosts[DnsHostsKey("er_both", ADDRESS_FAMILY_IPV4)] = local_ipv4;
-  hosts[DnsHostsKey("er_both", ADDRESS_FAMILY_IPV6)] = local_ipv6;
+  hosts[DnsHostsKey("nx_ipv4", ADDRESS_FAMILY_IPV4)] = local_ipv4;
+  hosts[DnsHostsKey("nx_ipv6", ADDRESS_FAMILY_IPV6)] = local_ipv6;
+  hosts[DnsHostsKey("nx_both", ADDRESS_FAMILY_IPV4)] = local_ipv4;
+  hosts[DnsHostsKey("nx_both", ADDRESS_FAMILY_IPV6)] = local_ipv6;
 
   // Update HOSTS file.
   config.hosts = hosts;
   ChangeDnsConfig(config);
 
-  Request* req1 = CreateRequest("er_ipv4", 80);
+  Request* req1 = CreateRequest("nx_ipv4", 80);
   EXPECT_EQ(OK, req1->Resolve());
   EXPECT_TRUE(req1->HasOneAddress("127.0.0.1", 80));
 
-  Request* req2 = CreateRequest("er_ipv6", 80);
+  Request* req2 = CreateRequest("nx_ipv6", 80);
   EXPECT_EQ(OK, req2->Resolve());
   EXPECT_TRUE(req2->HasOneAddress("::1", 80));
 
-  Request* req3 = CreateRequest("er_both", 80);
+  Request* req3 = CreateRequest("nx_both", 80);
   EXPECT_EQ(OK, req3->Resolve());
   EXPECT_TRUE(req3->HasAddress("127.0.0.1", 80) &&
               req3->HasAddress("::1", 80));
 
   // Requests with specified AddressFamily.
-  Request* req4 = CreateRequest("er_ipv4", 80, MEDIUM, ADDRESS_FAMILY_IPV4);
+  Request* req4 = CreateRequest("nx_ipv4", 80, MEDIUM, ADDRESS_FAMILY_IPV4);
   EXPECT_EQ(OK, req4->Resolve());
   EXPECT_TRUE(req4->HasOneAddress("127.0.0.1", 80));
 
-  Request* req5 = CreateRequest("er_ipv6", 80, MEDIUM, ADDRESS_FAMILY_IPV6);
+  Request* req5 = CreateRequest("nx_ipv6", 80, MEDIUM, ADDRESS_FAMILY_IPV6);
   EXPECT_EQ(OK, req5->Resolve());
   EXPECT_TRUE(req5->HasOneAddress("::1", 80));
 
   // Request with upper case.
-  Request* req6 = CreateRequest("er_IPV4", 80);
+  Request* req6 = CreateRequest("nx_IPV4", 80);
   EXPECT_EQ(OK, req6->Resolve());
   EXPECT_TRUE(req6->HasOneAddress("127.0.0.1", 80));
 }
@@ -1533,7 +1522,7 @@ TEST_F(HostResolverImplDnsTest, DisableDnsClientOnPersistentFailure) {
 
   for (unsigned i = 0; i < 20; ++i) {
     // Use custom names to require separate Jobs.
-    std::string hostname = base::StringPrintf("err_%u", i);
+    std::string hostname = base::StringPrintf("nx_%u", i);
     // Ensure fallback to ProcTask succeeds.
     proc_->AddRuleForAllFamilies(hostname, "192.168.1.101");
     EXPECT_EQ(ERR_IO_PENDING, CreateRequest(hostname, 80)->Resolve()) << i;
@@ -1567,7 +1556,7 @@ TEST_F(HostResolverImplDnsTest, DontDisableDnsClientOnSporadicFailure) {
   // 20 failures interleaved with 20 successes.
   for (unsigned i = 0; i < 40; ++i) {
     // Use custom names to require separate Jobs.
-    std::string hostname = (i % 2) == 0 ? base::StringPrintf("err_%u", i)
+    std::string hostname = (i % 2) == 0 ? base::StringPrintf("nx_%u", i)
                                         : base::StringPrintf("ok_%u", i);
     EXPECT_EQ(ERR_IO_PENDING, CreateRequest(hostname, 80)->Resolve()) << i;
   }