Retrieve client_id from GoogleUpdateSettings when its missing from Local State.
Precursor refactoring CL @ https://codereview.chromium.org/365133005/
Precursor kInstallDate move to metrics_pref_names @ https://codereview.chromium.org/370813003/
This is the Windows implementation, POSIX implementation to follow in https://codereview.chromium.org/377713002/ (easier to develop and test on Linux in a follow-up CL)
BUG=391338
Review URL: https://codereview.chromium.org/372473004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284805 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/app/DEPS b/chrome/app/DEPS
index ba43711..dcc07692 100644
--- a/chrome/app/DEPS
+++ b/chrome/app/DEPS
@@ -10,6 +10,7 @@
"+chromeos/chromeos_switches.h",
"+components/breakpad",
"+components/component_updater",
+ "+components/metrics/client_info.h",
"+components/nacl/common",
"+components/nacl/zygote",
"+components/startup_metric_utils",
diff --git a/chrome/app/client_util.cc b/chrome/app/client_util.cc
index ac6dbfc..efb79d5 100644
--- a/chrome/app/client_util.cc
+++ b/chrome/app/client_util.cc
@@ -34,6 +34,7 @@
#include "chrome/installer/util/util_constants.h"
#include "components/breakpad/app/breakpad_client.h"
#include "components/breakpad/app/breakpad_win.h"
+#include "components/metrics/client_info.h"
#include "content/public/app/startup_helper_win.h"
#include "sandbox/win/src/sandbox.h"
@@ -75,14 +76,14 @@
void GetPreReadPopulationAndGroup(double* population, double* group) {
// By default we use the metrics id for the user as stable pseudo-random
// input to a hash.
- std::string metrics_id;
- GoogleUpdateSettings::LoadMetricsClientId(&metrics_id);
+ scoped_ptr<metrics::ClientInfo> client_info =
+ GoogleUpdateSettings::LoadMetricsClientInfo();
- // If this user has not metrics id, we fall back to a purely random value
- // per browser session.
+ // If this user has no metrics id, we fall back to a purely random value per
+ // browser session.
const size_t kLength = 16;
- std::string random_value(metrics_id.empty() ? base::RandBytesAsString(kLength)
- : metrics_id);
+ std::string random_value(client_info ? client_info->client_id
+ : base::RandBytesAsString(kLength));
// To interpret the value as a random number we hash it and read the first 8
// bytes of the hash as a unit-interval representing a die-toss for being in
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index f534542..f9e7a72 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -101,6 +101,7 @@
#include "components/cdm/browser/cdm_message_filter_android.h"
#include "components/cloud_devices/common/cloud_devices_switches.h"
#include "components/google/core/browser/google_util.h"
+#include "components/metrics/client_info.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/signin/core/common/profile_management_switches.h"
#include "components/translate/core/common/translate_switches.h"
@@ -1485,10 +1486,11 @@
CommandLine* command_line, int child_process_id) {
#if defined(OS_POSIX)
if (breakpad::IsCrashReporterEnabled()) {
- std::string enable_crash_reporter;
- GoogleUpdateSettings::LoadMetricsClientId(&enable_crash_reporter);
+ scoped_ptr<metrics::ClientInfo> client_info =
+ GoogleUpdateSettings::LoadMetricsClientInfo();
command_line->AppendSwitchASCII(switches::kEnableCrashReporter,
- enable_crash_reporter);
+ client_info ? client_info->client_id
+ : std::string());
}
#endif // defined(OS_POSIX)
diff --git a/chrome/browser/google/google_update_settings_posix.cc b/chrome/browser/google/google_update_settings_posix.cc
index 99dc0cc..e6dad5e 100644
--- a/chrome/browser/google/google_update_settings_posix.cc
+++ b/chrome/browser/google/google_update_settings_posix.cc
@@ -67,24 +67,32 @@
}
// static
-bool GoogleUpdateSettings::LoadMetricsClientId(std::string* metrics_id) {
+// TODO(gab): Implement storing/loading for all ClientInfo fields on POSIX.
+scoped_ptr<metrics::ClientInfo> GoogleUpdateSettings::LoadMetricsClientInfo() {
+ scoped_ptr<metrics::ClientInfo> client_info(new metrics::ClientInfo);
+
base::AutoLock lock(g_posix_client_id_lock.Get());
- *metrics_id = g_posix_client_id.Get();
- return true;
+ if (g_posix_client_id.Get().empty())
+ return scoped_ptr<metrics::ClientInfo>();
+ client_info->client_id = g_posix_client_id.Get();
+
+ return client_info.Pass();
}
// static
-bool GoogleUpdateSettings::StoreMetricsClientId(const std::string& client_id) {
+// TODO(gab): Implement storing/loading for all ClientInfo fields on POSIX.
+void GoogleUpdateSettings::StoreMetricsClientInfo(
+ const metrics::ClientInfo& client_info) {
// Make sure that user has consented to send crashes.
if (!GoogleUpdateSettings::GetCollectStatsConsent())
- return false;
+ return;
{
// Since user has consented, write the metrics id to the file.
base::AutoLock lock(g_posix_client_id_lock.Get());
- g_posix_client_id.Get() = client_id;
+ g_posix_client_id.Get() = client_info.client_id;
}
- return GoogleUpdateSettings::SetCollectStatsConsent(true);
+ GoogleUpdateSettings::SetCollectStatsConsent(true);
}
// GetLastRunTime and SetLastRunTime are not implemented for posix. Their
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc
index f10cc15..def8cd6 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_service_client.cc
@@ -35,7 +35,6 @@
#include "chrome/common/crash_keys.h"
#include "chrome/common/pref_names.h"
#include "chrome/common/render_messages.h"
-#include "chrome/installer/util/google_update_settings.h"
#include "components/metrics/metrics_service.h"
#include "components/metrics/net/net_metrics_log_uploader.h"
#include "content/public/browser/browser_thread.h"
@@ -165,9 +164,6 @@
void ChromeMetricsServiceClient::SetMetricsClientId(
const std::string& client_id) {
crash_keys::SetCrashClientIdFromGUID(client_id);
-
- // Store a backup of the client id in Google Update settings.
- GoogleUpdateSettings::StoreMetricsClientId(client_id);
}
bool ChromeMetricsServiceClient::IsOffTheRecordSessionActive() {
diff --git a/chrome/browser/metrics/extensions_metrics_provider_unittest.cc b/chrome/browser/metrics/extensions_metrics_provider_unittest.cc
index f0fc87e..b18f6d9 100644
--- a/chrome/browser/metrics/extensions_metrics_provider_unittest.cc
+++ b/chrome/browser/metrics/extensions_metrics_provider_unittest.cc
@@ -6,7 +6,10 @@
#include <string>
+#include "base/memory/scoped_ptr.h"
#include "base/prefs/testing_pref_service.h"
+#include "components/metrics/client_info.h"
+#include "components/metrics/metrics_service.h"
#include "components/metrics/metrics_state_manager.h"
#include "components/metrics/proto/system_profile.pb.h"
#include "extensions/common/extension.h"
@@ -20,6 +23,13 @@
return true;
}
+void StoreNoClientInfoBackup(const metrics::ClientInfo& /* client_info */) {
+}
+
+scoped_ptr<metrics::ClientInfo> ReturnNoBackup() {
+ return scoped_ptr<metrics::ClientInfo>();
+}
+
class TestExtensionsMetricsProvider : public ExtensionsMetricsProvider {
public:
explicit TestExtensionsMetricsProvider(
@@ -93,10 +103,13 @@
TEST(ExtensionsMetricsProvider, SystemProtoEncoding) {
metrics::SystemProfileProto system_profile;
TestingPrefServiceSimple local_state;
- metrics::MetricsStateManager::RegisterPrefs(local_state.registry());
+ MetricsService::RegisterPrefs(local_state.registry());
scoped_ptr<metrics::MetricsStateManager> metrics_state_manager(
- metrics::MetricsStateManager::Create(&local_state,
- base::Bind(&IsMetricsReportingEnabled)));
+ metrics::MetricsStateManager::Create(
+ &local_state,
+ base::Bind(&IsMetricsReportingEnabled),
+ base::Bind(&StoreNoClientInfoBackup),
+ base::Bind(&ReturnNoBackup)));
TestExtensionsMetricsProvider extension_metrics(metrics_state_manager.get());
extension_metrics.ProvideSystemProfileMetrics(&system_profile);
ASSERT_EQ(2, system_profile.occupied_extension_bucket_size());
diff --git a/chrome/browser/metrics/metrics_services_manager.cc b/chrome/browser/metrics/metrics_services_manager.cc
index 8278913..74d3710 100644
--- a/chrome/browser/metrics/metrics_services_manager.cc
+++ b/chrome/browser/metrics/metrics_services_manager.cc
@@ -4,12 +4,16 @@
#include "chrome/browser/metrics/metrics_services_manager.h"
+#include <string>
+
#include "base/command_line.h"
+#include "base/logging.h"
#include "base/prefs/pref_service.h"
#include "chrome/browser/metrics/chrome_metrics_service_client.h"
#include "chrome/browser/metrics/variations/variations_service.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/pref_names.h"
+#include "chrome/installer/util/google_update_settings.h"
#include "components/metrics/metrics_service.h"
#include "components/metrics/metrics_state_manager.h"
#include "components/rappor/rappor_service.h"
@@ -70,7 +74,9 @@
metrics_state_manager_ = metrics::MetricsStateManager::Create(
local_state_,
base::Bind(&MetricsServicesManager::IsMetricsReportingEnabled,
- base::Unretained(this)));
+ base::Unretained(this)),
+ base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo),
+ base::Bind(&GoogleUpdateSettings::LoadMetricsClientInfo));
}
return metrics_state_manager_.get();
}
diff --git a/chrome/chrome_installer_util.gypi b/chrome/chrome_installer_util.gypi
index af05c26..ef981c1 100644
--- a/chrome/chrome_installer_util.gypi
+++ b/chrome/chrome_installer_util.gypi
@@ -121,6 +121,7 @@
'<(DEPTH)/chrome/chrome_resources.gyp:chrome_resources',
'<(DEPTH)/chrome/chrome_resources.gyp:chrome_strings',
'<(DEPTH)/chrome/common_constants.gyp:common_constants',
+ '<(DEPTH)/components/components.gyp:metrics',
'<(DEPTH)/courgette/courgette.gyp:courgette_lib',
'<(DEPTH)/crypto/crypto.gyp:crypto',
'<(DEPTH)/third_party/bspatch/bspatch.gyp:bspatch',
@@ -189,6 +190,10 @@
'<(SHARED_INTERMEDIATE_DIR)',
],
'sources': [
+ # Include |client_info.cc| directly here to avoid having to create a
+ # metrics_win64 target solely for this purpose.
+ '../components/metrics/client_info.cc',
+ '../components/metrics/client_info.h',
'installer/util/google_chrome_distribution_dummy.cc',
'installer/util/master_preferences.h',
'installer/util/master_preferences_dummy.cc',
diff --git a/chrome/common/DEPS b/chrome/common/DEPS
index 888c227..7f28bda 100644
--- a/chrome/common/DEPS
+++ b/chrome/common/DEPS
@@ -7,6 +7,7 @@
"+components/bookmarks/common",
"+components/cloud_devices/common",
"+components/data_reduction_proxy/common",
+ "+components/metrics/client_info.h",
"+components/metrics/metrics_pref_names.h",
"+components/nacl/common",
"+components/password_manager/core/common",
diff --git a/chrome/common/child_process_logging_win.cc b/chrome/common/child_process_logging_win.cc
index b849734..37a8d3e 100644
--- a/chrome/common/child_process_logging_win.cc
+++ b/chrome/common/child_process_logging_win.cc
@@ -7,10 +7,12 @@
#include <windows.h>
#include "base/debug/crash_logging.h"
+#include "base/memory/scoped_ptr.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/common/chrome_constants.h"
#include "chrome/common/crash_keys.h"
#include "chrome/installer/util/google_update_settings.h"
+#include "components/metrics/client_info.h"
namespace child_process_logging {
@@ -69,9 +71,10 @@
// because of the aforementioned issue, crash keys aren't ready yet at the
// time of Breakpad initialization, load the client id backed up in Google
// Update settings instead.
- std::string client_guid;
- if (GoogleUpdateSettings::LoadMetricsClientId(&client_guid))
- crash_keys::SetCrashClientIdFromGUID(client_guid);
+ scoped_ptr<metrics::ClientInfo> client_info =
+ GoogleUpdateSettings::LoadMetricsClientInfo();
+ if (client_info)
+ crash_keys::SetCrashClientIdFromGUID(client_info->client_id);
}
} // namespace child_process_logging
diff --git a/chrome/installer/util/DEPS b/chrome/installer/util/DEPS
new file mode 100644
index 0000000..0148070
--- /dev/null
+++ b/chrome/installer/util/DEPS
@@ -0,0 +1,3 @@
+include_rules = [
+ "+components/metrics/client_info.h",
+]
diff --git a/chrome/installer/util/google_update_constants.cc b/chrome/installer/util/google_update_constants.cc
index 80c0f1b..431cf7b 100644
--- a/chrome/installer/util/google_update_constants.cc
+++ b/chrome/installer/util/google_update_constants.cc
@@ -45,6 +45,8 @@
const wchar_t kRegLastInstallerExtraField[] = L"LastInstallerExtraCode1";
const wchar_t kRegLastRunTimeField[] = L"lastrun";
const wchar_t kRegMetricsId[] = L"metricsid";
+const wchar_t kRegMetricsIdEnabledDate[] = L"metricsid_enableddate";
+const wchar_t kRegMetricsIdInstallDate[] = L"metricsid_installdate";
const wchar_t kRegMSIField[] = L"msi";
const wchar_t kRegNameField[] = L"name";
const wchar_t kRegOemInstallField[] = L"oeminstall";
diff --git a/chrome/installer/util/google_update_constants.h b/chrome/installer/util/google_update_constants.h
index 9f658ea..8718059 100644
--- a/chrome/installer/util/google_update_constants.h
+++ b/chrome/installer/util/google_update_constants.h
@@ -55,6 +55,8 @@
extern const wchar_t kRegLastInstallerErrorField[];
extern const wchar_t kRegLastInstallerExtraField[];
extern const wchar_t kRegMetricsId[];
+extern const wchar_t kRegMetricsIdEnabledDate[];
+extern const wchar_t kRegMetricsIdInstallDate[];
extern const wchar_t kRegMSIField[];
extern const wchar_t kRegNameField[];
extern const wchar_t kRegOemInstallField[];
diff --git a/chrome/installer/util/google_update_settings.cc b/chrome/installer/util/google_update_settings.cc
index 4d53eca..3c4cba6 100644
--- a/chrome/installer/util/google_update_settings.cc
+++ b/chrome/installer/util/google_update_settings.cc
@@ -296,16 +296,43 @@
return (result == ERROR_SUCCESS);
}
-bool GoogleUpdateSettings::LoadMetricsClientId(std::string* metrics_id) {
- base::string16 metrics_id16;
- bool rv = ReadGoogleUpdateStrKey(google_update::kRegMetricsId, &metrics_id16);
- *metrics_id = base::UTF16ToUTF8(metrics_id16);
- return rv;
+scoped_ptr<metrics::ClientInfo> GoogleUpdateSettings::LoadMetricsClientInfo() {
+ base::string16 client_id_16;
+ if (!ReadGoogleUpdateStrKey(google_update::kRegMetricsId, &client_id_16) ||
+ client_id_16.empty()) {
+ return scoped_ptr<metrics::ClientInfo>();
+ }
+
+ scoped_ptr<metrics::ClientInfo> client_info(new metrics::ClientInfo);
+ client_info->client_id = base::UTF16ToUTF8(client_id_16);
+
+ base::string16 installation_date_str;
+ if (ReadGoogleUpdateStrKey(google_update::kRegMetricsIdInstallDate,
+ &installation_date_str)) {
+ base::StringToInt64(installation_date_str, &client_info->installation_date);
+ }
+
+ base::string16 reporting_enbaled_date_date_str;
+ if (ReadGoogleUpdateStrKey(google_update::kRegMetricsIdEnabledDate,
+ &reporting_enbaled_date_date_str)) {
+ base::StringToInt64(reporting_enbaled_date_date_str,
+ &client_info->reporting_enabled_date);
+ }
+
+ return client_info.Pass();
}
-bool GoogleUpdateSettings::StoreMetricsClientId(const std::string& metrics_id) {
- base::string16 metrics_id16 = base::UTF8ToUTF16(metrics_id);
- return WriteGoogleUpdateStrKey(google_update::kRegMetricsId, metrics_id16);
+void GoogleUpdateSettings::StoreMetricsClientInfo(
+ const metrics::ClientInfo& client_info) {
+ // Attempt a best-effort at backing |client_info| in the registry (but don't
+ // handle/report failures).
+ WriteGoogleUpdateStrKey(google_update::kRegMetricsId,
+ base::UTF8ToUTF16(client_info.client_id));
+ WriteGoogleUpdateStrKey(google_update::kRegMetricsIdInstallDate,
+ base::Int64ToString16(client_info.installation_date));
+ WriteGoogleUpdateStrKey(
+ google_update::kRegMetricsIdEnabledDate,
+ base::Int64ToString16(client_info.reporting_enabled_date));
}
// EULA consent is only relevant for system-level installs.
diff --git a/chrome/installer/util/google_update_settings.h b/chrome/installer/util/google_update_settings.h
index a8e43c4..ca7fd7f 100644
--- a/chrome/installer/util/google_update_settings.h
+++ b/chrome/installer/util/google_update_settings.h
@@ -8,10 +8,12 @@
#include <string>
#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
#include "base/strings/string16.h"
#include "base/time/time.h"
#include "base/version.h"
#include "chrome/installer/util/util_constants.h"
+#include "components/metrics/client_info.h"
class AppRegistrationData;
class BrowserDistribution;
@@ -86,12 +88,15 @@
bool consented);
#endif
- // Returns the metrics client id backed up in the registry. If none found,
- // returns empty string.
- static bool LoadMetricsClientId(std::string* metrics_id);
+ // Returns the metrics client info backed up in the registry. NULL
+ // if-and-only-if the client_id couldn't be retrieved (failure to retrieve
+ // other fields only makes them keep their default value). A non-null return
+ // will NEVER contain an empty client_id field.
+ static scoped_ptr<metrics::ClientInfo> LoadMetricsClientInfo();
- // Stores a backup of the metrics client id in the registry.
- static bool StoreMetricsClientId(const std::string& metrics_id);
+ // Stores a backup of the metrics client info in the registry. Storing a
+ // |client_info| with an empty client id will effectively void the backup.
+ static void StoreMetricsClientInfo(const metrics::ClientInfo& client_info);
// Sets the machine-wide EULA consented flag required on OEM installs.
// Returns false if the setting could not be recorded.
diff --git a/components/metrics.gypi b/components/metrics.gypi
index 964e4fa..cf43cf7 100644
--- a/components/metrics.gypi
+++ b/components/metrics.gypi
@@ -18,10 +18,12 @@
'variations',
],
'sources': [
- 'metrics/compression_utils.cc',
- 'metrics/compression_utils.h',
+ 'metrics/client_info.cc',
+ 'metrics/client_info.h',
'metrics/cloned_install_detector.cc',
'metrics/cloned_install_detector.h',
+ 'metrics/compression_utils.cc',
+ 'metrics/compression_utils.h',
'metrics/machine_id_provider.h',
'metrics/machine_id_provider_stub.cc',
'metrics/machine_id_provider_win.cc',
@@ -29,10 +31,10 @@
'metrics/metrics_hashes.h',
'metrics/metrics_log.cc',
'metrics/metrics_log.h',
- 'metrics/metrics_log_uploader.cc',
- 'metrics/metrics_log_uploader.h',
'metrics/metrics_log_manager.cc',
'metrics/metrics_log_manager.h',
+ 'metrics/metrics_log_uploader.cc',
+ 'metrics/metrics_log_uploader.h',
'metrics/metrics_pref_names.cc',
'metrics/metrics_pref_names.h',
'metrics/metrics_provider.h',
diff --git a/components/metrics/client_info.cc b/components/metrics/client_info.cc
new file mode 100644
index 0000000..57ad394
--- /dev/null
+++ b/components/metrics/client_info.cc
@@ -0,0 +1,13 @@
+// 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/client_info.h"
+
+namespace metrics {
+
+ClientInfo::ClientInfo() : installation_date(0), reporting_enabled_date(0) {}
+
+ClientInfo::~ClientInfo() {}
+
+} // namespace metrics
diff --git a/components/metrics/client_info.h b/components/metrics/client_info.h
new file mode 100644
index 0000000..8fcaec2
--- /dev/null
+++ b/components/metrics/client_info.h
@@ -0,0 +1,38 @@
+// 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.
+
+#ifndef COMPONENTS_METRICS_CLIENT_INFO_H_
+#define COMPONENTS_METRICS_CLIENT_INFO_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/macros.h"
+
+namespace metrics {
+
+// A data object used to pass data from outside the metrics component into the
+// metrics component.
+struct ClientInfo {
+ public:
+ ClientInfo();
+ ~ClientInfo();
+
+ // The metrics ID of this client: represented as a GUID string.
+ std::string client_id;
+
+ // The installation date: represented as an epoch time in seconds.
+ int64 installation_date;
+
+ // The date on which metrics reporting was enabled: represented as an epoch
+ // time in seconds.
+ int64 reporting_enabled_date;
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(ClientInfo);
+};
+
+} // namespace metrics
+
+#endif // COMPONENTS_METRICS_CLIENT_INFO_H_
diff --git a/components/metrics/metrics_service_unittest.cc b/components/metrics/metrics_service_unittest.cc
index 694c45a..c580287 100644
--- a/components/metrics/metrics_service_unittest.cc
+++ b/components/metrics/metrics_service_unittest.cc
@@ -7,9 +7,11 @@
#include <string>
#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
#include "base/message_loop/message_loop.h"
#include "base/prefs/testing_pref_service.h"
#include "base/threading/platform_thread.h"
+#include "components/metrics/client_info.h"
#include "components/metrics/compression_utils.h"
#include "components/metrics/metrics_log.h"
#include "components/metrics/metrics_pref_names.h"
@@ -23,6 +25,13 @@
using metrics::MetricsLogManager;
+void StoreNoClientInfoBackup(const metrics::ClientInfo& /* client_info */) {
+}
+
+scoped_ptr<metrics::ClientInfo> ReturnNoBackup() {
+ return scoped_ptr<metrics::ClientInfo>();
+}
+
class TestMetricsService : public MetricsService {
public:
TestMetricsService(metrics::MetricsStateManager* state_manager,
@@ -62,7 +71,9 @@
metrics_state_manager_ = metrics::MetricsStateManager::Create(
GetLocalState(),
base::Bind(&MetricsServiceTest::is_metrics_reporting_enabled,
- base::Unretained(this)));
+ base::Unretained(this)),
+ base::Bind(&StoreNoClientInfoBackup),
+ base::Bind(&ReturnNoBackup));
}
virtual ~MetricsServiceTest() {
diff --git a/components/metrics/metrics_state_manager.cc b/components/metrics/metrics_state_manager.cc
index 3b71976..08875f3 100644
--- a/components/metrics/metrics_state_manager.cc
+++ b/components/metrics/metrics_state_manager.cc
@@ -45,9 +45,13 @@
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 @@
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();
+
+ // 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 != 0
+ ? client_info_backup->installation_date
+ : now.ToTimeT());
+ local_state_->SetInt64(prefs::kMetricsReportingEnabledTimestamp,
+ client_info_backup->reporting_enabled_date != 0
+ ? 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 @@
UMA_HISTOGRAM_BOOLEAN("UMA.ClientIdMigrated", true);
}
local_state_->ClearPref(prefs::kMetricsOldClientID);
+
+ BackUpCurrentClientInfo();
}
void MetricsStateManager::CheckForClonedInstall(
@@ -141,12 +188,16 @@
// 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,48 @@
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);
+
+ 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());
+ 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();
+}
+
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 +305,9 @@
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
diff --git a/components/metrics/metrics_state_manager.h b/components/metrics/metrics_state_manager.h
index 6f13ee9..30b6285 100644
--- a/components/metrics/metrics_state_manager.h
+++ b/components/metrics/metrics_state_manager.h
@@ -10,8 +10,10 @@
#include "base/basictypes.h"
#include "base/callback.h"
#include "base/gtest_prod_util.h"
+#include "base/macros.h"
#include "base/memory/scoped_ptr.h"
#include "base/metrics/field_trial.h"
+#include "components/metrics/client_info.h"
class PrefService;
class PrefRegistrySimple;
@@ -25,6 +27,15 @@
// not be instantiating or using this class directly.
class MetricsStateManager {
public:
+ // A callback that can be invoked to store client info to persistent storage.
+ // Storing an empty client_id will resulted in the backup being voided.
+ typedef base::Callback<void(const ClientInfo& client_info)>
+ StoreClientInfoCallback;
+
+ // A callback that can be invoked to load client info stored through the
+ // StoreClientInfoCallback.
+ typedef base::Callback<scoped_ptr<ClientInfo>(void)> LoadClientInfoCallback;
+
virtual ~MetricsStateManager();
// Returns true if the user opted in to sending metric reports.
@@ -57,7 +68,9 @@
// of the class exists at a time. Returns NULL if an instance exists already.
static scoped_ptr<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& load_client_info);
// Registers local state prefs used by this class.
static void RegisterPrefs(PrefRegistrySimple* registry);
@@ -81,11 +94,22 @@
// Creates the MetricsStateManager with the given |local_state|. Calls
// |is_reporting_enabled_callback| to query whether metrics reporting is
- // enabled. Clients should instead use Create(), which enforces a single
- // instance of this class is alive at any given time.
+ // enabled. Clients should instead use Create(), which enforces that a single
+ // instance of this class be alive at any given time.
+ // |store_client_info| should back up client info to persistent storage such
+ // that it is later retrievable by |load_client_info|.
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& load_client_info);
+
+ // Backs up the current client info via |store_client_info_|.
+ void BackUpCurrentClientInfo();
+
+ // Loads the client info via |load_client_info_| and potentially migrates it
+ // before returning it if it comes back in its old form.
+ scoped_ptr<ClientInfo> LoadClientInfoAndMaybeMigrate();
// Returns the low entropy source for this client. This is a random value
// that is non-identifying amongst browser clients. This method will
@@ -110,8 +134,18 @@
// Weak pointer to the local state prefs store.
PrefService* const local_state_;
+ // A callback run by this MetricsStateManager to know whether reporting is
+ // enabled.
const base::Callback<bool(void)> is_reporting_enabled_callback_;
+ // A callback run during client id creation so this MetricsStateManager can
+ // store a backup of the newly generated ID.
+ const StoreClientInfoCallback store_client_info_;
+
+ // A callback run if this MetricsStateManager can't get the client id from
+ // its typical location and wants to attempt loading it from this backup.
+ const LoadClientInfoCallback load_client_info_;
+
// The identifier that's sent to the server with the log reports.
std::string client_id_;
diff --git a/components/metrics/metrics_state_manager_unittest.cc b/components/metrics/metrics_state_manager_unittest.cc
index 2f7d85b..0e540a5 100644
--- a/components/metrics/metrics_state_manager_unittest.cc
+++ b/components/metrics/metrics_state_manager_unittest.cc
@@ -10,7 +10,9 @@
#include "base/bind.h"
#include "base/command_line.h"
#include "base/prefs/testing_pref_service.h"
+#include "components/metrics/client_info.h"
#include "components/metrics/metrics_pref_names.h"
+#include "components/metrics/metrics_service.h"
#include "components/metrics/metrics_switches.h"
#include "components/variations/caching_permuted_entropy_provider.h"
#include "components/variations/pref_names.h"
@@ -21,13 +23,17 @@
class MetricsStateManagerTest : public testing::Test {
public:
MetricsStateManagerTest() : is_metrics_reporting_enabled_(false) {
- MetricsStateManager::RegisterPrefs(prefs_.registry());
+ MetricsService::RegisterPrefs(prefs_.registry());
}
scoped_ptr<MetricsStateManager> CreateStateManager() {
return MetricsStateManager::Create(
&prefs_,
base::Bind(&MetricsStateManagerTest::is_metrics_reporting_enabled,
+ base::Unretained(this)),
+ base::Bind(&MetricsStateManagerTest::MockStoreClientInfoBackup,
+ base::Unretained(this)),
+ base::Bind(&MetricsStateManagerTest::LoadFakeClientInfoBackup,
base::Unretained(this))).Pass();
}
@@ -39,11 +45,50 @@
protected:
TestingPrefServiceSimple prefs_;
+ // Last ClientInfo stored by the MetricsStateManager via
+ // MockStoreClientInfoBackup.
+ scoped_ptr<ClientInfo> stored_client_info_backup_;
+
+ // If set, will be returned via LoadFakeClientInfoBackup if requested by the
+ // MetricsStateManager.
+ scoped_ptr<ClientInfo> fake_client_info_backup_;
+
private:
bool is_metrics_reporting_enabled() const {
return is_metrics_reporting_enabled_;
}
+ // Stores the |client_info| in |stored_client_info_backup_| for verification
+ // by the tests later.
+ void MockStoreClientInfoBackup(const ClientInfo& client_info) {
+ stored_client_info_backup_.reset(new ClientInfo);
+ stored_client_info_backup_->client_id = client_info.client_id;
+ stored_client_info_backup_->installation_date =
+ client_info.installation_date;
+ stored_client_info_backup_->reporting_enabled_date =
+ client_info.reporting_enabled_date;
+
+ // Respect the contract that storing an empty client_id voids the existing
+ // backup (required for the last section of the ForceClientIdCreation test
+ // below).
+ if (client_info.client_id.empty())
+ fake_client_info_backup_.reset();
+ }
+
+ // Hands out a copy of |fake_client_info_backup_| if it is set.
+ scoped_ptr<ClientInfo> LoadFakeClientInfoBackup() {
+ if (!fake_client_info_backup_)
+ return scoped_ptr<ClientInfo>();
+
+ scoped_ptr<ClientInfo> backup_copy(new ClientInfo);
+ backup_copy->client_id = fake_client_info_backup_->client_id;
+ backup_copy->installation_date =
+ fake_client_info_backup_->installation_date;
+ backup_copy->reporting_enabled_date =
+ fake_client_info_backup_->reporting_enabled_date;
+ return backup_copy.Pass();
+ }
+
bool is_metrics_reporting_enabled_;
DISALLOW_COPY_AND_ASSIGN(MetricsStateManagerTest);
@@ -171,4 +216,160 @@
EXPECT_NE(kInitialClientId, prefs_.GetString(prefs::kMetricsClientID));
}
+TEST_F(MetricsStateManagerTest, ForceClientIdCreation) {
+ const int64 kFakeInstallationDate = 12345;
+ prefs_.SetInt64(prefs::kInstallDate, kFakeInstallationDate);
+
+ // Holds ClientInfo from previous scoped test for extra checks.
+ scoped_ptr<ClientInfo> previous_client_info;
+
+ {
+ scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+
+ // client_id shouldn't be auto-generated if metrics reporting is not
+ // enabled.
+ EXPECT_EQ(std::string(), state_manager->client_id());
+ EXPECT_EQ(0, prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp));
+
+ // Confirm that the initial ForceClientIdCreation call creates the client id
+ // and backs it up via MockStoreClientInfoBackup.
+ EXPECT_FALSE(stored_client_info_backup_);
+ state_manager->ForceClientIdCreation();
+ EXPECT_NE(std::string(), state_manager->client_id());
+ EXPECT_GT(prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp), 0);
+
+ ASSERT_TRUE(stored_client_info_backup_);
+ EXPECT_EQ(state_manager->client_id(),
+ stored_client_info_backup_->client_id);
+ EXPECT_EQ(kFakeInstallationDate,
+ stored_client_info_backup_->installation_date);
+ EXPECT_EQ(prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp),
+ stored_client_info_backup_->reporting_enabled_date);
+
+ previous_client_info = stored_client_info_backup_.Pass();
+ }
+
+ EnableMetricsReporting();
+
+ {
+ EXPECT_FALSE(stored_client_info_backup_);
+
+ scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+
+ // client_id should be auto-obtained from the constructor when metrics
+ // reporting is enabled.
+ EXPECT_EQ(previous_client_info->client_id, state_manager->client_id());
+
+ // The backup should also be refreshed when the client id re-initialized.
+ ASSERT_TRUE(stored_client_info_backup_);
+ EXPECT_EQ(previous_client_info->client_id,
+ stored_client_info_backup_->client_id);
+ EXPECT_EQ(kFakeInstallationDate,
+ stored_client_info_backup_->installation_date);
+ EXPECT_EQ(previous_client_info->reporting_enabled_date,
+ stored_client_info_backup_->reporting_enabled_date);
+
+ // Re-forcing client id creation shouldn't cause another backup and
+ // shouldn't affect the existing client id.
+ stored_client_info_backup_.reset();
+ state_manager->ForceClientIdCreation();
+ EXPECT_FALSE(stored_client_info_backup_);
+ EXPECT_EQ(previous_client_info->client_id, state_manager->client_id());
+ }
+
+ const int64 kBackupInstallationDate = 1111;
+ const int64 kBackupReportingEnabledDate = 2222;
+ const char kBackupClientId[] = "AAAAAAAA-BBBB-CCCC-DDDD-EEEEEEEEEEEE";
+ fake_client_info_backup_.reset(new ClientInfo);
+ fake_client_info_backup_->client_id = kBackupClientId;
+ fake_client_info_backup_->installation_date = kBackupInstallationDate;
+ fake_client_info_backup_->reporting_enabled_date =
+ kBackupReportingEnabledDate;
+
+ {
+ // The existence of a backup should result in the same behaviour as
+ // before if we already have a client id.
+
+ EXPECT_FALSE(stored_client_info_backup_);
+
+ scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+ EXPECT_EQ(previous_client_info->client_id, state_manager->client_id());
+
+ // The backup should also be refreshed when the client id re-initialized.
+ ASSERT_TRUE(stored_client_info_backup_);
+ EXPECT_EQ(previous_client_info->client_id,
+ stored_client_info_backup_->client_id);
+ EXPECT_EQ(kFakeInstallationDate,
+ stored_client_info_backup_->installation_date);
+ EXPECT_EQ(previous_client_info->reporting_enabled_date,
+ stored_client_info_backup_->reporting_enabled_date);
+ stored_client_info_backup_.reset();
+ }
+
+ prefs_.ClearPref(prefs::kMetricsClientID);
+ prefs_.ClearPref(prefs::kMetricsReportingEnabledTimestamp);
+
+ {
+ // The backup should kick in if the client id has gone missing. It should
+ // replace remaining and missing dates as well.
+
+ EXPECT_FALSE(stored_client_info_backup_);
+
+ scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+ EXPECT_EQ(kBackupClientId, state_manager->client_id());
+ EXPECT_EQ(kBackupInstallationDate, prefs_.GetInt64(prefs::kInstallDate));
+ EXPECT_EQ(kBackupReportingEnabledDate,
+ prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp));
+
+ EXPECT_TRUE(stored_client_info_backup_);
+ stored_client_info_backup_.reset();
+ }
+
+ const char kNoDashesBackupClientId[] = "AAAAAAAABBBBCCCCDDDDEEEEEEEEEEEE";
+ fake_client_info_backup_.reset(new ClientInfo);
+ fake_client_info_backup_->client_id = kNoDashesBackupClientId;
+
+ prefs_.ClearPref(prefs::kInstallDate);
+ prefs_.ClearPref(prefs::kMetricsClientID);
+ prefs_.ClearPref(prefs::kMetricsReportingEnabledTimestamp);
+
+ {
+ // When running the backup from old-style client ids, dashes should be
+ // re-added. And missing dates in backup should be replaced by Time::Now().
+
+ EXPECT_FALSE(stored_client_info_backup_);
+
+ scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+ EXPECT_EQ(kBackupClientId, state_manager->client_id());
+ EXPECT_GT(prefs_.GetInt64(prefs::kInstallDate), 0);
+ EXPECT_GT(prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp), 0);
+
+ EXPECT_TRUE(stored_client_info_backup_);
+ previous_client_info = stored_client_info_backup_.Pass();
+ }
+
+ prefs_.SetBoolean(prefs::kMetricsResetIds, true);
+
+ {
+ // Upon request to reset metrics ids, the existing backup should not be
+ // restored.
+
+ EXPECT_FALSE(stored_client_info_backup_);
+
+ scoped_ptr<MetricsStateManager> state_manager(CreateStateManager());
+
+ // A brand new client id should have been generated.
+ EXPECT_NE(std::string(), state_manager->client_id());
+ EXPECT_NE(previous_client_info->client_id, state_manager->client_id());
+
+ // Dates should not have been affected.
+ EXPECT_EQ(previous_client_info->installation_date,
+ prefs_.GetInt64(prefs::kInstallDate));
+ EXPECT_EQ(previous_client_info->reporting_enabled_date,
+ prefs_.GetInt64(prefs::kMetricsReportingEnabledTimestamp));
+
+ stored_client_info_backup_.reset();
+ }
+}
+
} // namespace metrics
