Retrieve client_id from GoogleUpdateSettings when its missing from Local State.

components/metrics/metrics_state_manager.cc

 // Copyright 2014 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 "components/metrics/metrics_state_manager.h"
 
 #include "base/command_line.h"
 #include "base/guid.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
@@ skipping 27 matching lines @@
   return base::RandInt(0, kMaxLowEntropySize - 1);
 }
 
 }  // namespace
 
 // static
 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();
   if (IsMetricsReportingEnabled())
     ForceClientIdCreation();
 
   DCHECK(!instance_exists_);
   instance_exists_ = true;
 }
 
 MetricsStateManager::~MetricsStateManager() {
   DCHECK(instance_exists_);
   instance_exists_ = false;
 }
 
 bool MetricsStateManager::IsMetricsReportingEnabled() {
   return is_reporting_enabled_callback_.Run();
 }
 
 void MetricsStateManager::ForceClientIdCreation() {
   if (!client_id_.empty())
     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_);
 
   if (local_state_->GetString(prefs::kMetricsOldClientID).empty()) {
     // Record the timestamp of when the user opted in to UMA.
     local_state_->SetInt64(prefs::kMetricsReportingEnabledTimestamp,
                            base::Time::Now().ToTimeT());
   } else {
     UMA_HISTOGRAM_BOOLEAN("UMA.ClientIdMigrated", true);
   }
   local_state_->ClearPref(prefs::kMetricsOldClientID);
+
+  BackUpCurrentClientInfo();
 }
 
 void MetricsStateManager::CheckForClonedInstall(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
   DCHECK(!cloned_install_detector_);
 
   MachineIdProvider* provider = MachineIdProvider::CreateInstance();
   if (!provider)
     return;
 
@@ skipping 35 matching lines @@
 #else
   return scoped_ptr<const base::FieldTrial::EntropyProvider>(
       new PermutedEntropyProvider(low_entropy_source_value,
                                   kMaxLowEntropySize));
 #endif
 }
 
 // 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();
 }
 
 // static
 void MetricsStateManager::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(prefs::kMetricsResetIds, false);
   registry->RegisterStringPref(prefs::kMetricsClientID, std::string());
   registry->RegisterInt64Pref(prefs::kMetricsReportingEnabledTimestamp, 0);
   registry->RegisterIntegerPref(prefs::kMetricsLowEntropySource,
                                 kLowEntropySourceNotSet);
 
   ClonedInstallDetector::RegisterPrefs(registry);
   CachingPermutedEntropyProvider::RegisterPrefs(registry);
 
   // TODO(asvitkine): Remove these once a couple of releases have passed.
   // http://crbug.com/357704
   registry->RegisterStringPref(prefs::kMetricsOldClientID, std::string());
   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
   // value has been set or not.
   if (low_entropy_source_ != kLowEntropySourceNotSet)
     return low_entropy_source_;
 
   const CommandLine* command_line(CommandLine::ForCurrentProcess());
   // Only try to load the value from prefs if the user did not request a
   // reset.
@@ skipping 24 matching lines @@
     return;
 
   UMA_HISTOGRAM_BOOLEAN("UMA.MetricsIDsReset", true);
 
   DCHECK(client_id_.empty());
   DCHECK_EQ(kLowEntropySourceNotSet, low_entropy_source_);
 
   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