[Password Manager] Manual fallbacks don't set the |provisional_save_manager|

When showing the manual fallbacks, we create a PasswordFormManager
and pass it to the UI. Previously, this was done by creating a provisional
save manager and moving it, but if we failed to move it, it would remain and
signal that a submission in the previous navigation.

I've refactored the showing of the manual fallbacks to have no side effects
on |provisional_save_manager|.

Bug: 859156
Change-Id: Iaf41b201bc6c812f483fdb355f8414a77b0a034f
Reviewed-on: https://chromium-review.googlesource.com/1138322
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576036}

components/password_manager/core/browser/password_manager.cc

@@ -251,6 +251,27 @@ void LogShouldShouldPromptComparison(bool should_show_prompt_old,
                             outcome);
 }
 
+// Returns true if the user needs to be prompted before a password can be
+// saved (instead of automatically saving the password), based on inspecting
+// the state of |manager|.
+bool ShouldPromptUserToSavePassword(const PasswordFormManager& manager) {
+  if (IsPasswordUpdate(manager)) {
+    // Updating a credential might erase a useful stored value by accident.
+    // Always ask the user to confirm.
+    return true;
+  }
+
+  // User successfully signed-in with PSL match credentials. These credentials
+  // should be automatically saved in order to be autofilled on next login.
+  if (manager.IsPendingCredentialsPublicSuffixMatch())
+    return false;
+
+  if (manager.has_generated_password())
+    return false;
+
+  return manager.IsNewLogin();
+}
+
 }  // namespace
 
 // static
@@ -550,15 +571,17 @@ void PasswordManager::ShowManualFallbackForSaving(
       FormFetcher::State::WAITING) {
     return;
   }
-  ProvisionallySaveManager(password_form, matched_manager, nullptr);
+
+  std::unique_ptr<PasswordFormManager> manager = matched_manager->Clone();
+  PasswordForm form(password_form);
+  form.preferred = true;
+  manager->ProvisionallySave(form);
 
   // Show the fallback if a prompt or a confirmation bubble should be available.
-  bool has_generated_password =
-      provisional_save_manager_->has_generated_password();
-  if (ShouldPromptUserToSavePassword() || has_generated_password) {
-    DCHECK(provisional_save_manager_);
-    bool is_update = IsPasswordUpdate(*provisional_save_manager_);
-    client_->ShowManualFallbackForSaving(std::move(provisional_save_manager_),
+  bool has_generated_password = manager->has_generated_password();
+  if (ShouldPromptUserToSavePassword(*manager) || has_generated_password) {
+    bool is_update = IsPasswordUpdate(*manager);
+    client_->ShowManualFallbackForSaving(std::move(manager),
                                          has_generated_password, is_update);
     matched_manager->GetMetricsRecorder()->RecordShowManualFallbackForSaving(
         has_generated_password, is_update);
@@ -808,24 +831,6 @@ bool PasswordManager::ShouldBlockPasswordForSameOriginButDifferentScheme(
          !new_origin.SchemeIsCryptographic();
 }
 
-bool PasswordManager::ShouldPromptUserToSavePassword() const {
-  if (IsPasswordUpdate(*provisional_save_manager_)) {
-    // Updating a credential might erase a useful stored value by accident.
-    // Always ask the user to confirm.
-    return true;
-  }
-
-  // User successfully signed-in with PSL match credentials. These credentials
-  // should be automatically saved in order to be autofilled on next login.
-  if (provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch())
-    return false;
-
-  if (provisional_save_manager_->has_generated_password())
-    return false;
-
-  return provisional_save_manager_->IsNewLogin();
-}
-
 bool PasswordManager::ShouldPromptUserToSavePasswordOld() const {
   return (provisional_save_manager_->IsNewLogin() ||
           provisional_save_manager_
@@ -957,9 +962,10 @@ void PasswordManager::OnLoginSuccessful() {
   DCHECK(!provisional_save_manager_->GetPendingCredentials()
               .only_for_fallback_saving);
 
-  LogShouldShouldPromptComparison(ShouldPromptUserToSavePasswordOld(),
-                                  ShouldPromptUserToSavePassword());
-  if (ShouldPromptUserToSavePassword()) {
+  LogShouldShouldPromptComparison(
+      ShouldPromptUserToSavePasswordOld(),
+      ShouldPromptUserToSavePassword(*provisional_save_manager_));
+  if (ShouldPromptUserToSavePassword(*provisional_save_manager_)) {
     bool empty_password = provisional_save_manager_->GetPendingCredentials()
                               .username_value.empty();
     UMA_HISTOGRAM_BOOLEAN("PasswordManager.EmptyUsernames.OfferedToSave",

components/password_manager/core/browser/password_manager.h

@@ -172,6 +172,10 @@ class PasswordManager : public LoginModel, public FormSubmissionObserver {
   const std::vector<std::unique_ptr<NewPasswordFormManager>>& form_managers() {
     return form_managers_;
   }
+
+  const PasswordFormManager* provisional_save_manager() {
+    return provisional_save_manager_.get();
+  }
 #endif
 
   NavigationEntryToCheck entry_to_check() const { return entry_to_check_; }
@@ -208,12 +212,6 @@ class PasswordManager : public LoginModel, public FormSubmissionObserver {
   bool ShouldBlockPasswordForSameOriginButDifferentScheme(
       const autofill::PasswordForm& form) const;
 
-  // Returns true if the user needs to be prompted before a password can be
-  // saved (instead of automatically saving
-  // the password), based on inspecting the state of
-  // |provisional_save_manager_|.
-  bool ShouldPromptUserToSavePassword() const;
-
   // The old version of ShouldPromptUserToSavePassword, it is left for
   // comparison and metric sending.
   // TODO(crbug.com/856543): Remove it after M-70.

components/password_manager/core/browser/password_manager_unittest.cc

@@ -44,6 +44,8 @@ using base::ASCIIToUTF16;
 using base::TestMockTimeTaskRunner;
 using testing::_;
 using testing::AnyNumber;
+using testing::IsNull;
+using testing::NotNull;
 using testing::Return;
 using testing::ReturnRef;
 using testing::SaveArg;
@@ -2552,4 +2554,39 @@ TEST_F(PasswordManagerTest, CreatingFormManagers) {
   EXPECT_EQ(1u, manager()->form_managers().size());
 }
 
+TEST_F(PasswordManagerTest,
+       ShowManualFallbacksDontChangeProvisionalSaveManager) {
+  EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
+      .WillRepeatedly(Return(true));
+
+  std::vector<PasswordForm> observed;
+  PasswordForm form(MakeSimpleForm());
+  observed.push_back(form);
+  EXPECT_CALL(*store_, GetLogins(_, _))
+      .WillRepeatedly(WithArg<1>(InvokeConsumer(form)));
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(2);
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+  manager()->OnPasswordFormsRendered(&driver_, observed, true);
+
+  EXPECT_THAT(manager()->provisional_save_manager(), IsNull());
+  manager()->ShowManualFallbackForSaving(&driver_, form);
+  EXPECT_THAT(manager()->provisional_save_manager(), IsNull());
+
+  // The user submits the form and a provisional save manager is set.
+  OnPasswordFormSubmitted(form);
+
+  EXPECT_THAT(manager()->provisional_save_manager(), NotNull());
+  const PasswordFormManager* last_provisional_save_manager =
+      manager()->provisional_save_manager();
+
+  EXPECT_CALL(client_, HideManualFallbackForSaving());
+  // The call to manual fallback with |form| equal to already saved should close
+  // the fallback.
+  manager()->ShowManualFallbackForSaving(&driver_, form);
+
+  EXPECT_THAT(manager()->provisional_save_manager(), NotNull());
+  EXPECT_EQ(last_provisional_save_manager,
+            manager()->provisional_save_manager());
+}
+
 }  // namespace password_manager