Store a stacktrace of PrefService destruction in PrefChangeRegistrar

Store a stacktrace of PrefService destruction in PrefChangeRegistrar

Store a stacktrace of PrefService destruction in PrefChangeRegistrar to debug a
race condition: ~PrefChangeRegistrar causes a crash. The current hypothesis is
that this is caused because the PrefChangeRegistrar is registered to a Profile
that is already destroyed. This code stores stacktrace when the Profile's
PrefService is destroyed to see whether this is correct.

BUG=373435
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271416

[battre, 2014-05-16 14:00:39]

Hi Balazs,

please review this CL.

Thanks,
Dominic

[battre, 2014-05-16 14:20:23]

Minor improvements

[engedy, 2014-05-16 15:19:58]

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
File base/prefs/pref_change_registrar.cc (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:8: // TODO(battre): Delete this. See
crbug.com/373435
nit: period to the end of the comment.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:57: // was destroyed, it stored a stacktrack
of the destruction in
nit: s/stacktrack/stack trace

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:62: std::min<int>(sizeof(tmp) - 1,
pref_service_destruction_.length());
Could we use strncat() here, like so:

char tmp[2048] = {};
strncat(tmp, pref_service_destruction_.c_str(), sizeof(tmp)-1);

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:104: const std::string& stacktrace) {
nit: stack_trace.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
File base/prefs/pref_change_registrar.h (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.h:69: // TODO(battre): Remove function
(debugging tool for crbug.com/373435).
nit: I am not sure saying "debugging tool" is too informative. The same way you
are doing it in some other places, could we just consistently say everywhere:
 
// TODO(battre): Remove function. See: crbug.com/373435.

And add some details to the bug itself. Anyone interested can then look up the
bug for a detailed explanation.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier.h
File base/prefs/pref_notifier.h (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
base/prefs/pref_notifier.h:28: virtual void SetPrefServiceDestructionTrace(
nit: I would call this BroadcastPrefServiceDestructionTrace().

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
base/prefs/pref_notifier.h:29: const std::string& stacktrace) = 0;
nit: stack_trace.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
File base/prefs/pref_notifier_impl.h (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
base/prefs/pref_notifier_impl.h:42: // PrefNotifier overrides.
I might be missing something, but could we not just move this a further down and
group it with the other overrides?

[battre, 2014-05-16 15:58:36]

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
File base/prefs/pref_change_registrar.cc (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:8: // TODO(battre): Delete this. See
crbug.com/373435
On 2014/05/16 15:19:58, engedy wrote:
> nit: period to the end of the comment.

Done.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:57: // was destroyed, it stored a stacktrack
of the destruction in
On 2014/05/16 15:19:58, engedy wrote:
> nit: s/stacktrack/stack trace

Done.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:62: std::min<int>(sizeof(tmp) - 1,
pref_service_destruction_.length());
On 2014/05/16 15:19:58, engedy wrote:
> Could we use strncat() here, like so:
> 
> char tmp[2048] = {};
> strncat(tmp, pref_service_destruction_.c_str(), sizeof(tmp)-1);

Done.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.cc:104: const std::string& stacktrace) {
On 2014/05/16 15:19:58, engedy wrote:
> nit: stack_trace.

Done.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
File base/prefs/pref_change_registrar.h (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_change_r...
base/prefs/pref_change_registrar.h:69: // TODO(battre): Remove function
(debugging tool for crbug.com/373435).
On 2014/05/16 15:19:58, engedy wrote:
> nit: I am not sure saying "debugging tool" is too informative. The same way
you
> are doing it in some other places, could we just consistently say everywhere:
>  
> // TODO(battre): Remove function. See: crbug.com/373435.
> 
> And add some details to the bug itself. Anyone interested can then look up the
> bug for a detailed explanation.

Done.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier.h
File base/prefs/pref_notifier.h (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
base/prefs/pref_notifier.h:28: virtual void SetPrefServiceDestructionTrace(
On 2014/05/16 15:19:58, engedy wrote:
> nit: I would call this BroadcastPrefServiceDestructionTrace().

Done.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
base/prefs/pref_notifier.h:29: const std::string& stacktrace) = 0;
On 2014/05/16 15:19:58, engedy wrote:
> nit: stack_trace.

Done.

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
File base/prefs/pref_notifier_impl.h (right):

https://codereview.chromium.org/290083006/diff/20001/base/prefs/pref_notifier...
base/prefs/pref_notifier_impl.h:42: // PrefNotifier overrides.
On 2014/05/16 15:19:58, engedy wrote:
> I might be missing something, but could we not just move this a further down
and
> group it with the other overrides?

Done. I need to cast at the caller site now, but that's fine by me.

[engedy, 2014-05-16 16:02:53]

LGTM. Have you verified locally that it works by artificially introducing an
error?

[battre, 2014-05-16 16:04:04]

On 2014/05/16 16:02:53, engedy wrote:
> LGTM. Have you verified locally that it works by artificially introducing an
> error?

Yes

[battre, 2014-05-19 15:44:59]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2014-05-19 15:45:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/battre@chromium.org/290083006/60001

[commit-bot: I haz the power, 2014-05-19 17:44:45]

Change committed as 271416