[AF] Multiple Nameless/Unowned forms

with repetitive names. The correct form should get filled.

Problem:
1. When an element would go invalid, we would use the section of
elements as one of the hints to find the new version of the old
element in the refill, but only the section of the autofilled fields
were passed. This would break the refills when the visibility of a
field changes. => Set and pass section of all fields.
2. In finding the new version of the old element, the case with more
than two elements with the matching names were not considered.
3. There was a bug in the logic for the nameless forms.

Fixes: https://jsfiddle.net/parastoog/tnh8j9ws/, ~/ds5h7kg4/,
~/w81yc6La/

Bug: 900185
Change-Id: I26fda7a50b3beb7f7c484a697633ab3b7b5a76d8
Reviewed-on: https://chromium-review.googlesource.com/c/1336492
Commit-Queue: Parastoo Geranmayeh <parastoog@google.com>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609921}

chrome/browser/autofill/autofill_interactive_uitest.cc

@@ -2231,6 +2231,84 @@ IN_PROC_BROWSER_TEST_F(AutofillInteractiveTest,
   EXPECT_EQ("red.swingline@initech.com", value) << "for second field";
 }
 
+// The following three tests verify that we can autofill forms with multiple
+// nameless forms, and repetitive field names and make sure that the dynamic
+// refill would not trigger a wrong refill, regardless of the form.
+IN_PROC_BROWSER_TEST_F(AutofillInteractiveTest,
+                       Dynamic_MultipleNoNameForms_BadNames_LastForm) {
+  CreateTestProfile();
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kAutofillDynamicForms);
+
+  GURL url = embedded_test_server()->GetURL(
+      "a.com", "/autofill/multiple_noname_forms_badnames.html");
+
+  ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
+
+  TriggerFormFill("firstname_3");
+  DoNothingAndWait(2);  // Wait to make sure possible refills have happened.
+  // Make sure the correct form was filled.
+  ExpectFieldValue("firstname_1", "");
+  ExpectFieldValue("lastname_1", "");
+  ExpectFieldValue("email_1", "");
+  ExpectFieldValue("firstname_2", "");
+  ExpectFieldValue("lastname_2", "");
+  ExpectFieldValue("email_2", "");
+  ExpectFieldValue("firstname_3", "Milton");
+  ExpectFieldValue("lastname_3", "Waddams");
+  ExpectFieldValue("email_3", "red.swingline@initech.com");
+}
+
+IN_PROC_BROWSER_TEST_F(AutofillInteractiveTest,
+                       Dynamic_MultipleNoNameForms_BadNames_SecondForm) {
+  CreateTestProfile();
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kAutofillDynamicForms);
+
+  GURL url = embedded_test_server()->GetURL(
+      "a.com", "/autofill/multiple_noname_forms_badnames.html");
+
+  ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
+
+  TriggerFormFill("firstname_2");
+  DoNothingAndWait(2);  // Wait to make sure possible refills have happened.
+  // Make sure the correct form was filled.
+  ExpectFieldValue("firstname_1", "");
+  ExpectFieldValue("lastname_1", "");
+  ExpectFieldValue("email_1", "");
+  ExpectFieldValue("firstname_2", "Milton");
+  ExpectFieldValue("lastname_2", "Waddams");
+  ExpectFieldValue("email_2", "red.swingline@initech.com");
+  ExpectFieldValue("firstname_3", "");
+  ExpectFieldValue("lastname_3", "");
+  ExpectFieldValue("email_3", "");
+}
+
+IN_PROC_BROWSER_TEST_F(AutofillInteractiveTest,
+                       Dynamic_MultipleNoNameForms_BadNames_FirstForm) {
+  CreateTestProfile();
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kAutofillDynamicForms);
+
+  GURL url = embedded_test_server()->GetURL(
+      "a.com", "/autofill/multiple_noname_forms_badnames.html");
+
+  ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
+
+  TriggerFormFill("firstname_1");
+  DoNothingAndWait(2);  // Wait to make sure possible refills have happened.
+  // Make sure the correct form was filled.
+  ExpectFieldValue("firstname_1", "Milton");
+  ExpectFieldValue("lastname_1", "Waddams");
+  ExpectFieldValue("email_1", "red.swingline@initech.com");
+  ExpectFieldValue("firstname_2", "");
+  ExpectFieldValue("lastname_2", "");
+  ExpectFieldValue("email_2", "");
+  ExpectFieldValue("firstname_3", "");
+  ExpectFieldValue("lastname_3", "");
+  ExpectFieldValue("email_3", "");
+}
+
 // Test that we can Autofill forms where some fields name change during the
 // fill.
 IN_PROC_BROWSER_TEST_P(AutofillCompanyInteractiveTest, FieldsChangeName) {
@@ -3024,6 +3102,38 @@ IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest,
   ExpectFieldValue("country_8", "US");
 }
 
+// Test that we can autofill forms that dynamically change the element that
+// has been clicked on, even though there are multiple forms with no name.
+IN_PROC_BROWSER_TEST_P(
+    AutofillDynamicFormInteractiveTest,
+    DynamicFormFill_FirstElementDisappearsMultipleNoNameForms) {
+  CreateTestProfile();
+
+  GURL url = embedded_test_server()->GetURL(
+      "a.com",
+      "/autofill/dynamic_form_element_invalid_multiple_noname_forms.html");
+
+  ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
+
+  TriggerFormFill("firstname_5");
+
+  // Wait for the re-fill to happen.
+  bool has_refilled = false;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      GetWebContents(), "hasRefilled()", &has_refilled));
+
+  ASSERT_TRUE(has_refilled);
+  // Make sure the second form was filled correctly, and the first form was left
+  // unfilled.
+  ExpectFieldValue("firstname_1", "");
+  ExpectFieldValue("firstname_2", "");
+  ExpectFieldValue("address1_3", "");
+  ExpectFieldValue("country_4", "CA");  // default
+  ExpectFieldValue("firstname_6", "Milton");
+  ExpectFieldValue("address1_7", "4120 Freidrich Lane");
+  ExpectFieldValue("country_8", "US");
+}
+
 // Test that we can autofill forms that dynamically change the element that
 // has been clicked on, even though the elements are unowned.
 IN_PROC_BROWSER_TEST_P(AutofillDynamicFormInteractiveTest,

chrome/browser/autofill/autofill_uitest.cc

@@ -170,6 +170,12 @@ void AutofillUiTest::SendKeyToPopupAndWait(
   widget->RemoveKeyPressEventCallback(key_press_event_sink_);
 }
 
+void AutofillUiTest::DoNothingAndWait(unsigned seconds) {
+  test_delegate()->Reset();
+  ASSERT_FALSE(test_delegate()->Wait({ObservedUiEvents::kNoEvent},
+                                     base::TimeDelta::FromSeconds(seconds)));
+}
+
 void AutofillUiTest::SendKeyToDataListPopup(ui::DomKey key) {
   ui::KeyboardCode key_code = ui::NonPrintableDomKeyToKeyboardCode(key);
   ui::DomCode code = ui::UsLayoutKeyboardCodeToDomCode(key_code);

chrome/browser/autofill/autofill_uitest.h

@@ -25,6 +25,7 @@ enum class ObservedUiEvents {
   kPreviewFormData,
   kFormDataFilled,
   kSuggestionShown,
+  kNoEvent,
 };
 
 class AutofillManagerTestDelegateImpl
@@ -71,6 +72,7 @@ class AutofillUiTest : public InProcessBrowserTest {
                             ui::DomCode code,
                             ui::KeyboardCode key_code,
                             std::list<ObservedUiEvents> expected_events);
+  void DoNothingAndWait(unsigned seconds);
   void SendKeyToPopup(content::RenderFrameHost* render_frame_host,
                       const ui::DomKey key);
   // Send key to the render host view's widget if |widget| is null.

chrome/test/data/autofill/dynamic_form_element_invalid_multiple_noname_forms.html

+<!-- A page that is used to test that a dynamic form fill feature works properly.
+Two nameless forms whose fields have identical names, and the name field goes
+invalid when the country is changed.-->
+<body>
+  <form>
+    Name: <input type="text" name="firstname" id="firstname_1" autocomplete="given-name"><br>
+   <input type="text" name="firstname" id="firstname_2" autocomplete="given-name" style="display: none"><br>
+    Address: <input type="text" name="address1" id="address1_3"><br>
+    Country: <select name="country" id="country_4" onchange="CountryChanged()">
+      <option value="CA">Canada</option>
+      <option value="US">United States</option>
+    </select>
+    <input type="reset" value="Reset">
+    <input type="submit" value="Submit" id="profile_submit">
+  </form>
+    <form>
+    Name: <input type="text" name="firstname" id="firstname_5" autocomplete="given-name"><br>
+   <input type="text" name="firstname" id="firstname_6" autocomplete="given-name" style="display: none"><br>
+    Address: <input type="text" name="address1" id="address1_7"><br>
+    Country: <select name="country" id="country_8" onchange="CountryChanged()">
+      <option value="CA">Canada</option>
+      <option value="US">United States</option>
+    </select>
+    <input type="reset" value="Reset">
+    <input type="submit" value="Submit" id="profile_submit">
+  </form>
+</body>
+
+<script>
+
+
+var notify_on_address_input_change = false;
+var address_input_changed = false;
+
+function CountryChanged() {
+  // Reset the value of the address field.
+  var address1 = document.getElementById('address1_7');
+  address1.value = '';
+  address1.onchange = function() {
+    if (notify_on_address_input_change)
+      window.domAutomationController.send(address1.value != '');
+    else
+      address_input_changed = true;
+  }
+
+  // Change the element that triggered the autofill. Remove it, and make another
+  // field visible. This is to test if the autofill can handle the case where
+  // the clicked on element is no longer valid.
+  var first_name_input = document.getElementById("firstname_5");
+  first_name_input.parentNode.removeChild(first_name_input);
+
+  var name_later = document.getElementById("firstname_6");
+  name_later.removeAttribute('style');
+}
+
+
+function hasRefilled() {
+  var address1 = document.getElementById('address1_7');
+  if (address1 && address_input_changed) {
+    window.domAutomationController.send(address1.value != '');
+  } else {
+    notify_on_address_input_change = true;
+  }
+}
+
+</script>
+

chrome/test/data/autofill/multiple_noname_forms_badnames.html

+<!-- A page that is used to test that a dynamic refill would not go wrong.
+Three nameless forms whose fields have identical names-->
+<body>
+
+<hr />
+
+<form>
+  Name:<input type="text" name="firstname" id="firstname_1" />
+  Last Name:<input type="text" name="lastname" id="lastname_1"/>
+  Email:<input type="text" name="email" id="email_1"/>
+</form>
+
+<hr />
+
+<form>
+  Name:<input type="text" name="firstname" id="firstname_2" />
+  Last Name:<input type="text" name="lastname" id="lastname_2"/>
+  Email:<input type="text" name="email" id="email_2"/>
+</form>
+<hr />
+
+<form>
+  Name:<input type="text" name="firstname" id="firstname_3" />
+  Last Name:<input type="text" name="lastname" id="lastname_3">
+  Email:<input type="text" name="email" id="email_3"/>
+</form>
+
+<hr />
+</body>

components/autofill/content/renderer/autofill_agent.cc

@@ -1106,32 +1106,44 @@ void AutofillAgent::OnFormNoLongerSubmittable() {
 }
 
 bool AutofillAgent::FindTheUniqueNewVersionOfOldElement(
-    WebVector<WebFormControlElement>& elements,
-    bool& element_found,
-    const WebString& original_element_section,
+    const WebVector<WebFormControlElement>& elements,
+    bool& potential_match_encountered,
+    WebFormControlElement& matching_element,
     const WebFormControlElement& original_element) {
+  if (original_element.IsNull())
+    return false;
+
+  const auto original_element_section = original_element.AutofillSection();
   for (const WebFormControlElement& current_element : elements) {
     if (current_element.IsFocusable() &&
         original_element.NameForAutofill() ==
             current_element.NameForAutofill()) {
-      if (!element_found) {
-        element_ = current_element;
-        element_found = true;
+      // If this is the first matching element, or is the first with the right
+      // section, this is the best match so far.
+      // In other words: bad, then good. => pick good.
+      if (!potential_match_encountered ||
+          (current_element.AutofillSection() == original_element_section &&
+           (matching_element.IsNull() ||
+            matching_element.AutofillSection() != original_element_section))) {
+        matching_element = current_element;
+        potential_match_encountered = true;
+      } else if (current_element.AutofillSection() !=
+                     original_element_section &&
+                 matching_element.AutofillSection() !=
+                     original_element_section) {
+        // The so far matching fields are equally bad. Continue the search if
+        // none of them have the correct section.
+        // In other words: bad, then bad => pick none.
+        matching_element.Reset();
       } else if (current_element.AutofillSection() ==
-                     element_.AutofillSection() ||
-                 (current_element.AutofillSection() !=
-                      original_element_section &&
-                  element_.AutofillSection() != original_element_section)) {
-        // If there are two elements that share the same name with the element_,
-        // and the section can't tell them apart, we can't decide between the
-        // two.
-        element_ = original_element;
+                     original_element_section &&
+                 matching_element.AutofillSection() ==
+                     original_element_section) {
+        // If two or more fields have the matching name and section, we can't
+        // decide. Two equally good fields => fail.
+        matching_element.Reset();
         return false;
-      } else if (current_element.AutofillSection() ==
-                 original_element_section) {
-        // If the current element has the right section, update the element_.
-        element_ = current_element;
-      }
+      }  // For the good, then bad case => keep good. Continue the search.
     }
   }
   return true;
@@ -1147,20 +1159,26 @@ void AutofillAgent::ReplaceElementIfNowInvalid(const FormData& original_form) {
   WebVector<WebFormElement> forms;
   WebVector<WebFormControlElement> elements;
 
+  const auto original_element = element_;
+  WebFormControlElement matching_element;
+  bool potential_match_encountered = false;
+
   if (original_form.name.empty()) {
     // If the form has no name, check all the forms.
-    bool element_found = false;
     element_.GetDocument().Forms(forms);
     for (const WebFormElement& form : forms) {
       form.GetFormControlElements(elements);
-      // If finding a unique element is impossible, return.
+      // If finding a unique element is impossible, don't look further.
       if (!FindTheUniqueNewVersionOfOldElement(
-              elements, element_found, element_.AutofillSection(), element_))
+              elements, potential_match_encountered, matching_element,
+              original_element))
         return;
     }
     // If the element is not found, we should still check for unowned elements.
-    if (element_found)
+    if (!matching_element.IsNull()) {
+      element_ = matching_element;
       return;
+    }
   }
 
   if (!element_.Form().IsNull()) {
@@ -1173,14 +1191,17 @@ void AutofillAgent::ReplaceElementIfNowInvalid(const FormData& original_form) {
   }
 
   WebFormElement form_element;
+  bool form_is_found = false;
   if (!original_form.name.empty()) {
     // Try to find the new version of the form.
     element_.GetDocument().Forms(forms);
     for (const WebFormElement& form : forms) {
       if (original_form.name == form.GetName().Utf16() ||
           original_form.name == form.GetAttribute("id").Utf16()) {
-        form_element = form;
-        break;
+        if (!form_is_found)
+          form_element = form;
+        else  // multiple forms with the matching name.
+          return;
       }
     }
   }
@@ -1190,20 +1211,23 @@ void AutofillAgent::ReplaceElementIfNowInvalid(const FormData& original_form) {
     std::vector<WebElement> fieldsets;
     elements = form_util::GetUnownedAutofillableFormFieldElements(
         element_.GetDocument().All(), &fieldsets);
-    bool element_found = false;
-    FindTheUniqueNewVersionOfOldElement(elements, element_found,
-                                        element_.AutofillSection(), element_);
+    // If a unique match was found.
+    if (FindTheUniqueNewVersionOfOldElement(
+            elements, potential_match_encountered, matching_element,
+            original_element) &&
+        !matching_element.IsNull()) {
+      element_ = matching_element;
+    }
     return;
   }
   // This is the case for owned fields that belong to the right named form.
   // Get all the elements of the new version of the form.
   form_element.GetFormControlElements(elements);
-  // Try to find the new version of the last interacted element.
-  for (const WebFormControlElement& element : elements) {
-    if (element_.NameForAutofill() == element.NameForAutofill()) {
-      element_ = element;
-      return;
-    }
+  // If a unique match was found.
+  if (FindTheUniqueNewVersionOfOldElement(elements, potential_match_encountered,
+                                          matching_element, original_element) &&
+      !matching_element.IsNull()) {
+    element_ = matching_element;
   }
 }
 

components/autofill/content/renderer/autofill_agent.h

@@ -29,7 +29,6 @@
 namespace blink {
 class WebNode;
 class WebView;
-class WebString;
 class WebFormControlElement;
 template <typename T>
 class WebVector;
@@ -273,12 +272,16 @@ class AutofillAgent : public content::RenderFrameObserver,
   void OnFormNoLongerSubmittable();
 
   // For no name forms, and unowned elements, try to see if there is a unique
-  // element in the updated form that corresponds to the old |element_|.
-  // Returns false if more than one element matches the |element_|.
+  // element in the updated form that corresponds to the |original_element|.
+  // Returns false if more than one element matches the |original_element|.
+  // Sets the matching element to |matching_element| and updates the
+  // |potential_match_encountered|, based on the search result. Returns false if
+  // more than one element match the name and section, therefore finding a
+  // unique match is impossible.
   bool FindTheUniqueNewVersionOfOldElement(
-      blink::WebVector<blink::WebFormControlElement>& elements,
-      bool& element_found,
-      const blink::WebString& original_element_section,
+      const blink::WebVector<blink::WebFormControlElement>& elements,
+      bool& potential_match_encountered,
+      blink::WebFormControlElement& matching_element,
       const blink::WebFormControlElement& original_element);
 
   // Check whether |element_| was removed or replaced dynamically on the page.

components/autofill/content/renderer/form_autofill_util.cc

@@ -882,6 +882,8 @@ void ForEachMatchingFormFieldCommon(
   // are appended to the end of the form and are not visible.
   for (size_t i = 0; i < control_elements->size(); ++i) {
     WebFormControlElement* element = &(*control_elements)[i];
+    element->SetAutofillSection(WebString::FromUTF8(data.fields[i].section));
+
     bool is_initiating_element = (*element == initiating_element);
 
     // Only autofill empty fields (or those with the field's default value
@@ -1000,7 +1002,6 @@ void FillFormField(const FormFieldData& data,
     return;
 
   field->SetAutofillState(WebAutofillState::kAutofilled);
-  field->SetAutofillSection(WebString::FromUTF8(data.section));
 
   if (is_initiating_node &&
       ((IsTextInput(input_element) || IsMonthInput(input_element)) ||

components/autofill/core/browser/autofill_manager.cc

@@ -1281,6 +1281,9 @@ void AutofillManager::FillOrPreviewDataModelForm(
       !is_refill && !is_credit_card;
 
   for (size_t i = 0; i < form_structure->field_count(); ++i) {
+    // On the renderer, the section is used regardless of the autofill status.
+    result.fields[i].section = form_structure->field(i)->section;
+
     if (form_structure->field(i)->section != autofill_field->section)
       continue;
 
@@ -1352,9 +1355,6 @@ void AutofillManager::FillOrPreviewDataModelForm(
     // will be sent to the renderer.
     FillFieldWithValue(cached_field, data_model, &result.fields[i],
                        should_notify, cvc);
-    // On the renderer, only the section of newly autofilled fields are updated.
-    if (result.fields[i].is_autofilled)
-      result.fields[i].section = form_structure->field(i)->section;
 
     if (!cached_field->IsVisible() && result.fields[i].is_autofilled) {
       AutofillMetrics::LogHiddenOrPresentationalSelectFieldsFilled();