[Autofill] Migrated refill logic to renderer IDs.

This CL replaces the usage of the form's name (<form>'s name or id
attribute) and Autofill::unique_name() in the refill logic with
unique form renderer IDs, if the feature
AutofillRefillWithRendererIds.

Bug: 896689, 1114787
Change-Id: Ib21b838f9dc5f31ac30087e347d7e1b0171aff58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2235600
Commit-Queue: Christoph Schwering <schwering@google.com>
Reviewed-by: Matthias Körber <koerber@google.com>
Cr-Commit-Position: refs/heads/master@{#825311}

chrome/browser/autofill/autofill_interactive_uitest.cc

@@ -2905,8 +2905,23 @@ class AutofillDynamicFormInteractiveTest : public AutofillInteractiveTestBase {
   }
 };
 
+class AutofillDynamicFormReplacementInteractiveTest
+    : public AutofillInteractiveTestBase,
+      public testing::WithParamInterface<bool> {
+ public:
+  AutofillDynamicFormReplacementInteractiveTest()
+      : refill_with_renderer_ids_(GetParam()) {
+    scoped_feature_.InitWithFeatureState(
+        features::kAutofillRefillWithRendererIds, refill_with_renderer_ids_);
+  }
+
+ protected:
+  bool refill_with_renderer_ids_;
+  base::test::ScopedFeatureList scoped_feature_;
+};
+
 // Test that we can Autofill dynamically generated forms.
-IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
+IN_PROC_BROWSER_TEST_P(AutofillDynamicFormReplacementInteractiveTest,
                        DynamicChangingFormFill) {
   CreateTestProfile();
 
@@ -2914,6 +2929,12 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
       embedded_test_server()->GetURL("a.com", "/autofill/dynamic_form.html");
   ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
 
+  // TODO(crbug/896689): Cleanup feature, also in JS code.
+  if (refill_with_renderer_ids_) {
+    ASSERT_TRUE(content::ExecuteScript(
+        GetWebContents(), "window.kAutofillRefillWithRendererIds = true;"));
+  }
+
   TriggerFormFill("firstname");
 
   // Wait for the re-fill to happen.
@@ -2932,7 +2953,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
   ExpectFieldValue("phone_form1", "15125551234");
 }
 
-IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
+IN_PROC_BROWSER_TEST_P(AutofillDynamicFormReplacementInteractiveTest,
                        TwoDynamicChangingFormsFill) {
   // Setup that the test expects a re-fill to happen.
   test_delegate()->SetIsExpectingDynamicRefill(true);
@@ -2943,6 +2964,12 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
                                             "/autofill/two_dynamic_forms.html");
   ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
 
+  // TODO(crbug/896689): Cleanup feature, also in JS code.
+  if (refill_with_renderer_ids_) {
+    ASSERT_TRUE(content::ExecuteScript(
+        GetWebContents(), "window.kAutofillRefillWithRendererIds = true;"));
+  }
+
   TriggerFormFill("firstname_form1");
 
   // Wait for the re-fill to happen.
@@ -2979,7 +3006,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
 }
 
 // Test that forms that dynamically change a second time do not get filled.
-IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
+IN_PROC_BROWSER_TEST_P(AutofillDynamicFormReplacementInteractiveTest,
                        DynamicChangingFormFill_SecondChange) {
   CreateTestProfile();
 
@@ -2987,6 +3014,12 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
       "a.com", "/autofill/double_dynamic_form.html");
   ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
 
+  // TODO(crbug/896689): Cleanup feature, also in JS code.
+  if (refill_with_renderer_ids_) {
+    ASSERT_TRUE(content::ExecuteScript(
+        GetWebContents(), "window.kAutofillRefillWithRendererIds = true;"));
+  }
+
   TriggerFormFill("firstname");
 
   // Wait for two dynamic changes to happen.
@@ -3033,7 +3066,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
 }
 
 // Test that only field of a type group that was filled initially get refilled.
-IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
+IN_PROC_BROWSER_TEST_P(AutofillDynamicFormReplacementInteractiveTest,
                        DynamicChangingFormFill_AddsNewFieldTypeGroups) {
   CreateTestProfile();
 
@@ -3041,6 +3074,12 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
       "a.com", "/autofill/dynamic_form_new_field_types.html");
   ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
 
+  // TODO(crbug/896689): Cleanup feature, also in JS code.
+  if (refill_with_renderer_ids_) {
+    ASSERT_TRUE(content::ExecuteScript(
+        GetWebContents(), "window.kAutofillRefillWithRendererIds = true;"));
+  }
+
   TriggerFormFill("firstname");
 
   // Wait for the dynamic change to happen.
@@ -3294,7 +3333,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
 }
 
 // Test that credit card fields are re-filled.
-IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
+IN_PROC_BROWSER_TEST_P(AutofillDynamicFormReplacementInteractiveTest,
                        DynamicChangingFormFill_AlsoForCreditCard) {
   CreateTestCreditCart();
 
@@ -3303,6 +3342,12 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
                                     "/autofill/dynamic_form_credit_card.html");
   ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
 
+  // TODO(crbug/896689): Cleanup feature, also in JS code.
+  if (refill_with_renderer_ids_) {
+    ASSERT_TRUE(content::ExecuteScript(
+        GetWebContents(), "window.kAutofillRefillWithRendererIds = true;"));
+  }
+
   // Trigger the initial fill.
   FocusFieldByName("cc-name");
   SendKeyToPageAndWait(ui::DomKey::FromCharacter('M'), ui::DomCode::US_M,
@@ -3383,7 +3428,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
 
 // Test that we can Autofill dynamically generated forms with no name if the
 // NameForAutofill of the first field matches.
-IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
+IN_PROC_BROWSER_TEST_P(AutofillDynamicFormReplacementInteractiveTest,
                        DynamicChangingFormFill_FormWithoutName) {
   CreateTestProfile();
 
@@ -3391,6 +3436,12 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
       "a.com", "/autofill/dynamic_form_no_name.html");
   ASSERT_NO_FATAL_FAILURE(ui_test_utils::NavigateToURL(browser(), url));
 
+  // TODO(crbug/896689): Cleanup feature, also in JS code.
+  if (refill_with_renderer_ids_) {
+    ASSERT_TRUE(content::ExecuteScript(
+        GetWebContents(), "window.kAutofillRefillWithRendererIds = true;"));
+  }
+
   TriggerFormFill("firstname");
 
   // Wait for the re-fill to happen.
@@ -3495,6 +3546,10 @@ IN_PROC_BROWSER_TEST_F(AutofillDynamicFormInteractiveTest,
   ExpectFieldValue("phone", "15125551234");
 }
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         AutofillDynamicFormReplacementInteractiveTest,
+                         testing::Bool());
+
 INSTANTIATE_TEST_SUITE_P(All,
                          AutofillRestrictUnownedFieldsTest,
                          testing::Bool());

chrome/test/data/autofill/dynamic_form_credit_card.html

@@ -44,7 +44,9 @@ var notify_on_credit_card_name_input_appear = false;
 function ExpChanged() {
   RemoveForm("cc1");
 
-  var new_form = document.createElement("form");
+  var new_form =
+    (window.kAutofillRefillWithRendererIds && document.getElementById('cc1')) ||
+     document.createElement('form');
   new_form.setAttribute('method',"post");
   //new_form.setAttribute('action',"https://example.com/")
   new_form.setAttribute('name',"cc1.1");
@@ -119,4 +121,4 @@ function hasRefilled() {
   }
 }
 
-</script>
+</script>
\ No newline at end of file

chrome/test/data/autofill/dynamic_form_utils.js

@@ -7,14 +7,18 @@
 /** Removes the initial form. */
 function RemoveForm(form_id) {
   var initial_form = document.getElementById(form_id);
-  initial_form.parentNode.removeChild(initial_form);
   initial_form.innerHTML = '';
-  initial_form.remove();
+  if (!window.kAutofillRefillWithRendererIds) {
+    initial_form.parentNode.removeChild(initial_form);
+    initial_form.remove();
+  }
 }
 
 /** Adds a new form and fields for the dynamic form. */
 function AddNewFormAndFields(form_id, form_name) {
-  var new_form = document.createElement('form');
+  var new_form = (window.kAutofillRefillWithRendererIds &&
+                  document.getElementById(form_id)) ||
+                 document.createElement('form');
   new_form.setAttribute('method', 'post');
   new_form.setAttribute('action', 'https://example.com/')
   new_form.setAttribute('name', form_name);
@@ -72,4 +76,4 @@ function AddNewFormAndFields(form_id, form_name) {
   new_form.appendChild(i);
 
   return new_form;
-}
+}
\ No newline at end of file

components/autofill/core/browser/autofill_manager.cc

@@ -451,7 +451,9 @@ AutofillManager::FillingContext::FillingContext(
                             *optional_credit_card,
                             optional_cvc ? *optional_cvc : base::string16()))
                       : base::nullopt),
-      filled_field_name(field.unique_name()),
+      filled_field_renderer_id(field.unique_renderer_id),
+      filled_field_signature(field.GetFieldSignature()),
+      filled_field_unique_name(field.unique_name()),
       original_fill_time(AutofillTickClock::NowTicks()) {
   DCHECK(optional_profile || optional_credit_card);
   DCHECK(optional_credit_card || !optional_cvc);
@@ -1613,7 +1615,8 @@ void AutofillManager::Reset() {
   credit_card_action_ = AutofillDriver::FORM_DATA_ACTION_PREVIEW;
   initial_interaction_timestamp_ = TimeTicks();
   external_delegate_->Reset();
-  filling_contexts_map_.clear();
+  filling_context_by_renderer_id_.clear();
+  filling_context_by_unique_name_.clear();
 }
 
 AutofillManager::AutofillManager(
@@ -1745,9 +1748,9 @@ void AutofillManager::FillOrPreviewDataModelForm(
   DCHECK_EQ(form_structure->field_count(), form.fields.size());
 
   if (action == AutofillDriver::FORM_DATA_ACTION_FILL && !is_refill) {
-    filling_contexts_map_[form_structure->GetIdentifierForRefill()] =
-        std::make_unique<FillingContext>(*autofill_field, optional_profile,
-                                         optional_credit_card, optional_cvc);
+    SetFillingContext(*form_structure, std::make_unique<FillingContext>(
+                                           *autofill_field, optional_profile,
+                                           optional_credit_card, optional_cvc));
   }
 
   // Only record the types that are filled for an eventual refill if all the
@@ -1756,11 +1759,7 @@ void AutofillManager::FillOrPreviewDataModelForm(
   //  A form with the given name is already filled.
   //  A refill has not been attempted for that form yet.
   //  This fill is not a refill attempt.
-  FillingContext* filling_context = nullptr;
-  auto itr =
-      filling_contexts_map_.find(form_structure->GetIdentifierForRefill());
-  if (itr != filling_contexts_map_.end())
-    filling_context = itr->second.get();
+  FillingContext* filling_context = GetFillingContext(*form_structure);
   bool could_attempt_refill = filling_context != nullptr &&
                               !filling_context->attempted_refill && !is_refill;
 
@@ -2100,10 +2099,8 @@ void AutofillManager::OnFormsParsed(const std::vector<const FormData*>& forms,
     // been a refill attempt on that form yet, start the process of triggering a
     // refill.
     if (ShouldTriggerRefill(*form_structure)) {
-      auto itr =
-          filling_contexts_map_.find(form_structure->GetIdentifierForRefill());
-      DCHECK(itr != filling_contexts_map_.end());
-      FillingContext* filling_context = itr->second.get();
+      FillingContext* filling_context = GetFillingContext(*form_structure);
+      DCHECK(filling_context != nullptr);
 
       // If a timer for the refill was already running, it means the form
       // changed again. Stop the timer and start it again.
@@ -2470,18 +2467,44 @@ void AutofillManager::FillFieldWithValue(AutofillField* autofill_field,
   }
 }
 
+// TODO(crbug/896689): Remove code duplication once experiment is finished.
+void AutofillManager::SetFillingContext(
+    const FormStructure& form,
+    std::unique_ptr<FillingContext> context) {
+  if (base::FeatureList::IsEnabled(features::kAutofillRefillWithRendererIds)) {
+    filling_context_by_renderer_id_[form.unique_renderer_id()] =
+        std::move(context);
+  } else {
+    filling_context_by_unique_name_[form.GetIdentifierForRefill()] =
+        std::move(context);
+  }
+}
+
+// TODO(crbug/896689): Remove code duplication once experiment is finished.
+AutofillManager::FillingContext* AutofillManager::GetFillingContext(
+    const FormStructure& form) {
+  if (base::FeatureList::IsEnabled(features::kAutofillRefillWithRendererIds)) {
+    auto it = filling_context_by_renderer_id_.find(form.unique_renderer_id());
+    return it != filling_context_by_renderer_id_.end() ? it->second.get()
+                                                       : nullptr;
+  } else {
+    auto it =
+        filling_context_by_unique_name_.find(form.GetIdentifierForRefill());
+    return it != filling_context_by_unique_name_.end() ? it->second.get()
+                                                       : nullptr;
+  }
+}
+
 bool AutofillManager::ShouldTriggerRefill(const FormStructure& form_structure) {
-  // Should not refill if a form with the same name has not been filled
-  // before.
-  auto itr =
-      filling_contexts_map_.find(form_structure.GetIdentifierForRefill());
-  if (itr == filling_contexts_map_.end())
+  // Should not refill if a form with the same FormRendererId has not been
+  // filled before.
+  FillingContext* filling_context = GetFillingContext(form_structure);
+  if (filling_context == nullptr)
     return false;
 
   address_form_event_logger_->OnDidSeeFillableDynamicForm(sync_state_,
                                                           form_structure);
 
-  FillingContext* filling_context = itr->second.get();
   base::TimeTicks now = AutofillTickClock::NowTicks();
   base::TimeDelta delta = now - filling_context->original_fill_time;
 
@@ -2505,16 +2528,14 @@ void AutofillManager::TriggerRefill(const FormData& form) {
 
   address_form_event_logger_->OnDidRefill(sync_state_, *form_structure);
 
-  auto itr =
-      filling_contexts_map_.find(form_structure->GetIdentifierForRefill());
+  FillingContext* filling_context = GetFillingContext(*form_structure);
 
   // Since GetIdentifierForRefill() is not stable across dynamic changes,
-  // |filling_contexts_map_| may not contain the element anymore.
-  if (itr == filling_contexts_map_.end())
+  // |filling_context_by_unique_name_| may not contain the element anymore.
+  // TODO(crbug/896689): Can be replaced with DCHECK once experiment is over.
+  if (filling_context == nullptr)
     return;
 
-  FillingContext* filling_context = itr->second.get();
-
   // The refill attempt can happen from different paths, some of which happen
   // after waiting for a while. Therefore, although this condition has been
   // checked prior to calling TriggerRefill, it may not hold, when we get
@@ -2525,14 +2546,44 @@ void AutofillManager::TriggerRefill(const FormData& form) {
   filling_context->attempted_refill = true;
 
   // Try to find the field from which the original field originated.
+  // Precedence is given to look up by |filled_field_renderer_id|.
+  // If that is unsuccessful, look up is done by |filled_field_signature|.
+  // TODO(crbug/896689): Clean up after feature launch.
   AutofillField* autofill_field = nullptr;
   for (const std::unique_ptr<AutofillField>& field : *form_structure) {
-    if (field->unique_name() == filling_context->filled_field_name) {
+    // TODO(crbug/896689): Clean up once experiment is over.
+    if ((base::FeatureList::IsEnabled(
+             features::kAutofillRefillWithRendererIds) &&
+         field->unique_renderer_id ==
+             filling_context->filled_field_renderer_id) ||
+        (!base::FeatureList::IsEnabled(
+             features::kAutofillRefillWithRendererIds) &&
+         field->unique_name() == filling_context->filled_field_unique_name)) {
       autofill_field = field.get();
       break;
     }
   }
 
+  // If the field was deleted, look for one with a matching signature. Prefer
+  // visible and newer fields over invisible or older ones.
+  if (base::FeatureList::IsEnabled(features::kAutofillRefillWithRendererIds) &&
+      autofill_field == nullptr) {
+    auto is_better = [](const AutofillField& f, const AutofillField& g) {
+      return std::forward_as_tuple(f.IsVisible(),
+                                   f.unique_renderer_id.value()) >
+             std::forward_as_tuple(g.IsVisible(), g.unique_renderer_id.value());
+    };
+
+    for (const std::unique_ptr<AutofillField>& field : *form_structure) {
+      if (field->GetFieldSignature() ==
+          filling_context->filled_field_signature) {
+        if (autofill_field == nullptr || is_better(*field, *autofill_field)) {
+          autofill_field = field.get();
+        }
+      }
+    }
+  }
+
   // If it was not found cancel the refill.
   if (!autofill_field)
     return;

components/autofill/core/browser/autofill_manager.h

@@ -409,8 +409,12 @@ class AutofillManager : public AutofillHandler,
     // empty.
     const base::Optional<AutofillProfile> profile;
     const base::Optional<std::pair<CreditCard, base::string16>> credit_card;
-    // The name of the field that was initially filled.
-    const base::string16 filled_field_name;
+    // Possible identifiers of the field that was focused when the form was
+    // initially filled. A refill shall be triggered from the same field.
+    // TODO(crbug/896689): Remove |filled_field_unique_name|.
+    const FieldRendererId filled_field_renderer_id;
+    const FieldSignature filled_field_signature;
+    const base::string16 filled_field_unique_name;
     // The time at which the initial fill occurred.
     const base::TimeTicks original_fill_time;
     // The timer used to trigger a refill.
@@ -585,7 +589,14 @@ class AutofillManager : public AutofillHandler,
                           uint32_t profile_form_bitmask,
                           std::string* failure_to_fill);
 
-  // Whether there should be an attemps to refill the form. Returns true if all
+  // TODO(crbug/896689): Remove code duplication once experiment is finished.
+  void SetFillingContext(const FormStructure& form,
+                         std::unique_ptr<FillingContext> context);
+
+  // TODO(crbug/896689): Remove code duplication once experiment is finished.
+  FillingContext* GetFillingContext(const FormStructure& form);
+
+  // Whether there should be an attempts to refill the form. Returns true if all
   // the following are satisfied:
   //  There have been no refill on that page yet.
   //  A non empty form name was recorded in a previous fill
@@ -621,6 +632,7 @@ class AutofillManager : public AutofillHandler,
 
   void SetDataList(const std::vector<base::string16>& values,
                    const std::vector<base::string16>& labels);
+
   AutofillClient* const client_;
 
   LogManager* log_manager_;
@@ -717,8 +729,11 @@ class AutofillManager : public AutofillHandler,
 
   // A map of form names to FillingContext instances used to make refill
   // attempts for dynamic forms.
+  // TODO(crbug/896689): Remove code duplication once experiment is finished.
+  std::map<FormRendererId, std::unique_ptr<FillingContext>>
+      filling_context_by_renderer_id_;
   std::map<base::string16, std::unique_ptr<FillingContext>>
-      filling_contexts_map_;
+      filling_context_by_unique_name_;
 
   // Tracks whether or not rich query encoding is enabled for this client.
   const bool is_rich_query_enabled_ = false;

components/autofill/core/common/autofill_features.cc

@@ -133,6 +133,12 @@ const base::Feature kAutofillExtractAllDatalists{
 const base::Feature kAutofillFixFillableFieldTypes{
     "AutofillFixFillableFieldTypes", base::FEATURE_DISABLED_BY_DEFAULT};
 
+// When enabled, Autofill will use FormRendererIds instead of
+// GetIdentifierForRefill() to identify forms during refills.
+// TODO(crbug/896689): Remove once experiment is finished.
+const base::Feature kAutofillRefillWithRendererIds{
+    "AutofillRefillWithRendererIds", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // When enabled, Autofill will use FieldRendererIds instead of unique_names
 // to align forms in FormStructure::RetrieveFromCache().
 const base::Feature kAutofillRetrieveFromCacheWithRendererIds{

components/autofill/core/common/autofill_features.h

@@ -41,6 +41,7 @@ extern const base::Feature kAutofillEnableSupportForMergingSubsetNames;
 extern const base::Feature kAutofillEnableUIForHonorificPrefixesInSettings;
 extern const base::Feature kAutofillExtractAllDatalists;
 extern const base::Feature kAutofillFixFillableFieldTypes;
+extern const base::Feature kAutofillRefillWithRendererIds;
 extern const base::Feature kAutofillRetrieveFromCacheWithRendererIds;
 extern const base::Feature kAutofillKeyboardAccessory;
 extern const base::Feature kAutofillLabelAffixRemoval;