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

net/base/host_resolver_impl.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
@@ skipping 204 matching lines @@
   RequestInfoParameters(const HostResolver::RequestInfo& info,
                         const NetLog::Source& source)
       : info_(info), source_(source) {}
 
   virtual Value* ToValue() const {
     DictionaryValue* dict = new DictionaryValue();
     dict->SetString("host", info_.host_port_pair().ToString());
     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());
 
     if (source_.is_valid())
       dict->Set("source_dependency", source_.ToValue());
 
     return dict;
   }
 
  private:
@@ skipping 891 matching lines @@
   CHECK(jobs_.empty()) << "Can only set constraints during setup";
   JobPool* pool = job_pools_[pool_index];
   pool->SetConstraints(max_outstanding_jobs, max_pending_requests);
 }
 
 int HostResolverImpl::Resolve(const RequestInfo& info,
                               AddressList* addresses,
                               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.
   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);
 
-  // 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
   // asked for it (out_req != NULL).
   Request* req = new Request(source_net_log, request_net_log, request_id, info,
                              callback, addresses);
   if (out_req)
     *out_req = reinterpret_cast<RequestHandle>(req);
 
   // Next we need to attach our request to a "job". This job is responsible for
@@ skipping 11 matching lines @@
       CreateAndStartJob(req);
     } else {
       return EnqueueRequest(pool, req);
     }
   }
 
   // Completion happens during OnJobComplete(Job*).
   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) {
   DCHECK(CalledOnValidThread());
   Request* req = reinterpret_cast<Request*>(req_handle);
   DCHECK(req);
 
   scoped_ptr<Request> request_deleter;  // Frees at end of function.
 
   if (!req->job()) {
@@ skipping 39 matching lines @@
 }
 
 AddressFamily HostResolverImpl::GetDefaultAddressFamily() const {
   return default_address_family_;
 }
 
 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);
   found_job = job;
 
   JobPool* pool = GetPoolForRequest(job->initial_request());
   pool->AdjustNumOutstandingJobs(1);
 }
 
 HostResolverImpl::Job* HostResolverImpl::FindOutstandingJob(const Key& key) {
@@ skipping 279 matching lines @@
     additional_resolver_flags_ |= HOST_RESOLVER_LOOPBACK_ONLY;
   } else {
     additional_resolver_flags_ &= ~HOST_RESOLVER_LOOPBACK_ONLY;
   }
 #endif
   AbortAllInProgressJobs();
   // |this| may be deleted inside AbortAllInProgressJobs().
 }
 
 }  // namespace net