Eliminate CHECK from CryptohomeLibrary::LoadSystemSalt

chrome/browser/renderer_host/pepper/device_id_fetcher.cc

Index: chrome/browser/renderer_host/pepper/device_id_fetcher.cc
diff --git a/chrome/browser/renderer_host/pepper/device_id_fetcher.cc b/chrome/browser/renderer_host/pepper/device_id_fetcher.cc
index 585d62d19867b1f376748584d5f65a52931cc22b..49a7dda007cc4f73c5455fb24af4a24fcf4aa1a8 100644
--- a/chrome/browser/renderer_host/pepper/device_id_fetcher.cc
+++ b/chrome/browser/renderer_host/pepper/device_id_fetcher.cc
@@ -36,11 +36,34 @@ 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));
 }
 
@@ -74,14 +97,6 @@ void DeviceIDFetcher::RegisterProfilePrefs(
       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));
 
@@ -110,34 +125,33 @@ void DeviceIDFetcher::CheckPrefsOnUIThread() {
     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();
+  if (salt_bytes.size() != kSaltLength) {
+    LOG(ERROR) << "Unexpected salt bytes length: " << salt_bytes.size();
+    RunCallbackOnIOThread(std::string());
+    return;
+  }
 
-  // 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();
+  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);
@@ -156,28 +170,6 @@ void DeviceIDFetcher::ComputeOnIOThread(const std::string& salt) {
   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(
@@ -189,19 +181,4 @@ void DeviceIDFetcher::RunCallbackOnIOThread(const std::string& id) {
   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