[Password Manager] Autofill forms with field name and id attributes missing.

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 3c48077a45dcd4a1a17296963002730cb223f91c..ed60cb7d235ca01c2db662a4f736183e4e1adee4 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -55,6 +55,10 @@ const char kFillOnAccountSelectFieldTrialEnabledWithHighlightGroup[] =
 const char kFillOnAccountSelectFieldTrialEnabledWithNoHighlightGroup[] =
     "EnableWithNoHighlight";
 
+const char kUsernameField[] = "anonymous_username";
+const char kPasswordField[] = "anonymous_password";
+const char kInputTypePassword[] = "password";

[vabr (Chromium), 2015/08/26 09:36:01]

nit: You can just inline this constant in the helper function below. The type
name will not change frequently, as it is part of the HTML norm.

[Pritam Nikam, 2015/08/26 13:25:41]

Done.

+
 // Maps element names to the actual elements to simplify form filling.
 typedef std::map<base::string16, blink::WebInputElement> FormInputElementMap;
 
@@ -81,44 +85,80 @@ bool FillDataContainsFillableUsername(const PasswordFormFillData& fill_data) {
           !fill_data.username_field.value.empty());
 }
 
+// Returns true if password form has username and password fields with either
+// same or no name and id attributes supplied.
+bool PasswordFormWithAmbiguousOrNoNameAndIdAttributes(
+    const PasswordFormFillData& fill_data) {
+  return fill_data.username_field.name == fill_data.password_field.name;
+}
+
+bool IsPasswordField(const FormFieldData& field) {
+  return (field.form_control_type == kInputTypePassword);
+}
+
+// Returns the |field|'s autofillable name. If no name or id attribute is
+// specified returns a dummy name.
+base::string16 FieldName(const FormFieldData& field,
+                         bool ambiguousOrNoNameAndIdAttribute) {

[vabr (Chromium), 2015/08/26 09:36:00]

style: Please use lowercase_with_underscore, not camelCase. (Also below.)

[Pritam Nikam, 2015/08/26 13:25:41]

Done.

+  return ambiguousOrNoNameAndIdAttribute
+             ? IsPasswordField(field) ? base::ASCIIToUTF16(kPasswordField)
+                                      : base::ASCIIToUTF16(kUsernameField)
+             : field.name;
+}
+
+// Helper function to check whether the |element| qualifies for autofilling.
+bool IsFillableInputElement(const blink::WebInputElement& element,
+                            const FormFieldData& field) {
+  return element.hasHTMLTagName("input") && element.isTextField() &&
+         element.isPasswordField() == IsPasswordField(field);
+}
+
+// Helper function to get the password form control elements. On supplied with

[vabr (Chromium), 2015/08/26 09:36:00]

Please reformulate the second sentence. You probably mean something like: "If
|field_name| is ... then only returns the fields with that name." Also, it is
not clear, what is meant by "valid". Do you mean non-empty?

[Pritam Nikam, 2015/08/26 13:25:40]

Done.

valid -> non-empty and non-ambiguous.

+// valid |field_name| gets the elements matching it.

[vabr (Chromium), 2015/08/26 09:36:00]

You should comment on what the bool argument is used for (and possibly update
the whole comment if it is outdated).

[Pritam Nikam, 2015/08/26 13:25:41]

Done.

+void GetFormControlElements(
+    blink::WebFormElement* form_element,
+    const base::string16& field_name,
+    bool ambiguousOrNoNameAndIdAttribute,
+    blink::WebVector<blink::WebFormControlElement>* elements) {
+  ambiguousOrNoNameAndIdAttribute

[vabr (Chromium), 2015/08/26 09:36:00]

Please use if-else here, do not create a value just to throw it out and only use
its side-effects.

[Pritam Nikam, 2015/08/26 13:25:41]

Done.

+      ? form_element->getFormControlElements(*elements)
+      : form_element->getNamedElements(
+            field_name, (blink::WebVector<blink::WebNode>&)*elements);
+}
+
 // Utility function to find the unique entry of the |form_element| for the
 // specified input |field|. On successful find, adds it to |result| and returns
 // |true|. Otherwise clears the references from each |HTMLInputElement| from
 // |result| and returns |false|.
 bool FindFormInputElement(blink::WebFormElement* form_element,
                           const FormFieldData& field,
+                          bool ambiguousOrNoNameAndIdAttribute,
                           FormElements* result) {
-  blink::WebVector<blink::WebNode> temp_elements;
-  form_element->getNamedElements(field.name, temp_elements);
+  blink::WebVector<blink::WebFormControlElement> input_elements;
+  GetFormControlElements(form_element, field.name,
+                         ambiguousOrNoNameAndIdAttribute, &input_elements);
 
   // Match the first input element, if any.
-  // |getNamedElements| may return non-input elements where the names match,
-  // so the results are filtered for input elements.
-  // If more than one match is made, then we have ambiguity (due to misuse
-  // of "name" attribute) so is it considered not found.
+  // |GetFormControlElements| may return non-input elements where the names
+  // match, so the results are filtered for input elements.
   bool found_input = false;
-  for (size_t i = 0; i < temp_elements.size(); ++i) {
-    if (temp_elements[i].to<blink::WebElement>().hasHTMLTagName("input")) {
+  base::string16 field_name = FieldName(field, ambiguousOrNoNameAndIdAttribute);
+  for (size_t i = 0; i < input_elements.size(); ++i) {
+    blink::WebInputElement input_element =
+        input_elements[i].to<blink::WebInputElement>();
+    if (IsFillableInputElement(input_element, field)) {
       // Check for a non-unique match.
       if (found_input) {
         found_input = false;
         break;
       }
 
-      // Only fill saved passwords into password fields and usernames into
-      // text fields.
-      blink::WebInputElement input_element =
-          temp_elements[i].to<blink::WebInputElement>();
-      if (input_element.isPasswordField() !=
-          (field.form_control_type == "password"))
-        continue;
-
       // This element matched, add it to our temporary result. It's possible
       // there are multiple matches, but for purposes of identifying the form
       // one suffices and if some function needs to deal with multiple
       // matching elements it can get at them through the FormElement*.
       // Note: This assignment adds a reference to the InputElement.
-      result->input_elements[field.name] = input_element;
+      result->input_elements[field_name] = input_element;
       found_input = true;
     }
   }
@@ -180,15 +220,20 @@ bool ShouldHighlightFields() {
 // |data|, and add results to |result|.
 bool FindFormInputElements(blink::WebFormElement* form_element,
                            const PasswordFormFillData& data,
+                           bool ambiguousOrNoNameAndIdAttribute,
                            FormElements* result) {
-  return FindFormInputElement(form_element, data.password_field, result) &&
-         (!FillDataContainsFillableUsername(data) ||
-          FindFormInputElement(form_element, data.username_field, result));
+  return FindFormInputElement(form_element, data.password_field,
+                              ambiguousOrNoNameAndIdAttribute, result) &&
+         (!(ambiguousOrNoNameAndIdAttribute ||
+            FillDataContainsFillableUsername(data)) ||
+          FindFormInputElement(form_element, data.username_field,
+                               ambiguousOrNoNameAndIdAttribute, result));
 }
 
 // Helper to locate form elements identified by |data|.
 void FindFormElements(content::RenderFrame* render_frame,
                       const PasswordFormFillData& data,
+                      bool ambiguousOrNoNameAndIdAttribute,
                       FormElementsList* results) {
   DCHECK(results);
 
@@ -210,8 +255,10 @@ void FindFormElements(content::RenderFrame* render_frame,
       continue;
 
     scoped_ptr<FormElements> curr_elements(new FormElements);
-    if (!FindFormInputElements(&fe, data, curr_elements.get()))
+    if (!FindFormInputElements(&fe, data, ambiguousOrNoNameAndIdAttribute,
+                               curr_elements.get())) {
       continue;
+    }
 
     // We found the right element.
     // Note: this assignment adds a reference to |fe|.
@@ -468,8 +515,10 @@ bool FillFormOnPasswordReceived(
   if (!IsElementAutocompletable(password_element))
     return false;
 
-  bool form_contains_fillable_username_field =
-      FillDataContainsFillableUsername(fill_data);
+  bool ambiguousOrNoNameAndIdAttribute =
+      PasswordFormWithAmbiguousOrNoNameAndIdAttributes(fill_data);
+  base::string16 username_field_name =
+      FieldName(fill_data.username_field, ambiguousOrNoNameAndIdAttribute);
   // If the form contains an autocompletable username field, try to set the
   // username to the preferred name, but only if:
   //   (a) The fill-on-account-select flag is not set, and
@@ -486,7 +535,7 @@ bool FillFormOnPasswordReceived(
   // in the "no highlighting" group.
   //
   // In all other cases, do nothing.
-  bool form_has_fillable_username = form_contains_fillable_username_field &&
+  bool form_has_fillable_username = !username_field_name.empty() &&
                                     IsElementAutocompletable(username_element);
 
   if (ShouldFillOnAccountSelect()) {
@@ -1164,27 +1213,31 @@ void PasswordAutofillAgent::LegacyDidStartProvisionalLoad(
 void PasswordAutofillAgent::OnFillPasswordForm(
     int key,
     const PasswordFormFillData& form_data) {
-
+  bool ambiguousOrNoNameAndIdAttribute =
+      PasswordFormWithAmbiguousOrNoNameAndIdAttributes(form_data);
   FormElementsList forms;
   // We own the FormElements* in forms.
-  FindFormElements(render_frame(), form_data, &forms);
+  FindFormElements(render_frame(), form_data, ambiguousOrNoNameAndIdAttribute,
+                   &forms);
   FormElementsList::iterator iter;
   for (iter = forms.begin(); iter != forms.end(); ++iter) {
     scoped_ptr<FormElements> form_elements(*iter);
 
+    base::string16 username_field_name =
+        FieldName(form_data.username_field, ambiguousOrNoNameAndIdAttribute);
+    base::string16 password_field_name =
+        FieldName(form_data.password_field, ambiguousOrNoNameAndIdAttribute);
+
     // Attach autocomplete listener to enable selecting alternate logins.
     blink::WebInputElement username_element, password_element;
 
     // Check whether the password form has a username input field.
-    bool form_contains_fillable_username_field =
-        FillDataContainsFillableUsername(form_data);
-    if (form_contains_fillable_username_field) {
-      username_element =
-          form_elements->input_elements[form_data.username_field.name];
+    if (!username_field_name.empty()) {
+      username_element = form_elements->input_elements[username_field_name];
     }
 
     // No password field, bail out.
-    if (form_data.password_field.name.empty())
+    if (password_field_name.empty())
       break;
 
     // We might have already filled this form if there are two <form> elements
@@ -1195,8 +1248,7 @@ void PasswordAutofillAgent::OnFillPasswordForm(
 
     // Get pointer to password element. (We currently only support single
     // password forms).
-    password_element =
-        form_elements->input_elements[form_data.password_field.name];
+    password_element = form_elements->input_elements[password_field_name];
 
     // If wait_for_username is true, we don't want to initially fill the form
     // until the user types in a valid username.