Retrieve client_id from GoogleUpdateSettings when its missing from Local State.

components/metrics/metrics_state_manager.cc

Index: components/metrics/metrics_state_manager.cc
diff --git a/components/metrics/metrics_state_manager.cc b/components/metrics/metrics_state_manager.cc
index 3b71976581b30d789d37c05ea2f0cb1d86bfa658..ac92a746f17cdf5fb0c073ee5f434ba0ab47c77a 100644
--- a/components/metrics/metrics_state_manager.cc
+++ b/components/metrics/metrics_state_manager.cc
@@ -45,9 +45,13 @@ bool MetricsStateManager::instance_exists_ = false;
 
 MetricsStateManager::MetricsStateManager(
     PrefService* local_state,
-    const base::Callback<bool(void)>& is_reporting_enabled_callback)
+    const base::Callback<bool(void)>& is_reporting_enabled_callback,
+    const StoreClientInfoCallback& store_client_info,
+    const LoadClientInfoCallback& retrieve_client_info)
     : local_state_(local_state),
       is_reporting_enabled_callback_(is_reporting_enabled_callback),
+      store_client_info_(store_client_info),
+      load_client_info_(retrieve_client_info),
       low_entropy_source_(kLowEntropySourceNotSet),
       entropy_source_returned_(ENTROPY_SOURCE_NONE) {
   ResetMetricsIDsIfNecessary();
@@ -72,9 +76,50 @@ void MetricsStateManager::ForceClientIdCreation() {
     return;
 
   client_id_ = local_state_->GetString(prefs::kMetricsClientID);
-  if (!client_id_.empty())
+  if (!client_id_.empty()) {
+    // It is technically sufficient to only save a backup of the client id when
+    // it is initially generated below, but since the backup was only introduced
+    // in M38, seed it explicitly from here for some time.
+    BackUpCurrentClientInfo();
+    return;
+  }
+
+  const scoped_ptr<ClientInfo> client_info_backup =
+      LoadClientInfoAndMaybeMigrate();
+  if (client_info_backup) {
+    client_id_ = client_info_backup->client_id;
+
+    const base::Time now(base::Time::Now());

[Alexei Svitkine (slow), 2014/07/15 12:53:25]

Nit: =

[gab, 2014/07/15 20:50:06]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

explicitly allows both.

In general I prefer to use () to init objects and = to init POD types/refs.

[Alexei Svitkine (slow), 2014/07/16 14:39:56]

base::Time is essentially a POD type, since it just wraps an int64. Anyway, I
know both are allowed, so in those cases the guideline is to be consistent with
surrounding code and/or follow the preference of the reviewer/owner (who will be
maintaining this code afterwards). In this case, my preference as owner/review
is to use =.

[gab, 2014/07/16 15:51:43]

As you wish, done.

+
+    // Save the recovered client id and also try to reinstantiate the backup
+    // values for the dates corresponding with that client id in order to avoid
+    // weird scenarios where we could report an old client id with a recent
+    // install date.
+    local_state_->SetString(prefs::kMetricsClientID, client_id_);
+    local_state_->SetInt64(prefs::kInstallDate,
+                           client_info_backup->installation_date ?

[Alexei Svitkine (slow), 2014/07/15 12:53:25]

Nit: != 0, since it's an int. Same below.

[gab, 2014/07/15 20:50:06]

0 is explicitly false in C++; I don't think there is a need to be more explicit
here, ints are first-class citizen in conditionals, there is no implicit
conversion to bool required to evaluate this statement (in fact !=0 would
probably generate more assembly).

[Alexei Svitkine (slow), 2014/07/16 14:39:56]

I am aware how C++ works, my argument is not about correctness but about
readability.

It's better to use != 0 checks for int types, since this avoids possible
confusion about what's being checked (e.g. whether its a NULL ptr check or an
integer check). That's why I (and many other Chromium reviewers) prefer that for
int types, that non-zero check is explicit.

(Also, your assertion that != 0 will generate worse machine code is unfounded.
All modern compilers are smart enough to generate the same code for both cases.)

[gab, 2014/07/16 15:51:43]

Apologies if it felt like I was putting your C++ knowledge in doubt; I was only
trying to justify that it doesn't matter... I have never seen such pushbacks
from other reviewers, but I'm happy to do as you wish here.

+                               client_info_backup->installation_date :
+                               now.ToTimeT());
+    local_state_->SetInt64(prefs::kMetricsReportingEnabledTimestamp,
+                           client_info_backup->reporting_enabled_date ?
+                               client_info_backup->reporting_enabled_date :
+                               now.ToTimeT());
+
+    base::TimeDelta recovered_installation_age;
+    if (client_info_backup->installation_date != 0) {
+      recovered_installation_age =
+          now - base::Time::FromTimeT(client_info_backup->installation_date);
+    }
+    UMA_HISTOGRAM_COUNTS_10000("UMA.ClientIdBackupRecoveredWithAge",
+                               recovered_installation_age.InHours());
+
+    // Flush the backup back to persistent storage in case we re-generated
+    // missing data above.
+    BackUpCurrentClientInfo();
     return;
+  }
 
+  // Failing attempts at getting an existing client ID, generate a new one.
   client_id_ = base::GenerateGUID();
   local_state_->SetString(prefs::kMetricsClientID, client_id_);
 
@@ -86,6 +131,8 @@ void MetricsStateManager::ForceClientIdCreation() {
     UMA_HISTOGRAM_BOOLEAN("UMA.ClientIdMigrated", true);
   }
   local_state_->ClearPref(prefs::kMetricsOldClientID);
+
+  BackUpCurrentClientInfo();
 }
 
 void MetricsStateManager::CheckForClonedInstall(
@@ -141,12 +188,16 @@ MetricsStateManager::CreateEntropyProvider() {
 // static
 scoped_ptr<MetricsStateManager> MetricsStateManager::Create(
     PrefService* local_state,
-    const base::Callback<bool(void)>& is_reporting_enabled_callback) {
+    const base::Callback<bool(void)>& is_reporting_enabled_callback,
+    const StoreClientInfoCallback& store_client_info,
+    const LoadClientInfoCallback& retrieve_client_info) {
   scoped_ptr<MetricsStateManager> result;
   // Note: |instance_exists_| is updated in the constructor and destructor.
   if (!instance_exists_) {
-    result.reset(
-        new MetricsStateManager(local_state, is_reporting_enabled_callback));
+    result.reset(new MetricsStateManager(local_state,
+                                         is_reporting_enabled_callback,
+                                         store_client_info,
+                                         retrieve_client_info));
   }
   return result.Pass();
 }
@@ -168,6 +219,49 @@ void MetricsStateManager::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterIntegerPref(prefs::kMetricsOldLowEntropySource, 0);
 }
 
+void MetricsStateManager::BackUpCurrentClientInfo() {
+  ClientInfo client_info;
+  client_info.client_id = client_id_;
+  client_info.installation_date = local_state_->GetInt64(prefs::kInstallDate);
+  client_info.reporting_enabled_date =
+      local_state_->GetInt64(prefs::kMetricsReportingEnabledTimestamp);
+  store_client_info_.Run(client_info);
+}
+
+scoped_ptr<ClientInfo> MetricsStateManager::LoadClientInfoAndMaybeMigrate() {
+  scoped_ptr<metrics::ClientInfo> client_info = load_client_info_.Run();
+
+  // Prior to 2014-07, the client ID was stripped of its dashes before being
+  // saved. Migrate back to a proper GUID if this is the case. This migration
+  // code can be removed in M41+.
+  const size_t kGUIDLengthWithoutDashes = 32U;
+  if (client_info &&
+      client_info->client_id.length() == kGUIDLengthWithoutDashes) {
+    DCHECK(client_info->client_id.find('-') == std::string::npos);

[Alexei Svitkine (slow), 2014/07/15 12:53:25]

Nit: DCHECK_EQ

[gab, 2014/07/15 20:50:06]

DCHECK_EQ is useful when the output of "actual"/"expected" is both:
 1) Useful
 2) Printable

In this case DCHECK_EQ won't even compile as the macro wouldn't know how to
"print" an iterator on failure.

[Alexei Svitkine (slow), 2014/07/16 14:39:56]

Fair enough.

+
+    std::string client_id_with_dashes;
+    client_id_with_dashes.reserve(kGUIDLengthWithoutDashes + 4U);
+    std::string::const_iterator client_id_it = client_info->client_id.begin();
+    for (size_t i = 0; i < kGUIDLengthWithoutDashes + 4U; ++i) {
+      if (i == 8U || i == 13U || i == 18U || i == 23U) {
+        client_id_with_dashes.push_back('-');
+      } else {
+        client_id_with_dashes.push_back(*client_id_it);
+        ++client_id_it;
+      }
+    }
+    DCHECK(client_id_it == client_info->client_id.end());

[Alexei Svitkine (slow), 2014/07/15 12:53:25]

Nit: DCHECK_EQ

[gab, 2014/07/15 20:50:06]

Nope, same as above.

+    client_info->client_id.assign(client_id_with_dashes);
+  }
+
+  // The GUID retrieved (and possibly fixed above) should be valid unless
+  // retrieval failed.
+  DCHECK(!client_info || base::IsValidGUID(client_info->client_id));
+
+  return client_info.Pass();
+}
+

[Alexei Svitkine (slow), 2014/07/15 12:53:25]

Nit: Remove empty line.

[gab, 2014/07/15 20:50:06]

Done.

+
 int MetricsStateManager::GetLowEntropySource() {
   // Note that the default value for the low entropy source and the default pref
   // value are both kLowEntropySourceNotSet, which is used to identify if the
@@ -212,6 +306,9 @@ void MetricsStateManager::ResetMetricsIDsIfNecessary() {
   local_state_->ClearPref(prefs::kMetricsClientID);
   local_state_->ClearPref(prefs::kMetricsLowEntropySource);
   local_state_->ClearPref(prefs::kMetricsResetIds);
+
+  // Also clear the backed up client info.
+  store_client_info_.Run(ClientInfo());
 }
 
 }  // namespace metrics