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

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 658223c1dc4df7b607f73ce2586e67a957b2014c..b223bf882858df5ca2924abe4b3fd87f5ba1c482 100644
--- a/chrome/browser/net/predictor.cc
+++ b/chrome/browser/net/predictor.cc
@@ -12,6 +12,7 @@
 #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"
@@ -131,8 +132,10 @@ Predictor* Predictor::CreatePredictor(bool preconnect_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. ------------------------------------
@@ -146,6 +149,10 @@ void Predictor::InitNetworkPredictor(PrefService* user_prefs,
   user_prefs_ = user_prefs;
   url_request_context_getter_ = getter;
 
+  // base::WeakPtrFactory instances need to be created and destroyed
+  // on the same thread. Initialize the UI thread weak factory now.
+  ui_weak_factory_.reset(new base::WeakPtrFactory<Predictor>(this));

[Bernhard Bauer, 2016/08/01 09:06:24]

You could just initialize this in the constructor.

[Charlie Harrison, 2016/08/01 12:38:24]

Done.

+
   // Gather the list of hostnames to prefetch on startup.
   std::vector<GURL> urls = GetPredictedUrlListAtStartup(user_prefs);
 
@@ -310,9 +317,9 @@ std::vector<GURL> Predictor::GetPredictedUrlListAtStartup(
 
 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();
 }
 
@@ -334,6 +341,7 @@ void Predictor::set_max_parallel_resolves(size_t max_parallel_resolves) {
 
 void Predictor::ShutdownOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  ui_weak_factory_->InvalidateWeakPtrs();
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
@@ -604,11 +612,8 @@ void Predictor::FinalizeInitializationOnIOThread(
   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);
@@ -676,68 +681,51 @@ void Predictor::DnsPrefetchMotivatedList(
 // 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 = base::WrapUnique(

[Bernhard Bauer, 2016/08/01 09:06:24]

You can just initialize the unique_ptr with the raw pointer.

[Charlie Harrison, 2016/08/01 12:38:24]

Done.

+      user_prefs_->GetList(prefs::kDnsPrefetchingStartupList)->DeepCopy());
+  std::unique_ptr<base::ListValue> referral_list = base::WrapUnique(
+      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();
+
+  // It is safe to use unretained to the task on the IO thread, because this

[Bernhard Bauer, 2016/08/01 09:06:24]

If you still have the |io_weak_factory_|, you could just use a weak pointer from
there. I don't really have a strong opinion about it :)

[Charlie Harrison, 2016/08/01 12:38:24]

No, that makes the code much clearer (+robust). No more relying on complex
destruction ordering.

+  // method is guaranteed to be called before ShutdownOnUIThread, which posts a
+  // task to shutdown on the IO thread. So WriteDnsPrefetchState is guaranteed
+  // to have access to valid data, because it will be called before Shutdown.
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&Predictor::WriteDnsPrefetchState, base::Unretained(this),
+                 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))));
+}
 
-  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();
-    }
-  }
+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::SaveDnsPrefetchStateForNextStartup(
-    base::ListValue* startup_list,
-    base::ListValue* referral_list,
-    base::WaitableEvent* completion) {
+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,
@@ -1032,7 +1020,7 @@ void Predictor::StartSomeQueuedResolutions() {
     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_++;