[Passwords] Fix for showing the Save prompt after deleting the password

Bug: 1069376
Change-Id: Ied89b907d20859ea44ea4611f0705a9cd722a395
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2148602Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Vadym Doroshenko  <dvadym@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805789}

chrome/renderer/autofill/fake_mojo_password_manager_driver.cc

@@ -67,16 +67,12 @@ void FakeMojoPasswordManagerDriver::CheckSafeBrowsingReputation(
   called_check_safe_browsing_reputation_cnt_++;
 }
 
-void FakeMojoPasswordManagerDriver::ShowManualFallbackForSaving(
+void FakeMojoPasswordManagerDriver::InformAboutUserInput(
     const autofill::FormData& form_data) {
-  called_show_manual_fallback_for_saving_count_++;
+  called_inform_about_user_input_count_++;
   form_data_maybe_submitted_ = form_data;
 }
 
-void FakeMojoPasswordManagerDriver::HideManualFallbackForSaving() {
-  called_show_manual_fallback_for_saving_count_ = 0;
-}
-
 void FakeMojoPasswordManagerDriver::FocusedInputChanged(
     autofill::mojom::FocusedFieldType focused_field_type) {
   last_focused_field_type_ = focused_field_type;

chrome/renderer/autofill/fake_mojo_password_manager_driver.h

@@ -111,8 +111,8 @@ class FakeMojoPasswordManagerDriver
     return called_check_safe_browsing_reputation_cnt_;
   }
 
-  int called_show_manual_fallback_for_saving_count() const {
-    return called_show_manual_fallback_for_saving_count_;
+  int called_inform_about_user_input_count() const {
+    return called_inform_about_user_input_count_;
   }
 
   autofill::mojom::FocusedFieldType last_focused_field_type() const {
@@ -143,9 +143,8 @@ class FakeMojoPasswordManagerDriver
   void CheckSafeBrowsingReputation(const GURL& form_action,
                                    const GURL& frame_url) override;
 
-  void ShowManualFallbackForSaving(
-      const autofill::FormData& form_data) override;
-  void HideManualFallbackForSaving() override;
+  void InformAboutUserInput(const autofill::FormData& form_data) override;
+
   void FocusedInputChanged(
       autofill::mojom::FocusedFieldType focused_field_type) override;
   void LogFirstFillingResult(autofill::FormRendererId form_renderer_id,
@@ -181,9 +180,8 @@ class FakeMojoPasswordManagerDriver
   // Records number of times CheckSafeBrowsingReputation() gets called.
   int called_check_safe_browsing_reputation_cnt_ = 0;
 
-  // Records the number of request to show manual fallback for password saving.
-  // If it is zero, the fallback is not available.
-  int called_show_manual_fallback_for_saving_count_ = 0;
+  // Records the number of request to inform about user input.
+  int called_inform_about_user_input_count_ = 0;
 
   // Records the last focused field type that FocusedInputChanged() was called
   // with.

chrome/renderer/autofill/password_generation_agent_browsertest.cc

@@ -895,21 +895,20 @@ TEST_F(PasswordGenerationAgentTest, FallbackForSaving) {
   LoadHTMLWithUserGesture(kAccountCreationFormHTML);
   SimulateElementRightClick("first_password");
   SelectGenerationFallbackAndExpect(true);
-  EXPECT_EQ(0, fake_driver_.called_show_manual_fallback_for_saving_count());
+  EXPECT_EQ(0, fake_driver_.called_inform_about_user_input_count());
   base::string16 password = base::ASCIIToUTF16("random_password");
   EXPECT_CALL(fake_pw_client_, PresaveGeneratedPassword(_, Eq(password)))
       .WillOnce(testing::InvokeWithoutArgs([this]() {
         // Make sure that generation event was propagated to the browser before
         // the fallback showing. Otherwise, the fallback for saving provides a
         // save bubble instead of a confirmation bubble.
-        EXPECT_EQ(0,
-                  fake_driver_.called_show_manual_fallback_for_saving_count());
+        EXPECT_EQ(0, fake_driver_.called_inform_about_user_input_count());
       }));
   password_generation_->GeneratedPasswordAccepted(password);
   fake_driver_.Flush();
   // Two fallback requests are expected because generation changes either new
   // password and confirmation fields.
-  EXPECT_EQ(2, fake_driver_.called_show_manual_fallback_for_saving_count());
+  EXPECT_EQ(2, fake_driver_.called_inform_about_user_input_count());
 }
 
 TEST_F(PasswordGenerationAgentTest, FormClassifierDisabled) {

components/autofill/content/common/mojom/autofill_driver.mojom

@@ -96,13 +96,10 @@ interface PasswordManagerDriver {
   // Notification that this password form was submitted by the user.
   PasswordFormSubmitted(FormData form_data);
 
-  // Notification that a user starts typing in password fields and the omnibox
-  // icon with anchored save/update prompt should be available.
-  ShowManualFallbackForSaving(FormData form_data);
-
-  // Notification that there is no user input in password fields and the
-  // save/update prompt anchored to the omnibox icon should be removed.
-  HideManualFallbackForSaving();
+  // Notification that a user has modified a password field. This is fired both
+  // when typing new characters and deleting characters, so the password field
+  // in `form_data` may or may not be empty
+  InformAboutUserInput(FormData form_data);
 
   // Notification that same-document navigation happened. We use this as a
   // signal for successful login.

components/autofill/content/renderer/form_tracker.h

@@ -33,6 +33,7 @@ class FormTracker : public content::RenderFrameObserver {
       SELECT_CHANGED,
     };
 
+    // TODO(crbug.com/1126017): Find a better name for this method.
     // Invoked when form needs to be saved because of |source|, |element| is
     // valid if the callback caused by source other than
     // WILL_SEND_SUBMIT_EVENT, |form| is valid for the callback caused by

components/autofill/content/renderer/password_autofill_agent.cc

@@ -356,17 +356,6 @@ bool FormHasPasswordField(const FormData& form) {
   return false;
 }
 
-// Whether any of the fields in |form| is a non-empty password field.
-bool FormHasNonEmptyPasswordField(const FormData& form) {
-  for (const auto& field : form.fields) {
-    if (field.IsPasswordInputElement()) {
-      if (!field.value.empty() || !field.typed_value.empty())
-        return true;
-    }
-  }
-  return false;
-}
-
 void AnnotateFieldWithParsingResult(WebDocument doc,
                                     FieldRendererId renderer_id,
                                     const std::string& text) {
@@ -564,7 +553,7 @@ void PasswordAutofillAgent::UpdateStateForTextChange(
   field_data_manager_->UpdateFieldDataMap(element_id, element_value,
                                           FieldPropertiesFlags::kUserTyped);
 
-  ProvisionallySavePassword(element.Form(), element, RESTRICTION_NONE);
+  InformBrowserAboutUserInput(element.Form(), element);
 
   if (element.IsPasswordFieldForAutofill()) {
     auto iter = password_to_username_.find(element);
@@ -659,8 +648,7 @@ void PasswordAutofillAgent::FillPasswordFieldAndSave(
   DCHECK(password_input);
   DCHECK(password_input->IsPasswordFieldForAutofill());
   FillField(password_input, credential);
-  ProvisionallySavePassword(password_input->Form(), *password_input,
-                            RESTRICTION_NONE);
+  InformBrowserAboutUserInput(password_input->Form(), *password_input);
 }
 
 bool PasswordAutofillAgent::PreviewSuggestion(
@@ -1454,29 +1442,23 @@ void PasswordAutofillAgent::ClearPreview(WebInputElement* username,
     password->SetAutofillState(password_autofill_state_);
   }
 }
-void PasswordAutofillAgent::ProvisionallySavePassword(
+void PasswordAutofillAgent::InformBrowserAboutUserInput(
     const WebFormElement& form,
-    const WebInputElement& element,
-    ProvisionallySaveRestriction restriction) {
+    const WebInputElement& element) {
   DCHECK(!form.IsNull() || !element.IsNull());
+  if (!FrameCanAccessPasswordManager())
+    return;
   SetLastUpdatedFormAndField(form, element);
   std::unique_ptr<FormData> form_data =
-      (form.IsNull() ? GetFormDataFromUnownedInputElements()
-                     : GetFormDataFromWebForm(form));
+      form.IsNull() ? GetFormDataFromUnownedInputElements()
+                    : GetFormDataFromWebForm(form);
   if (!form_data)
     return;
 
-  bool has_password = FormHasNonEmptyPasswordField(*form_data);
-  if (restriction == RESTRICTION_NON_EMPTY_PASSWORD && !has_password)
+  if (!FormHasPasswordField(*form_data))
     return;
 
-  if (!FrameCanAccessPasswordManager())
-    return;
-
-  if (has_password)
-    GetPasswordManagerDriver()->ShowManualFallbackForSaving(*form_data);
-  else
-    GetPasswordManagerDriver()->HideManualFallbackForSaving();
+  GetPasswordManagerDriver()->InformAboutUserInput(*form_data);
 
   browser_has_form_to_process_ = true;
 }
@@ -1639,8 +1621,7 @@ void PasswordAutofillAgent::OnProvisionallySaveForm(
   }
 
   DCHECK_EQ(ElementChangeSource::WILL_SEND_SUBMIT_EVENT, source);
-  ProvisionallySavePassword(form, input_element,
-                            RESTRICTION_NON_EMPTY_PASSWORD);
+  InformBrowserAboutUserInput(form, input_element);
 }
 
 void PasswordAutofillAgent::OnFormSubmitted(const WebFormElement& form) {

components/autofill/content/renderer/password_autofill_agent.h

@@ -243,12 +243,6 @@ class PasswordAutofillAgent : public content::RenderFrameObserver,
  private:
   using OnPasswordField = util::StrongAlias<class OnPasswordFieldTag, bool>;
 
-  // Ways to restrict which passwords are saved in ProvisionallySavePassword.
-  enum ProvisionallySaveRestriction {
-    RESTRICTION_NONE,
-    RESTRICTION_NON_EMPTY_PASSWORD
-  };
-
   // Enumeration representing possible Touch To Fill states. This is used to
   // make sure that Touch To Fill will only be shown in response to the first
   // password form focus during a frame's life time and to suppress the soft
@@ -397,14 +391,12 @@ class PasswordAutofillAgent : public content::RenderFrameObserver,
   void FillPasswordFieldAndSave(blink::WebInputElement* password_input,
                                 const base::string16& credential);
 
-  // Saves |form| and |input| in |provisionally_saved_form_|, as long as it
-  // satisfies |restriction|. |form| and |input| are the elements user has just
-  // been interacting with before the form save. |form| or |input| can be null
-  // but not both at the same time. For example: if the form is unowned, |form|
-  // will be null; if the user has submitted the form, |input| will be null.
-  void ProvisionallySavePassword(const blink::WebFormElement& form,
-                                 const blink::WebInputElement& input,
-                                 ProvisionallySaveRestriction restriction);
+  // |form| and |input| are the elements user has just been interacting with
+  // before the form save. |form| or |input| can be null but not both at the
+  // same time. For example: if the form is unowned, |form| will be null; if the
+  // user has submitted the form, |input| will be null.
+  void InformBrowserAboutUserInput(const blink::WebFormElement& form,
+                                   const blink::WebInputElement& input);
 
   // This function attempts to fill |username_element| and |password_element|
   // with values from |fill_data|. The |username_element| and |password_element|

components/autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc

@@ -50,10 +50,7 @@ class FakeContentPasswordManagerDriver : public mojom::PasswordManagerDriver {
 
   void PasswordFormSubmitted(const autofill::FormData& form_data) override {}
 
-  void ShowManualFallbackForSaving(
-      const autofill::FormData& form_data) override {}
-
-  void HideManualFallbackForSaving() override {}
+  void InformAboutUserInput(const autofill::FormData& form_data) override {}
 
   void SameDocumentNavigation(autofill::mojom::SubmissionIndicatorEvent
                                   submission_indication_event) override {}

components/autofill/core/common/form_data.cc

@@ -142,6 +142,16 @@ bool FormData::IdentityComparator::operator()(const FormData& a,
                                       FormFieldData::IdentityComparator());
 }
 
+bool FormHasNonEmptyPasswordField(const FormData& form) {
+  for (const auto& field : form.fields) {
+    if (field.IsPasswordInputElement()) {
+      if (!field.value.empty() || !field.typed_value.empty())
+        return true;
+    }
+  }
+  return false;
+}
+
 std::ostream& operator<<(std::ostream& os, const FormData& form) {
   os << base::UTF16ToUTF8(form.name) << " " << form.url << " " << form.action
      << " " << form.main_frame_origin << " " << form.is_form_tag << " "

components/autofill/core/common/form_data.h

@@ -120,6 +120,9 @@ struct FormData {
 #endif
 };
 
+// Whether any of the fields in |form| is a non-empty password field.
+bool FormHasNonEmptyPasswordField(const FormData& form);
+
 // For testing.
 std::ostream& operator<<(std::ostream& os, const FormData& form);
 

components/password_manager/content/browser/bad_message.h

@@ -34,7 +34,7 @@ enum class BadMessageReason {
   CPMD_BAD_ORIGIN_PASSWORD_NO_LONGER_GENERATED = 6,
   CPMD_BAD_ORIGIN_PRESAVE_GENERATED_PASSWORD = 7,
   CPMD_BAD_ORIGIN_SAVE_GENERATION_FIELD_DETECTED_BY_CLASSIFIER = 8,
-  CPMD_BAD_ORIGIN_SHOW_FALLBACK_FOR_SAVING = 9,
+  CPMD_BAD_ORIGIN_UPON_USER_INPUT_CHANGE = 9,
   CPMD_BAD_ORIGIN_AUTOMATIC_GENERATION_STATUS_CHANGED = 10,
   CPMD_BAD_ORIGIN_SHOW_MANUAL_PASSWORD_GENERATION_POPUP = 11,
   CPMD_BAD_ORIGIN_SHOW_PASSWORD_EDITING_POPUP = 12,

components/password_manager/content/browser/content_password_manager_driver.cc

@@ -246,15 +246,16 @@ void ContentPasswordManagerDriver::PasswordFormSubmitted(
   LogSiteIsolationMetricsForSubmittedForm(render_frame_host_);
 }
 
-void ContentPasswordManagerDriver::ShowManualFallbackForSaving(
+void ContentPasswordManagerDriver::InformAboutUserInput(
     const autofill::FormData& form_data) {
   if (!password_manager::bad_message::CheckChildProcessSecurityPolicyForURL(
           render_frame_host_, form_data.url,
-          BadMessageReason::CPMD_BAD_ORIGIN_SHOW_FALLBACK_FOR_SAVING))
+          BadMessageReason::CPMD_BAD_ORIGIN_UPON_USER_INPUT_CHANGE))
     return;
-  GetPasswordManager()->ShowManualFallbackForSaving(this, form_data);
+  GetPasswordManager()->OnInformAboutUserInput(this, form_data);
 
-  if (client_->IsIsolationForPasswordSitesEnabled()) {
+  if (FormHasNonEmptyPasswordField(form_data) &&
+      client_->IsIsolationForPasswordSitesEnabled()) {
     // This function signals that a password field has been filled (whether by
     // the user, JS, autofill, or some other means) or a password form has been
     // submitted. Use this as a heuristic to start site-isolating the form's
@@ -266,10 +267,6 @@ void ContentPasswordManagerDriver::ShowManualFallbackForSaving(
   }
 }
 
-void ContentPasswordManagerDriver::HideManualFallbackForSaving() {
-  GetPasswordManager()->HideManualFallbackForSaving();
-}
-
 void ContentPasswordManagerDriver::SameDocumentNavigation(
     autofill::mojom::SubmissionIndicatorEvent submission_indication_event) {
   GetPasswordManager()->OnPasswordFormSubmittedNoChecks(

components/password_manager/content/browser/content_password_manager_driver.h

@@ -100,9 +100,7 @@ class ContentPasswordManagerDriver
       const std::vector<autofill::FormData>& visible_forms_data,
       bool did_stop_loading) override;
   void PasswordFormSubmitted(const autofill::FormData& form_data) override;
-  void ShowManualFallbackForSaving(
-      const autofill::FormData& form_data) override;
-  void HideManualFallbackForSaving() override;
+  void InformAboutUserInput(const autofill::FormData& form_data) override;
   void SameDocumentNavigation(autofill::mojom::SubmissionIndicatorEvent
                                   submission_indication_event) override;
   void RecordSavePasswordProgress(const std::string& log) override;

components/password_manager/core/browser/password_manager.cc

@@ -488,8 +488,8 @@ void PasswordManager::OnUserModifiedNonPasswordField(
                              renderer_id, value, base::Time::Now(), driver_id);
 }
 
-void PasswordManager::ShowManualFallbackForSaving(PasswordManagerDriver* driver,
-                                                  const FormData& form_data) {
+void PasswordManager::OnInformAboutUserInput(PasswordManagerDriver* driver,
+                                             const FormData& form_data) {
   PasswordFormManager* manager = ProvisionallySaveForm(form_data, driver, true);
 
   if (manager && form_data.is_gaia_with_skip_save_password_form) {
@@ -504,7 +504,7 @@ void PasswordManager::ShowManualFallbackForSaving(PasswordManagerDriver* driver,
   if (client_ && client_->GetMetricsRecorder())
     client_->GetMetricsRecorder()->RecordFormManagerAvailable(availability);
 
-  ShowManualFallbackForSavingImpl(manager, form_data);
+  ShowManualFallbackForSaving(manager, form_data);
 }
 
 void PasswordManager::HideManualFallbackForSaving() {
@@ -724,8 +724,7 @@ void PasswordManager::UpdateStateOnUserInput(
     if (manager->UpdateStateOnUserInput(form_id, field_id, field_value)) {
       ProvisionallySaveForm(*manager->observed_form(), driver, true);
       if (manager->is_submitted() && !manager->HasGeneratedPassword()) {
-        ShowManualFallbackForSavingImpl(manager.get(),
-                                        *manager->observed_form());
+        ShowManualFallbackForSaving(manager.get(), *manager->observed_form());
       } else {
         HideManualFallbackForSaving();
       }
@@ -1197,10 +1196,19 @@ void PasswordManager::TryToFindPredictionsToPossibleUsernameData() {
   }
 }
 
-void PasswordManager::ShowManualFallbackForSavingImpl(
+void PasswordManager::ShowManualFallbackForSaving(
     PasswordFormManager* form_manager,
     const FormData& form_data) {
-  if (!form_manager || !form_manager->is_submitted())
+  // Where `form_manager` is nullptr, make sure the manual fallback isn't
+  // shown. One scenario where this is relevant is when the user inputs some
+  // password and then removes it. Upon removing the password, the
+  // `form_manager` will become nullptr.
+  if (!form_manager) {
+    HideManualFallbackForSaving();
+    return;
+  }
+
+  if (!form_manager->is_submitted())
     return;
 
   if (!client_->GetProfilePasswordStore()->IsAbleToSavePasswords() ||

components/password_manager/core/browser/password_manager.h

@@ -166,10 +166,10 @@ class PasswordManager : public PasswordManagerInterface {
                                       autofill::FieldRendererId renderer_id,
                                       const base::string16& value);
 
-  // Handles a request to show manual fallback for password saving, i.e. the
-  // omnibox icon with the anchored hidden prompt.
-  void ShowManualFallbackForSaving(PasswordManagerDriver* driver,
-                                   const autofill::FormData& form_data);
+  // Handles user input and decides whether to show manual fallback for password
+  // saving, i.e. the omnibox icon with the anchored hidden prompt.
+  void OnInformAboutUserInput(PasswordManagerDriver* driver,
+                              const autofill::FormData& form_data);
 
   // Handles a request to hide manual fallback for password saving.
   void HideManualFallbackForSaving();
@@ -320,8 +320,8 @@ class PasswordManager : public PasswordManagerInterface {
 
   // Handles a request to show manual fallback for password saving, i.e. the
   // omnibox icon with the anchored hidden prompt. todo
-  void ShowManualFallbackForSavingImpl(PasswordFormManager* form_manager,
-                                       const autofill::FormData& form_data);
+  void ShowManualFallbackForSaving(PasswordFormManager* form_manager,
+                                   const autofill::FormData& form_data);
 
   // Returns the timeout for the disabling Password Manager's prompts.
   base::TimeDelta GetTimeoutForDisablingPrompts();

tools/metrics/histograms/enums.xml

@@ -6432,7 +6432,7 @@ Called by update_bad_message_reasons.py.-->
   <int value="7" label="CPMD_BAD_ORIGIN_PRESAVE_GENERATED_PASSWORD"/>
   <int value="8"
       label="CPMD_BAD_ORIGIN_SAVE_GENERATION_FIELD_DETECTED_BY_CLASSIFIER"/>
-  <int value="9" label="CPMD_BAD_ORIGIN_SHOW_FALLBACK_FOR_SAVING"/>
+  <int value="9" label="CPMD_BAD_ORIGIN_UPON_USER_INPUT_CHANGE"/>
   <int value="10" label="CPMD_BAD_ORIGIN_AUTOMATIC_GENERATION_STATUS_CHANGED"/>
   <int value="11"
       label="CPMD_BAD_ORIGIN_SHOW_MANUAL_PASSWORD_GENERATION_POPUP"/>

weblayer/browser/password_manager_driver_factory.cc

@@ -38,16 +38,16 @@ class PasswordManagerDriverFactory::PasswordManagerDriver
       const std::vector<autofill::FormData>& visible_forms_data,
       bool did_stop_loading) override {}
   void PasswordFormSubmitted(const autofill::FormData& form_data) override {}
-  void ShowManualFallbackForSaving(
-      const autofill::FormData& form_data) override {
+  void InformAboutUserInput(const autofill::FormData& form_data) override {
     if (!password_manager::bad_message::CheckChildProcessSecurityPolicyForURL(
             render_frame_host_, form_data.url,
             password_manager::BadMessageReason::
-                CPMD_BAD_ORIGIN_SHOW_FALLBACK_FOR_SAVING)) {
+                CPMD_BAD_ORIGIN_UPON_USER_INPUT_CHANGE)) {
       return;
     }
 
-    if (site_isolation::SiteIsolationPolicy::
+    if (FormHasNonEmptyPasswordField(form_data) &&
+        site_isolation::SiteIsolationPolicy::
             IsIsolationForPasswordSitesEnabled()) {
       // This function signals that a password field has been filled (whether by
       // the user, JS, autofill, or some other means) or a password form has
@@ -59,7 +59,6 @@ class PasswordManagerDriverFactory::PasswordManagerDriver
           form_data.url);
     }
   }
-  void HideManualFallbackForSaving() override {}
   void SameDocumentNavigation(autofill::mojom::SubmissionIndicatorEvent
                                   submission_indication_event) override {}
   void RecordSavePasswordProgress(const std::string& log) override {}