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

net/base/host_resolver_impl.cc

Index: net/base/host_resolver_impl.cc
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index b317ca03ebcb89d29916fe81e30ec36246fe7920..6697507e7b41dd99d0e52f0feb338f611a6044c0 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -222,8 +222,6 @@ class RequestInfoParameters : public NetLog::EventParameters {
     dict->SetInteger("address_family",
                      static_cast<int>(info_.address_family()));
     dict->SetBoolean("allow_cached_response", info_.allow_cached_response());
-    dict->SetBoolean("only_use_cached_response",
-                     info_.only_use_cached_response());
     dict->SetBoolean("is_speculative", info_.is_speculative());
     dict->SetInteger("priority", info_.priority());
 
@@ -1135,6 +1133,8 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
                               CompletionCallback* callback,
                               RequestHandle* out_req,
                               const BoundNetLog& source_net_log) {
+  DCHECK(addresses);
+  DCHECK(callback);
   DCHECK(CalledOnValidThread());
 
   // Choose a unique ID number for observers to see.
@@ -1147,82 +1147,17 @@ 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);
 
-  // Check for IP literal.
-  IPAddressNumber ip_number;
-  if (ParseIPLiteralToNumber(info.hostname(), &ip_number)) {
-    DCHECK_EQ(key.host_resolver_flags &
-                  ~(HOST_RESOLVER_CANONNAME | HOST_RESOLVER_LOOPBACK_ONLY |
-                    HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6),
-              0) << " Unhandled flag";
-    bool ipv6_disabled = default_address_family_ == ADDRESS_FAMILY_IPV4 &&
-        !ipv6_probe_monitoring_;
-    int net_error = OK;
-    if (ip_number.size() == 16 && ipv6_disabled) {
-      net_error = ERR_NAME_NOT_RESOLVED;
-    } else {
-      *addresses = AddressList::CreateFromIPAddressWithCname(
-          ip_number, info.port(),
-          (key.host_resolver_flags & HOST_RESOLVER_CANONNAME));
-    }
-    // Update the net log and notify registered observers.
+  int rv = ResolveHelper(request_id, key, info, addresses,
+                         source_net_log, request_net_log);

[wtc, 2011/10/11 02:42:53]

This ResolveHelper call passes source_net_log, request_net_log
in the opposite order of the function's prototype.

I wrote a CL http://codereview.chromium.org/8220028 to
fix this, assuming the order in the function's prototype
is right.  But I suspect the function prototype may have
the wrong order, because the source_net_log, request_net_log
order is prevalent in this file.

+  if (rv != ERR_DNS_CACHE_MISS) {
     OnFinishRequest(source_net_log, request_net_log, request_id, info,
-                    net_error, 0  /* os_error (unknown since from cache) */);
-    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(
-        key, base::TimeTicks::Now());
-    if (cache_entry) {
-      request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CACHE_HIT, NULL);
-      int net_error = cache_entry->error;
-      if (net_error == OK) {
-        *addresses = CreateAddressListUsingPort(
-            cache_entry->addrlist, info.port());
-      }
-
-      // Update the net log and notify registered observers.
-      OnFinishRequest(source_net_log, request_net_log, request_id, info,
-                      net_error,
-                      0  /* os_error (unknown since from cache) */);
-
-      return net_error;
-    }
-  }
-
-  if (info.only_use_cached_response()) {  // Not allowed to do a real lookup.
-    OnFinishRequest(source_net_log,
-                    request_net_log,
-                    request_id,
-                    info,
-                    ERR_NAME_NOT_RESOLVED,
-                    0);
-    return ERR_NAME_NOT_RESOLVED;
+                    rv,
+                    0  /* os_error (unknown since from cache) */);
+    return rv;
   }
 
    // Create a handle for this request, and pass it back to the user if they
@@ -1254,6 +1189,54 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   return ERR_IO_PENDING;
 }
 
+int HostResolverImpl::ResolveHelper(int request_id,
+                                    const Key& key,

[cbentzel, 2011/08/04 21:18:36]

Ah, I guess the OnStartRequest/OnFinishRequest can't go in here. Not quite as
useful as I hoped but thanks for doing it.

+                                    const RequestInfo& info,
+                                    AddressList* addresses,
+                                    const BoundNetLog& request_net_log,
+                                    const BoundNetLog& source_net_log) {

[wtc, 2011/10/11 02:42:53]

ResolveHelper does not use its source_net_log argument.

+  // 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)
+    return ERR_NAME_NOT_RESOLVED;
+
+  int net_error = ERR_UNEXPECTED;
+  if (ResolveAsIP(key, info, &net_error, addresses))
+    return net_error;
+  net_error = ERR_DNS_CACHE_MISS;
+  ServeFromCache(key, info, request_net_log, &net_error, addresses);
+  return net_error;
+}
+
+int HostResolverImpl::ResolveFromCache(const RequestInfo& info,
+                                       AddressList* addresses,
+                                       const BoundNetLog& source_net_log) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(addresses);
+
+  // Choose a unique ID number for observers to see.
+  int request_id = next_request_id_++;
+
+  // Make a log item for the request.
+  BoundNetLog request_net_log = BoundNetLog::Make(net_log_,
+      NetLog::SOURCE_HOST_RESOLVER_IMPL_REQUEST);
+
+  // Update the net log and notify registered observers.
+  OnStartRequest(source_net_log, request_net_log, request_id, info);
+
+  // Build a key that identifies the request in the cache and in the
+  // outstanding jobs map.
+  Key key = GetEffectiveKeyForRequest(info);
+
+  int rv = ResolveHelper(request_id, key, info, addresses, request_net_log,
+                         source_net_log);
+  OnFinishRequest(source_net_log, request_net_log, request_id, info,
+                  rv,
+                  0  /* os_error (unknown since from cache) */);
+  return rv;
+}
+
 // See OnJobComplete(Job*) for why it is important not to clean out
 // cancelled requests from Job::requests_.
 void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
@@ -1313,6 +1296,56 @@ HostResolverImpl* HostResolverImpl::GetAsHostResolverImpl() {
   return this;
 }
 
+
+bool HostResolverImpl::ResolveAsIP(const Key& key,
+                                   const RequestInfo& info,
+                                   int* net_error,
+                                   AddressList* addresses) {
+  DCHECK(addresses);
+  DCHECK(net_error);
+  IPAddressNumber ip_number;
+  if (!ParseIPLiteralToNumber(key.hostname, &ip_number))
+    return false;
+
+  DCHECK_EQ(key.host_resolver_flags &
+      ~(HOST_RESOLVER_CANONNAME | HOST_RESOLVER_LOOPBACK_ONLY |
+        HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6),
+            0) << " Unhandled flag";
+  bool ipv6_disabled = default_address_family_ == ADDRESS_FAMILY_IPV4 &&
+    !ipv6_probe_monitoring_;
+  *net_error = OK;
+  if (ip_number.size() == 16 && ipv6_disabled) {
+    *net_error = ERR_NAME_NOT_RESOLVED;
+  } else {
+    *addresses = AddressList::CreateFromIPAddressWithCname(
+        ip_number, info.port(),
+        (key.host_resolver_flags & HOST_RESOLVER_CANONNAME));
+  }
+  return true;
+}
+
+bool HostResolverImpl::ServeFromCache(const Key& key,
+                                      const RequestInfo& info,
+                                      const BoundNetLog& request_net_log,
+                                      int* net_error,
+                                      AddressList* addresses) {
+  DCHECK(addresses);
+  DCHECK(net_error);
+  if (!info.allow_cached_response() || !cache_.get())
+    return false;
+
+  const HostCache::Entry* cache_entry = cache_->Lookup(
+      key, base::TimeTicks::Now());
+  if (!cache_entry)
+    return false;
+
+  request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CACHE_HIT, NULL);
+  *net_error = cache_entry->error;
+  if (*net_error == OK)
+    *addresses = CreateAddressListUsingPort(cache_entry->addrlist, info.port());
+  return true;
+}
+
 void HostResolverImpl::AddOutstandingJob(Job* job) {
   scoped_refptr<Job>& found_job = jobs_[job->key()];
   DCHECK(!found_job);