Fix destruction concurrency issue in PrerenderLocalPredictor:

Fix destruction concurrency issue in PrerenderLocalPredictor:
PrerenderLocalPredictor will now keep around a reference to the HistoryService
that it subscribed too, and will unsubscribe itself on destruction.
Since PrerenderLocalPredictor is owned by PrerenderManager, and both
PrerenderManager and HistoryService have the same lifetime as the underlying
profile, the HistoryService object will not be kept around unnecessarily
longer as a result of this.
R=brettw, cbentzel
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133744

[brettw, 2012-04-24 18:17:14]

lgtm

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.h:61: // service.
I think it would be worth mentioning here why you can't just grab it from the
profile. I could imagine doing refactoring and "fixing" this case by removing
your cached value.

[cbentzel, 2012-04-24 18:33:57]

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.cc:97: HistoryService*
history = GetHistoryIfExists();
Should this be using observing_history_service_?

[tburkard, 2012-04-24 18:39:57]

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.cc:97: HistoryService*
history = GetHistoryIfExists();
Makes no difference, since it'd be the same one.  Since it is not critical to
use the same (old) one, I thought it'd be cleaner here to request a fresh copy
from the profile.  Also, if the profile no longer has a historyservice, probably
a good idea to not issue new requests against it if we are already in the
process of shutting down the profile.
On 2012/04/24 18:33:57, cbentzel wrote:
> Should this be using observing_history_service_?

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.h:61: // service.
On 2012/04/24 18:17:15, brettw wrote:
> I think it would be worth mentioning here why you can't just grab it from the
> profile. I could imagine doing refactoring and "fixing" this case by removing
> your cached value.

Done.

[cbentzel, 2012-04-24 18:47:22]

LGTM

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://chromiumcodereview.appspot.com/10214004/diff/1/chrome/browser/prerend...
chrome/browser/prerender/prerender_local_predictor.cc:97: HistoryService*
history = GetHistoryIfExists();
On 2012/04/24 18:39:57, tburkard wrote:
> Makes no difference, since it'd be the same one.  Since it is not critical to
> use the same (old) one, I thought it'd be cleaner here to request a fresh copy
> from the profile.  Also, if the profile no longer has a historyservice,
probably
> a good idea to not issue new requests against it if we are already in the
> process of shutting down the profile.
> On 2012/04/24 18:33:57, cbentzel wrote:
> > Should this be using observing_history_service_?
> 

Sounds reasonable. Can you add a comment about this here?

[cbentzel, 2012-04-24 18:57:02]

Also, is there a BUG associated with this?

[tburkard, 2012-04-24 19:13:40]

No bug.

On Tue, Apr 24, 2012 at 11:57 AM, <cbentzel@chromium.org> wrote:

> Also, is there a BUG associated with this?
>
>
http://codereview.chromium.**org/10214004/<http://codereview.chromium.org/102...
>