Add an additional per-request DNS cache when executing FindProxyForURL() from a PAC script.

This mini-cache is more aggressive in caching negative resolutions, to avoid performance problems with PAC scripts that do lots of serial DNS resolves.

This is necessary now that we no longer globally cache DNS resolve failures to avoid performance regressions.

BUG=46821

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

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

5 files changed, 237 insertions, 32 deletions

diff --git a/net/proxy/proxy_resolver_js_bindings.cc b/net/proxy/proxy_resolver_js_bindings.cc
index 4703f2e..347b953 100644
--- a/net/proxy/proxy_resolver_js_bindings.cc
+++ b/net/proxy/proxy_resolver_js_bindings.cc
@@ -6,11 +6,13 @@
 
 #include "base/logging.h"
 #include "net/base/address_list.h"
+#include "net/base/host_cache.h"
 #include "net/base/host_resolver.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
 #include "net/base/sys_addrinfo.h"
+#include "net/proxy/proxy_resolver_request_context.h"
 
 namespace net {
 namespace {
@@ -49,28 +51,27 @@ class DefaultJSBindings : public ProxyResolverJSBindings {
     // See http://crbug.com/24641 for more details.
     HostResolver::RequestInfo info(host, 80);  // Port doesn't matter.
     info.set_address_family(ADDRESS_FAMILY_IPV4);
-    net::AddressList address_list;
-    int result = host_resolver_->Resolve(info, &address_list, NULL, NULL,
-                                         BoundNetLog());
+    AddressList address_list;
 
+    int result = DnsResolveHelper(info, &address_list);
     if (result != OK)
       return std::string();  // Failed.
 
+    // TODO(eroman): Is this check really needed? Can I remove it?
     if (!address_list.head())
       return std::string();
 
     // There may be multiple results; we will just use the first one.
     // This returns empty string on failure.
-    return net::NetAddressToString(address_list.head());
+    return NetAddressToString(address_list.head());
   }
 
   // Handler for "dnsResolveEx(host)". Returns empty string on failure.
   virtual std::string DnsResolveEx(const std::string& host) {
     // Do a sync resolve of the hostname.
     HostResolver::RequestInfo info(host, 80);  // Port doesn't matter.
-    net::AddressList address_list;
-    int result = host_resolver_->Resolve(info, &address_list, NULL, NULL,
-                                         BoundNetLog());
+    AddressList address_list;
+    int result = DnsResolveHelper(info, &address_list);
 
     if (result != OK)
       return std::string();  // Failed.
@@ -82,7 +83,7 @@ class DefaultJSBindings : public ProxyResolverJSBindings {
     while (current_address) {
       if (!address_list_str.empty())
         address_list_str += ";";
-      address_list_str += net::NetAddressToString(current_address);
+      address_list_str += NetAddressToString(current_address);
       current_address = current_address->ai_next;
     }
 
@@ -98,6 +99,43 @@ class DefaultJSBindings : public ProxyResolverJSBindings {
   }
 
  private:
+  // Helper to execute a syncrhonous DNS resolve, using the per-request
+  // DNS cache if there is one.
+  int DnsResolveHelper(const HostResolver::RequestInfo& info,
+                       AddressList* address_list) {
+    HostCache::Key cache_key(info.hostname(),
+                             info.address_family(),
+                             info.host_resolver_flags());
+
+    HostCache* host_cache = current_request_context() ?
+        current_request_context()->host_cache : NULL;
+
+    // First try to service this request from the per-request DNS cache.
+    // (we cache DNS failures much more aggressively within the context
+    // of a FindProxyForURL() request).
+    if (host_cache) {
+      const HostCache::Entry* entry =
+          host_cache->Lookup(cache_key, base::TimeTicks::Now());
+      if (entry) {
+        if (entry->error == OK)
+          *address_list = entry->addrlist;
+        return entry->error;
+      }
+    }
+
+    // Otherwise ask the resolver.
+    int result = host_resolver_->Resolve(info, address_list, NULL, NULL,
+                                         BoundNetLog());
+
+    // Save the result back to the per-request DNS cache.
+    if (host_cache) {
+      host_cache->Set(cache_key, result, *address_list,
+                      base::TimeTicks::Now());
+    }
+
+    return result;
+  }
+
   scoped_refptr<HostResolver> host_resolver_;
 };
 
diff --git a/net/proxy/proxy_resolver_js_bindings.h b/net/proxy/proxy_resolver_js_bindings.h
index 09e6e1c..6fc377b 100644
--- a/net/proxy/proxy_resolver_js_bindings.h
+++ b/net/proxy/proxy_resolver_js_bindings.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef NET_PROXY_PROXY_JS_BINDINGS_H
-#define NET_PROXY_PROXY_JS_BINDINGS_H
+#ifndef NET_PROXY_PROXY_RESOLVER_JS_BINDINGS_H_
+#define NET_PROXY_PROXY_RESOLVER_JS_BINDINGS_H_
 
 #include <string>
 
@@ -12,10 +12,13 @@ class MessageLoop;
 namespace net {
 
 class HostResolver;
+struct ProxyResolverRequestContext;
 
 // Interface for the javascript bindings.
 class ProxyResolverJSBindings {
  public:
+  ProxyResolverJSBindings() : current_request_context_(NULL) {}
+
   virtual ~ProxyResolverJSBindings() {}
 
   // Handler for "alert(message)"
@@ -50,8 +53,22 @@ class ProxyResolverJSBindings {
   //
   // Note that |host_resolver| will be used in sync mode mode.
   static ProxyResolverJSBindings* CreateDefault(HostResolver* host_resolver);
+
+  // Sets details about the currently executing FindProxyForURL() request.
+  void set_current_request_context(
+      ProxyResolverRequestContext* current_request_context) {
+    current_request_context_ = current_request_context;
+  }
+
+  // Retrieves details about the currently executing FindProxyForURL() request.
+  ProxyResolverRequestContext* current_request_context() {
+    return current_request_context_;
+  }
+
+ private:
+  ProxyResolverRequestContext* current_request_context_;
 };
 
 }  // namespace net
 
-#endif  // NET_PROXY_PROXY_JS_BINDINGS_H
+#endif  // NET_PROXY_PROXY_RESOLVER_JS_BINDINGS_H_
diff --git a/net/proxy/proxy_resolver_js_bindings_unittest.cc b/net/proxy/proxy_resolver_js_bindings_unittest.cc
index 93ca633..cdb1800 100644
--- a/net/proxy/proxy_resolver_js_bindings_unittest.cc
+++ b/net/proxy/proxy_resolver_js_bindings_unittest.cc
@@ -10,6 +10,7 @@
 #include "net/base/net_util.h"
 #include "net/base/sys_addrinfo.h"
 #include "net/proxy/proxy_resolver_js_bindings.h"
+#include "net/proxy/proxy_resolver_request_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -78,6 +79,33 @@ class MockHostResolverWithMultipleResults : public HostResolver {
   }
 };
 
+class MockFailingHostResolver : public HostResolver {
+ public:
+  MockFailingHostResolver() : count_(0) {}
+
+  // HostResolver methods:
+  virtual int Resolve(const RequestInfo& info,
+                      AddressList* addresses,
+                      CompletionCallback* callback,
+                      RequestHandle* out_req,
+                      const BoundNetLog& net_log) {
+    count_++;
+    return ERR_NAME_NOT_RESOLVED;
+  }
+
+  virtual void CancelRequest(RequestHandle req) {}
+  virtual void AddObserver(Observer* observer) {}
+  virtual void RemoveObserver(Observer* observer) {}
+  virtual void Shutdown() {}
+
+  // Returns the number of times Resolve() has been called.
+  int count() const { return count_; }
+  void ResetCount() { count_ = 0; }
+
+ private:
+  int count_;
+};
+
 TEST(ProxyResolverJSBindingsTest, DnsResolve) {
   scoped_refptr<MockHostResolver> host_resolver(new MockHostResolver);
 
@@ -186,5 +214,52 @@ TEST(ProxyResolverJSBindingsTest, ExFunctionsReturnList) {
             bindings->DnsResolveEx("FOO"));
 }
 
+TEST(ProxyResolverJSBindingsTest, PerRequestDNSCache) {
+  scoped_refptr<MockFailingHostResolver> host_resolver(
+      new MockFailingHostResolver);
+
+  // Get a hold of a DefaultJSBindings* (it is a hidden impl class).
+  scoped_ptr<ProxyResolverJSBindings> bindings(
+      ProxyResolverJSBindings::CreateDefault(host_resolver));
+
+  // Call DnsResolve() 4 times for the same hostname -- this should issue
+  // 4 separate calls to the underlying host resolver, since there is no
+  // current request context.
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ(4, host_resolver->count());
+
+  host_resolver->ResetCount();
+
+  // Now setup a per-request context, and try the same experiment -- we
+  // expect the underlying host resolver to receive only 1 request this time,
+  // since it will service the others from the per-request DNS cache.
+  HostCache cache(50,
+                  base::TimeDelta::FromMinutes(10),
+                  base::TimeDelta::FromMinutes(10));
+  ProxyResolverRequestContext context(NULL, NULL, &cache);
+  bindings->set_current_request_context(&context);
+
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ("", bindings->DnsResolve("foo"));
+  EXPECT_EQ(1, host_resolver->count());
+
+  host_resolver->ResetCount();
+
+  // The "Ex" version shares this same cache, however since the flags
+  // are different it won't reuse this particular entry.
+  EXPECT_EQ("", bindings->DnsResolveEx("foo"));
+  EXPECT_EQ(1, host_resolver->count());
+  EXPECT_EQ("", bindings->DnsResolveEx("foo"));
+  EXPECT_EQ("", bindings->DnsResolveEx("foo"));
+  EXPECT_EQ(1, host_resolver->count());
+
+  bindings->set_current_request_context(NULL);
+}
+
 }  // namespace
 }  // namespace net
diff --git a/net/proxy/proxy_resolver_request_context.h b/net/proxy/proxy_resolver_request_context.h
new file mode 100755
index 0000000..750ce10
--- /dev/null
+++ b/net/proxy/proxy_resolver_request_context.h
@@ -0,0 +1,34 @@
+// Copyright (c) 2010 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.
+
+#ifndef NET_PROXY_PROXY_RESOLVER_REQUEST_CONTEXT_H_
+#define NET_PROXY_PROXY_RESOLVER_REQUEST_CONTEXT_H_
+
+namespace net {
+
+class HostCache;
+class BoundNetLog;
+
+// This data structure holds state related to an invocation of
+// "FindProxyForURL()". It is used to associate per-request
+// data that can be retrieved by the bindings.
+struct ProxyResolverRequestContext {
+  // All of these pointers are expected to remain valid for duration of
+  // this instance's lifetime.
+  ProxyResolverRequestContext(const GURL* query_url,
+                              const BoundNetLog* net_log,
+                              HostCache* host_cache)
+    : query_url(query_url),
+      net_log(net_log),
+      host_cache(host_cache) {
+  }
+
+  const GURL* query_url;
+  const BoundNetLog* net_log;
+  HostCache* host_cache;
+};
+
+}  // namespace net
+
+#endif  // NET_PROXY_PROXY_RESOLVER_REQUEST_CONTEXT_H_
diff --git a/net/proxy/proxy_resolver_v8.cc b/net/proxy/proxy_resolver_v8.cc
index 3b04b91..22c4a95 100644
--- a/net/proxy/proxy_resolver_v8.cc
+++ b/net/proxy/proxy_resolver_v8.cc
@@ -7,10 +7,12 @@
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/host_cache.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/proxy/proxy_info.h"
 #include "net/proxy/proxy_resolver_js_bindings.h"
+#include "net/proxy/proxy_resolver_request_context.h"
 #include "net/proxy/proxy_resolver_script.h"
 #include "v8/include/v8.h"
 
@@ -214,8 +216,8 @@ class ProxyResolverV8::Context {
     return OK;
   }
 
-  void SetCurrentRequestNetLog(const BoundNetLog& net_log) {
-    current_request_net_log_ = net_log;
+  void SetCurrentRequestContext(ProxyResolverRequestContext* context) {
+    js_bindings_->set_current_request_context(context);
   }
 
   void PurgeMemory() {
@@ -298,15 +300,19 @@ class ProxyResolverV8::Context {
     {
       v8::Unlocker unlocker;
 
-      context->current_request_net_log_.BeginEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_BEGIN,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS,
+                               NULL);
 
       // We shouldn't be called with any arguments, but will not complain if
       // we are.
       result = context->js_bindings_->MyIpAddress();
 
-      context->current_request_net_log_.EndEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_END,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS,
+                               NULL);
     }
 
     if (result.empty())
@@ -325,15 +331,19 @@ class ProxyResolverV8::Context {
     {
       v8::Unlocker unlocker;
 
-      context->current_request_net_log_.BeginEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS_EX, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_BEGIN,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS_EX,
+                               NULL);
 
       // We shouldn't be called with any arguments, but will not complain if
       // we are.
       context->js_bindings_->MyIpAddressEx();
 
-      context->current_request_net_log_.EndEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS_EX, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_END,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_MY_IP_ADDRESS_EX,
+                               NULL);
     }
 
     return StdStringToV8String(result);
@@ -354,13 +364,17 @@ class ProxyResolverV8::Context {
     {
       v8::Unlocker unlocker;
 
-      context->current_request_net_log_.BeginEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_BEGIN,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE,
+                               NULL);
 
       result = context->js_bindings_->DnsResolve(host);
 
-      context->current_request_net_log_.EndEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_END,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE,
+                               NULL);
     }
 
     // DnsResolve() returns empty string on failure.
@@ -382,20 +396,33 @@ class ProxyResolverV8::Context {
     {
       v8::Unlocker unlocker;
 
-      context->current_request_net_log_.BeginEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE_EX, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_BEGIN,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE_EX,
+                               NULL);
 
       result = context->js_bindings_->DnsResolveEx(host);
 
-      context->current_request_net_log_.EndEvent(
-          NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE_EX, NULL);
+      LogEventToCurrentRequest(context,
+                               NetLog::PHASE_END,
+                               NetLog::TYPE_PROXY_RESOLVER_V8_DNS_RESOLVE_EX,
+                               NULL);
     }
 
     return StdStringToV8String(result);
   }
 
+  static void LogEventToCurrentRequest(Context* context,
+                                       NetLog::EventPhase phase,
+                                       NetLog::EventType type,
+                                       NetLog::EventParameters* params) {
+    if (context->js_bindings_->current_request_context()) {
+      context->js_bindings_->current_request_context()->net_log->AddEntry(
+          type, phase, params);
+    }
+  }
+
   ProxyResolverJSBindings* js_bindings_;
-  BoundNetLog current_request_net_log_;
   v8::Persistent<v8::External> v8_this_;
   v8::Persistent<v8::Context> v8_context_;
 };
@@ -420,10 +447,24 @@ int ProxyResolverV8::GetProxyForURL(const GURL& query_url,
   if (!context_.get())
     return ERR_FAILED;
 
+  // Associate some short-lived context with this request. This context will be
+  // available to any of the javascript "bindings" that are subsequently invoked
+  // from the javascript.
+  //
+  // In particular, we create a HostCache that is aggressive about caching
+  // failed DNS resolves.
+  HostCache host_cache(
+      50,
+      base::TimeDelta::FromMinutes(5),
+      base::TimeDelta::FromMinutes(5));
+
+  ProxyResolverRequestContext request_context(
+      &query_url, &net_log, &host_cache);
+
   // Otherwise call into V8.
-  context_->SetCurrentRequestNetLog(net_log);
+  context_->SetCurrentRequestContext(&request_context);
   int rv = context_->ResolveProxy(query_url, results);
-  context_->SetCurrentRequestNetLog(BoundNetLog());
+  context_->SetCurrentRequestContext(NULL);
 
   return rv;
 }