Fix the hash generation algorithm to be consistent with prior implementation.

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
index 4a787a8a1abb699d8a3447c3a34caf3eee638bb8..356502c205b7f15ac61b1fd77ba7331c60babc01 100644
--- a/chrome/browser/prefs/pref_hash_calculator.cc
+++ b/chrome/browser/prefs/pref_hash_calculator.cc
@@ -16,6 +16,17 @@
 
 namespace {
 
+// Calculates an HMAC of |message| using |key|, encoded as a hexadecimal string.
+std::string GetHMACHex(const std::string& key, const std::string& message) {
+  crypto::HMAC hmac(crypto::HMAC::SHA256);
+  std::vector<uint8> digest(hmac.DigestLength());

[gab, 2014/01/07 19:18:07]

This is nicer than the previous method hardcoding the digest size, etc., but is
it required for this CL?

I would like to keep cleanup tasks separate and do them only once we have
confirmed that we are back to a working state and are confident that we have
tests making sure future changes do not break existing hashes.

[erikwright (departed), 2014/01/08 22:11:24]

We now have two HMAC calculations. They are virtually identical and rather than
have duplicate code it made sense to factor this out. It's not really a cleanup,
it's the right way to add this functionality.

[gab, 2014/01/08 23:48:20]

Of course, my bad, I'd overlooked this initially.

+  if (!hmac.Init(key) || !hmac.Sign(message, &digest[0], digest.size())) {

[Bernhard Bauer, 2014/01/07 19:20:45]

When does this fail? Is it really necessary to handle the DCHECK failure in
production code?

[erikwright (departed), 2014/01/08 22:11:24]

It should presumably never fail, but it's inappropriate to ignore the result of
these methods. To me that's the definition of NOTREACHED. In release,
NOTREACHED() is nothing.

I think this is quite common in Chromium (a NOTREACHED that really will never be
reached in production code). See for example:

https://code.google.com/p/chromium/codesearch#chromium/src/crypto/hmac.cc&q=N...

[Bernhard Bauer, 2014/01/09 08:51:35]

Yes, but in production the code will look like this:

  if (<signing fails>) {
    return std::string();
  }

Basically, you're handling failure of a condition that can never fail, which is
something you're not supposed to do (see
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-).
What you can do instead is put the result into a boolean variable, then DCHECK
that. 

+    NOTREACHED();
+    return std::string();
+  }
+  return base::HexEncode(digest.data(), digest.size());
+}
+
 // 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) {
@@ -42,29 +53,40 @@ 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);
+  return GetHMACHex(seed, message);
+}
 
-  crypto::HMAC hmac(crypto::HMAC::SHA256);
-  unsigned char digest[kSHA256DigestSize];
-  if (!hmac.Init(seed) || !hmac.Sign(message, digest, arraysize(digest))) {
-    NOTREACHED();
-    return std::string();
-  }
+// Calculate a hash using a deprecated hash algorithm. For validating old
+// hashes during migration.
+std::string CalculateLegacyHash(
+    const std::string& seed, const base::Value* value) {
+  return CalculateFromValueAndComponents(
+      seed, value, std::vector<std::string>());
+}
 
-  return base::HexEncode(digest, arraysize(digest));
+// Generates a device ID based on the input device ID. The derived device ID has
+// no useful properties beyond those of the input device ID except that it is
+// consistent with previous implementations.
+std::string GenerateDeviceIdLikePrefMetricsServiceDid(
+    const std::string& original_device_id) {
+  if (original_device_id.empty())
+    return std::string();
+  return StringToLowerASCII(
+      GetHMACHex(original_device_id, "PrefMetricsService"));
 }
 
 }  // namespace
 
 PrefHashCalculator::PrefHashCalculator(const std::string& seed,
                                        const std::string& device_id)
-    : seed_(seed), device_id_(device_id) {}
+    : seed_(seed),
+      device_id_(GenerateDeviceIdLikePrefMetricsServiceDid(device_id)) {}
 
 std::string PrefHashCalculator::Calculate(const std::string& path,
                                           const base::Value* value) const {
   std::vector<std::string> components;
+
   if (!device_id_.empty())
     components.push_back(device_id_);
   components.push_back(path);
@@ -77,14 +99,7 @@ PrefHashCalculator::ValidationResult PrefHashCalculator::Validate(
     const std::string& hash) const {
   if (hash == Calculate(path, value))
     return VALID;
-  if (hash == CalculateLegacyHash(path, value))
+  if (hash == CalculateLegacyHash(seed_, value))
     return VALID_LEGACY;
   return INVALID;
 }
-
-std::string PrefHashCalculator::CalculateLegacyHash(
-    const std::string& path, const base::Value* value) const {

[gab, 2014/01/07 19:18:07]

Why move this? The move doesn't appear to be related to the problem at hand. I
would leave it here (although it appears the |path| arg isn't required, feel
free to clean that up) at least for now.

-  return CalculateFromValueAndComponents(seed_,
-                                         value,
-                                         std::vector<std::string>());
-}