Use lossy prefs in the net predictor, and update them more frequently

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 <iterator>
 #include <set>
 #include <sstream>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/containers/mru_cache.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
@@ skipping 79 matching lines @@
       max_dns_queue_delay_(
           TimeDelta::FromMilliseconds(g_max_queueing_delay_ms)),
       transport_security_state_(nullptr),
       ssl_config_service_(nullptr),
       proxy_service_(nullptr),
       preconnect_enabled_(preconnect_enabled),
       consecutive_omnibox_preconnect_count_(0),
       referrers_(kMaxReferrers),
       observer_(nullptr),
       timed_cache_(new TimedCache(base::TimeDelta::FromSeconds(
-          kMaxUnusedSocketLifetimeSecondsWithoutAGet))) {
+          kMaxUnusedSocketLifetimeSecondsWithoutAGet))),
+      ui_weak_factory_(new base::WeakPtrFactory<Predictor>(this)) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
 Predictor::~Predictor() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(shutdown_);
 }
 
 // static
 Predictor* Predictor::CreatePredictor(bool preconnect_enabled,
                                       bool predictor_enabled,
                                       bool simple_shutdown) {
   if (simple_shutdown)
     return new SimplePredictor(preconnect_enabled, predictor_enabled);
   return new Predictor(preconnect_enabled, predictor_enabled);
 }
 
 void Predictor::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
-  registry->RegisterListPref(prefs::kDnsPrefetchingStartupList);
-  registry->RegisterListPref(prefs::kDnsPrefetchingHostReferralList);
+  registry->RegisterListPref(prefs::kDnsPrefetchingStartupList,
+                             PrefRegistry::LOSSY_PREF);
+  registry->RegisterListPref(prefs::kDnsPrefetchingHostReferralList,
+                             PrefRegistry::LOSSY_PREF);
 }
 
 // --------------------- Start UI methods. ------------------------------------
 
 void Predictor::InitNetworkPredictor(PrefService* user_prefs,
                                      IOThread* io_thread,
                                      net::URLRequestContextGetter* getter,
                                      ProfileIOData* profile_io_data) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ skipping 157 matching lines @@
   }
 
   if (urls.empty())
     urls.push_back(GURL("http://www.google.com:80"));
 
   return urls;
 }
 
 void Predictor::DiscardAllResultsAndClearPrefsOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&Predictor::DiscardAllResults, weak_factory_->GetWeakPtr()));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&Predictor::DiscardAllResults,
+                                     io_weak_factory_->GetWeakPtr()));
   ClearPrefsOnUIThread();
 }
 
 void Predictor::ClearPrefsOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   user_prefs_->ClearPref(prefs::kDnsPrefetchingStartupList);
   user_prefs_->ClearPref(prefs::kDnsPrefetchingHostReferralList);
 }
 
 void Predictor::set_max_queueing_delay(int max_queueing_delay_ms) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   g_max_queueing_delay_ms = max_queueing_delay_ms;
 }
 
 void Predictor::set_max_parallel_resolves(size_t max_parallel_resolves) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   g_max_parallel_resolves = max_parallel_resolves;
 }
 
 void Predictor::ShutdownOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  ui_weak_factory_->InvalidateWeakPtrs();
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&Predictor::Shutdown, base::Unretained(this)));
 }
 
 // ---------------------- End UI methods. -------------------------------------
 
 // --------------------- Start IO methods. ------------------------------------
 
@@ skipping 250 matching lines @@
   profile_io_data_ = profile_io_data;
   initial_observer_.reset(new InitialObserver());
 
   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));
+  // on the same thread. Initialize the IO thread weak factory now.
+  io_weak_factory_.reset(new base::WeakPtrFactory<Predictor>(this));
 
   // Prefetch these hostnames on startup.
   DnsPrefetchMotivatedList(startup_urls, UrlInfo::STARTUP_LIST_MOTIVATED);
 
   DeserializeReferrers(*referral_list);
 
   LogStartupMetrics();
 }
 
 //-----------------------------------------------------------------------------
@@ skipping 47 matching lines @@
         FROM_HERE,
         base::Bind(&Predictor::ResolveList, base::Unretained(this),
                    urls, motivation));
   }
 }
 
 //-----------------------------------------------------------------------------
 // Functions to handle saving of hostnames from one session to the next, to
 // expedite startup times.
 
-static void SaveDnsPrefetchStateForNextStartupOnIOThread(
-    base::ListValue* startup_list,
-    base::ListValue* referral_list,
-    base::WaitableEvent* completion,
-    Predictor* predictor) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  if (nullptr == predictor) {
-    completion->Signal();
-    return;
-  }
-  predictor->SaveDnsPrefetchStateForNextStartup(startup_list, referral_list,
-                                                completion);
-}
-
 void Predictor::SaveStateForNextStartup() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!predictor_enabled_)
     return;
   if (!CanPreresolveAndPreconnect())
     return;
 
-  base::WaitableEvent completion(
-      base::WaitableEvent::ResetPolicy::MANUAL,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  std::unique_ptr<base::ListValue> startup_list(
+      user_prefs_->GetList(prefs::kDnsPrefetchingStartupList)->DeepCopy());

[mmenke, 2016/08/01 15:11:20]

I'm not following this code.  WriteDnsPrefetchState calls
GetInitialDnsResolutionList, which looks to clear startup_list, so why are we
copying stuff to it here?  Similarly, SerializeReferrers looks to clear
referral_list.

[Bernhard Bauer, 2016/08/01 15:18:52]

Huh, you're right! Which makes me wonder why this was using ListPrefUpdate in
the first place... :)

[Charlie Harrison, 2016/08/01 15:20:03]

Yeah, good catch Matt. I think we can just initialize an empty list here and
replace the pref wholesale in UpdatePrefsOnUIThread.

[mmenke, 2016/08/01 15:27:48]

Or could just create them on the IO thread.  Guess there's not an event we could
watch for there to use to know when to save the updated list.  Otherwise, we
could save ourselves the double hop, too.

[Charlie Harrison, 2016/08/01 15:34:23]

Given the double hop, I think I prefer initializing the list on the UI thread
here, as it makes the control flow / data flow explicit.

+  std::unique_ptr<base::ListValue> referral_list(
+      user_prefs_->GetList(prefs::kDnsPrefetchingHostReferralList)->DeepCopy());
+  // Get raw pointers to pass to the first task. Ownership of the unique_ptrs
+  // will be passed to the reply task.
+  base::ListValue* startup_list_raw = startup_list.get();
+  base::ListValue* referral_list_raw = referral_list.get();
 
-  ListPrefUpdate update_startup_list(user_prefs_,
-                                     prefs::kDnsPrefetchingStartupList);
-  ListPrefUpdate update_referral_list(user_prefs_,
-                                      prefs::kDnsPrefetchingHostReferralList);
-  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-    SaveDnsPrefetchStateForNextStartupOnIOThread(update_startup_list.Get(),
-                                                 update_referral_list.Get(),
-                                                 &completion, this);
-  } else {
-    bool posted = BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::Bind(&SaveDnsPrefetchStateForNextStartupOnIOThread,
-                   update_startup_list.Get(), update_referral_list.Get(),
-                   &completion, this));
-
-    // TODO(jar): Synchronous waiting for the IO thread is a potential source
-    // to deadlocks and should be investigated. See http://crbug.com/78451.
-    DCHECK(posted);
-    if (posted) {
-      // http://crbug.com/124954
-      base::ThreadRestrictions::ScopedAllowWait allow_wait;
-      completion.Wait();
-    }
-  }
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&Predictor::WriteDnsPrefetchState,
+                 io_weak_factory_->GetWeakPtr(), startup_list_raw,
+                 referral_list_raw),
+      base::Bind(&Predictor::UpdatePrefsOnUIThread,
+                 ui_weak_factory_->GetWeakPtr(),
+                 base::Passed(std::move(startup_list)),
+                 base::Passed(std::move(referral_list))));
 }
 
-void Predictor::SaveDnsPrefetchStateForNextStartup(
-    base::ListValue* startup_list,
-    base::ListValue* referral_list,
-    base::WaitableEvent* completion) {
+void Predictor::UpdatePrefsOnUIThread(
+    std::unique_ptr<base::ListValue> startup_list,
+    std::unique_ptr<base::ListValue> referral_list) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  user_prefs_->Set(prefs::kDnsPrefetchingStartupList, *startup_list);
+  user_prefs_->Set(prefs::kDnsPrefetchingHostReferralList, *referral_list);
+}
+
+void Predictor::WriteDnsPrefetchState(base::ListValue* startup_list,
+                                      base::ListValue* referral_list) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (initial_observer_.get())
     initial_observer_->GetInitialDnsResolutionList(startup_list);
 
   SerializeReferrers(referral_list);
-
-  completion->Signal();
 }
 
 void Predictor::PreconnectUrl(const GURL& url,
                               const GURL& first_party_for_cookies,
                               UrlInfo::ResolutionMotivation motivation,
                               bool allow_credentials,
                               int count) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
          BrowserThread::CurrentlyOn(BrowserThread::IO));
 
@@ skipping 274 matching lines @@
     info->SetAssignedState();
 
     if (CongestionControlPerformed(info)) {
       DCHECK(work_queue_.IsEmpty());
       return;
     }
 
     int status =
         content::PreresolveUrl(profile_io_data_->GetResourceContext(), url,
                                base::Bind(&Predictor::OnLookupFinished,
-                                          weak_factory_->GetWeakPtr(), url));
+                                          io_weak_factory_->GetWeakPtr(), url));
     if (status == net::ERR_IO_PENDING) {
       // Will complete asynchronously.
       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(url, status == net::OK);
@@ skipping 176 matching lines @@
 }
 
 void SimplePredictor::ShutdownOnUIThread() {
   SetShutdown(true);
 }
 
 bool SimplePredictor::CanPrefetchAndPrerender() const { return true; }
 bool SimplePredictor::CanPreresolveAndPreconnect() const { return true; }
 
 }  // namespace chrome_browser_net