Fix a race condition in preference metric reporting.

chrome/browser/prefs/pref_hash_store_impl.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/prefs/pref_hash_store_impl.h"
+
+#include "base/logging.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+#include "base/prefs/scoped_user_pref_update.h"
+#include "base/values.h"
+#include "chrome/common/pref_names.h"
+
+PrefHashStoreImpl::PrefHashStoreImpl(const PrefHashCalculator& hash_calculator,
+                                     PrefService* local_state,
+                                     const std::string& hash_store_id)
+    : local_state_(local_state),
+      hash_store_id_(hash_store_id),
+      pref_hash_calculator_(hash_calculator) {}
+
+// static
+void PrefHashStoreImpl::RegisterPrefs(PrefRegistrySimple* registry) {
+  // Register the top level dictionary to map profile names to dictionaries of
+  // tracked preferences.
+  registry->RegisterDictionaryPref(prefs::kProfilePreferenceHashes);
+}
+
+PrefHashStore::ValueState PrefHashStoreImpl::CheckValue(
+    const std::string& path, const base::Value* initial_value) {
+  const base::DictionaryValue* pref_hash_dicts =
+      local_state_->GetDictionary(prefs::kProfilePreferenceHashes);
+  const base::DictionaryValue* hashed_prefs = NULL;
+  pref_hash_dicts->GetDictionaryWithoutPathExpansion(hash_store_id_,
+                                                     &hashed_prefs);
+
+  std::string last_hash;
+  if (!hashed_prefs || !hashed_prefs->GetString(path, &last_hash))
+    return PrefHashStore::UNKNOWN_VALUE;
+
+  PrefHashCalculator::ValidationResult validation_result =
+      pref_hash_calculator_.Validate(path, initial_value, last_hash);
+  switch (validation_result) {
+    case PrefHashCalculator::VALID:
+      return PrefHashStore::UNCHANGED;
+    case PrefHashCalculator::VALID_LEGACY:
+      return PrefHashStore::MIGRATED;
+    case PrefHashCalculator::INVALID:
+      return initial_value ? PrefHashStore::CHANGED : PrefHashStore::CLEARED;
+    default:

[gab, 2013/12/06 17:23:38]

Remove the default block; you're already handling all enum values above and not
having a default value will force anyone adding a new
PrefHashCalculator::ValidationResult to change this switch to compile (whereas a
default block wouldn't).

If VS2010 isn't smart enough to compile this (i.e. complains about a path with
no return), I suggest having the NOTREACHED() at the end of the method outside
of this switch.

[erikwright (departed), 2013/12/06 19:00:47]

It's impossible for the compiler to assert that there is no path where
validation_result will not equal one of those 'case' covered values. Remember
that at runtime the enum is really just an int, it could have any value at all.

There will either be a NOTREACHED in the default or following the switch
statement. The current structure protects against two things:

(1) Each case must have a return value. Else a path without a return.
(2) NOTREACHED if someone adds a new enum and forgets to update this.

The NOTREACHED has a very clear comment that is guaranteed to be correct.
Whereas if we put it at the bottom, and then someone forgets to add a return
statement in one of the cases, the log message will be misleading.

[gab, 2013/12/06 23:22:38]

We clarified this offline already, but for the sake of the public discussion I
just stumbled upon an example of what I'm suggesting:
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_serv...

This has the benefit of forcing the switch to handle each possible enum case (or
gcc/clang will warn on compile -- VS2010 doesn't do it unfortunately).

[erikwright (departed), 2013/12/09 17:59:40]

Done.

+      NOTREACHED() << "Unexpected PrefHashCalculator::ValidationResult: "
+                   << validation_result;
+      return PrefHashStore::UNKNOWN_VALUE;
+  }
+}
+
+void PrefHashStoreImpl::StoreValue(
+    const std::string& path, const base::Value* new_value) {
+  DictionaryPrefUpdate update(local_state_, prefs::kProfilePreferenceHashes);
+  DictionaryValue* child_dictionary = NULL;
+
+  // Get the dictionary corresponding to the profile name,
+  // which may have a '.'

[gab, 2013/12/06 17:23:38]

Feels like more of this comment can fit on the first line.

+  if (!update->GetDictionaryWithoutPathExpansion(hash_store_id_,
+                                                 &child_dictionary)) {
+    child_dictionary = new DictionaryValue;
+    update->SetWithoutPathExpansion(hash_store_id_, child_dictionary);
+  }
+
+  child_dictionary->SetString(
+      path, pref_hash_calculator_.Calculate(path, new_value));
+}