HostResolverImpl: don't interpret NULL callback argument as a request to do synchronous resolution.

BUG=90547,60149
TEST=net_unittests

Review URL: http://codereview.chromium.org/7520026

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

6 files changed, 82 insertions, 146 deletions

diff --git a/net/base/host_resolver.h b/net/base/host_resolver.h
index 77977c1..53a86da 100644
--- a/net/base/host_resolver.h
+++ b/net/base/host_resolver.h
@@ -148,13 +148,16 @@ class NET_API HostResolver {
   // the sin(6)_port field of the sockaddr_in{6} struct.  Returns OK if
   // successful or an error code upon failure.
   //
-  // When callback is null, the operation completes synchronously.
-  //
-  // When callback is non-null, the operation may be performed asynchronously.
   // If the operation cannnot be completed synchronously, ERR_IO_PENDING will
   // be returned and the real result code will be passed to the completion
   // callback.  Otherwise the result code is returned immediately from this
   // call.
+  //
+  // When |callback| is null, there are two possibilities: either an IP
+  // address literal is being resolved or lookup should be performed from
+  // cache only, meaning info.only_use_cached_response() should be true; in
+  // both cases operation should complete synchronously.
+  //
   // If |out_req| is non-NULL, then |*out_req| will be filled with a handle to
   // the async request. This handle is not valid after the request has
   // completed.
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index af7d0d3..b317ca0 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -44,6 +44,10 @@ namespace net {
 
 namespace {
 
+// Limit the size of hostnames that will be resolved to combat issues in
+// some platform's resolvers.
+const size_t kMaxHostLength = 4096;
+
 // Helper to mutate the linked list contained by AddressList to the given
 // port. Note that in general this is dangerous since the AddressList's
 // data might be shared (and you should use AddressList::SetPort).
@@ -1143,6 +1147,19 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   // Update the net log and notify registered observers.
   OnStartRequest(source_net_log, request_net_log, request_id, info);
 
+  // The result of |getaddrinfo| for empty hosts is inconsistent across systems.
+  // On Windows it gives the default interface's address, whereas on Linux it
+  // gives an error. We will make it fail on all platforms for consistency.
+  if (info.hostname().empty() || info.hostname().size() > kMaxHostLength) {
+    OnFinishRequest(source_net_log,
+                    request_net_log,
+                    request_id,
+                    info,
+                    ERR_NAME_NOT_RESOLVED,
+                    0);
+    return ERR_NAME_NOT_RESOLVED;
+  }
+
   // Build a key that identifies the request in the cache and in the
   // outstanding jobs map.
   Key key = GetEffectiveKeyForRequest(info);
@@ -1170,6 +1187,13 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
     return net_error;
   }
 
+  // Sanity check -- it shouldn't be the case that allow_cached_response is
+  // false while only_use_cached_response is true.
+  DCHECK(info.allow_cached_response() || !info.only_use_cached_response());
+
+  // If callback is NULL, we must be doing cache-only lookup.
+  DCHECK(callback || info.only_use_cached_response());
+
   // If we have an unexpired cache entry, use it.
   if (info.allow_cached_response() && cache_.get()) {
     const HostCache::Entry* cache_entry = cache_->Lookup(
@@ -1201,30 +1225,7 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
     return ERR_NAME_NOT_RESOLVED;
   }
 
-  // If no callback was specified, do a synchronous resolution.
-  if (!callback) {
-    AddressList addrlist;
-    int os_error = 0;
-    int error = ResolveAddrInfo(
-        effective_resolver_proc(), key.hostname, key.address_family,
-        key.host_resolver_flags, &addrlist, &os_error);
-    if (error == OK) {
-      MutableSetPort(info.port(), &addrlist);
-      *addresses = addrlist;
-    }
-
-    // Write to cache.
-    if (cache_.get())
-      cache_->Set(key, error, addrlist, base::TimeTicks::Now());
-
-    // Update the net log and notify registered observers.
-    OnFinishRequest(source_net_log, request_net_log, request_id, info, error,
-                    os_error);
-
-    return error;
-  }
-
-  // Create a handle for this request, and pass it back to the user if they
+   // Create a handle for this request, and pass it back to the user if they
   // asked for it (out_req != NULL).
   Request* req = new Request(source_net_log, request_net_log, request_id, info,
                              callback, addresses);
diff --git a/net/base/host_resolver_impl_unittest.cc b/net/base/host_resolver_impl_unittest.cc
index f89c38b..e01f328 100644
--- a/net/base/host_resolver_impl_unittest.cc
+++ b/net/base/host_resolver_impl_unittest.cc
@@ -356,41 +356,6 @@ class HostResolverImplTest : public testing::Test {
   }
 };
 
-TEST_F(HostResolverImplTest, SynchronousLookup) {
-  AddressList addrlist;
-  const int kPortnum = 80;
-
-  scoped_refptr<RuleBasedHostResolverProc> resolver_proc(
-      new RuleBasedHostResolverProc(NULL));
-  resolver_proc->AddRule("just.testing", "192.168.1.42");
-
-  scoped_ptr<HostResolver> host_resolver(
-      CreateHostResolverImpl(resolver_proc));
-
-  HostResolver::RequestInfo info(HostPortPair("just.testing", kPortnum));
-  CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
-  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, log.bound());
-  EXPECT_EQ(OK, err);
-
-  CapturingNetLog::EntryList entries;
-  log.GetEntries(&entries);
-
-  EXPECT_EQ(2u, entries.size());
-  EXPECT_TRUE(LogContainsBeginEvent(
-      entries, 0, NetLog::TYPE_HOST_RESOLVER_IMPL));
-  EXPECT_TRUE(LogContainsEndEvent(
-      entries, 1, NetLog::TYPE_HOST_RESOLVER_IMPL));
-
-  const struct addrinfo* ainfo = addrlist.head();
-  EXPECT_EQ(static_cast<addrinfo*>(NULL), ainfo->ai_next);
-  EXPECT_EQ(sizeof(struct sockaddr_in), ainfo->ai_addrlen);
-
-  const struct sockaddr* sa = ainfo->ai_addr;
-  const struct sockaddr_in* sa_in = (const struct sockaddr_in*) sa;
-  EXPECT_TRUE(htons(kPortnum) == sa_in->sin_port);
-  EXPECT_TRUE(htonl(0xc0a8012a) == sa_in->sin_addr.s_addr);
-}
-
 TEST_F(HostResolverImplTest, AsynchronousLookup) {
   AddressList addrlist;
   const int kPortnum = 80;
@@ -570,7 +535,8 @@ TEST_F(HostResolverImplTest, EmptyHost) {
   AddressList addrlist;
   const int kPortnum = 5555;
   HostResolver::RequestInfo info(HostPortPair("", kPortnum));
-  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
+  int err = host_resolver->Resolve(info, &addrlist, NULL, NULL,
+                                   BoundNetLog());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, err);
 }
 
@@ -990,7 +956,11 @@ TEST_F(HostResolverImplTest, Observers) {
   // Resolve "host1".
   HostResolver::RequestInfo info1(HostPortPair("host1", 70));
   CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
-  int rv = host_resolver->Resolve(info1, &addrlist, NULL, NULL, log.bound());
+  TestCompletionCallback callback;
+  int rv = host_resolver->Resolve(info1, &addrlist, &callback, NULL,
+                                  log.bound());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
 
   CapturingNetLog::EntryList entries;
@@ -1012,7 +982,6 @@ TEST_F(HostResolverImplTest, Observers) {
 
   // Resolve "host1" again -- this time it  will be served from cache, but it
   // should still notify of completion.
-  TestCompletionCallback callback;
   rv = host_resolver->Resolve(info1, &addrlist, &callback, NULL, BoundNetLog());
   ASSERT_EQ(OK, rv);  // Should complete synchronously.
 
@@ -1027,7 +996,9 @@ TEST_F(HostResolverImplTest, Observers) {
   // Resolve "host2", setting referrer to "http://foobar.com"
   HostResolver::RequestInfo info2(HostPortPair("host2", 70));
   info2.set_referrer(GURL("http://foobar.com"));
-  rv = host_resolver->Resolve(info2, &addrlist, NULL, NULL, BoundNetLog());
+  rv = host_resolver->Resolve(info2, &addrlist, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
 
   EXPECT_EQ(3U, observer.start_log.size());
@@ -1043,7 +1014,7 @@ TEST_F(HostResolverImplTest, Observers) {
 
   // Resolve "host3"
   HostResolver::RequestInfo info3(HostPortPair("host3", 70));
-  host_resolver->Resolve(info3, &addrlist, NULL, NULL, BoundNetLog());
+  host_resolver->Resolve(info3, &addrlist, &callback, NULL, BoundNetLog());
 
   // No effect this time, since observer was removed.
   EXPECT_EQ(3U, observer.start_log.size());
@@ -1648,61 +1619,6 @@ TEST_F(HostResolverImplTest, SetDefaultAddressFamily_IPv6) {
   EXPECT_EQ("192.2.104.1", NetAddressToString(addrlist[2].head()));
 }
 
-// This tests that the default address family is respected for synchronous
-// resolutions.
-TEST_F(HostResolverImplTest, SetDefaultAddressFamily_Synchronous) {
-  scoped_refptr<CapturingHostResolverProc> resolver_proc(
-      new CapturingHostResolverProc(new EchoingHostResolverProc));
-
-  scoped_ptr<HostResolverImpl> host_resolver(new HostResolverImpl(
-      resolver_proc, HostCache::CreateDefaultCache(), kMaxJobs,
-      kMaxRetryAttempts, NULL));
-
-  host_resolver->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
-
-  // Unblock the resolver thread so the requests can run.
-  resolver_proc->Signal();
-
-  HostResolver::RequestInfo req[] = {
-      CreateResolverRequestForAddressFamily("b", MEDIUM,
-                                            ADDRESS_FAMILY_UNSPECIFIED),
-      CreateResolverRequestForAddressFamily("b", MEDIUM, ADDRESS_FAMILY_IPV6),
-      CreateResolverRequestForAddressFamily("b", MEDIUM,
-                                            ADDRESS_FAMILY_UNSPECIFIED),
-      CreateResolverRequestForAddressFamily("b", MEDIUM, ADDRESS_FAMILY_IPV4),
-  };
-  AddressList addrlist[arraysize(req)];
-
-  // Start and run all of the requests synchronously.
-  for (size_t i = 0; i < arraysize(req); ++i) {
-    int rv = host_resolver->Resolve(req[i], &addrlist[i],
-                                    NULL, NULL, BoundNetLog());
-    EXPECT_EQ(OK, rv) << i;
-  }
-
-  // We should have sent 2 requests to the resolver --
-  // one for (b, IPv4), and one for (b, IPv6).
-  CapturingHostResolverProc::CaptureList capture_list =
-      resolver_proc->GetCaptureList();
-  ASSERT_EQ(2u, capture_list.size());
-
-  EXPECT_EQ("b", capture_list[0].hostname);
-  EXPECT_EQ(ADDRESS_FAMILY_IPV4, capture_list[0].address_family);
-
-  EXPECT_EQ("b", capture_list[1].hostname);
-  EXPECT_EQ(ADDRESS_FAMILY_IPV6, capture_list[1].address_family);
-
-  // Now check that the correct resolved IP addresses were returned.
-  // Addresses take the form: 192.x.y.z
-  //    x = length of hostname
-  //    y = ASCII value of hostname[0]
-  //    z = value of address family
-  EXPECT_EQ("192.1.98.1", NetAddressToString(addrlist[0].head()));
-  EXPECT_EQ("192.1.98.2", NetAddressToString(addrlist[1].head()));
-  EXPECT_EQ("192.1.98.1", NetAddressToString(addrlist[2].head()));
-  EXPECT_EQ("192.1.98.1", NetAddressToString(addrlist[3].head()));
-}
-
 TEST_F(HostResolverImplTest, DisallowNonCachedResponses) {
   AddressList addrlist;
   const int kPortnum = 80;
@@ -1723,7 +1639,10 @@ TEST_F(HostResolverImplTest, DisallowNonCachedResponses) {
 
   // This time, we fetch normally.
   info.set_only_use_cached_response(false);
-  err = host_resolver->Resolve(info, &addrlist, NULL, NULL, log.bound());
+  TestCompletionCallback callback;
+  err = host_resolver->Resolve(info, &addrlist, &callback, NULL, log.bound());
+  EXPECT_EQ(ERR_IO_PENDING, err);
+  err = callback.WaitForResult();
   EXPECT_EQ(OK, err);
 
   // Now we should be able to fetch from the cache.
diff --git a/net/base/host_resolver_proc.cc b/net/base/host_resolver_proc.cc
index 88e0ba1..755f119 100644
--- a/net/base/host_resolver_proc.cc
+++ b/net/base/host_resolver_proc.cc
@@ -122,22 +122,9 @@ int SystemHostResolverProc(const std::string& host,
                            HostResolverFlags host_resolver_flags,
                            AddressList* addrlist,
                            int* os_error) {
-  static const size_t kMaxHostLength = 4096;
-
   if (os_error)
     *os_error = 0;
 
-  // The result of |getaddrinfo| for empty hosts is inconsistent across systems.
-  // On Windows it gives the default interface's address, whereas on Linux it
-  // gives an error. We will make it fail on all platforms for consistency.
-  if (host.empty())
-    return ERR_NAME_NOT_RESOLVED;
-
-  // Limit the size of hostnames that will be resolved to combat issues in some
-  // platform's resolvers.
-  if (host.size() > kMaxHostLength)
-    return ERR_NAME_NOT_RESOLVED;
-
   struct addrinfo* ai = NULL;
   struct addrinfo hints = {0};
 
diff --git a/net/base/mapped_host_resolver_unittest.cc b/net/base/mapped_host_resolver_unittest.cc
index 3531003..2a03287 100644
--- a/net/base/mapped_host_resolver_unittest.cc
+++ b/net/base/mapped_host_resolver_unittest.cc
@@ -8,6 +8,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
+#include "net/base/test_completion_callback.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -31,9 +32,12 @@ TEST(MappedHostResolverTest, Inclusion) {
 
   // Try resolving "www.google.com:80". There are no mappings yet, so this
   // hits |resolver_impl| and fails.
+  TestCompletionCallback callback;
   rv = resolver->Resolve(HostResolver::RequestInfo(
                              HostPortPair("www.google.com", 80)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, rv);
 
   // Remap *.google.com to baz.com.
@@ -42,7 +46,9 @@ TEST(MappedHostResolverTest, Inclusion) {
   // Try resolving "www.google.com:80". Should be remapped to "baz.com:80".
   rv = resolver->Resolve(HostResolver::RequestInfo(
                              HostPortPair("www.google.com", 80)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.5", NetAddressToString(address_list.head()));
   EXPECT_EQ(80, address_list.GetPort());
@@ -50,7 +56,9 @@ TEST(MappedHostResolverTest, Inclusion) {
   // Try resolving "foo.com:77". This will NOT be remapped, so result
   // is "foo.com:77".
   rv = resolver->Resolve(HostResolver::RequestInfo(HostPortPair("foo.com", 77)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.8", NetAddressToString(address_list.head()));
   EXPECT_EQ(77, address_list.GetPort());
@@ -61,7 +69,9 @@ TEST(MappedHostResolverTest, Inclusion) {
   // Try resolving "chromium.org:61". Should be remapped to "proxy:99".
   rv = resolver->Resolve(HostResolver::RequestInfo
                              (HostPortPair("chromium.org", 61)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.11", NetAddressToString(address_list.head()));
   EXPECT_EQ(99, address_list.GetPort());
@@ -80,6 +90,7 @@ TEST(MappedHostResolverTest, Exclusion) {
 
   int rv;
   AddressList address_list;
+  TestCompletionCallback callback;
 
   // Remap "*.com" to "baz".
   EXPECT_TRUE(resolver->AddRuleFromString("map *.com baz"));
@@ -90,7 +101,9 @@ TEST(MappedHostResolverTest, Exclusion) {
   // Try resolving "www.google.com". Should not be remapped due to exclusion).
   rv = resolver->Resolve(HostResolver::RequestInfo(
                              HostPortPair("www.google.com", 80)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.3", NetAddressToString(address_list.head()));
   EXPECT_EQ(80, address_list.GetPort());
@@ -98,7 +111,9 @@ TEST(MappedHostResolverTest, Exclusion) {
   // Try resolving "chrome.com:80". Should be remapped to "baz:80".
   rv = resolver->Resolve(HostResolver::RequestInfo(
                              HostPortPair("chrome.com", 80)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.5", NetAddressToString(address_list.head()));
   EXPECT_EQ(80, address_list.GetPort());
@@ -116,6 +131,7 @@ TEST(MappedHostResolverTest, SetRulesFromString) {
 
   int rv;
   AddressList address_list;
+  TestCompletionCallback callback;
 
   // Remap "*.com" to "baz", and *.net to "bar:60".
   resolver->SetRulesFromString("map *.com baz , map *.net bar:60");
@@ -123,7 +139,9 @@ TEST(MappedHostResolverTest, SetRulesFromString) {
   // Try resolving "www.google.com". Should be remapped to "baz".
   rv = resolver->Resolve(HostResolver::RequestInfo(
                              HostPortPair("www.google.com", 80)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.7", NetAddressToString(address_list.head()));
   EXPECT_EQ(80, address_list.GetPort());
@@ -131,7 +149,9 @@ TEST(MappedHostResolverTest, SetRulesFromString) {
   // Try resolving "chrome.net:80". Should be remapped to "bar:60".
   rv = resolver->Resolve(HostResolver::RequestInfo(
                              HostPortPair("chrome.net", 80)),
-                         &address_list, NULL, NULL, BoundNetLog());
+                         &address_list, &callback, NULL, BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  rv = callback.WaitForResult();
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("192.168.1.9", NetAddressToString(address_list.head()));
   EXPECT_EQ(60, address_list.GetPort());
diff --git a/net/base/mock_host_resolver.cc b/net/base/mock_host_resolver.cc
index 3cc4a82..f822bde 100644
--- a/net/base/mock_host_resolver.cc
+++ b/net/base/mock_host_resolver.cc
@@ -11,6 +11,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/base/sys_addrinfo.h"
+#include "net/base/test_completion_callback.h"
 
 namespace net {
 
@@ -100,8 +101,13 @@ int MockHostResolverBase::Resolve(const RequestInfo& info,
                                   RequestHandle* out_req,
                                   const BoundNetLog& net_log) {
   if (synchronous_mode_) {
-    callback = NULL;
-    out_req = NULL;
+    TestCompletionCallback sync_callback;
+    int rv = impl_->Resolve(info, addresses, &sync_callback, out_req, net_log);
+    if (rv == ERR_IO_PENDING) {
+      MessageLoop::ScopedNestableTaskAllower nestable(MessageLoop::current());
+      return sync_callback.WaitForResult();
+    }
+    return rv;
   }
   return impl_->Resolve(info, addresses, callback, out_req, net_log);
 }