Add UMA to report Preferences File Corruption

Add UMA to report Preferences File Corruption
Adds PrefMetricsService methods to track changes to user preferences
that happen outside of Chrome and report discrepancies via UMA.

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

[bbudge, 2013-08-15 01:16:48]

battre and mnissler for everything.
mpearson for OWNERS on tools/metrics/histograms

There is some problem on linux_chromeos, where Profile::GetProfileName is
failing. Is this because ChromeOS doesn't really have profiles? How should the
code be modified to work around this?

[Mark P, 2013-08-15 01:21:04]

How do you plan to normalize these histograms?

For instance, suppose the "changed" histogram has entries from 50k unique
users.  Is that good or bad?  Well, it depends on how many users did
profile loads over that time that have this code running.  Where do you get
that number from?

--mark



On Wed, Aug 14, 2013 at 6:16 PM, <bbudge@chromium.org> wrote:

> Reviewers: battre, Mattias Nissler, Mark P,
>
> Message:
> battre and mnissler for everything.
> mpearson for OWNERS on tools/metrics/histograms
>
> There is some problem on linux_chromeos, where Profile::GetProfileName is
> failing. Is this because ChromeOS doesn't really have profiles? How should
> the
> code be modified to work around this?
>
> Description:
> Add UMA to report Preferences File Corruption
> Adds PrefMetricsService methods to track changes to user preferences
> that happen outside of Chrome and report discrepancies via UMA.
>
> BUG=266569
>
> Please review this at
https://codereview.chromium.**org/22676002/<https://codereview.chromium.org/2...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/prefs/browser_**prefs.cc
>   M chrome/browser/prefs/pref_**metrics_service.h
>   M chrome/browser/prefs/pref_**metrics_service.cc
>   M tools/metrics/histograms/**histograms.xml
>
>
>

[Mattias Nissler (ping if slow), 2013-08-15 10:14:51]

Some high-level comments:

1. What Mark says. One way could be to record a sample unconditionally every
time you validate a pref, e.g. adding a new histogram
Settings.TrackedPreferenceChecked that's used to establish the baseline.
2. You should write unit tests for this.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:15: #include
"chrome/browser/extensions/api/music_manager_private/device_id.h"
I'd consider this a layering violation. chrome/browser/prefs/ should definitely
not depend on extensions.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:32: // save selected prefs in Local
Settings. When Chrome is started up, we compare
local state, here and below

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:35: // values, grouped by profile.
To make the system faster and more resistant to
I don't think hashing makes things faster, given that calculating the hashes
actually takes considerable time.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:48:
extensions::ExtensionPrefs::kExtensionsPref,
FWIW, this should be defined in pref_names.{h,cc} as well. Not your fault, feel
free to ignore this comment :)

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:65:
g_browser_process->local_state();
This shouldn't be required, local state gets initialized on browser startup.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:47: // Registers preferences in
local settings.
nit: s/local settings/local state/ for consistency with most of the rest of the
code.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:96: // Updates the hash of the
tracked preference in local settings. This must be
local state

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:100: // Removes the tracked
preference from local settings. Returns 'true' iff. the
local state

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:104: // Gets the path to the
preference value hash in local settings.
local state

[bbudge, 2013-08-15 17:33:48]

There are still a few tests that break because this writes to local state.

I'm working on unit tests but would like to continue to work on the test
failures at the same time so I may upload a few more patchsets.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:15: #include
"chrome/browser/extensions/api/music_manager_private/device_id.h"
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> I'd consider this a layering violation. chrome/browser/prefs/ should
definitely
> not depend on extensions.

This appears to be the only way to get a unique per-device id. Perhaps another
CL is needed to expose this more widely?

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:32: // save selected prefs in Local
Settings. When Chrome is started up, we compare
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> local state, here and below

Done.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:35: // values, grouped by profile.
To make the system faster and more resistant to
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> I don't think hashing makes things faster, given that calculating the hashes
> actually takes considerable time.

I was thinking of I/O and comparing values, but I will remove it as the main
point is to discourage spoofing.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:48:
extensions::ExtensionPrefs::kExtensionsPref,
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> FWIW, this should be defined in pref_names.{h,cc} as well. Not your fault,
feel
> free to ignore this comment :)

Perhaps a separate CL.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:65:
g_browser_process->local_state();
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> This shouldn't be required, local state gets initialized on browser startup.

I thought it might fix some tests that are crashing because profile name isn't
registered in settings. Unfortunately, it doesn't. Removed.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:47: // Registers preferences in
local settings.
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> nit: s/local settings/local state/ for consistency with most of the rest of
the
> code.

Done.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:96: // Updates the hash of the
tracked preference in local settings. This must be
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> local state

Done.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:100: // Removes the tracked
preference from local settings. Returns 'true' iff. the
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> local state

Done.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:104: // Gets the path to the
preference value hash in local settings.
On 2013/08/15 10:14:51, Mattias Nissler wrote:
> local state

Done.

[bbudge, 2013-08-19 22:30:41]

Added unit tests. PTAL

[bbudge, 2013-08-20 05:00:30]

On 2013/08/19 22:30:41, bbudge1 wrote:
> Added unit tests. PTAL

Hold off on reviewing. I need to rework unit tests since some configurations of
TestingProfile register the tracked preferences.

[bbudge, 2013-08-20 12:07:39]

I think this is ready for another round of review.

[Mattias Nissler (ping if slow), 2013-08-20 12:53:54]

LGTM w/ nits.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:15: #include
"chrome/browser/extensions/api/music_manager_private/device_id.h"
On 2013/08/15 17:33:49, bbudge1 wrote:
> On 2013/08/15 10:14:51, Mattias Nissler wrote:
> > I'd consider this a layering violation. chrome/browser/prefs/ should
> definitely
> > not depend on extensions.
> 
> This appears to be the only way to get a unique per-device id. Perhaps another
> CL is needed to expose this more widely?

That'd be appreciated. Can you file a bug and put a TODO?

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:37: const char
kProfilePreferenceHashes[] = "profile.preference_hashes";
This should be in pref_names.{h,cc}

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:11: #include
"base/gtest_prod_util.h"
Not used?

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service_unittest.cc (right):

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service_unittest.cc:20: const char
kProfilePreferenceHashes[] = "profile.preference_hashes";
Use the constants defined in pref_names.{h,cc} instead.

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service_unittest.cc:22: static int
preferences_checked_total;
These should probably have a comment indicating that keeping these values is
necessary because there's cross-test interference (i.e. sample counts don't
reset). Also, for robustness I think it's better to make these member variables
of the test fixture and just run UpdateHistogramSamples once in SetUp() to
record the initial values when the test starts.

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service_unittest.cc:225: 
nit: remove trailing newline.

[Mark P, 2013-08-20 14:14:57]

The histograms.xml file is not up to date.  For instance, it references enums
that does not exist.  Please tell me when it's ready for review.

--mark

[bbudge, 2013-08-20 14:46:42]

On 2013/08/20 14:14:57, Mark P (likely away 8.26-9.15) wrote:
> The histograms.xml file is not up to date.  For instance, it references enums
> that does not exist.  Please tell me when it's ready for review.
> 
> --mark

I've re-synced and uploaded a new patch. Does this fix it?

[Mark P, 2013-08-20 14:56:18]

https://codereview.chromium.org/22676002/diff/101001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22676002/diff/101001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13478: +    profile is loaded.
How is "not present" different from "unknown"?

Note: in common usage, "removed" is not the same thing as "not present".  i.e.,
histogram name and description do not jibe

https://codereview.chromium.org/22676002/diff/101001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13484: +    The number of times
preferences are checked when a profile is loaded.
Can we rephrase this to be analogous to the other histograms?  Perhaps
The id of a preference whose value is checked when a profile is loaded.

[bbudge, 2013-08-20 18:17:50]

I think I addressed everything.
We are still tracking down why a browser_test (ReOpenedWithID) is failing.

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/46001/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:15: #include
"chrome/browser/extensions/api/music_manager_private/device_id.h"
On 2013/08/20 12:53:54, Mattias Nissler wrote:
> On 2013/08/15 17:33:49, bbudge1 wrote:
> > On 2013/08/15 10:14:51, Mattias Nissler wrote:
> > > I'd consider this a layering violation. chrome/browser/prefs/ should
> > definitely
> > > not depend on extensions.
> > 
> > This appears to be the only way to get a unique per-device id. Perhaps
another
> > CL is needed to expose this more widely?
> 
> That'd be appreciated. Can you file a bug and put a TODO?

Done.

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.cc:37: const char
kProfilePreferenceHashes[] = "profile.preference_hashes";
On 2013/08/20 12:53:54, Mattias Nissler wrote:
> This should be in pref_names.{h,cc}

Done. I reworked the comment and moved it in front of CheckTrackedPreferences to
explain the algorithm.

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service.h (right):

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service.h:11: #include
"base/gtest_prod_util.h"
On 2013/08/20 12:53:54, Mattias Nissler wrote:
> Not used?

Done.

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_metrics_service_unittest.cc (right):

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service_unittest.cc:20: const char
kProfilePreferenceHashes[] = "profile.preference_hashes";
On 2013/08/20 12:53:54, Mattias Nissler wrote:
> Use the constants defined in pref_names.{h,cc} instead.

Done.

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service_unittest.cc:22: static int
preferences_checked_total;
On 2013/08/20 12:53:54, Mattias Nissler wrote:
> These should probably have a comment indicating that keeping these values is
> necessary because there's cross-test interference (i.e. sample counts don't
> reset). Also, for robustness I think it's better to make these member
variables
> of the test fixture and just run UpdateHistogramSamples once in SetUp() to
> record the initial values when the test starts.

Added a comment, made them member variables, and added a call in Setup(). Done.

https://codereview.chromium.org/22676002/diff/55002/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_metrics_service_unittest.cc:225: 
On 2013/08/20 12:53:54, Mattias Nissler wrote:
> nit: remove trailing newline.

Done.

https://codereview.chromium.org/22676002/diff/101001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22676002/diff/101001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13478: +    profile is loaded.
On 2013/08/20 14:56:18, Mark P (likely away 8.26-9.15) wrote:
> How is "not present" different from "unknown"?
> 
> Note: in common usage, "removed" is not the same thing as "not present". 
i.e.,
> histogram name and description do not jibe

I think the name is good, so rewrote the comment. Done.

https://codereview.chromium.org/22676002/diff/101001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13484: +    The number of times
preferences are checked when a profile is loaded.
On 2013/08/20 14:56:18, Mark P (likely away 8.26-9.15) wrote:
> Can we rephrase this to be analogous to the other histograms?  Perhaps
> The id of a preference whose value is checked when a profile is loaded.

This enum is a boolean, to give us a normalizing value for the other events. It
records the number of times we check all preferences in
PrefMetricsService::CheckTrackedPreferences().

I reworked the comment to be a little clearer.

[Mark P, 2013-08-20 19:26:25]

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13473: +    been removed from local
state.
This implies that every time something is emitted to the removed histogram,
it'll also be emitted to this histogram.  Is that true?

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13479: +    The id of a preference that
has been removed outside of Chrome when a
please be consistent and either use "tracked preference" or just "preference" in
all these descriptions.  Otherwise someone may wonder if there's a difference.

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13486: +    The number of times all
tracked preferences are checked when a profile is
nit: does "all" add anything here?  Is it possible that sometimes only some but
not all tracked preferences are checked?

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13488: +    samples.
nit: histogram samples -> histograms

[bbudge, 2013-08-20 20:43:34]

TrackedPreferenceRemoved histogram renamed, TrackedPreferenceCleared.
Removed the TrackedPreferencesChecked boolean histogram and added a new
TrackedPreferenceUnchanged histogram, which should better normalize the other
histograms.

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13473: +    been removed from local
state.
On 2013/08/20 19:26:25, Mark P (likely away 8.26-9.15) wrote:
> This implies that every time something is emitted to the removed histogram,
> it'll also be emitted to this histogram.  Is that true?

"removed" here means the hashed value from local state. It's confusing so I
tried rewording it again.

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13479: +    The id of a preference that
has been removed outside of Chrome when a
On 2013/08/20 19:26:25, Mark P (likely away 8.26-9.15) wrote:
> please be consistent and either use "tracked preference" or just "preference"
in
> all these descriptions.  Otherwise someone may wonder if there's a difference.

Done. Note that I've changed this to Settings.TrackedPreferenceCleared.

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13486: +    The number of times all
tracked preferences are checked when a profile is
On 2013/08/20 19:26:25, Mark P (likely away 8.26-9.15) wrote:
> nit: does "all" add anything here?  Is it possible that sometimes only some
but
> not all tracked preferences are checked?

Actually, this histogram isn't quite right, since it was counting startups. I've
changed it to be like the other histograms and track how many times each pref is
"unchanged". (tip of the hat to Erik Kay)

https://codereview.chromium.org/22676002/diff/107001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:13488: +    samples.
On 2013/08/20 19:26:25, Mark P (likely away 8.26-9.15) wrote:
> nit: histogram samples -> histograms

Done.

[bbudge, 2013-08-20 20:46:04]

NOTE: I also added "prefs::kGoogleServicesLastUsername" to the list of tracked
preferences. see crbug.com/276152

[bbudge, 2013-08-21 17:44:33]

I'm making PrefMetricsService work with existing browser_tests, which may stress
shutdown behavior and which don't always register real user prefs.

I also added more prefs to track, and may add more as they are requested.

We still need to use a private key in the hash. I won't land this until that is
in place, as it will change all the hash values, causing histogram anomalies.

[Mark P, 2013-08-21 22:27:03]

At this point, histograms.xml lgtm

[battre, 2013-09-02 08:33:46]

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:267: if (BrowserShuttingDown())
Don't you want to move this down so that it becomes:

if (!BrowserShuttingDown()) {
  UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
      i, tracked_pref_path_count_);
} else {
  UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChangedInShutdown", i,
...);
}
?

Alternatively, you could put this into
PrefMetricsService::UpdateTrackedPreference.

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:281: bool unchanged = true;
how about removing one negation and calling it changed?

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:360: serializer.Serialize(*value);
This is pretty brittle. If somebody modifies the serialization it will trigger a
spike in the UMA. Currently, the code is deterministic because dictionaries are
std::maps instead of std::hash_maps that don't preserve order. This could change
for example. Or whitespacing could change. Do you want to store concrete values
in the unit tests that trigger in these cases?

[bbudge, 2013-09-04 15:00:09]

Addressed comments. PTAL

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:267: if (BrowserShuttingDown())
I added these guards to get some browser_tests to pass. Since I modified
PrefMetricsService to not check for pref tampering during testing, I don't think
I need these two guard if's any more so I'm removing them.

Do you think it's important to detect pref changes at shutdown?

On 2013/09/02 08:33:46, battre wrote:
> Don't you want to move this down so that it becomes:
> 
> if (!BrowserShuttingDown()) {
>   UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChanged",
>       i, tracked_pref_path_count_);
> } else {
>   UMA_HISTOGRAM_ENUMERATION("Settings.TrackedPreferenceChangedInShutdown", i,
> ...);
> }
> ?
> 
> Alternatively, you could put this into
> PrefMetricsService::UpdateTrackedPreference.

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:281: bool unchanged = true;
On 2013/09/02 08:33:46, battre wrote:
> how about removing one negation and calling it changed?

Done.

https://codereview.chromium.org/22676002/diff/160001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:360: serializer.Serialize(*value);
On 2013/09/02 08:33:46, battre wrote:
> This is pretty brittle. If somebody modifies the serialization it will trigger
a
> spike in the UMA. Currently, the code is deterministic because dictionaries
are
> std::maps instead of std::hash_maps that don't preserve order. This could
change
> for example. Or whitespacing could change. Do you want to store concrete
values
> in the unit tests that trigger in these cases?

Good point. I added some test code to protect against this.

[bbudge, 2013-09-05 17:29:30]

battre@ Ping

[battre, 2013-09-06 11:35:32]

Sorry, I found some more issues.

Please feel free to find a reviewer in your time zone or ping me - your choice.
If you get a local reviewer, this LGTM.

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:71: profile_name_ =
profile_->GetProfileName();
This should probably use profile_->GetPath() because the name can be changed by
the user.

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:87:
extensions::api::DeviceId::GetDeviceId(
This can deliver a synchronous callback at a time when the constructor did not
finish execution. This might cause some problems.

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:325: }
A value is created on demand by DictionaryPrefUpdate. If you like, you could
simplify lines 321..324 to

update->SetString(..., ...)

Same applies below.

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service_unittest.cc (right):

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service_unittest.cc:263: dict.Set("d", new
StringValue("bad"));
Please move this line 3 lines up to confirm that the insertion order does not
matter.

Ii would also add a list and some other values that may break the hash values if
their serialization changes.

[bbudge, 2013-09-06 13:31:33]

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service.cc (right):

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:71: profile_name_ =
profile_->GetProfileName();
On 2013/09/06 11:35:33, battre wrote:
> This should probably use profile_->GetPath() because the name can be changed
by
> the user.

Done.

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:87:
extensions::api::DeviceId::GetDeviceId(
On 2013/09/06 11:35:33, battre wrote:
> This can deliver a synchronous callback at a time when the constructor did not
> finish execution. This might cause some problems.

I moved this to the end of the ctor, with a comment. I don't see any
Initialization method that I can override and that the instantiating code will
call. Since this class isn't ref-counted, and all fields are initialized at this
point, I don't see any danger. I added a comment to keep the code near the end.

I wonder if we might get pref change notifications before the device id is
initialized. That would cause false change UMAs.

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service.cc:325: }
On 2013/09/06 11:35:33, battre wrote:
> A value is created on demand by DictionaryPrefUpdate. If you like, you could
> simplify lines 321..324 to
> 
> update->SetString(..., ...)
> 
> Same applies below.

Nice. Done.

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_metrics_service_unittest.cc (right):

https://codereview.chromium.org/22676002/diff/175001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_metrics_service_unittest.cc:263: dict.Set("d", new
StringValue("bad"));
On 2013/09/06 11:35:33, battre wrote:
> Please move this line 3 lines up to confirm that the insertion order does not
> matter.
> 
> Ii would also add a list and some other values that may break the hash values
if
> their serialization changes.

Done.

[bbudge, 2013-09-06 16:44:05]

+pam for optional review. I think it's ready to land.

[commit-bot: I haz the power, 2013-09-06 23:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/189001

[commit-bot: I haz the power, 2013-09-06 23:19:37]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bbudge, 2013-09-06 23:25:29]

-Pam
+John for browser_resources.grd

[jam, 2013-09-06 23:28:12]

On 2013/09/06 23:25:29, bbudge1 wrote:
> -Pam
> +John for browser_resources.grd

[jam, 2013-09-06 23:28:19]

On 2013/09/06 23:25:29, bbudge1 wrote:
> -Pam
> +John for browser_resources.grd

lgtm

[commit-bot: I haz the power, 2013-09-06 23:31:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/189001

[commit-bot: I haz the power, 2013-09-06 23:49:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-07 00:04:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/213001

[commit-bot: I haz the power, 2013-09-07 00:45:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-07 00:45:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/231001

[commit-bot: I haz the power, 2013-09-07 06:39:35]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-09-07 14:29:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/259001

[commit-bot: I haz the power, 2013-09-07 15:18:39]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-09-07 16:28:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/259001

[commit-bot: I haz the power, 2013-09-07 17:28:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/270001

[commit-bot: I haz the power, 2013-09-07 17:41:52]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-07 18:48:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/22676002/270001

[commit-bot: I haz the power, 2013-09-08 03:37:34]

Change committed as 221944