Fast-fail WPAD detection.

net/proxy/proxy_script_decider.cc

Index: net/proxy/proxy_script_decider.cc
diff --git a/net/proxy/proxy_script_decider.cc b/net/proxy/proxy_script_decider.cc
index 38bf751cd4da3e3c89ccdacbe079854911e1c058..5d8db70d8b2aeb0042521ff42f2d69a9e4ceaf80 100644
--- a/net/proxy/proxy_script_decider.cc
+++ b/net/proxy/proxy_script_decider.cc
@@ -9,6 +9,7 @@
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
@@ -16,6 +17,7 @@
 #include "net/proxy/dhcp_proxy_script_fetcher.h"
 #include "net/proxy/dhcp_proxy_script_fetcher_factory.h"
 #include "net/proxy/proxy_script_fetcher.h"
+#include "net/url_request/url_request_context.h"
 
 namespace net {
 
@@ -47,6 +49,8 @@ bool LooksLikePacScript(const base::string16& script) {
 // http://code.google.com/p/chromium/issues/detail?id=18575#c20
 static const char kWpadUrl[] = "http://wpad/wpad.dat";
 
+static const int kQuickCheckDelayMs = 1000;

[cbentzel, 2013/08/26 18:19:50]

Probably better to move this into the anonymous namespace and remove the static
declaration (ditto for KWpadUrl).

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

+
 base::Value* ProxyScriptDecider::PacSource::NetLogCallback(
     const GURL* effective_pac_url,
     NetLog::LogLevel /* log_level */) const {
@@ -81,7 +85,9 @@ ProxyScriptDecider::ProxyScriptDecider(
       next_state_(STATE_NONE),
       net_log_(BoundNetLog::Make(
           net_log, NetLog::SOURCE_PROXY_SCRIPT_DECIDER)),
-      fetch_pac_bytes_(false) {
+      fetch_pac_bytes_(false),
+      host_resolver_(
+          proxy_script_fetcher->GetRequestContext()->host_resolver()) {

[cbentzel, 2013/08/26 18:19:50]

Nit: I think I'd prefer passing in the HostResolver* as an argument instead of
fetching via the proxy_script_fetcher.

[cbentzel, 2013/08/26 18:19:50]

I'm a bit confused. I thought the reason we are using the
SingleRequestHostResolver (rather than URLRequestContext's host resolver) is to
try to force using getaddrinfo when resolving wpad. I don't see anything here
which forces that.

[szym, 2013/08/26 18:31:34]

The rationale here is that QUICK_CHECK simply establishes a short timeout on the
HostResolver part of proxy_script_fetcher. To do that, it should be using the
same HostResolver instance. This way there is only one DNS query and it ends up
in one HostCache.

[szym, 2013/08/26 18:31:34]

I am not sure this matters. If it does matter, we should be using a custom
version of HostResolver (as in an earlier, since then deleted, patch) and on
success, stuff the result in the HostCache of proxy_script_fetcher's
HostResolver.

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

 }
 
 ProxyScriptDecider::~ProxyScriptDecider() {
@@ -172,6 +178,13 @@ int ProxyScriptDecider::DoLoop(int result) {
       case STATE_WAIT_COMPLETE:
         rv = DoWaitComplete(rv);
         break;
+      case STATE_QUICK_CHECK:
+        DCHECK_EQ(OK, rv);
+        rv = DoQuickCheck();
+        break;
+      case STATE_QUICK_CHECK_COMPLETE:
+        rv = DoQuickCheckComplete(rv);
+        break;
       case STATE_FETCH_PAC_SCRIPT:
         DCHECK_EQ(OK, rv);
         rv = DoFetchPacScript();
@@ -221,10 +234,50 @@ int ProxyScriptDecider::DoWaitComplete(int result) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_PROXY_SCRIPT_DECIDER_WAIT,
                                       result);
   }
-  next_state_ = GetStartState();
+  next_state_ = STATE_QUICK_CHECK;
   return OK;
 }
 
+int ProxyScriptDecider::DoQuickCheck() {
+  quick_check_start_time_ = base::Time::Now();
+  HostResolver::RequestInfo reqinfo(HostPortPair("wpad", 80));
+  CompletionCallback callback = base::Bind(
+      &ProxyScriptDecider::OnIOCompletion,
+      base::Unretained(this));
+
+  // We use HIGHEST here because proxy decision blocks doing any other requests.
+  int rv = host_resolver_.Resolve(reqinfo, HIGHEST, &wpad_addresses_,
+                                  callback, net_log_);
+
+  // we can't get an error response - the name is known to be valid, and we

[szym, 2013/08/26 17:24:31]

nit: capitalize first letter in a sentence

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

+  // don't cache negative dns responses.
+  CHECK(rv == OK || rv == ERR_IO_PENDING);

[szym, 2013/08/26 17:24:31]

Change this to a DCHECK.

Start the timer only if rv == ERR_IO_PENDING

return rv at the end.

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

+
+  if (rv == OK) {
+    // already in cache or something and valid, we're golden

[szym, 2013/08/26 17:24:31]

No need for the comment.

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

+    next_state_ = STATE_FETCH_PAC_SCRIPT;

[szym, 2013/08/26 17:25:39]

Should be GetStartState()

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

+    return OK;
+  }
+
+  quick_check_timer_.Start(FROM_HERE,
+                           base::TimeDelta::FromMilliseconds(
+                              kQuickCheckDelayMs),
+                           base::Bind(callback, ERR_NAME_NOT_RESOLVED));
+
+  next_state_ = STATE_QUICK_CHECK_COMPLETE;
+  return ERR_IO_PENDING;
+}
+
+int ProxyScriptDecider::DoQuickCheckComplete(int result) {
+  base::TimeDelta delta = base::Time::Now()
+                        - quick_check_start_time_;
+  UMA_HISTOGRAM_TIMES("Net.WpadQuickCheck", delta);

[szym, 2013/08/26 17:24:31]

I suggest you distinguish successes from failures.

[cbentzel, 2013/08/26 18:19:50]

+1

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

+  host_resolver_.Cancel();
+  quick_check_timer_.Stop();
+  next_state_ = result == OK ? GetStartState() : STATE_NONE;

[szym, 2013/08/26 17:24:31]

if (result == OK)
  next_state_ = GetStartState();

(STATE_NONE is the default next_state_)

[Elly Fong-Jones, 2013/09/09 22:07:42]

Done.

+  return result;
+}
+
 int ProxyScriptDecider::DoFetchPacScript() {
   DCHECK(fetch_pac_bytes_);
 
@@ -399,6 +452,8 @@ void ProxyScriptDecider::Cancel() {
     case STATE_FETCH_PAC_SCRIPT_COMPLETE:
       proxy_script_fetcher_->Cancel();
       break;
+    case STATE_FAILED:
+      break;
     default:
       NOTREACHED();
       break;