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 236316d7d20f3c01f9324e2357906226eb3bd81c..ad3447a2a3c2db1e7380d9dc369872fbe7036789 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"
@@ -128,8 +129,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. ------------------------------------
@@ -673,68 +676,35 @@ 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);
-}
+static void RetainUpdateClosure(ListPrefUpdate* update_startup_list,

[Bernhard Bauer, 2016/07/29 11:07:53]

Move this to an anonymous namespace?

[Charlie Harrison, 2016/07/29 13:42:13]

This was changed to actually perform the update, so it's staying a method that
needs access to user_prefs_.

+                                ListPrefUpdate* update_referral_list) {}
 
 void Predictor::SaveStateForNextStartup() {
-  if (!predictor_enabled_)
-    return;
-  if (!CanPreresolveAndPreconnect())
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!predictor_enabled_ || !CanPreresolveAndPreconnect())
     return;
 
-  base::WaitableEvent completion(
-      base::WaitableEvent::ResetPolicy::MANUAL,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  ListPrefUpdate update_startup_list(user_prefs_,
-                                     prefs::kDnsPrefetchingStartupList);
-  ListPrefUpdate update_referral_list(user_prefs_,
-                                      prefs::kDnsPrefetchingHostReferralList);
-  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {

[Bernhard Bauer, 2016/07/29 11:07:53]

Thanks for cleaning this up, BTW! This looks like the code used to access the
prefs on the IO thread? That's definitely wrong...

[Charlie Harrison, 2016/07/29 13:42:13]

No problem, this also triggers a synchronous thread hop which I think attempts
to make the IO thread access safe. Not sure if it *actually* does :/

-    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();
-    }
-  }
+  ListPrefUpdate* update_startup_list =

[Bernhard Bauer, 2016/07/29 11:07:53]

This is not allowed, because ownership of the returned value remains with the
PrefService, which could e.g. delete it on the UI thread in the meantime. 

What I would instead do is this:
1) Read the values from the PrefService and make a DeepCopy() so you actually
own them
2) Write to the values on the IO thread
3) Back on the UI thread, write them to the PrefService.

You also have to ensure that the PrefService is alive when you're back on the UI
thread. How does shutdown of this object work again? Is there a method that is
called on the UI thread at shutdown?

[Charlie Harrison, 2016/07/29 13:42:13]

Thanks this is a good suggestion. We can clear the |user_prefs_| pointer in
ShutdownOnUIThread as an indicator that we probably shouldn't be doing any
update. The only problem is that it seems like we can't use WeakPtrs for both
callbacks in the PostTaskAndReply due to thread safety concerns. Is there a
common way to solve this? Obviously I could add another WeakFactory but that
seems like overkill. The current patch punts on this issue by using unretained.

[Bernhard Bauer, 2016/07/29 14:08:14]

Hm, that's a tricky one. Like I said, it all depends on the lifetime of this
class -- IIRC, on profile destruction ShutdownOnUIThread() is called and then a
task is posted to delete this on the IO thread. In that case we don't actually
need the weak pointer for posting to the IO thread, as long as we know that
ShutdownOnUIThread() hasn't been called (because anything we post to the IO
thread at that point is guaranteed to run before the object is destroyed). We do
need to watch out when posting back to the UI thread though (which doesn't
currently happen yet AFAICT), which might require a WeakPtrFactory, which would
invalidate its pointers in ShutdownOnUIThread(). That way at least we won't have
more than one WeakPtrFactory :)

(Also, documenting the reasoning I'm outlining above would be very good. Hint,
hint 😉)

[Charlie Harrison, 2016/07/29 16:50:12]

Hm okay so I chatted with mmenke@ about this (cced), and I think there is a
problem with using the current weak_factory_ for this, which is used for weak
pointers on the IO thread.

I don't think there's a way around this, unless we defer saving prefs until the
IO thread weak_factory has no more weak pointers. So, I've added another weak
factory for UI thread weak pointers.

+      new ListPrefUpdate(user_prefs_, prefs::kDnsPrefetchingStartupList);
+  ListPrefUpdate* update_referral_list =
+      new ListPrefUpdate(user_prefs_, prefs::kDnsPrefetchingHostReferralList);
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&Predictor::SaveDnsPrefetchStateForNextStartup,
+                 weak_factory_->GetWeakPtr(), update_startup_list->Get(),
+                 update_referral_list->Get()),
+      base::Bind(&RetainUpdateClosure, base::Owned(update_startup_list),
+                 base::Owned(update_referral_list)));
 }
 
 void Predictor::SaveDnsPrefetchStateForNextStartup(
     base::ListValue* startup_list,
-    base::ListValue* referral_list,
-    base::WaitableEvent* completion) {
+    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,