Clear notifier when PrefService is destroyed.

Clear notifier when PrefService is destroyed.

Clear out the notifier in the PrefService destructor, to make it easier to diagnose
cases where a PrefChangeRegistrar outlives the PrefService.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141248

[Andrew T Wilson (Slow), 2012-06-06 21:02:00]

PTAL. I keep running into test failures where objects containing a
PrefChangeRegistrar outlive the PrefService, and this will make it easier to
track them down in the future.

[Bernhard Bauer, 2012-06-08 13:08:46]

http://codereview.chromium.org/10547002/diff/1/chrome/browser/prefs/pref_serv...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/10547002/diff/1/chrome/browser/prefs/pref_serv...
chrome/browser/prefs/pref_service.cc:228: pref_notifier_.reset();
I don't really understand. What does this change do? Note that |pref_notifier_|
is a scoped_ptr member of PrefService, so it should be destroyed immediately
after the destructor runs, no?

[Andrew T Wilson (Slow), 2012-06-08 14:58:53]

On 2012/06/08 13:08:46, Bernhard Bauer wrote:
>
http://codereview.chromium.org/10547002/diff/1/chrome/browser/prefs/pref_serv...
> File chrome/browser/prefs/pref_service.cc (right):
> 
>
http://codereview.chromium.org/10547002/diff/1/chrome/browser/prefs/pref_serv...
> chrome/browser/prefs/pref_service.cc:228: pref_notifier_.reset();
> I don't really understand. What does this change do? Note that
|pref_notifier_|
> is a scoped_ptr member of PrefService, so it should be destroyed immediately
> after the destructor runs, no?

Yes, but the member variable isn't set to NULL (scoped_ptr destructor just frees
the contents without clearing them).

Basically, on multiple occasions over the last month I've run into bugs where an
object outlived the Profile due to some bug, and the only indication was that
the PrefChangeRegistrar destructor in the object got an error trying to use the
pref_notifier_ after it had already been freed. 

It's easier to debug if you get an actual null pointer dereference, rather than
some obscure death way down in the stack, and we're already doing this for other
data members like pref_value_store_ and pref_sync_associator_ - I'm just adding
this behavior for pref_notifier_ also.

[Bernhard Bauer, 2012-06-08 15:54:58]

On 2012/06/08 14:58:53, Andrew T Wilson wrote:
> On 2012/06/08 13:08:46, Bernhard Bauer wrote:
> >
>
http://codereview.chromium.org/10547002/diff/1/chrome/browser/prefs/pref_serv...
> > File chrome/browser/prefs/pref_service.cc (right):
> > 
> >
>
http://codereview.chromium.org/10547002/diff/1/chrome/browser/prefs/pref_serv...
> > chrome/browser/prefs/pref_service.cc:228: pref_notifier_.reset();
> > I don't really understand. What does this change do? Note that
> |pref_notifier_|
> > is a scoped_ptr member of PrefService, so it should be destroyed immediately
> > after the destructor runs, no?
> 
> Yes, but the member variable isn't set to NULL (scoped_ptr destructor just
frees
> the contents without clearing them).
> 
> Basically, on multiple occasions over the last month I've run into bugs where
an
> object outlived the Profile due to some bug, and the only indication was that
> the PrefChangeRegistrar destructor in the object got an error trying to use
the
> pref_notifier_ after it had already been freed. 
> 
> It's easier to debug if you get an actual null pointer dereference, rather
than
> some obscure death way down in the stack, and we're already doing this for
other
> data members like pref_value_store_ and pref_sync_associator_ - I'm just
adding
> this behavior for pref_notifier_ also.

I see. LGTM!

[commit-bot: I haz the power, 2012-06-08 17:23:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/10547002/1

[commit-bot: I haz the power, 2012-06-08 19:11:55]

Change committed as 141248