Skip to content

Commit

Permalink
Record metrics about migrations in AccountTrackerService
Browse files Browse the repository at this point in the history 
AccountTrackerService does two migration when loading data:
- removal of deprecated list keyed by "service_flags", moving
  the only value still used to "is_child_account" key
- migration of the account_id from normalized email to gaia id.

No metrics were recorded for those migration, so it is not
possible to check whether they are complete or not in the wild.
Add metrics to fix this deficiency.

Also ensure that the migration state is set to MIGRATION_DONE
if once the migration has been completed (previously it only
happened if the AccountTrackerService became empty).

Bug: none
Change-Id: I329ddb9b4c7c150eee5cfb9d2fd80df60214f79b
Reviewed-on: https://chromium-review.googlesource.com/1162165
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588061}
  • Loading branch information
@sdefresne
sdefresne authored and Commit Bot committed Aug 31, 2018
1 parent 15466b4 commit 5e03fe2
Show file tree
Hide file tree
Showing 4 changed files with 213 additions and 64 deletions.
146 changes: 91 additions & 55 deletions components/signin/core/browser/account_tracker_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
#include "base/metrics/histogram_macros.h"
#include "base/strings/string_split.h"
#include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h"
Expand Down Expand Up @@ -221,21 +222,10 @@ AccountTrackerService::GetMigrationState() const {
return GetMigrationState(pref_service_);
}

void AccountTrackerService::SetMigrationState(AccountIdMigrationState state) {
pref_service_->SetInteger(prefs::kAccountIdMigrationState, state);
}

void AccountTrackerService::SetMigrationDone() {
SetMigrationState(MIGRATION_DONE);
}

// static
AccountTrackerService::AccountIdMigrationState
AccountTrackerService::GetMigrationState(const PrefService* pref_service) {
return static_cast<AccountTrackerService::AccountIdMigrationState>(
pref_service->GetInteger(prefs::kAccountIdMigrationState));
}

void AccountTrackerService::NotifyAccountUpdated(const AccountState& state) {
DCHECK(!state.info.gaia.empty());
for (auto& observer : observer_list_)
Expand Down Expand Up @@ -355,55 +345,92 @@ void AccountTrackerService::SetIsAdvancedProtectionAccount(
SaveToPrefs(state);
}

bool AccountTrackerService::IsMigratable() const {
void AccountTrackerService::MigrateToGaiaId() {
DCHECK_EQ(GetMigrationState(), MIGRATION_IN_PROGRESS);

std::vector<std::string> to_remove;
std::vector<AccountState> migrated_accounts;
for (const auto& pair : accounts_) {
const std::string& new_account_id = pair.second.info.gaia;
if (pair.first == new_account_id)
continue;

to_remove.push_back(pair.first);

// If there is already an account keyed to the current account's gaia id,
// assume this is the result of a partial migration and skip the account
// that is currently inspected.
if (base::ContainsKey(accounts_, new_account_id))
continue;

AccountState new_state = pair.second;
new_state.info.account_id = new_account_id;
SaveToPrefs(new_state);
migrated_accounts.emplace_back(std::move(new_state));
}

// Insert the new migrated accounts.
for (AccountState& new_state : migrated_accounts) {
// Copy the AccountState |gaia| member field so that it is not left in
// an undeterminate state in the structure after std::map::emplace call.
std::string account_id = new_state.info.gaia;
SaveToPrefs(new_state);

accounts_.emplace(std::move(account_id), std::move(new_state));
}

// Remove any obsolete account.
for (const auto& account_id : to_remove) {
DCHECK(base::ContainsKey(accounts_, account_id));
AccountState& state = accounts_[account_id];
RemoveAccountImageFromDisk(account_id);
RemoveFromPrefs(state);
accounts_.erase(account_id);
}
}

bool AccountTrackerService::IsMigrationDone() const {
if (!IsMigrationSupported())
return false;

for (std::map<std::string, AccountState>::const_iterator it =
accounts_.begin();
it != accounts_.end(); ++it) {
const AccountState& state = it->second;
if ((it->first).empty() || state.info.gaia.empty())
for (const auto& pair : accounts_) {
if (pair.first != pair.second.info.gaia)
return false;
}

return true;
}

void AccountTrackerService::MigrateToGaiaId() {
std::set<std::string> to_remove;
std::map<std::string, AccountState> migrated_accounts;
for (std::map<std::string, AccountState>::const_iterator it =
accounts_.begin();
it != accounts_.end(); ++it) {
const AccountState& state = it->second;
std::string account_id = it->first;
if (account_id != state.info.gaia) {
std::string new_account_id = state.info.gaia;
if (!base::ContainsKey(accounts_, new_account_id)) {
AccountState new_state = state;
new_state.info.account_id = new_account_id;
migrated_accounts.insert(make_pair(new_account_id, new_state));
SaveToPrefs(new_state);
}
to_remove.insert(account_id);
}
}
AccountTrackerService::AccountIdMigrationState
AccountTrackerService::ComputeNewMigrationState() const {
// If migration is not supported, skip migration.
if (!IsMigrationSupported())
return MIGRATION_NOT_STARTED;

// Remove any obsolete account.
for (auto account_id : to_remove) {
if (base::ContainsKey(accounts_, account_id)) {
AccountState& state = accounts_[account_id];
RemoveFromPrefs(state);
RemoveAccountImageFromDisk(account_id);
accounts_.erase(account_id);
}
}
bool migration_required = false;
for (const auto& pair : accounts_) {
// If there is any non-migratable account, skip migration.
if (pair.first.empty() || pair.second.info.gaia.empty())
return MIGRATION_NOT_STARTED;

for (std::map<std::string, AccountState>::const_iterator it =
migrated_accounts.begin();
it != migrated_accounts.end(); ++it) {
accounts_.insert(*it);
// Migration is required if at least one account is not keyed to its
// gaia id.
migration_required |= (pair.first != pair.second.info.gaia);
}

return migration_required ? MIGRATION_IN_PROGRESS : MIGRATION_DONE;
}

void AccountTrackerService::SetMigrationState(AccountIdMigrationState state) {
DCHECK(state != MIGRATION_DONE || IsMigrationDone());
pref_service_->SetInteger(prefs::kAccountIdMigrationState, state);
}

// static
AccountTrackerService::AccountIdMigrationState
AccountTrackerService::GetMigrationState(const PrefService* pref_service) {
return static_cast<AccountTrackerService::AccountIdMigrationState>(
pref_service->GetInteger(prefs::kAccountIdMigrationState));
}

base::FilePath AccountTrackerService::GetImagePathFor(
Expand Down Expand Up @@ -520,6 +547,9 @@ void AccountTrackerService::LoadFromPrefs() {
}
}

UMA_HISTOGRAM_BOOLEAN("Signin.AccountTracker.DeprecatedServiceFlagDeleted",
contains_deprecated_service_flags);

if (contains_deprecated_service_flags)
RemoveDeprecatedServiceFlags(pref_service_);

Expand All @@ -531,16 +561,22 @@ void AccountTrackerService::LoadFromPrefs() {
RemoveAccountImageFromDisk(account_id);
}

if (GetMigrationState() != MIGRATION_DONE) {
if (IsMigratable()) {
if (accounts_.empty()) {
SetMigrationDone();
} else {
SetMigrationState(MIGRATION_IN_PROGRESS);
if (IsMigrationSupported()) {
if (GetMigrationState() != MIGRATION_DONE) {
const AccountIdMigrationState new_state = ComputeNewMigrationState();
SetMigrationState(new_state);

if (new_state == MIGRATION_IN_PROGRESS) {
MigrateToGaiaId();
}
}
} else {
DCHECK_EQ(GetMigrationState(), MIGRATION_NOT_STARTED);
}

DCHECK(GetMigrationState() != MIGRATION_DONE || IsMigrationDone());
UMA_HISTOGRAM_ENUMERATION("Signin.AccountTracker.GaiaIdMigrationState",
GetMigrationState(), NUM_MIGRATION_STATES);
}

void AccountTrackerService::SaveToPrefs(const AccountState& state) {
Expand Down
31 changes: 23 additions & 8 deletions components/signin/core/browser/account_tracker_service.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,13 @@ class AccountTrackerService : public KeyedService {
};

// Possible values for the kAccountIdMigrationState preference.
// Keep in sync with OAuth2LoginAccountRevokedMigrationState histogram enum.
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
enum AccountIdMigrationState {
MIGRATION_NOT_STARTED,
MIGRATION_IN_PROGRESS,
MIGRATION_DONE,
// Keep in sync with OAuth2LoginAccountRevokedMigrationState histogram enum.
MIGRATION_NOT_STARTED = 0,
MIGRATION_IN_PROGRESS = 1,
MIGRATION_DONE = 2,
NUM_MIGRATION_STATES
};

Expand Down Expand Up @@ -135,8 +137,6 @@ class AccountTrackerService : public KeyedService {

AccountIdMigrationState GetMigrationState() const;
void SetMigrationDone();
static AccountIdMigrationState GetMigrationState(
const PrefService* pref_service);

protected:
// Available to be called in tests.
Expand Down Expand Up @@ -177,11 +177,26 @@ class AccountTrackerService : public KeyedService {
const gfx::Image& image);
void RemoveAccountImageFromDisk(const std::string& account_id);

// Gaia id migration.
bool IsMigratable() const;
// Migrate accounts to be keyed by gaia id instead of normalized email.
// Requires that the migration state is set to MIGRATION_IN_PROGRESS.
void MigrateToGaiaId();

// Returns whether the accounts are all keyed by gaia id. This should
// be the case when the migration state is set to MIGRATION_DONE.
bool IsMigrationDone() const;

// Computes the new migration state. The state is saved to preference
// before performing the migration in order to support resuming the
// migration if necessary during the next load.
AccountIdMigrationState ComputeNewMigrationState() const;

// Updates the migration state in the preferences.
void SetMigrationState(AccountIdMigrationState state);

// Returns the saved migration state in the preferences.
static AccountIdMigrationState GetMigrationState(
const PrefService* pref_service);

PrefService* pref_service_ = nullptr; // Not owned.
std::map<std::string, AccountState> accounts_;
base::ObserverList<Observer>::Unchecked observer_list_;
Expand Down
65 changes: 64 additions & 1 deletion components/signin/core/browser/account_tracker_service_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/files/scoped_temp_dir.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_task_environment.h"
#include "build/build_config.h"
#include "components/image_fetcher/core/image_data_fetcher.h"
Expand Down Expand Up @@ -945,6 +946,56 @@ TEST_F(AccountTrackerServiceTest, LegacyDottedAccountIds) {
}
}

TEST_F(AccountTrackerServiceTest, NoDeprecatedServiceFlags) {
std::string email_alpha = AccountKeyToEmail(kAccountKeyAlpha);
std::string gaia_alpha = AccountKeyToGaiaId(kAccountKeyAlpha);

{
ListPrefUpdate update(prefs(), AccountTrackerService::kAccountInfoPref);

std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
dict->SetString("account_id", base::UTF8ToUTF16(email_alpha));
dict->SetString("email", base::UTF8ToUTF16(email_alpha));
dict->SetString("gaia", base::UTF8ToUTF16(gaia_alpha));
update->Append(std::move(dict));
}

{
base::HistogramTester tester;

ResetAccountTracker();
tester.ExpectBucketCount(
"Signin.AccountTracker.DeprecatedServiceFlagDeleted", false, 1);
}
}

TEST_F(AccountTrackerServiceTest, MigrateDeprecatedServiceFlags) {
std::string email_alpha = AccountKeyToEmail(kAccountKeyAlpha);
std::string gaia_alpha = AccountKeyToGaiaId(kAccountKeyAlpha);

{
ListPrefUpdate update(prefs(), AccountTrackerService::kAccountInfoPref);

std::unique_ptr<base::ListValue> service_flags(new base::ListValue());
service_flags->Append(std::make_unique<base::Value>("uca"));

std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
dict->SetString("account_id", base::UTF8ToUTF16(email_alpha));
dict->SetString("email", base::UTF8ToUTF16(email_alpha));
dict->SetString("gaia", base::UTF8ToUTF16(gaia_alpha));
dict->SetList("service_flags", std::move(service_flags));
update->Append(std::move(dict));
}

{
base::HistogramTester tester;

ResetAccountTracker();
tester.ExpectBucketCount(
"Signin.AccountTracker.DeprecatedServiceFlagDeleted", true, 1);
}
}

TEST_F(AccountTrackerServiceTest, MigrateAccountIdToGaiaId) {
if (!AccountTrackerService::IsMigrationSupported())
return;
Expand All @@ -971,8 +1022,11 @@ TEST_F(AccountTrackerServiceTest, MigrateAccountIdToGaiaId) {
}

{
base::HistogramTester tester;
ResetAccountTracker();

tester.ExpectBucketCount("Signin.AccountTracker.GaiaIdMigrationState",
AccountTrackerService::MIGRATION_IN_PROGRESS, 1);
EXPECT_EQ(account_tracker()->GetMigrationState(),
AccountTrackerService::MIGRATION_IN_PROGRESS);

Expand Down Expand Up @@ -1016,8 +1070,11 @@ TEST_F(AccountTrackerServiceTest, CanNotMigrateAccountIdToGaiaId) {
}

{
base::HistogramTester tester;
ResetAccountTracker();

tester.ExpectBucketCount("Signin.AccountTracker.GaiaIdMigrationState",
AccountTrackerService::MIGRATION_NOT_STARTED, 1);
EXPECT_EQ(account_tracker()->GetMigrationState(),
AccountTrackerService::MIGRATION_NOT_STARTED);

Expand Down Expand Up @@ -1068,8 +1125,11 @@ TEST_F(AccountTrackerServiceTest, GaiaIdMigrationCrashInTheMiddle) {
}

{
base::HistogramTester tester;
ResetAccountTracker();

tester.ExpectBucketCount("Signin.AccountTracker.GaiaIdMigrationState",
AccountTrackerService::MIGRATION_IN_PROGRESS, 1);
EXPECT_EQ(account_tracker()->GetMigrationState(),
AccountTrackerService::MIGRATION_IN_PROGRESS);

Expand All @@ -1088,10 +1148,13 @@ TEST_F(AccountTrackerServiceTest, GaiaIdMigrationCrashInTheMiddle) {
}

{
base::HistogramTester tester;
ResetAccountTracker();

tester.ExpectBucketCount("Signin.AccountTracker.GaiaIdMigrationState",
AccountTrackerService::MIGRATION_DONE, 1);
EXPECT_EQ(account_tracker()->GetMigrationState(),
AccountTrackerService::MIGRATION_IN_PROGRESS);
AccountTrackerService::MIGRATION_DONE);

AccountInfo account_info = account_tracker()->GetAccountInfo(gaia_alpha);
EXPECT_EQ(account_info.account_id, gaia_alpha);
Expand Down
Loading

0 comments on commit 5e03fe2

Please sign in to comment.