Add a Dns preresolve interface in //content

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 f496d9c6cd5fc4fb743bfea190f7e4d402920296..b248dea012171e74d8bae3a8384b7a9d50d08ce2 100644
--- a/chrome/browser/net/predictor.cc
+++ b/chrome/browser/net/predictor.cc
@@ -104,61 +104,17 @@ static size_t g_max_parallel_resolves =
 // we change the format so that we discard old data.
 static const int kPredictorStartupFormatVersion = 1;
 
-class Predictor::LookupRequest {
- public:
-  LookupRequest(Predictor* predictor,
-                net::HostResolver* host_resolver,
-                const GURL& url)
-      : predictor_(predictor),
-        url_(url),
-        resolver_(host_resolver) {
-  }
-
-  // Return underlying network resolver status.
-  // net::OK ==> Host was found synchronously.
-  // net:ERR_IO_PENDING ==> Network will callback later with result.
-  // anything else ==> Host was not found synchronously.
-  int Start() {
-    net::HostResolver::RequestInfo resolve_info(
-        net::HostPortPair::FromURL(url_));
-
-    // Make a note that this is a speculative resolve request. This allows us
-    // to separate it from real navigations in the observer's callback, and
-    // lets the HostResolver know it can de-prioritize it.
-    resolve_info.set_is_speculative(true);
-    return resolver_.Resolve(
-        resolve_info,
-        net::DEFAULT_PRIORITY,
-        &addresses_,
-        base::Bind(&LookupRequest::OnLookupFinished, base::Unretained(this)),
-        net::BoundNetLog());
-  }
-
- private:
-  void OnLookupFinished(int result) {
-    predictor_->OnLookupFinished(this, url_, result == net::OK);
-  }
-
-  Predictor* predictor_;  // The predictor which started us.
-
-  const GURL url_;  // Hostname to resolve.
-  net::SingleRequestHostResolver resolver_;
-  net::AddressList addresses_;
-
-  DISALLOW_COPY_AND_ASSIGN(LookupRequest);
-};
-
 Predictor::Predictor(bool preconnect_enabled, bool predictor_enabled)
     : url_request_context_getter_(NULL),
       predictor_enabled_(predictor_enabled),
       user_prefs_(NULL),
       profile_io_data_(NULL),
+      num_pending_lookups_(0),
       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),
@@ -398,8 +354,6 @@ void Predictor::Shutdown() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!shutdown_);
   shutdown_ = true;
-
-  STLDeleteElements(&pending_lookups_);
 }
 
 void Predictor::DiscardAllResults() {
@@ -417,8 +371,7 @@ void Predictor::DiscardAllResults() {
     info->SetAssignedState();
     info->SetNoSuchNameState();
   }
-  // Now every result_ is either resolved, or is being resolved
-  // (see LookupRequest).
+  // Now every result_ is either resolved, or is being resolved.
 
   // Step through result_, recording names of all hosts that can't be erased.
   // We can't erase anything being worked on.
@@ -693,7 +646,6 @@ void Predictor::FinalizeInitializationOnIOThread(
 
   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();
@@ -876,16 +828,14 @@ void Predictor::PreconnectUrlOnIOThread(
   GURL url = GetHSTSRedirectOnIOThread(original_url);
 
   // TODO(csharrison): The observer should only be notified after the null check
-  // for the URLRequestContextGetter. The predictor tests should be fixed to
-  // allow for this, as they currently expect a callback with no getter.
-  // URLRequestContextGetter is null. Tests rely on this behavior.
+  // for the ProfileIOData. The predictor tests should be fixed to allow for
+  // this, as they currently expect a callback with no getter.
   if (observer_) {
     observer_->OnPreconnectUrl(
         url, first_party_for_cookies, motivation, count);
   }
 
-  net::URLRequestContextGetter* getter = url_request_context_getter_.get();
-  if (!getter)
+  if (!profile_io_data_)
     return;
 
   // Translate the motivation from UrlRequest motivations to HttpRequest
@@ -911,8 +861,9 @@ void Predictor::PreconnectUrlOnIOThread(
   }
   UMA_HISTOGRAM_ENUMERATION("Net.PreconnectMotivation", motivation,
                             UrlInfo::MAX_MOTIVATED);
-  content::PreconnectUrl(getter, url, first_party_for_cookies, count,
-                         allow_credentials, request_motivation);
+  content::PreconnectUrl(profile_io_data_->GetResourceContext(), url,
+                         first_party_for_cookies, count, allow_credentials,
+                         request_motivation);
 }
 
 void Predictor::PredictFrameSubresources(const GURL& url,
@@ -1023,19 +974,16 @@ void Predictor::PrepareFrameSubresources(const GURL& original_url,
   }
 }
 
-void Predictor::OnLookupFinished(LookupRequest* request, const GURL& url,
-                                 bool found) {
+void Predictor::OnLookupFinished(const GURL& url, int result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  LookupFinished(request, url, found);
-  pending_lookups_.erase(request);
-  delete request;
-
+  LookupFinished(url, result == net::OK);
+  DCHECK_GT(num_pending_lookups_, 0u);
+  num_pending_lookups_--;
   StartSomeQueuedResolutions();
 }
 
-void Predictor::LookupFinished(LookupRequest* request, const GURL& url,
-                               bool found) {
+void Predictor::LookupFinished(const GURL& url, bool found) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   UrlInfo* info = &results_[url];
   DCHECK(info->HasUrl(url));
@@ -1115,11 +1063,11 @@ bool Predictor::CongestionControlPerformed(UrlInfo* info) {
 void Predictor::StartSomeQueuedResolutions() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // If the queue is disabled, just make LookupRequests for all entries.
+  // If the queue is disabled, just make requests for all entries.
   bool enable_queue = base::FeatureList::IsEnabled(kUsePredictorDNSQueue);
-  while (!work_queue_.IsEmpty() &&
-         (!enable_queue ||
-          pending_lookups_.size() < max_concurrent_dns_lookups_)) {
+  while (
+      !work_queue_.IsEmpty() &&
+      (!enable_queue || num_pending_lookups_ < max_concurrent_dns_lookups_)) {
     const GURL url(work_queue_.Pop());
     UrlInfo* info = &results_[url];
     DCHECK(info->HasUrl(url));
@@ -1131,20 +1079,19 @@ void Predictor::StartSomeQueuedResolutions() {
       return;
     }
 
-    LookupRequest* request = new LookupRequest(this, host_resolver_, url);
-
-    int status = request->Start();
+    int status = content::PreresolveUrl(
+        profile_io_data_->GetResourceContext(), url,
+        base::Bind(&Predictor::OnLookupFinished, base::Unretained(this), url));

[kinuko, 2016/05/24 09:15:27]

Who guarantees the callback is called while |this| is alive?

[Charlie Harrison, 2016/05/24 13:14:22]

Good question. The semantics shouldn't be changed from the previous version of
the code, and in fact we do see a small amount of crashes for OnLookupFinished
today. I filed crbug.com/614350 that hopefully this patch will fix.

I'm not sure what the best way to fix it is. Possibly we could add an
intermediate object scoped to the callback that is notified of predictor
destruction. I'd hate to add that after we've removed the LookupRequest here
though :(.

[kinuko, 2016/05/24 14:38:34]

If we just want to disallow running the callback when |this| gets destructed we
could possibly just use weak ptr here (and in the other place too)..?  (Note
that I'm naively writing this without looking into the detailed object
relationship around this code)

[Charlie Harrison, 2016/05/24 16:06:36]

That works :) I scanned through the ownership and it all looks good. The only
thing we mutate in OnLookupFinished is the results_ map which should be
destructed fine when the Predictor goes away. Thanks for the suggestion.

     if (status == net::ERR_IO_PENDING) {
       // Will complete asynchronously.
-      pending_lookups_.insert(request);
-      peak_pending_lookups_ = std::max(peak_pending_lookups_,
-                                       pending_lookups_.size());
+      num_pending_lookups_++;
+      peak_pending_lookups_ =
+          std::max(peak_pending_lookups_, num_pending_lookups_);
     } else {
       // Completed synchronously (was already cached by HostResolver), or else
       // there was (equivalently) some network error that prevents us from
       // finding the name.  Status net::OK means it was "found."
-      LookupFinished(request, url, status == net::OK);
-      delete request;
+      LookupFinished(url, status == net::OK);
     }
   }
 }