Added HostResolverImpl::fallback_to_proctask_ to vary according to AsyncDnsNoFallback group in AsynDns finch trial. Fixed use-after-delete issue.

patch from issue 16881005

BUG=224248
TEST=net_unittests --gtest_filter=HostResolverImplDnsTest.NoFallbackToProcTask
TEST=net_unittests --gtest_filter=HostResolverImplDnsTest.OnDnsTaskFailureAbortedJob

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

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

3 files changed, 148 insertions, 16 deletions

diff --git a/net/dns/host_resolver_impl.cc b/net/dns/host_resolver_impl.cc
index 6c41181..25e5ded 100644
--- a/net/dns/host_resolver_impl.cc
+++ b/net/dns/host_resolver_impl.cc
@@ -243,6 +243,20 @@ void RecordTTL(base::TimeDelta ttl) {
                              base::TimeDelta::FromDays(1), 100);
 }
 
+bool ConfigureAsyncDnsNoFallbackFieldTrial() {
+  const bool kDefault = false;
+
+  // Configure the AsyncDns field trial as follows:
+  // groups AsyncDnsNoFallbackA and AsyncDnsNoFallbackB: return true,
+  // groups AsyncDnsA and AsyncDnsB: return false,
+  // groups SystemDnsA and SystemDnsB: return false,
+  // otherwise (trial absent): return default.
+  std::string group_name = base::FieldTrialList::FindFullName("AsyncDns");
+  if (!group_name.empty())
+    return StartsWithASCII(group_name, "AsyncDnsNoFallback", false);
+  return kDefault;
+}
+
 //-----------------------------------------------------------------------------
 
 AddressList EnsurePortOnAddressList(const AddressList& list, uint16 port) {
@@ -1475,19 +1489,60 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
 
   void StartDnsTask() {
     DCHECK(resolver_->HaveDnsConfig());
+    base::TimeTicks start_time = base::TimeTicks::Now();
     dns_task_.reset(new DnsTask(
         resolver_->dns_client_.get(),
         key_,
-        base::Bind(&Job::OnDnsTaskComplete, base::Unretained(this),
-                   base::TimeTicks::Now()),
+        base::Bind(&Job::OnDnsTaskComplete, base::Unretained(this), start_time),
         net_log_));
 
     int rv = dns_task_->Start();
-    if (rv != ERR_IO_PENDING) {
-      DCHECK_NE(OK, rv);
+    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));
+    }
+  }
+
+  // Called if DnsTask fails. It is posted from StartDnsTask, so Job may be
+  // deleted before this callback. In this case dns_task is deleted as well,
+  // so we use it as indicator whether Job is still valid.
+  void OnDnsTaskFailure(const base::WeakPtr<DnsTask>& dns_task,
+                        base::TimeDelta duration,
+                        int net_error) {
+    DNS_HISTOGRAM("AsyncDNS.ResolveFail", duration);
+
+    if (dns_task == NULL)
+      return;
+
+    dns_task_error_ = net_error;
+
+    // TODO(szym): Run ServeFromHosts now if nsswitch.conf says so.
+    // http://crbug.com/117655
+
+    // TODO(szym): Some net errors indicate lack of connectivity. Starting
+    // ProcTask in that case is a waste of time.
+    if (resolver_->fallback_to_proctask_) {
+      dns_task_.reset();
+      StartProcTask();
+    } else {
+      UmaAsyncDnsResolveStatus(RESOLVE_STATUS_FAIL);
+      CompleteRequestsWithError(net_error);
     }
   }
 
@@ -1500,17 +1555,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
 
     base::TimeDelta duration = base::TimeTicks::Now() - start_time;
     if (net_error != OK) {
-      DNS_HISTOGRAM("AsyncDNS.ResolveFail", duration);
-
-      dns_task_error_ = net_error;
-      dns_task_.reset();
-
-      // TODO(szym): Run ServeFromHosts now if nsswitch.conf says so.
-      // http://crbug.com/117655
-
-      // TODO(szym): Some net errors indicate lack of connectivity. Starting
-      // ProcTask in that case is a waste of time.
-      StartProcTask();
+      OnDnsTaskFailure(dns_task_->AsWeakPtr(), duration, net_error);
       return;
     }
     DNS_HISTOGRAM("AsyncDNS.ResolveSuccess", duration);
@@ -1704,7 +1749,8 @@ HostResolverImpl::HostResolverImpl(
       num_dns_failures_(0),
       ipv6_probe_monitoring_(false),
       resolved_known_ipv6_hostname_(false),
-      additional_resolver_flags_(0) {
+      additional_resolver_flags_(0),
+      fallback_to_proctask_(true) {
 
   DCHECK_GE(dispatcher_.num_priorities(), static_cast<size_t>(NUM_PRIORITIES));
 
@@ -1734,6 +1780,8 @@ HostResolverImpl::HostResolverImpl(
     NetworkChangeNotifier::GetDnsConfig(&dns_config);
     received_dns_config_ = dns_config.IsValid();
   }
+
+  fallback_to_proctask_ = !ConfigureAsyncDnsNoFallbackFieldTrial();
 }
 
 HostResolverImpl::~HostResolverImpl() {
diff --git a/net/dns/host_resolver_impl.h b/net/dns/host_resolver_impl.h
index 9d1be0b..a4e7068d 100644
--- a/net/dns/host_resolver_impl.h
+++ b/net/dns/host_resolver_impl.h
@@ -279,6 +279,9 @@ class NET_EXPORT HostResolverImpl
   // Any resolver flags that should be added to a request by default.
   HostResolverFlags additional_resolver_flags_;
 
+  // Allow fallback to ProcTask if DnsTask fails.
+  bool fallback_to_proctask_;
+
   DISALLOW_COPY_AND_ASSIGN(HostResolverImpl);
 };
 
diff --git a/net/dns/host_resolver_impl_unittest.cc b/net/dns/host_resolver_impl_unittest.cc
index 320d7ba..8257983 100644
--- a/net/dns/host_resolver_impl_unittest.cc
+++ b/net/dns/host_resolver_impl_unittest.cc
@@ -509,6 +509,11 @@ class HostResolverImplTest : public testing::Test {
     return resolver_->num_running_jobs_for_tests();
   }
 
+  void set_fallback_to_proctask(bool fallback_to_proctask) {
+    DCHECK(resolver_.get());
+    resolver_->fallback_to_proctask_ = fallback_to_proctask;
+  }
+
   scoped_refptr<MockHostResolverProc> proc_;
   scoped_ptr<HostResolverImpl> resolver_;
   ScopedVector<Request> requests_;
@@ -1334,6 +1339,82 @@ TEST_F(HostResolverImplDnsTest, DnsTask) {
   EXPECT_TRUE(requests_[5]->HasOneAddress("192.168.1.102", 80));
 }
 
+// Test successful and failing resolutions in HostResolverImpl::DnsTask when
+// fallback to ProcTask is disabled.
+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_.
+
+  // Set empty DnsConfig.
+  ChangeDnsConfig(DnsConfig());
+  // 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());
+  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));
+
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_abort", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_abort", 80)->Resolve());
+
+  // Simulate the case when the preference or policy has disabled the DNS client
+  // causing AbortDnsTasks.
+  resolver_->SetDnsClient(CreateMockDnsClient(DnsConfig(), dns_rules_));
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  // 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());
+
+  // Aborted due to Network Change.
+  EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[2]->WaitForResult());
+  EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[3]->WaitForResult());
+  // Resolved by MockDnsClient.
+  EXPECT_EQ(OK, requests_[4]->WaitForResult());
+  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());
+  // Abort all jobs here.
+  CreateResolver();
+  proc_->SignalMultiple(requests_.size());
+  // Run to completion.
+  base::MessageLoop::current()->RunUntilIdle();  // Notification happens async.
+  // It shouldn't crash during OnDnsTaskFailure callbacks.
+  EXPECT_EQ(ERR_IO_PENDING, requests_[0]->result());
+
+  // Repeat test with Fallback to ProcTask disabled
+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
+  set_fallback_to_proctask(false);
+  ChangeDnsConfig(CreateValidDnsConfig());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_abort", 80)->Resolve());
+  // Abort all jobs here.
+  CreateResolver();
+  // Run to completion.
+  base::MessageLoop::current()->RunUntilIdle();  // Notification happens async.
+  // It shouldn't crash during OnDnsTaskFailure callbacks.
+  EXPECT_EQ(ERR_IO_PENDING, requests_[1]->result());
+}
+
 TEST_F(HostResolverImplDnsTest, DnsTaskUnspec) {
   ChangeDnsConfig(CreateValidDnsConfig());