Enable re-running crOS Account Manager migrations

|AccountManagerMigrator| uses |AccountMigrationRunner| for actually
running its migrations. A given instance of |AccountMigrationRunner|
silently ignores multiple calls to its |Run| API, which makes it
impossible to run migrations multiple times.

Fix this by re-creating the |AccountMigrationRunner| object when a
migration re-run is required.

Bug: 998111
Change-Id: Iead9ce954a5488c5ed3844bbe1077a92c9d80b73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1769453
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690786}

chrome/browser/chromeos/account_manager/account_manager_migrator.cc

@@ -472,6 +472,12 @@ void AccountManagerMigrator::Start() {
   if (!chromeos::IsAccountManagerAvailable(profile_))
     return;
 
+  if (migration_runner_ && (migration_runner_->GetStatus() ==
+                            AccountMigrationRunner::Status::kRunning)) {
+    return;
+  }
+  migration_runner_ = std::make_unique<AccountMigrationRunner>();
+
   ran_migration_steps_ = false;
   if (ShouldRunMigrations()) {
     ran_migration_steps_ = true;
@@ -481,7 +487,7 @@ void AccountManagerMigrator::Start() {
   // Cleanup tasks (like re-enabling Chrome account reconciliation) rely on the
   // migration being run, even if they were no-op. Check
   // |OnMigrationRunComplete| and |RunCleanupTasks|.
-  migration_runner_.Run(
+  migration_runner_->Run(
       base::BindOnce(&AccountManagerMigrator::OnMigrationRunComplete,
                      weak_factory_.GetWeakPtr()));
 }
@@ -524,7 +530,7 @@ void AccountManagerMigrator::AddMigrationSteps() {
   signin::IdentityManager* identity_manager =
       IdentityManagerFactory::GetForProfile(profile_);
 
-  migration_runner_.AddStep(std::make_unique<DeviceAccountMigration>(
+  migration_runner_->AddStep(std::make_unique<DeviceAccountMigration>(
       GetDeviceAccount(profile_),
       ProfileHelper::Get()
           ->GetUserByProfile(profile_)
@@ -538,14 +544,14 @@ void AccountManagerMigrator::AddMigrationSteps() {
           chromeos::prefs::kSecondaryGoogleAccountSigninAllowed);
 
   if (is_secondary_google_account_signin_allowed) {
-    migration_runner_.AddStep(std::make_unique<ContentAreaAccountsMigration>(
+    migration_runner_->AddStep(std::make_unique<ContentAreaAccountsMigration>(
         account_manager, identity_manager));
 
     if (arc::IsArcProvisioned(profile_)) {
       // Add a migration step for ARC only if ARC has been provisioned. If ARC
       // has not been provisioned yet, there cannot be any accounts that need to
       // be migrated.
-      migration_runner_.AddStep(std::make_unique<ArcAccountsMigration>(
+      migration_runner_->AddStep(std::make_unique<ArcAccountsMigration>(
           account_manager, identity_manager,
           arc::ArcAuthService::GetForBrowserContext(
               profile_) /* arc_auth_service */));
@@ -557,14 +563,17 @@ void AccountManagerMigrator::AddMigrationSteps() {
 
   // This MUST be the last step. Check the class level documentation of
   // |SuccessStorage| for the reason.
-  migration_runner_.AddStep(
+  migration_runner_->AddStep(
       std::make_unique<SuccessStorage>(profile_->GetPrefs()));
 
   // TODO(sinhak): Verify Device Account LST state.
 }
 
 AccountMigrationRunner::Status AccountManagerMigrator::GetStatus() const {
-  return migration_runner_.GetStatus();
+  if (!migration_runner_)
+    return AccountMigrationRunner::Status::kNotStarted;
+
+  return migration_runner_->GetStatus();
 }
 
 base::Optional<AccountMigrationRunner::MigrationResult>
@@ -575,9 +584,9 @@ AccountManagerMigrator::GetLastMigrationRunResult() const {
 void AccountManagerMigrator::OnMigrationRunComplete(
     const AccountMigrationRunner::MigrationResult& result) {
   DCHECK_NE(AccountMigrationRunner::Status::kNotStarted,
-            migration_runner_.GetStatus());
+            migration_runner_->GetStatus());
   DCHECK_NE(AccountMigrationRunner::Status::kRunning,
-            migration_runner_.GetStatus());
+            migration_runner_->GetStatus());
 
   last_migration_run_result_ = base::make_optional(result);
 

chrome/browser/chromeos/account_manager/account_manager_migrator.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_CHROMEOS_ACCOUNT_MANAGER_ACCOUNT_MANAGER_MIGRATOR_H_
 #define CHROME_BROWSER_CHROMEOS_ACCOUNT_MANAGER_ACCOUNT_MANAGER_MIGRATOR_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
@@ -55,7 +57,7 @@ class AccountManagerMigrator : public KeyedService {
   Profile* const profile_;
 
   // Used for running migration steps.
-  chromeos::AccountMigrationRunner migration_runner_;
+  std::unique_ptr<AccountMigrationRunner> migration_runner_ = nullptr;
 
   // Stores if any migration steps were actually run. It is possible for the
   // migration flow to be a no-op, in which case this will be |false|.