Don't clear the net predictors prefs on startup

chrome/browser/chrome_net_benchmarking_message_filter.cc

Index: chrome/browser/chrome_net_benchmarking_message_filter.cc
diff --git a/chrome/browser/chrome_net_benchmarking_message_filter.cc b/chrome/browser/chrome_net_benchmarking_message_filter.cc
index b95edb47744fc37d27f1bc8e3b94023892d9f32c..74d61ba6fcc7e0a7a910a7b3fe36e073f43b4bc8 100644
--- a/chrome/browser/chrome_net_benchmarking_message_filter.cc
+++ b/chrome/browser/chrome_net_benchmarking_message_filter.cc
@@ -131,8 +131,13 @@ void ChromeNetBenchmarkingMessageFilter::OnClearPredictorCache() {
     return;
   }
   chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();

[eroman, 2016/06/23 23:48:05]

I am confused by the threading model here.

We are on the IO thread, and then we call profile_->GetNetworkPredictor().
  * Accessing profile from non-UI thread doesn't seem right -- shouldn't Profile
be exclusively used on UI thread?
  * What guarantees that Profile is still alive at this point? Is that
guaranteed by the MessageFilter mechanism?
  

[Charlie Harrison, 2016/06/24 00:00:52]

This looks like a bug. If you look at the comments above |profile_| in the .h
file it says to only access this on the UI thread.

[Charlie Harrison, 2016/06/24 16:12:09]

Going to punt on this one because I don't have a great solution. MessageFilter
lifetimes are complicated. I'll write up a crbug explaining this existing bug.
Note that we could do what the chrome message filter does and save the
Predictor*. This is also a bug and can cause uaf. 

Maybe we can just kill all this code? The extension looks very unloved.

[Bernhard Bauer, 2016/06/28 11:43:46]

Here's what I think we should do:
1) Dispatch this message on the UI thread via OverrideThreadForMessage()
2) Make DiscardAllResultsAndClearPrefs() run on the UI thread and post a task to
DiscardAllResults() to the IO thread instead of the other way around. This will
also solve the destruction issue below.
3) Store a pointer to the predictor instead of the profile in this class
4) Observe profile shutdown on the UI thread via KeyedServiceShutdownNotifier
and clear the predictor pointer when that happens

Longer term, it would probably make sense to split the Predictor up into a UI
thread part and an IO thread part. The IO thread part could be refcounted if
(and only if) necessary, and the UI thread part could be made a
ProfileKeyedService so it would automatically be shut down with together with
the profile.

I think at the very least 1) and 2) we could do as part of this CL, because
they're rather small changes in areas of the code you're already touching
anyway. If you are filing a crbug, could you reference it with a TODO here? Feel
free to make it TODO(bauerb).

[Charlie Harrison, 2016/06/28 17:01:43]

Thanks for the suggestions. Those sound good to me.

-  if (predictor)
+  if (predictor) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&chrome_browser_net::Predictor::ClearPrefsOnUIThread,
+                   base::Unretained(predictor)));

[eroman, 2016/06/23 23:48:05]

is it guaranteed that predictor (and by extension profile) is alive when this
runs?

[Charlie Harrison, 2016/06/24 00:00:52]

Hm I think so but I can't prove it off the top of my head. I can move this call
into DiscardAllResults using a weak reference.

[eroman, 2016/06/24 00:17:28]

That might work.

I am not totally sure on the destruction orders TBH.
Predictor is deleted when the ProfileIO is deleted -- which is on the UI thread.
Is that in turn guaranteed to happen before the Profile itself is deleted on UI
thread?

[Bernhard Bauer, 2016/06/28 11:43:46]

Sadly, no, because the Predictor is destroyed on the IO thread, so dereferencing
a weak pointer to it on the UI thread is a race condition.
The Predictor is owned by ProfileIOData, so it is destroyed on the IO thread,
after the profile has been destroyed on the UI thread. The Profile itself just
keeps a raw pointer to the Predictor.

     predictor->DiscardAllResults();
+  }
 }
 
 bool ChromeNetBenchmarkingMessageFilter::CheckBenchmarkingEnabled() const {