Don't preresolve DNS if a fixed proxy configuration is in place.

chrome/browser/net/predictor.cc

Index: chrome/browser/net/predictor.cc
diff --git a/chrome/browser/net/predictor.cc b/chrome/browser/net/predictor.cc
index df12e02534c37b11e1f49ad8d7a988b9131cd2d1..07f371de8b2eb0ca247308fe09e28f6f7b9569c6 100644
--- a/chrome/browser/net/predictor.cc
+++ b/chrome/browser/net/predictor.cc
@@ -38,6 +38,7 @@
 #include "net/base/address_list.h"
 #include "net/base/completion_callback.h"
 #include "net/base/host_port_pair.h"
+#include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/dns/host_resolver.h"
@@ -148,6 +149,7 @@ Predictor::Predictor(bool preconnect_enabled, bool predictor_enabled)
       host_resolver_(NULL),
       transport_security_state_(NULL),
       ssl_config_service_(NULL),
+      proxy_service_(NULL),
       preconnect_enabled_(preconnect_enabled),
       consecutive_omnibox_preconnect_count_(0),
       next_trim_time_(base::TimeTicks::Now() +
@@ -699,6 +701,7 @@ void Predictor::FinalizeInitializationOnIOThread(
       url_request_context_getter_->GetURLRequestContext();
   transport_security_state_ = context->transport_security_state();
   ssl_config_service_ = context->ssl_config_service();
+  proxy_service_ = context->proxy_service();
 
   // base::WeakPtrFactory instances need to be created and destroyed
   // on the same thread. The predictor lives on the IO thread and will die
@@ -1033,6 +1036,31 @@ void Predictor::LookupFinished(LookupRequest* request, const GURL& url,
   }
 }
 
+// Dummy function, used to discard asynchronous results from ResolveProxy.
+void DiscardInput(int x) {}
+
+bool Predictor::WouldDefinitelyProxyURL(const GURL& url) {

[eroman, 2014/09/10 01:26:04]

This title is misleading. It is still possible for the URL to go through direct
when this return true. And conversely, it is still possible for the URL to go
through a proxy when this returns false.

[bemasc, 2014/09/10 15:19:02]

OK.  Do you think I should change the name ("WouldTryProxy"?), or try to tighten
the requirements?  For example, I suppose I could add a method like
"ProxyInfo::contains_direct()", if you're concerned about correctly handling the
possibility of fallback to "direct".
That's fine.  (It's what I was trying to convey with the name.)

[eroman, 2014/09/18 17:51:28]

How about:

"WouldLikelyProxyURL()"

That name is more in line with the guarantees we can promise.

[bemasc, 2014/09/19 19:41:37]

Done.

+  if (!proxy_service_)
+    return false;
+
+  net::ProxyInfo info;
+  net::CompletionCallback callback = base::Bind(&DiscardInput);
+  net::ProxyService::PacRequest* pac_request = NULL;
+  int response_code = proxy_service_->ResolveProxy(url, net::LOAD_NORMAL, &info,
+      callback, &pac_request, NULL, net::BoundNetLog());
+  if (response_code != net::LOAD_NORMAL) {

[eroman, 2014/09/10 01:26:04]

This code is incorrect:
  (1) Use net::OK not net::LOAD_NORMAL. They both happen to be zero, but are
from different enum sets.

  (2) There are three types of responses to consider:
       == 0
       <0
       ==  ERROR_IO_PENDING

   (3) It is only correct to call CancelPacRequest() in the case where the
return code above was ERROR_IO_PENDING

[bemasc, 2014/09/19 19:41:37]

Done.

+    // Avoid performing the asynchronous proxy resolution work for no reason.
+    DCHECK(pac_request != NULL);
+    proxy_service_->CancelPacRequest(pac_request);

[eroman, 2014/09/10 01:26:04]

In the case where a PAC script _is_ configured, it would be better to not start
and then cancel a request. Since starting it may post a task to a worker thread,
which is not subsequently actually able to be cancelled.

How about adding synchronous-only support to ProxyService::ResolveProxy().
Perhaps triggered by passing in a null callback.

[bemasc, 2014/09/10 15:19:02]

OK.  There are 10 calls to ResolveProxy in the code, plus 80 in the unit tests. 
Should I convert them all to pass pointers to the callback?  Or would you prefer
to turn ResolveProxy into a wrapper function, or an overload, so that they don't
have to change?

[eroman, 2014/09/18 17:51:28]

My thinking was that ResolveProxy() keep the same signature as today, however
you pass in a "null" CompletionCallback.

ResolveProxy() would test it with:
  callback.is_null()

(Note that this is different from a NULL pointer, but achieves the same end).

[bemasc, 2014/09/19 19:41:37]

Done.

+
+    // The request did not synchronously return "success", so we do not know
+    // whether a proxy would be used for requests to this URL.
+    return false;
+  }
+
+  return !info.is_direct();
+}
+
 UrlInfo* Predictor::AppendToResolutionQueue(
     const GURL& url,
     UrlInfo::ResolutionMotivation motivation) {
@@ -1055,7 +1083,8 @@ UrlInfo* Predictor::AppendToResolutionQueue(
   }
 
   AdviseProxy(url, motivation, false /* is_preconnect */);
-  if (proxy_advisor_ && proxy_advisor_->WouldProxyURL(url)) {
+  if ((proxy_advisor_ && proxy_advisor_->WouldProxyURL(url)) ||
+      WouldDefinitelyProxyURL(url)) {
     info->DLogResultsStats("DNS PrefetchForProxiedRequest");
     return NULL;
   }