diff options
22 files changed, 525 insertions, 55 deletions
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 @@ include_rules = [ "+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 @@ bool PreReadExperimentIsActive() { 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 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches( 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 @@ bool GoogleUpdateSettings::SetCollectStatsConsent(bool consented) { } // 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::RegisterPrefs(PrefRegistrySimple* registry) { 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 @@ bool IsMetricsReportingEnabled() { 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, HashExtension) { 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::MetricsStateManager* MetricsServicesManager::GetMetricsStateManager() { 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 @@ include_rules = [ "+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 @@ void Init() { // 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 80c0f1b2..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 kRegLastInstallerErrorField[] = L"LastInstallerError"; 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 kRegLastInstallerResultField[]; 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 @@ bool GoogleUpdateSettings::SetCollectStatsConsentAtLevel(bool system_install, 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 @@ class GoogleUpdateSettings { 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 @@ namespace { 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 @@ class MetricsServiceTest : public testing::Test { 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 @@ 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(); + + // 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 @@ 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,48 @@ 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); + + 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 @@ 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 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 @@ class ClonedInstallDetector; // 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 @@ class MetricsStateManager { // 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 @@ class MetricsStateManager { // 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 @@ class MetricsStateManager { // 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 @@ namespace metrics { 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 @@ class MetricsStateManagerTest : public testing::Test { 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 @@ TEST_F(MetricsStateManagerTest, ResetMetricsIDs) { 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 |