Password autofill should not override explicitly typed password

components/autofill/content/renderer/password_autofill_agent.cc

Index: components/autofill/content/renderer/password_autofill_agent.cc
diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc
index fd52df402f4af45b7093dd304339c83e79794f98..68a12ca72f59b63bc59ad55c107dcf9ef4c222fd 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/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.wait_for_username_change)
+    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,26 @@ 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()) {
+      // The user changed the password after it was autofilled. Flipping the

[engedy, 2014/08/06 10:18:17]

nit: I would personally drop this comment, and integrate a summary into the
comment on new line 288.

[vabr (Chromium), 2014/08/25 14:53:29]

Comment dropped. The summary came out as a duplicate of the definition comment
of |password_was_edited_last|, so I dropped that as well.

+      // flag below to true makes sure it is not overwritten by autofill
+      // re-firing for the same username.
+      login_to_password_info_[iter->second].wait_for_username_change = true;

[engedy, 2014/08/06 10:18:17]

I think this is not really needed, but for clarity, we might consider resetting
the |suggested_value| here.

[vabr (Chromium), 2014/08/25 14:53:29]

Only made a comment to that effect. The suggested value is already reset in
HTMLInputElement::setValue.

+      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.wait_for_username_change = false;
 
   blink::WebInputElement password = iter->second.password_field;
   if (password.isAutofilled()) {
@@ -349,7 +365,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 +404,22 @@ 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();
+  if (username_element.value() != username_element.value())

[engedy, 2014/08/06 10:18:17]

If we are overwriting the password nevertheless, we might as well just reset the
flag too unconditionally.

And I think we should do both: This seems like a very explicit user action, and
I think the user expectation would be to re-autofill an accidentally edited
password.

[vabr (Chromium), 2014/08/25 14:53:29]

Good catch!
Agreed and done.

+    password_info->wait_for_username_change = 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 +429,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 +445,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 +455,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 +828,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 +846,10 @@ void PasswordAutofillAgent::OnSetLoggingState(bool active) {
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillAgent, private:
 
+PasswordAutofillAgent::PasswordInfo::PasswordInfo()
+    : backspace_pressed_last(false), wait_for_username_change(false) {
+}
+
 void PasswordAutofillAgent::GetSuggestions(
     const PasswordFormFillData& fill_data,
     const base::string16& input,
@@ -1055,10 +1077,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 +1096,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 +1110,7 @@ bool PasswordAutofillAgent::FindLoginInfo(const blink::WebNode& node,
     return false;
 
   *found_input = input;
-  *found_password = iter->second;
+  *found_password = &iter->second;
   return true;
 }