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 d23bbf1242dfed1c24e2b018cfc50db8a070bba8..038892db22eb3ffd42adefdc9fbba8720d4f5598 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -246,34 +246,42 @@ PasswordAutofillAgent::PasswordValueGatekeeper::PasswordValueGatekeeper()
 PasswordAutofillAgent::PasswordValueGatekeeper::~PasswordValueGatekeeper() {
 }
 
-void PasswordAutofillAgent::PasswordValueGatekeeper::RegisterElement(
-    blink::WebInputElement* element) {
-  if (was_user_gesture_seen_)
-    ShowValue(element);
-  else
-    elements_.push_back(*element);
+void PasswordAutofillAgent::PasswordValueGatekeeper::RegisterElementInfo(
+    PasswordInfo* element_info) {
+  DCHECK(element_info);
+  if (was_user_gesture_seen_) {
+    ShowValue(&element_info->password_field);
+  } else {
+    elements_info_.insert(element_info);
+  }
+}
+
+void PasswordAutofillAgent::PasswordValueGatekeeper::UnregisterElementInfo(
+    PasswordInfo* element_info) {
+  elements_info_.erase(element_info);
 }
 
 void PasswordAutofillAgent::PasswordValueGatekeeper::OnUserGesture() {
   was_user_gesture_seen_ = true;
 
-  for (std::vector<blink::WebInputElement>::iterator it = elements_.begin();
-       it != elements_.end();
+  for (std::set<PasswordInfo*>::iterator it = elements_info_.begin();
+       it != elements_info_.end();
        ++it) {
-    ShowValue(&(*it));
+    if (!(*it)->wait_for_username_change)

[engedy, 2014/07/31 17:14:46]

I would suggest keeping this logic out of PasswordValueGatekeeper.

In my understanding, what gets assigned to |suggested_value|, should
semantically be considered the "value" of the element already, it is just not
exposed to |value| so as to protect it from JS.

I think it should be the callers' responsibility not to latch a non-empty value
to |suggested_value| in the first place if they do not intend to overwrite it --
then we only need to make sure not to clear |value| on ShowValue() if
|suggested_value| is empty.

[vabr (Chromium), 2014/08/04 15:10:07]

I agree. I also realised that the callers, which need to care, already seem to
guard on wait_for_username_change, so I can just drop this check (and added
wiring).

Done.

+      ShowValue(&(*it)->password_field);
   }
 
-  elements_.clear();
+  elements_info_.clear();
 }
 
 void PasswordAutofillAgent::PasswordValueGatekeeper::Reset() {
   was_user_gesture_seen_ = false;
-  elements_.clear();
+  elements_info_.clear();
 }
 
 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,14 +292,15 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
   if (iter == login_to_password_info_.end())

[engedy, 2014/07/31 17:14:46]

nit: I would personally add an alias here and in TextDidChangeInTextField() to
increase readability, like:

PasswordInfo* password_info = iter->second;

[vabr (Chromium), 2014/08/04 15:10:07]

Done.

     return false;
 
-  const PasswordFormFillData& fill_data = iter->second.fill_data;
+  // Don't let autofill overwrite an explicit change made by the user.
+  if (iter->second->wait_for_username_change)
+    return false;
 
   // If wait_for_username is false, we should have filled when the text changed.
-  if (!fill_data.wait_for_username)
+  if (!iter->second->fill_data.wait_for_username)
     return false;
 
-  blink::WebInputElement password = iter->second.password_field;
-  if (!IsElementEditable(password))
+  if (!IsElementEditable(iter->second->password_field))
     return false;
 
   blink::WebInputElement username = element;  // We need a non-const.
@@ -299,8 +308,7 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
   // Do not set selection when ending an editing session, otherwise it can
   // mess with focus.
   FillUserNameAndPassword(&username,
-                          &password,
-                          fill_data,
+                          iter->second,
                           true /* exact_username_match */,
                           false /* set_selection */);
   return true;
@@ -308,6 +316,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,6 +328,15 @@ 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
+      // 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/07/31 17:14:46]

Perhaps this would be caught by tests, but we should double-check that this
event handler is not called when it is us who autofilled the password.

[vabr (Chromium), 2014/08/04 15:10:06]

I'm reasonably certain that TextDidChangeInTextField is called when the user
types in the input field:

Tracing this hook back to Blink leads me to
TextFieldInputType::didSetValueByUserEdit, which has a rather suggestive name.

+      mutable_element.setAutofilled(false);
+    }
     return false;
   }
 
@@ -327,17 +347,17 @@ bool PasswordAutofillAgent::TextDidChangeInTextField(
 
   // 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;

[engedy, 2014/07/31 17:14:46]

Hmm, let us discuss this logic tomorrow in person, I am not sure I understand.

[vabr (Chromium), 2014/08/04 15:10:07]

Acknowledged.

 
-  blink::WebInputElement password = iter->second.password_field;
+  blink::WebInputElement password = iter->second->password_field;
   if (password.isAutofilled()) {
     password.setValue(base::string16(), true);
     password.setAutofilled(false);
   }
 
   // If wait_for_username is true we will fill when the username loses focus.
-  if (iter->second.fill_data.wait_for_username)
+  if (iter->second->fill_data.wait_for_username)
     return false;
 
   if (!element.isText() || !IsElementAutocompletable(element) ||
@@ -348,8 +368,8 @@ bool PasswordAutofillAgent::TextDidChangeInTextField(
   // Don't inline autocomplete if the user is deleting, that would be confusing.
   // 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);
+  if (iter->second->backspace_pressed_last) {
+    ShowSuggestionPopup(iter->second->fill_data, element, false);
     return true;
   }
 
@@ -362,7 +382,7 @@ bool PasswordAutofillAgent::TextDidChangeInTextField(
     return false;
 
   // The caret position should have already been updated.
-  PerformInlineAutocomplete(element, password, iter->second.fill_data);
+  PerformInlineAutocomplete(&mutable_element, iter->second);
   return true;
 }
 
@@ -378,7 +398,7 @@ bool PasswordAutofillAgent::TextFieldHandlingKeyDown(
     return false;
 
   int win_key_code = event.windowsKeyCode;
-  iter->second.backspace_pressed_last =
+  iter->second->backspace_pressed_last =
       (win_key_code == ui::VKEY_BACK || win_key_code == ui::VKEY_DELETE);
   return true;
 }
@@ -388,21 +408,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())
+    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 +433,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 +449,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 +459,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;
 }
 
@@ -459,10 +480,10 @@ bool PasswordAutofillAgent::ShowSuggestions(
   // password form and that the request to show suggestions has been handled (as
   // a no-op).
   if (!IsElementAutocompletable(element) ||
-      !IsElementAutocompletable(iter->second.password_field))
+      !IsElementAutocompletable(iter->second->password_field))
     return true;
 
-  return ShowSuggestionPopup(iter->second.fill_data, element, show_all);
+  return ShowSuggestionPopup(iter->second->fill_data, element, show_all);
 }
 
 bool PasswordAutofillAgent::OriginCanAccessPasswordManager(
@@ -796,21 +817,23 @@ void PasswordAutofillAgent::OnFillPasswordForm(
     blink::WebInputElement password_element =
         form_elements->input_elements[form_data.basic_data.fields[1].name];
 
-    // If wait_for_username is true, we don't want to initially fill the form
-    // until the user types in a valid username.
-    if (!form_data.wait_for_username)
-      FillFormOnPasswordRecieved(form_data, username_element, password_element);
-
     // We might have already filled this form if there are two <form> elements
     // with identical markup.
     if (login_to_password_info_.find(username_element) !=
         login_to_password_info_.end())
       continue;
 
-    PasswordInfo password_info;
-    password_info.fill_data = form_data;
-    password_info.password_field = password_element;
-    login_to_password_info_[username_element] = password_info;
+    scoped_ptr<PasswordInfo> password_info(new PasswordInfo);
+    password_info->fill_data = form_data;
+    password_info->password_field = password_element;
+    login_to_password_info_[username_element] = password_info.get();
+    password_to_username_[password_element] = username_element;
+    // If wait_for_username is true, we don't want to initially fill the form
+    // until the user types in a valid username.
+    if (!form_data.wait_for_username)
+      FillFormOnPasswordRecieved(&username_element, password_info.get());
+    password_infos_[username_element.document().frame()].push_back(
+        make_linked_ptr(password_info.release()));
 
     FormData form;
     FormFieldData field;
@@ -828,6 +851,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,
@@ -901,45 +928,46 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
 }
 
 void PasswordAutofillAgent::FillFormOnPasswordRecieved(
-    const PasswordFormFillData& fill_data,
-    blink::WebInputElement username_element,
-    blink::WebInputElement password_element) {
+    blink::WebInputElement* username_element,
+    PasswordInfo* password_info) {
+  blink::WebInputElement* password_element = &password_info->password_field;
   // Do not fill if the password field is in an iframe.
-  DCHECK(password_element.document().frame());
-  if (password_element.document().frame()->parent())
+  DCHECK(password_element->document().frame());
+  if (password_element->document().frame()->parent())
     return;
 
   if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
-      !username_element.form().autoComplete())
+      !username_element->form().autoComplete())
     return;
 
   // If we can't modify the password, don't try to set the username
-  if (!IsElementAutocompletable(password_element))
+  if (!IsElementAutocompletable(*password_element))
     return;
 
   // Try to set the username to the preferred name, but only if the field
   // can be set and isn't prefilled.
-  if (IsElementAutocompletable(username_element) &&
-      username_element.value().isEmpty()) {
+  if (IsElementAutocompletable(*username_element) &&
+      username_element->value().isEmpty()) {
     // TODO(tkent): Check maxlength and pattern.
-    username_element.setValue(fill_data.basic_data.fields[0].value, true);
+    username_element->setValue(
+        password_info->fill_data.basic_data.fields[0].value, true);
   }
 
   // Fill if we have an exact match for the username. Note that this sets
   // username to autofilled.
-  FillUserNameAndPassword(&username_element,
-                          &password_element,
-                          fill_data,
+  FillUserNameAndPassword(username_element,
+                          password_info,
                           true /* exact_username_match */,
                           false /* set_selection */);
 }
 
 bool PasswordAutofillAgent::FillUserNameAndPassword(
     blink::WebInputElement* username_element,
-    blink::WebInputElement* password_element,
-    const PasswordFormFillData& fill_data,
+    PasswordInfo* password_info,
     bool exact_username_match,
     bool set_selection) {
+  blink::WebInputElement password_element = password_info->password_field;
+  const PasswordFormFillData& fill_data = password_info->fill_data;
   base::string16 current_username = username_element->value();
   // username and password will contain the match found if any.
   base::string16 username;
@@ -992,7 +1020,7 @@ bool PasswordAutofillAgent::FillUserNameAndPassword(
   // fields.
 
   // Don't fill username if password can't be set.
-  if (!IsElementAutocompletable(*password_element)) {
+  if (!IsElementAutocompletable(password_element)) {
     return false;
   }
 
@@ -1014,39 +1042,36 @@ bool PasswordAutofillAgent::FillUserNameAndPassword(
   // Wait to fill in the password until a user gesture occurs. This is to make
   // sure that we do not fill in the DOM with a password until we believe the
   // user is intentionally interacting with the page.
-  password_element->setSuggestedValue(password);
-  gatekeeper_.RegisterElement(password_element);
+  password_element.setSuggestedValue(password);
+  gatekeeper_.RegisterElementInfo(password_info);
 
-  password_element->setAutofilled(true);
+  password_element.setAutofilled(true);
   return true;
 }
 
 void PasswordAutofillAgent::PerformInlineAutocomplete(
-    const blink::WebInputElement& username_input,
-    const blink::WebInputElement& password_input,
-    const PasswordFormFillData& fill_data) {
-  DCHECK(!fill_data.wait_for_username);
+    blink::WebInputElement* username,
+    PasswordInfo* password_info) {
+  DCHECK(!password_info->fill_data.wait_for_username);
 
-  // We need non-const versions of the username and password inputs.
-  blink::WebInputElement username = username_input;
-  blink::WebInputElement password = password_input;
+  blink::WebInputElement password = password_info->password_field;
 
   // Don't inline autocomplete if the caret is not at the end.
   // TODO(jcivelli): is there a better way to test the caret location?
-  if (username.selectionStart() != username.selectionEnd() ||
-      username.selectionEnd() != static_cast<int>(username.value().length())) {
+  if (username->selectionStart() != username->selectionEnd() ||
+      username->selectionEnd() !=
+          static_cast<int>(username->value().length())) {
     return;
   }
 
   // Show the popup with the list of available usernames.
-  ShowSuggestionPopup(fill_data, username, false);
+  ShowSuggestionPopup(password_info->fill_data, *username, false);
 
 #if !defined(OS_ANDROID)
   // Fill the user and password field with the most relevant match. Android
   // only fills in the fields after the user clicks on the suggestion popup.
-  FillUserNameAndPassword(&username,
-                          &password,
-                          fill_data,
+  FillUserNameAndPassword(username,
+                          password_info,
                           false /* exact_username_match */,
                           true /* set_selection */);
 #endif
@@ -1055,11 +1080,15 @@ 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);
+      gatekeeper_.UnregisterElementInfo(iter->second);
       login_to_password_info_.erase(iter++);
-    else
+    } else {
       ++iter;
+    }
   }
+  password_infos_.erase(frame);
   for (FrameToPasswordFormMap::iterator iter =
            provisionally_saved_forms_.begin();
        iter != provisionally_saved_forms_.end();) {
@@ -1072,7 +1101,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;