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..4eb5f93875cde8aa620691cc7d1a672283a9685a 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;

[hashimoto, 2013/09/18 05:53:19]

nit: s/Ststem/System/?

[stevenjb, 2013/09/18 19:23:36]

Done.

+    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) {

[hashimoto, 2013/09/18 05:53:19]

DeviceIDFetcher is ref counted. Is threre any reason to bind WeakPtr, instead of
scoped_refptr, to callbacks?

[stevenjb, 2013/09/18 19:23:36]

Facepalm. Thanks. I don't know how I missed that. I've gotten too used to using
WeakPtr. Reverted.

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 }
 
@@ -80,8 +103,6 @@ base::FilePath DeviceIDFetcher::GetLegacyDeviceIDPath(
   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));
 
@@ -113,27 +134,36 @@ void DeviceIDFetcher::CheckPrefsOnUIThread() {
 #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.
-  BrowserThread::PostBlockingPoolTask(FROM_HERE,
-      base::Bind(&DeviceIDFetcher::ComputeOnBlockingPool, this,
+  BrowserThread::PostBlockingPoolTask(
+      FROM_HERE,
+      base::Bind(&DeviceIDFetcher::LegacyComputeOnBlockingPool,
+                 weak_ptr_factory_.GetWeakPtr(),
                  profile->GetPath(), salt));
 #else
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&DeviceIDFetcher::ComputeOnIOThread, this, salt));
+  // Get the machine ID and call ComputeOnUIThread with salt + machine_id.
+  GetMachineIdAsync(base::Bind(&DeviceIDFetcher::ComputeOnUIThread,
+                               weak_ptr_factory_.GetWeakPtr(),
+                               salt));
 #endif
 }
 
+void DeviceIDFetcher::ComputeOnUIThread(const std::string& salt,
+                                        const std::string& machine_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-void DeviceIDFetcher::ComputeOnIOThread(const std::string& salt) {
-  std::vector<uint8> salt_bytes;
-  if (!base::HexStringToBytes(salt, &salt_bytes))
-    salt_bytes.clear();
+  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))
-  std::string machine_id = GetMachineID();
-  if (machine_id.empty() || salt_bytes.size() != kSaltLength) {
-    NOTREACHED();
+  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;
   }
@@ -159,25 +189,36 @@ void DeviceIDFetcher::ComputeOnIOThread(const std::string& salt) {
 // 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) {
+// static
+void DeviceIDFetcher::LegacyComputeOnBlockingPool(
+    base::WeakPtr<DeviceIDFetcher> fetcher,
+    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;
-    }
+  base::FilePath id_path =
+      DeviceIDFetcher::GetLegacyDeviceIDPath(profile_path);
+  if (base::PathExists(id_path) &&
+      base::ReadFileToString(id_path, &id) &&
+      !id.empty()) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&DeviceIDFetcher::RunCallbackOnIOThread,
+                   fetcher,
+                   id));
+    return;
   }
-  // If we didn't find an ID, go back to the new code path to generate an ID.
+  // If we didn't find an ID, get the machine Id and call the new code path to
+  // generate an ID.
   BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&DeviceIDFetcher::ComputeOnIOThread, this, salt));
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&GetMachineIdAsync,
+                 base::Bind(&DeviceIDFetcher::ComputeOnUIThread,
+                            fetcher,
+                            salt)));
 }
 
-
 void DeviceIDFetcher::RunCallbackOnIOThread(const std::string& id) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
     BrowserThread::PostTask(
@@ -189,19 +230,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