[UPM] Fix crash on 2 migrations running at the same time.

This condition was checked for a migration following sync status
change only, as it was the obvious case, but there are other migration
types now, that can be combined in the case of the bad timing.
Not performing the reenrollment migration in this case is fine, we'll
attempt again on the next startup.

(cherry picked from commit 85dca53)

Bug: 1399845
Change-Id: I82525c4d078c44acffee25d8ea653451699d0ecb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4089970
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Maria Kazinova <kazinova@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1082369}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4110778
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Auto-Submit: Maria Kazinova <kazinova@google.com>
Cr-Commit-Position: refs/branch-heads/5414@{#748}
Cr-Branched-From: 4417ee5-refs/heads/main@{#1070088}

components/password_manager/core/browser/built_in_backend_to_android_backend_migrator.cc

@@ -153,6 +153,13 @@ void BuiltInBackendToAndroidBackendMigrator::StartMigrationIfNecessary(
           password_manager::prefs::kTimeOfLastMigrationAttempt));
   if (time_passed_since_last_migration_attempt < kMigrationThreshold)
     return;
+
+  // Do not migrate if a migration is already running. By the time it ends, the
+  // two backends will be identical, therefore the second migration won't be
+  // needed.
+  if (migration_in_progress_type_ != MigrationType::kNone)
+    return;
+
   MigrationType migration_type =
       GetMigrationType(should_attempt_upm_reenrollment);
   if (migration_type != MigrationType::kNone)
@@ -197,11 +204,7 @@ BuiltInBackendToAndroidBackendMigrator::GetMigrationType(
   // already transmitted through sync.
   // Once the local storage is supported, android backend becomes the only
   // active backend and there is no need to do this migration.
-  // Do not migrate if a migration is already running. By the time it ends, the
-  // two backends will be identical, therefore the second migration won't be
-  // needed.
   if (prefs_->GetBoolean(prefs::kRequiresMigrationAfterSyncStatusChange) &&
-      (migration_in_progress_type_ == MigrationType::kNone) &&
       !features::ManagesLocalPasswordsInUnifiedPasswordManager()) {
     return IsPasswordSyncEnabled(sync_service_)
                ? MigrationType::kNonSyncableToAndroidBackend

components/password_manager/core/browser/built_in_backend_to_android_backend_migrator_unittest.cc

@@ -962,4 +962,41 @@ TEST_F(BuiltInBackendToAndroidBackendMigratorWithMockAndroidBackendTest,
   RunUntilIdle();
 }
 
+TEST_F(BuiltInBackendToAndroidBackendMigratorWithMockAndroidBackendTest,
+       SecondMigrationCannotStartWhileTheFirstOneHasNotCompleted) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeatureWithParameters(
+      /*feature=*/features::kUnifiedPasswordManagerAndroid,
+      {{"migration_version", "1"}, {"stage", "0"}});
+  InitSyncService(/*is_password_sync_enabled=*/true);
+
+  // Add a form to the built-in backend to have something to migrate.
+  PasswordForm form = CreateTestPasswordForm();
+  built_in_backend().AddLoginAsync(form, base::DoNothing());
+
+  // Call StartMigrationIfNecessary for the first time.
+  migrator()->StartMigrationIfNecessary(
+      /*should_attempt_upm_reenrollment=*/false);
+  RunUntilIdle();
+
+  // If the user gets evicted from the experiment, migration-related prefs are
+  // cleared.
+  prefs()->ClearPref(password_manager::prefs::kTimeOfLastMigrationAttempt);
+
+  // Simulate some time passing before the second migration is triggered.
+  FastForwardBy(base::Milliseconds(123u));
+
+  // Call StartMigrationIfNecessary for the second time before the first
+  // migration finishes in an attempt to reenroll.
+  migrator()->StartMigrationIfNecessary(
+      /*should_attempt_upm_reenrollment=*/true);
+  RunUntilIdle();
+
+  // Check the recorded last migration attempt time. It should not be recorded
+  // after the pref was cleared, because the second migration should not be
+  // triggered.
+  EXPECT_EQ(0, prefs()->GetDouble(
+                   password_manager::prefs::kTimeOfLastMigrationAttempt));
+}
+
 }  // namespace password_manager