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

chrome/browser/net/predictor.cc

 // Copyright (c) 2012 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 "chrome/browser/net/predictor.h"
 
 #include <algorithm>
 #include <cmath>
 #include <set>
 #include <sstream>
@@ skipping 20 matching lines @@
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/data_reduction_proxy/browser/data_reduction_proxy_settings.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 #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"
 #include "net/dns/single_request_host_resolver.h"
 #include "net/http/transport_security_state.h"
 #include "net/ssl/ssl_config_service.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 
 using base::TimeDelta;
@@ skipping 90 matching lines @@
       user_prefs_(NULL),
       profile_io_data_(NULL),
       peak_pending_lookups_(0),
       shutdown_(false),
       max_concurrent_dns_lookups_(g_max_parallel_resolves),
       max_dns_queue_delay_(
           TimeDelta::FromMilliseconds(g_max_queueing_delay_ms)),
       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() +
                       TimeDelta::FromHours(kDurationBetweenTrimmingsHours)),
       observer_(NULL) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 Predictor::~Predictor() {
   // TODO(rlp): Add DCHECK for CurrentlyOn(BrowserThread::IO) when the
@@ skipping 531 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   profile_io_data_ = profile_io_data;
   initial_observer_.reset(new InitialObserver());
   host_resolver_ = io_thread->globals()->host_resolver.get();
 
   net::URLRequestContext* context =
       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
   // from there so now that we're on the IO thread we need to properly
   // initialize the base::WeakPtrFactory.
   // TODO(groby): Check if WeakPtrFactory has the same constraint.
   weak_factory_.reset(new base::WeakPtrFactory<Predictor>(this));
 
   // Prefetch these hostnames on startup.
   DnsPrefetchMotivatedList(startup_urls, UrlInfo::STARTUP_LIST_MOTIVATED);
@@ skipping 314 matching lines @@
   if (info->is_marked_to_delete()) {
     results_.erase(url);
   } else {
     if (found)
       info->SetFoundState();
     else
       info->SetNoSuchNameState();
   }
 }
 
+// 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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(url.has_host());
 
   if (shutdown_)
     return NULL;
 
   UrlInfo* info = &results_[url];
   info->SetUrl(url);  // Initialize or DCHECK.
   // TODO(jar):  I need to discard names that have long since expired.
   // Currently we only add to the domain map :-/
 
   DCHECK(info->HasUrl(url));
 
   if (!info->NeedsDnsUpdate()) {
     info->DLogResultsStats("DNS PrefetchNotUpdated");
     return NULL;
   }
 
   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;
   }
 
   info->SetQueuedState(motivation);
   work_queue_.Push(url, motivation);
   StartSomeQueuedResolutions();
   return info;
 }
 
@@ skipping 260 matching lines @@
 }
 
 void SimplePredictor::ShutdownOnUIThread() {
   SetShutdown(true);
 }
 
 bool SimplePredictor::CanPrefetchAndPrerender() const { return true; }
 bool SimplePredictor::CanPreresolveAndPreconnect() const { return true; }
 
 }  // namespace chrome_browser_net