Password autofill should not override explicitly typed password

Imagine the following:
(1) A user types a username, which matches saved credentials, so the password is autofilled.
(2) The user overwrites the suggested password in the password field.
(3) The user enters the username field again, but leaves it without changes.

Currently, the autofilled password after step 3 replaces the user-typed password from step 2.

This CL makes sure, that the password from step 2 stays.

BUG=385619

Review URL: https://codereview.chromium.org/414013003

Cr-Commit-Position: refs/heads/master@{#291938}

chrome/renderer/autofill/password_autofill_agent_browsertest.cc

@@ -261,21 +261,25 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
     password_element_.setAutofilled(false);
   }
 
-  void SimulateUsernameChangeForElement(const std::string& username,
-                                        bool move_caret_to_end,
-                                        WebFrame* input_frame,
-                                        WebInputElement& username_input,
-                                        bool is_user_input) {
-    username_input.setValue(WebString::fromUTF8(username), is_user_input);
+  void SimulateDidEndEditing(WebFrame* input_frame, WebInputElement& input) {
+    autofill_agent_->textFieldDidEndEditing(input);
+  }
+
+  void SimulateInputChangeForElement(const std::string& new_value,
+                                     bool move_caret_to_end,
+                                     WebFrame* input_frame,
+                                     WebInputElement& input,
+                                     bool is_user_input) {
+    input.setValue(WebString::fromUTF8(new_value), is_user_input);
     // The field must have focus or AutofillAgent will think the
     // change should be ignored.
-    while (!username_input.focused())
+    while (!input.focused())
       input_frame->document().frame()->view()->advanceFocus(false);
     if (move_caret_to_end)
-      username_input.setSelectionRange(username.length(), username.length());
+      input.setSelectionRange(new_value.length(), new_value.length());
     if (is_user_input)
       password_autofill_agent()->FirstUserGestureObserved();
-    autofill_agent_->textFieldDidChange(username_input);
+    autofill_agent_->textFieldDidChange(input);
     // Processing is delayed because of a Blink bug:
     // https://bugs.webkit.org/show_bug.cgi?id=16976
     // See PasswordAutofillAgent::TextDidChangeInTextField() for details.
@@ -310,11 +314,11 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
   void SimulateUsernameChange(const std::string& username,
                               bool move_caret_to_end,
                               bool is_user_input = false) {
-    SimulateUsernameChangeForElement(username,
-                                     move_caret_to_end,
-                                     GetMainFrame(),
-                                     username_element_,
-                                     is_user_input);
+    SimulateInputChangeForElement(username,
+                                  move_caret_to_end,
+                                  GetMainFrame(),
+                                  username_element_,
+                                  is_user_input);
   }
 
   // Tests that no suggestion popup is generated when the username_element_ is
@@ -368,7 +372,8 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
     EXPECT_EQ(password,
               static_cast<std::string>(
                   checkSuggestedValue ? password_element.suggestedValue().utf8()
-                                      : password_element.value().utf8()));
+                                      : password_element.value().utf8()))
+        << "checkSuggestedValue == " << checkSuggestedValue;
     EXPECT_EQ(password_autofilled, password_element.isAutofilled());
   }
 
@@ -920,7 +925,7 @@ TEST_F(PasswordAutofillAgentTest, IframeNoFillTest) {
 
   // Simulate the user typing in the username in the iframe which should cause
   // an autofill.
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       kAliceUsername, true, iframe, username_input, true);
 
   CheckTextFieldsStateForElements(username_input,
@@ -1495,9 +1500,9 @@ TEST_F(PasswordAutofillAgentTest, CredentialsOnClick) {
 // still remember the password typed by the user.
 TEST_F(PasswordAutofillAgentTest,
        RememberLastNonEmptyPasswordOnSubmit_ScriptCleared) {
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       "temp", true, GetMainFrame(), username_element_, true);
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       "random", true, GetMainFrame(), password_element_, true);
 
   // Simulate that the password value was cleared by the site's JavaScript
@@ -1516,13 +1521,13 @@ TEST_F(PasswordAutofillAgentTest,
 // the last non-empty password is not remembered.
 TEST_F(PasswordAutofillAgentTest,
        RememberLastNonEmptyPasswordOnSubmit_UserCleared) {
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       "temp", true, GetMainFrame(), username_element_, true);
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       "random", true, GetMainFrame(), password_element_, true);
 
   // Simulate that the user actually cleared the password again.
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       "", true, GetMainFrame(), password_element_, true);
   static_cast<content::RenderViewObserver*>(password_autofill_agent())
       ->WillSubmitForm(GetMainFrame(), username_element_.form());
@@ -1545,9 +1550,9 @@ TEST_F(PasswordAutofillAgentTest,
   LoadHTML(kNewPasswordFormHTML);
   UpdateUsernameAndPasswordElements();
 
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       "temp", true, GetMainFrame(), username_element_, true);
-  SimulateUsernameChangeForElement(
+  SimulateInputChangeForElement(
       "random", true, GetMainFrame(), password_element_, true);
 
   // Simulate that the password value was cleared by the site's JavaScript
@@ -1561,4 +1566,62 @@ TEST_F(PasswordAutofillAgentTest,
   ExpectFormSubmittedWithPasswords("", "random");
 }
 
+// The user first accepts a suggestion, but then overwrites the password. This
+// test checks that the overwritten password is not reverted back if the user
+// triggers autofill through focusing (but not changing) the username again.
+TEST_F(PasswordAutofillAgentTest,
+       NoopEditingDoesNotOverwriteManuallyEditedPassword) {
+  // Simulate having credentials which needed to wait until the user starts
+  // typing the username to be filled (e.g., PSL-matched credentials). Those are
+  // the ones which can be filled as a result of TextFieldDidEndEditing.
+  fill_data_.wait_for_username = true;
+  SimulateOnFillPasswordForm(fill_data_);
+  // Simulate that the user typed her name to make the autofill work.
+  SimulateInputChangeForElement(kAliceUsername,
+                                /*move_caret_to_end=*/true,
+                                GetMainFrame(),
+                                username_element_,
+                                /*is_user_input=*/true);
+  SimulateDidEndEditing(GetMainFrame(), username_element_);
+  const std::string old_username(username_element_.value().utf8());
+  const std::string old_password(password_element_.value().utf8());
+  const std::string new_password(old_password + "modify");
+
+  // The user changes the password.
+  SimulateInputChangeForElement(new_password,
+                                /*move_caret_to_end=*/true,
+                                GetMainFrame(),
+                                password_element_,
+                                /*is_user_input=*/true);
+
+  // The user switches back into the username field, but leaves that without
+  // changes.
+  SimulateDidEndEditing(GetMainFrame(), username_element_);
+
+  // The password should have stayed as the user changed it.
+  CheckTextFieldsDOMState(old_username, true, new_password, false);
+  // The password should not have a suggested value.
+  CheckTextFieldsState(old_username, true, std::string(), false);
+}
+
+TEST_F(PasswordAutofillAgentTest,
+       InlineAutocompleteOverwritesManuallyEditedPassword) {
+  // Simulate the browser sending back the login info.
+  SimulateOnFillPasswordForm(fill_data_);
+
+  ClearUsernameAndPasswordFields();
+
+  // The user enters a password
+  SimulateInputChangeForElement("someOtherPassword",
+                                /*move_caret_to_end=*/true,
+                                GetMainFrame(),
+                                password_element_,
+                                /*is_user_input=*/true);
+
+  // Simulate the user typing a stored username.
+  SimulateUsernameChange(kAliceUsername, true);
+  // The autofileld password should replace the typed one.
+  CheckTextFieldsDOMState(kAliceUsername, true, kAlicePassword, true);
+}
+
 }  // namespace autofill

components/autofill/content/renderer/password_autofill_agent.cc

@@ -273,7 +273,7 @@ void PasswordAutofillAgent::PasswordValueGatekeeper::Reset() {
 
 void PasswordAutofillAgent::PasswordValueGatekeeper::ShowValue(
     blink::WebInputElement* element) {
-  if (!element->isNull() && !element->suggestedValue().isNull())
+  if (!element->isNull() && !element->suggestedValue().isEmpty())
     element->setValue(element->suggestedValue(), true);
 }
 
@@ -284,13 +284,18 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
   if (iter == login_to_password_info_.end())
     return false;
 
-  const PasswordFormFillData& fill_data = iter->second.fill_data;
+  const PasswordInfo& password_info = iter->second;
+  // Don't let autofill overwrite an explicit change made by the user.
+  if (password_info.password_was_edited_last)
+    return false;
+
+  const PasswordFormFillData& fill_data = password_info.fill_data;
 
   // If wait_for_username is false, we should have filled when the text changed.
   if (!fill_data.wait_for_username)
     return false;
 
-  blink::WebInputElement password = iter->second.password_field;
+  blink::WebInputElement password = password_info.password_field;
   if (!IsElementEditable(password))
     return false;
 
@@ -308,6 +313,9 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
 
 bool PasswordAutofillAgent::TextDidChangeInTextField(
     const blink::WebInputElement& element) {
+  // TODO(vabr): Get a mutable argument instead. http://crbug.com/397083
+  blink::WebInputElement mutable_element = element;  // We need a non-const.
+
   if (element.isPasswordField()) {
     // Some login forms have event handlers that put a hash of the password into
     // a hidden field and then clear the password (http://crbug.com/28910,
@@ -317,18 +325,25 @@ bool PasswordAutofillAgent::TextDidChangeInTextField(
     // password will be saved here.
     ProvisionallySavePassword(
         element.document().frame(), element.form(), RESTRICTION_NONE);
+
+    PasswordToLoginMap::iterator iter = password_to_username_.find(element);
+    if (iter != password_to_username_.end()) {
+      login_to_password_info_[iter->second].password_was_edited_last = true;
+      // Note that the suggested value of |mutable_element| was reset when its
+      // value changed.
+      mutable_element.setAutofilled(false);
+    }
     return false;
   }
 
-  LoginToPasswordInfoMap::const_iterator iter =
-      login_to_password_info_.find(element);
+  LoginToPasswordInfoMap::iterator iter = login_to_password_info_.find(element);
   if (iter == login_to_password_info_.end())
     return false;
 
   // The input text is being changed, so any autofilled password is now
   // outdated.
-  blink::WebInputElement username = element;  // We need a non-const.
-  username.setAutofilled(false);
+  mutable_element.setAutofilled(false);
+  iter->second.password_was_edited_last = false;
 
   blink::WebInputElement password = iter->second.password_field;
   if (password.isAutofilled()) {
@@ -349,7 +364,7 @@ bool PasswordAutofillAgent::TextDidChangeInTextField(
   // But refresh the popup.  Note, since this is ours, return true to signal
   // no further processing is required.
   if (iter->second.backspace_pressed_last) {
-    ShowSuggestionPopup(iter->second.fill_data, username, false);
+    ShowSuggestionPopup(iter->second.fill_data, element, false);
     return true;
   }
 
@@ -388,21 +403,21 @@ bool PasswordAutofillAgent::FillSuggestion(
     const blink::WebString& username,
     const blink::WebString& password) {
   blink::WebInputElement username_element;
-  PasswordInfo password_info;
+  PasswordInfo* password_info;
 
   if (!FindLoginInfo(node, &username_element, &password_info) ||
       !IsElementAutocompletable(username_element) ||
-      !IsElementAutocompletable(password_info.password_field)) {
+      !IsElementAutocompletable(password_info->password_field)) {
     return false;
   }
 
-  base::string16 current_username = username_element.value();
+  password_info->password_was_edited_last = false;
   username_element.setValue(username, true);
   username_element.setAutofilled(true);
   username_element.setSelectionRange(username.length(), username.length());
 
-  password_info.password_field.setValue(password, true);
-  password_info.password_field.setAutofilled(true);
+  password_info->password_field.setValue(password, true);
+  password_info->password_field.setAutofilled(true);
 
   return true;
 }
@@ -412,11 +427,11 @@ bool PasswordAutofillAgent::PreviewSuggestion(
     const blink::WebString& username,
     const blink::WebString& password) {
   blink::WebInputElement username_element;
-  PasswordInfo password_info;
+  PasswordInfo* password_info;
 
   if (!FindLoginInfo(node, &username_element, &password_info) ||
       !IsElementAutocompletable(username_element) ||
-      !IsElementAutocompletable(password_info.password_field)) {
+      !IsElementAutocompletable(password_info->password_field)) {
     return false;
   }
 
@@ -428,9 +443,9 @@ bool PasswordAutofillAgent::PreviewSuggestion(
       username_selection_start_,
       username_element.suggestedValue().length());
 
-  was_password_autofilled_ = password_info.password_field.isAutofilled();
-  password_info.password_field.setSuggestedValue(password);
-  password_info.password_field.setAutofilled(true);
+  was_password_autofilled_ = password_info->password_field.isAutofilled();
+  password_info->password_field.setSuggestedValue(password);
+  password_info->password_field.setAutofilled(true);
 
   return true;
 }
@@ -438,11 +453,11 @@ bool PasswordAutofillAgent::PreviewSuggestion(
 bool PasswordAutofillAgent::DidClearAutofillSelection(
     const blink::WebNode& node) {
   blink::WebInputElement username_element;
-  PasswordInfo password_info;
+  PasswordInfo* password_info;
   if (!FindLoginInfo(node, &username_element, &password_info))
     return false;
 
-  ClearPreview(&username_element, &password_info.password_field);
+  ClearPreview(&username_element, &password_info->password_field);
   return true;
 }
 
@@ -811,6 +826,7 @@ void PasswordAutofillAgent::OnFillPasswordForm(
     password_info.fill_data = form_data;
     password_info.password_field = password_element;
     login_to_password_info_[username_element] = password_info;
+    password_to_username_[password_element] = username_element;
 
     FormData form;
     FormFieldData field;
@@ -828,6 +844,10 @@ void PasswordAutofillAgent::OnSetLoggingState(bool active) {
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillAgent, private:
 
+PasswordAutofillAgent::PasswordInfo::PasswordInfo()
+    : backspace_pressed_last(false), password_was_edited_last(false) {
+}
+
 void PasswordAutofillAgent::GetSuggestions(
     const PasswordFormFillData& fill_data,
     const base::string16& input,
@@ -1055,10 +1075,12 @@ void PasswordAutofillAgent::PerformInlineAutocomplete(
 void PasswordAutofillAgent::FrameClosing(const blink::WebFrame* frame) {
   for (LoginToPasswordInfoMap::iterator iter = login_to_password_info_.begin();
        iter != login_to_password_info_.end();) {
-    if (iter->first.document().frame() == frame)
+    if (iter->first.document().frame() == frame) {
+      password_to_username_.erase(iter->second.password_field);
       login_to_password_info_.erase(iter++);
-    else
+    } else {
       ++iter;
+    }
   }
   for (FrameToPasswordFormMap::iterator iter =
            provisionally_saved_forms_.begin();
@@ -1072,7 +1094,7 @@ void PasswordAutofillAgent::FrameClosing(const blink::WebFrame* frame) {
 
 bool PasswordAutofillAgent::FindLoginInfo(const blink::WebNode& node,
                                           blink::WebInputElement* found_input,
-                                          PasswordInfo* found_password) {
+                                          PasswordInfo** found_password) {
   if (!node.isElementNode())
     return false;
 
@@ -1086,7 +1108,7 @@ bool PasswordAutofillAgent::FindLoginInfo(const blink::WebNode& node,
     return false;
 
   *found_input = input;
-  *found_password = iter->second;
+  *found_password = &iter->second;
   return true;
 }
 

components/autofill/content/renderer/password_autofill_agent.h

@@ -94,15 +94,19 @@ class PasswordAutofillAgent : public content::RenderViewObserver {
     blink::WebInputElement password_field;
     PasswordFormFillData fill_data;
     bool backspace_pressed_last;
-    PasswordInfo() : backspace_pressed_last(false) {}
+    // The user manually edited the password more recently than the username was
+    // changed.
+    bool password_was_edited_last;
+    PasswordInfo();
   };
   typedef std::map<blink::WebElement, PasswordInfo> LoginToPasswordInfoMap;
+  typedef std::map<blink::WebElement, blink::WebElement> PasswordToLoginMap;
   typedef std::map<blink::WebFrame*,
                    linked_ptr<PasswordForm> > FrameToPasswordFormMap;
 
-  // This class holds a vector of autofilled password input elements and makes
-  // sure the autofilled password value is not accessible to JavaScript code
-  // until the user interacts with the page.
+  // This class keeps track of autofilled password input elements and makes sure
+  // the autofilled password value is not accessible to JavaScript code until
+  // the user interacts with the page.
   class PasswordValueGatekeeper {
    public:
     PasswordValueGatekeeper();
@@ -190,7 +194,7 @@ class PasswordAutofillAgent : public content::RenderViewObserver {
   // Finds login information for a |node| that was previously filled.
   bool FindLoginInfo(const blink::WebNode& node,
                      blink::WebInputElement* found_input,
-                     PasswordInfo* found_password);
+                     PasswordInfo** found_password);
 
   // Clears the preview for the username and password fields, restoring both to
   // their previous filled state.
@@ -210,6 +214,8 @@ class PasswordAutofillAgent : public content::RenderViewObserver {
 
   // The logins we have filled so far with their associated info.
   LoginToPasswordInfoMap login_to_password_info_;
+  // A (sort-of) reverse map to |login_to_password_info_|.
+  PasswordToLoginMap password_to_username_;
 
   // Used for UMA stats.
   OtherPossibleUsernamesUsage usernames_usage_;