Fix a race condition in preference metric reporting.

chrome/browser/prefs/pref_hash_calculator.cc

Index: chrome/browser/prefs/pref_hash_calculator.cc
diff --git a/chrome/browser/prefs/pref_hash_calculator.cc b/chrome/browser/prefs/pref_hash_calculator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..46b6c4663af7493d748610b189d6a132e4566711
--- /dev/null
+++ b/chrome/browser/prefs/pref_hash_calculator.cc
@@ -0,0 +1,137 @@
+// 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_calculator.h"
+
+#include <vector>
+
+#include "base/json/json_string_value_serializer.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/values.h"
+#include "crypto/hmac.h"
+
+#if defined (OS_WIN) && defined(ENABLE_RLZ)
+#include "rlz/lib/machine_id.h"
+#endif
+namespace {

[gab, 2013/11/27 23:43:27]

nit: +empty line above

+
+// Renders |value| as a string. |value| may be NULL, in which case the result
+// is an empty string.
+std::string ValueAsString(const base::Value* value) {
+  // Dictionary values may contain empty lists and sub-dictionaries. Make a
+  // deep copy with those removed to make the hash more stable.
+  const base::DictionaryValue* dict_value;
+  scoped_ptr<DictionaryValue> canonical_dict_value;
+  if (value && value->GetAsDictionary(&dict_value)) {
+    canonical_dict_value.reset(dict_value->DeepCopyWithoutEmptyChildren());
+    value = canonical_dict_value.get();
+  }
+
+  std::string value_as_string;
+  if (value) {
+    JSONStringValueSerializer serializer(&value_as_string);
+    serializer.Serialize(*value);
+  }
+
+  return value_as_string;
+}
+
+// Common helper for all hash algorithms.
+std::string CalculateFromValueAndComponents(
+    const std::string& seed,
+    const base::Value* value,
+    const std::vector<std::string>& extra_components) {
+  static const size_t kSHA256DigestSize = 32;
+
+  std::string message = JoinString(extra_components, "") + ValueAsString(value);
+
+  crypto::HMAC hmac(crypto::HMAC::SHA256);
+  unsigned char digest[kSHA256DigestSize];
+  if (!hmac.Init(seed) || !hmac.Sign(message, digest, arraysize(digest))) {
+    NOTREACHED();
+    return std::string();
+  }
+
+  return base::HexEncode(digest, arraysize(digest));
+}
+
+
+// Common signature for current and legacy hash algorithms.
+typedef std::string (CalculateFunctor)(const std::string& seed,
+                                       const std::string& path,
+                                       const base::Value* value);
+
+#if defined(OS_WIN) && defined(ENABLE_RLZ)
+// Current algorithm on Windows.
+std::string CalculateWithDeviceId(const std::string& seed,
+                                  const std::string& path,
+                                  const base::Value* value) {
+  std::vector<std::string> components;
+
+  // This is used by
+  // chrome/browser/extensions/api/music_manager_private/device_id_win.cc
+  // but that API is private and other platforms are not available
+  // synchronously.
+  // As part of improving pref metrics on other platforms we may want to find
+  // ways to defer preference loading until the device ID can be used.
+  components.resize(2);
+  rlz_lib::GetMachineId(&components[0]);
+  components[1] = path;
+  return CalculateFromValueAndComponents(seed, value, components);
+}
+#else
+// Current algorithm when device ID is not available synchronously.
+std::string CalculateWithPath(const std::string& seed,
+                              const std::string& path,
+                              const base::Value* value) {
+  std::vector<std::string> components;
+  components.insert(components.begin(), path);

[gab, 2013/11/27 23:43:27]

components.push_back(path); would be simpler here.

+  return CalculateFromValueAndComponents(seed, value, components);
+}
+#endif
+
+// Legacy algorithm.
+std::string CalculateWithValueOnly(const std::string& seed,
+                                   const std::string& path,
+                                   const base::Value* value) {
+  return CalculateFromValueAndComponents(seed,
+                                         value,
+                                         std::vector<std::string>());
+}
+
+// Put the "modern" calculator followed by all deprecated calculators from which
+// migration is supported.
+static CalculateFunctor* const kCalculators[] = {
+#if defined(OS_WIN) && defined(ENABLE_RLZ)
+  CalculateWithDeviceId,
+#else
+  CalculateWithPath,
+#endif
+  CalculateWithValueOnly
+};
+
+}  // namespace
+
+PrefHashCalculator::PrefHashCalculator(const std::string& seed) : seed_(seed) {}
+
+PrefHashCalculator::ValidationResult PrefHashCalculator::Validate(
+    const std::string& path,
+    const base::Value* value,
+    const std::string& hash) {
+  // Attempt validation with each of the supported algorithms.
+  for (size_t i = 0; i < arraysize(kCalculators); ++i) {
+    if (hash == kCalculators[i](seed_, path, value))
+      return i == 0 ? VALID : VALID_LEGACY;

[gab, 2013/11/27 23:43:27]

I love the CalculateFunctor approach, but it seems a bit weird to loop over
"all" yet have a binary return value based on if its the first one or second
one.

What about:

COMPILE_ASSERT(arraysize(kCalculators) == 2, unhandled_calculator);
if (hash == kCalculators[0](seed_, path, value))
  return VALID;
else if (hash == kCalculators[1](seed_, path, value))
  return VALID_LEGACY;

return INVALID;

----------------------

And someone can make this a loop one day if more calculators get added and it
makes sense then.

[erikwright (departed), 2013/11/28 17:48:07]

Actually already refactored this in the patchset that failed to upload.

+  }
+  return INVALID;
+}
+
+std::string PrefHashCalculator::Calculate(const std::string& path,
+                                          const base::Value* value) {
+  // Use the current (first listed) algorithm to calculate the hash.
+  return kCalculators[0](seed_, path, value);
+}