Don't clear the net predictors prefs on startup

Don't clear the net predictors prefs on startup

Traditionally, the net predictor had its database cleared from preferences
during startup, deleting them while transfering to a completely in-memory
representation.

This is non-ideal for Android, where unclean shutdowns are more common due
to oom-killer.

This patch removes the behavior in favor of clearing the prefs when the user
clears browsing history. Note that in order to properly persist db updates
in the presence of unclean shutdowns, we will have to sync the prefs more
often. This will be implemented as a follow up.

BUG=621604
Committed: https://crrev.com/f2113bc361ed6ad09f2e97ef5068be4e8c32e114
Cr-Commit-Position: refs/heads/master@{#402813}

[Charlie Harrison, 2016-06-23 22:26:43]

csharrison@chromium.org changed reviewers:
+ eroman@chromium.org

[Charlie Harrison, 2016-06-23 22:26:44]

Eric, do you mind taking a look at this? Before Android, I guess it was a good
idea to clear prefs on startup and save them on clean shutdown (simplifies data
logging, etc).

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

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:133:
chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();
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?

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:138:
base::Unretained(predictor)));
is it guaranteed that predictor (and by extension profile) is alive when this
runs?

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/net/pred...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/net/pred...
chrome/browser/net/predictor.h:163: // ------------- End UI thread methods.
Based on this comment, I would expect ClearPrefsOnUIThread to be up here.

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

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:133:
chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();
On 2016/06/23 23:48:05, eroman wrote:
> 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?
>   

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.

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:138:
base::Unretained(predictor)));
On 2016/06/23 23:48:05, eroman wrote:
> is it guaranteed that predictor (and by extension profile) is alive when this
> runs?

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.

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/net/pred...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/net/pred...
chrome/browser/net/predictor.h:163: // ------------- End UI thread methods.
On 2016/06/23 23:48:05, eroman wrote:
> Based on this comment, I would expect ClearPrefsOnUIThread to be up here.

Ack.

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

On a related note, what is the interaction with incognito mode?

Does incognito have its own Predictor, or otherwise mark the predictions as
coming from incognito?

Certainly we wouldn't want to end up writing incognito information to the prefs
by switching to the periodic flushing mode (not sure if the writing at shutdown
was partly to avoid writing incognito stuff).

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:138:
base::Unretained(predictor)));
On 2016/06/24 00:00:52, csharrison wrote:
> On 2016/06/23 23:48:05, eroman wrote:
> > is it guaranteed that predictor (and by extension profile) is alive when
this
> > runs?
> 
> 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.

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?

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

Note: predictor is nullptr in incognito mode.

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:133:
chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();
On 2016/06/24 00:00:52, csharrison wrote:
> On 2016/06/23 23:48:05, eroman wrote:
> > 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?
> >   
> 
> 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.

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.

[eroman, 2016-06-25 01:12:53]

lgtm after nits

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (left):

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:155: // have to worry about clearing our
in-memory state when Clearing Browsing
(Not familiar with this previous code, but the previous design seems a bit odd
if this was the only motivation)

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
chrome/browser/net/predictor.h:183: void ClearPrefsOnUIThread();
Same comment as earlier -- why not put this above the line 167?

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
chrome/browser/net/predictor.h:186: void DiscardAllResultAndClearPrefs();
"AllResultAnd" --> "AllResultsAnd"

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
chrome/browser/net/predictor_browsertest.cc:1380: // The target url will have a
expected connection count of 2 after this call.
nit: url --> URL, and "a expected" --> "an expected"

[Charlie Harrison, 2016-06-27 18:57:11]

csharrison@chromium.org changed reviewers:
+ thestig@chromium.org

[Charlie Harrison, 2016-06-27 18:57:13]

thestig@, do you mind taking a look at chrome/browser/browsing_data and
chrome/browser/chrome_net_benchmarking_message_filter?

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (left):

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:155: // have to worry about clearing our
in-memory state when Clearing Browsing
On 2016/06/25 at 01:12:53, eroman wrote:
> (Not familiar with this previous code, but the previous design seems a bit odd
if this was the only motivation)

Yeah this was my impression as well. It makes some sense considering some of the
complications here (i.e. having to jump to the UI thread to ensure deletion).
But still, I think this is saner.

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
File chrome/browser/net/predictor.h (right):

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
chrome/browser/net/predictor.h:183: void ClearPrefsOnUIThread();
On 2016/06/25 at 01:12:53, eroman wrote:
> Same comment as earlier -- why not put this above the line 167?

Sorry I missed this one. Done.

https://codereview.chromium.org/2085643002/diff/70001/chrome/browser/net/pred...
chrome/browser/net/predictor.h:186: void DiscardAllResultAndClearPrefs();
On 2016/06/25 at 01:12:53, eroman wrote:
> "AllResultAnd" --> "AllResultsAnd"

The perils of autocomplete :) Done.

[Lei Zhang, 2016-06-27 19:07:40]

thestig@chromium.org changed reviewers:
+ bauerb@chromium.org

[Lei Zhang, 2016-06-27 19:07:41]

I'm going to defer to bauerb@ for chrome/browser/browsing_data.

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

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:133:
chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();
On 2016/06/24 16:12:09, csharrison wrote:
> On 2016/06/24 00:00:52, csharrison wrote:
> > On 2016/06/23 23:48:05, eroman wrote:
> > > 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?
> > >   
> > 
> > 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.
> 
> 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.

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).

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:138:
base::Unretained(predictor)));
On 2016/06/24 00:17:28, eroman wrote:
> On 2016/06/24 00:00:52, csharrison wrote:
> > On 2016/06/23 23:48:05, eroman wrote:
> > > is it guaranteed that predictor (and by extension profile) is alive when
> this
> > > runs?
> > 
> > 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.
> 
> That might work.

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.

> 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?

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.

https://codereview.chromium.org/2085643002/diff/70002/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/2085643002/diff/70002/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:158: urls, referral_list,
I would be a bit careful here -- you're passing the ListValue to the UI thread,
but it's owned by the PrefService, which could in principle delete it at any
time. Maybe keep passing a copy (as a unique_ptr)?

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

Thanks for the detailed review!

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/50006/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:133:
chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();
On 2016/06/28 at 11:43:46, Bernhard Bauer wrote:
> On 2016/06/24 16:12:09, csharrison wrote:
> > On 2016/06/24 00:00:52, csharrison wrote:
> > > On 2016/06/23 23:48:05, eroman wrote:
> > > > 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?
> > > >   
> > > 
> > > 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.
> > 
> > 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.
> 
> 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).

Thanks for the suggestions. Those sound good to me.

[Bernhard Bauer, 2016-06-29 09:52:21]

LGTM

https://codereview.chromium.org/2085643002/diff/90001/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/90001/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:141:
chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();
Add a TODO here about ensuring we don't access the Predictor or Profile after
it's been shut down?

[Lei Zhang, 2016-06-29 14:16:06]

lgtm

[Charlie Harrison, 2016-06-29 14:19:09]

Thanks everyone!

https://codereview.chromium.org/2085643002/diff/90001/chrome/browser/chrome_n...
File chrome/browser/chrome_net_benchmarking_message_filter.cc (right):

https://codereview.chromium.org/2085643002/diff/90001/chrome/browser/chrome_n...
chrome/browser/chrome_net_benchmarking_message_filter.cc:141:
chrome_browser_net::Predictor* predictor = profile_->GetNetworkPredictor();
On 2016/06/29 09:52:20, Bernhard Bauer wrote:
> Add a TODO here about ensuring we don't access the Predictor or Profile after
> it's been shut down?

Done.

[Charlie Harrison, 2016-06-29 14:21:33]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-06-29 14:21:34]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
bauerb@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2085643002/#ps110001
(title: "add TODO")

[commit-bot: I haz the power, 2016-06-29 14:21:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 15:08:09]

Committed patchset #8 (id:110001)

[commit-bot: I haz the power, 2016-06-29 15:08:22]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 15:09:48]

Description was changed from

==========
Don't clear the net predictors prefs on startup

Traditionally, the net predictor had its database cleared from preferences
during startup, deleting them while transfering to a completely in-memory
representation.

This is non-ideal for Android, where unclean shutdowns are more common due
to oom-killer.

This patch removes the behavior in favor of clearing the prefs when the user
clears browsing history. Note that in order to properly persist db updates
in the presence of unclean shutdowns, we will have to sync the prefs more
often. This will be implemented as a follow up.

BUG=621604
==========

to

==========
Don't clear the net predictors prefs on startup

Traditionally, the net predictor had its database cleared from preferences
during startup, deleting them while transfering to a completely in-memory
representation.

This is non-ideal for Android, where unclean shutdowns are more common due
to oom-killer.

This patch removes the behavior in favor of clearing the prefs when the user
clears browsing history. Note that in order to properly persist db updates
in the presence of unclean shutdowns, we will have to sync the prefs more
often. This will be implemented as a follow up.

BUG=621604

Committed: https://crrev.com/f2113bc361ed6ad09f2e97ef5068be4e8c32e114
Cr-Commit-Position: refs/heads/master@{#402813}
==========

[commit-bot: I haz the power, 2016-06-29 15:09:50]

Patchset 8 (id:??) landed as
https://crrev.com/f2113bc361ed6ad09f2e97ef5068be4e8c32e114
Cr-Commit-Position: refs/heads/master@{#402813}