Eliminate CHECK from CryptohomeLibrary::LoadSystemSalt

chrome/browser/renderer_host/pepper/device_id_fetcher.cc

 // Copyright (c) 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/renderer_host/pepper/device_id_fetcher.h"
 
 #include "base/file_util.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 18 matching lines @@
 using content::RenderProcessHost;
 
 namespace chrome {
 
 namespace {
 
 const char kDRMIdentifierFile[] = "Pepper DRM ID.0";
 
 const uint32_t kSaltLength = 32;
 
+void GetMachineIdAsync(const DeviceIDFetcher::IDCallback& callback) {
+  std::string result;
+#if defined(OS_WIN) && defined(ENABLE_RLZ)
+  rlz_lib::GetMachineId(&result);
+#elif defined(OS_CHROMEOS)
+  result = chromeos::CryptohomeLibrary::Get()->GetSystemSalt();
+  if (result.empty()) {
+    // cryptohome must not be running; re-request after a delay.
+    const int64 kRequestStstemSaltDelayMs = 500;

[raymes, 2013/09/16 22:13:38]

-Ststem -> System
-Seems ok to retry but how did you decide upon the delay? Are there other
options you considered?

[stevenjb, 2013/09/16 22:32:48]

The delay is designed to be long enough not to spam the log with failed
attempts, but short enough not to trigger any timeouts. Unfortunately we do not
currently have an observer mechanism to inform the class when cryptohome has
started and the system salt is ready.

+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&GetMachineIdAsync, callback),
+        base::TimeDelta::FromMilliseconds(kRequestStstemSaltDelayMs));
+    return;
+  }
+#else
+  // Not implemented for other platforms.
+  NOTREACHED();
+#endif
+  callback.Run(result);
+}
+
 }  // namespace
 
 DeviceIDFetcher::DeviceIDFetcher(int render_process_id)
     : in_progress_(false),
-      render_process_id_(render_process_id) {
+      render_process_id_(render_process_id),
+      weak_ptr_factory_(this) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 }
 
 DeviceIDFetcher::~DeviceIDFetcher() {
 }
 
 bool DeviceIDFetcher::Start(const IDCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   if (in_progress_)
@@ skipping 13 matching lines @@
     user_prefs::PrefRegistrySyncable* prefs) {
   prefs->RegisterBooleanPref(prefs::kEnableDRM,
                              true,
                              user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
   prefs->RegisterStringPref(
       prefs::kDRMSalt,
       "",
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
-// static
-base::FilePath DeviceIDFetcher::GetLegacyDeviceIDPath(
-    const base::FilePath& profile_path) {
-  return profile_path.AppendASCII(kDRMIdentifierFile);
-}
-
-// TODO(raymes): Change this to just return the device id salt and call it with
-// PostTaskAndReply once the legacy ChromeOS codepath is removed.
 void DeviceIDFetcher::CheckPrefsOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   Profile* profile = NULL;
   RenderProcessHost* render_process_host =
       RenderProcessHost::FromID(render_process_id_);
   if (render_process_host && render_process_host->GetBrowserContext()) {
     profile = Profile::FromBrowserContext(
         render_process_host->GetBrowserContext());
   }
 
   if (!profile ||
       profile->IsOffTheRecord() ||
       !profile->GetPrefs()->GetBoolean(prefs::kEnableDRM)) {
     RunCallbackOnIOThread(std::string());
     return;
   }
 
   // Check if the salt pref is set. If it isn't, set it.
   std::string salt = profile->GetPrefs()->GetString(prefs::kDRMSalt);
   if (salt.empty()) {
     uint8_t salt_bytes[kSaltLength];
     crypto::RandBytes(salt_bytes, arraysize(salt_bytes));
     // Since it will be stored in a string pref, convert it to hex.
     salt = base::HexEncode(salt_bytes, arraysize(salt_bytes));
     profile->GetPrefs()->SetString(prefs::kDRMSalt, salt);
   }
 
-#if defined(OS_CHROMEOS)
-  // Try the legacy path first for ChromeOS. We pass the new salt in as well
-  // in case the legacy id doesn't exist.

[stevenjb, 2013/09/16 21:54:12]

Since Chrome and ChromeOS are synced, I believe that it should be safe to remove
this now?

[raymes, 2013/09/16 22:13:38]

Unfortunately I think we have to keep this. If someone using ChromeOS was using
the device ID code prior to when it was changed, they may have a device ID that
was computed in a different way. We need to add some uma stats to decide when we
can remove it.

[stevenjb, 2013/09/16 22:32:48]

Ugh, you man this isn't something we can automatically transition from? That is
very unfortunate, it means we have to maintain this code for quite a while. In
the meanwhile I will go ahead and this without eliminating the legacy code then.

It would be helpful to file a tracking issue with details on what the criteria
for removing this will be, and adding that to the comments.

[raymes, 2013/09/18 23:21:20]

Not easily. There's a bug filed at
https://code.google.com/p/chromium/issues/detail?id=234411

-  BrowserThread::PostBlockingPoolTask(FROM_HERE,
-      base::Bind(&DeviceIDFetcher::ComputeOnBlockingPool, this,
-                 profile->GetPath(), salt));
-#else
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&DeviceIDFetcher::ComputeOnIOThread, this, salt));
-#endif
-}
-
-
-void DeviceIDFetcher::ComputeOnIOThread(const std::string& salt) {
   std::vector<uint8> salt_bytes;
   if (!base::HexStringToBytes(salt, &salt_bytes))
     salt_bytes.clear();
-
-  // Build the identifier as follows:
-  // SHA256(machine-id||service||SHA256(machine-id||service||salt))
-  std::string machine_id = GetMachineID();

[stevenjb, 2013/09/16 21:54:12]

Calling this on the IO thread was actually incorrect. There should be no need at
this point to call anything here from the IO thread.

[raymes, 2013/09/16 22:13:38]

Thanks for catching this. That makes sense :)

-  if (machine_id.empty() || salt_bytes.size() != kSaltLength) {
-    NOTREACHED();
+  if (salt_bytes.size() != kSaltLength) {
+    LOG(ERROR) << "Unexpected salt bytes length: " << salt_bytes.size();
     RunCallbackOnIOThread(std::string());
     return;
   }
 
+  GetMachineIdAsync(
+      base::Bind(&DeviceIDFetcher::ComputeOnUIThread,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 salt_bytes));
+}
+
+void DeviceIDFetcher::ComputeOnUIThread(const std::vector<uint8>& salt_bytes,
+                                        const std::string& machine_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (machine_id.empty()) {
+    LOG(ERROR) << "Empty machine id";
+    RunCallbackOnIOThread(std::string());
+    return;
+  }
+
+  // Build the identifier as follows:
+  // SHA256(machine-id||service||SHA256(machine-id||service||salt))
   char id_buf[256 / 8];  // 256-bits for SHA256
   std::string input = machine_id;
   input.append(kDRMIdentifierFile);
   input.append(salt_bytes.begin(), salt_bytes.end());
   crypto::SHA256HashString(input, &id_buf, sizeof(id_buf));
   std::string id = StringToLowerASCII(
       base::HexEncode(reinterpret_cast<const void*>(id_buf), sizeof(id_buf)));
   input = machine_id;
   input.append(kDRMIdentifierFile);
   input.append(id);
   crypto::SHA256HashString(input, &id_buf, sizeof(id_buf));
   id = StringToLowerASCII(base::HexEncode(
         reinterpret_cast<const void*>(id_buf),
         sizeof(id_buf)));
 
   RunCallbackOnIOThread(id);
 }
 
-// TODO(raymes): This is temporary code to migrate ChromeOS devices to the new
-// scheme for generating device IDs. Delete this once we are sure most ChromeOS
-// devices have been migrated.
-void DeviceIDFetcher::ComputeOnBlockingPool(const base::FilePath& profile_path,
-                                            const std::string& salt) {
-  std::string id;
-  // First check if the legacy device ID file exists on ChromeOS. If it does, we
-  // should just return that.
-  base::FilePath id_path = GetLegacyDeviceIDPath(profile_path);
-  if (base::PathExists(id_path)) {
-    if (base::ReadFileToString(id_path, &id) && !id.empty()) {
-      RunCallbackOnIOThread(id);
-      return;
-    }
-  }
-  // If we didn't find an ID, go back to the new code path to generate an ID.
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&DeviceIDFetcher::ComputeOnIOThread, this, salt));
-}
-
-
 void DeviceIDFetcher::RunCallbackOnIOThread(const std::string& id) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&DeviceIDFetcher::RunCallbackOnIOThread, this, id));
     return;
   }
   in_progress_ = false;
   callback_.Run(id);
 }
 
-std::string DeviceIDFetcher::GetMachineID() {
-#if defined(OS_WIN) && defined(ENABLE_RLZ)
-  std::string result;
-  rlz_lib::GetMachineId(&result);
-  return result;
-#elif defined(OS_CHROMEOS)
-  chromeos::CryptohomeLibrary* c_home = chromeos::CryptohomeLibrary::Get();
-  return c_home->GetSystemSalt();
-#else
-  // Not implemented for other platforms.
-  NOTREACHED();
-  return "";
-#endif
-}
-
 }  // namespace chrome