[Autofill] Fix crash via unrelated field in address form
Prevents a NOTREACHED_NORETURN() call from being reached. This was
previously possible when a field with an unrelated type, e.g. a credit
card number field in an address form, was skipped because it had a value
on page load.
Fixed: 324811625
Change-Id: I7b8c2a54006c813d680f5a4a5867e7ff8062c382
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5285757
Reviewed-by: Jihad Hanna <jihadghanna@google.com>
Reviewed-by: Florian Leimgruber <fleimgruber@google.com>
Commit-Queue: Wolfgang Billenstein <bwolfgang@google.com>
Cr-Commit-Position: refs/heads/main@{#1259734}
diff --git a/components/autofill/core/browser/form_filler.cc b/components/autofill/core/browser/form_filler.cc
index efb5a47..f622e68e 100644
--- a/components/autofill/core/browser/form_filler.cc
+++ b/components/autofill/core/browser/form_filler.cc
@@ -296,18 +296,6 @@
continue;
}
- // Don't fill meaningfully pre-filled fields but overwrite placeholders.
- // TODO(b/40227496): 'autofill_field->value' should be the initial value of
- // the field.
- if (!is_triggering_field &&
- !autofill_field->IsSelectOrSelectListElement() &&
- !autofill_field->value.empty() &&
- (IsNotAPlaceholder(autofill_field) ||
- IsMeaningfullyPreFilled(autofill_field))) {
- skip_reasons[field_id] = FieldFillingSkipReason::kValuePrefilled;
- continue;
- }
-
// Usually, this should not happen because Autofill sectioning logic
// separates address fields from credit card fields. However, autofill
// respects the HTML `autocomplete` attribute when it is used to specify a
@@ -322,6 +310,25 @@
skip_reasons[field_id] = FieldFillingSkipReason::kFieldTypeUnrelated;
continue;
}
+
+ // Add new skip reasons above this comment.
+ // Skip reason `kValuePrefilled` needs to be checked last. For metrics
+ // purposes, `GetFieldFillingDataFor(Profile|CreditCard)()` is queried for
+ // fields that were skipped because of `kValuePrefilled`. Since the function
+ // assumes that the field's type is profile/card-related, it's important
+ // that this check happens after the `kFieldTypeUnrelated` check.
+
+ // Don't fill meaningfully pre-filled fields but overwrite placeholders.
+ // TODO(b/40227496): 'autofill_field->value' should be the initial value of
+ // the field.
+ if (!is_triggering_field &&
+ !autofill_field->IsSelectOrSelectListElement() &&
+ !autofill_field->value.empty() &&
+ (IsNotAPlaceholder(autofill_field) ||
+ IsMeaningfullyPreFilled(autofill_field))) {
+ skip_reasons[field_id] = FieldFillingSkipReason::kValuePrefilled;
+ continue;
+ }
CHECK_EQ(skip_reasons[field_id], FieldFillingSkipReason::kNotSkipped,
base::NotFatalUntil::M123);
}
diff --git a/components/autofill/core/browser/form_filler_unittest.cc b/components/autofill/core/browser/form_filler_unittest.cc
index c6666a48..c748c37 100644
--- a/components/autofill/core/browser/form_filler_unittest.cc
+++ b/components/autofill/core/browser/form_filler_unittest.cc
@@ -1903,6 +1903,27 @@
EXPECT_THAT(form_structure->field(1), AutofilledWithProfile(profile));
}
+// Regression test that a field with an unrelated type doesn't cause a crash
+// (crbug.com/324811625).
+TEST_F(FormFillerTest, PreFilledCCFieldInAddressFormDoesNotCauseCrash) {
+ base::test::ScopedFeatureList feature_list;
+ feature_list.InitWithFeatures({features::kAutofillSkipPreFilledFields,
+ features::kAutofillOverwritePlaceholdersOnly},
+ {});
+ FormData form = test::GetFormData(
+ {.fields = {{.role = NAME_FULL,
+ .value = u"pre-filled",
+ .autocomplete_attribute = "section-billing name"},
+ {.role = CREDIT_CARD_NUMBER,
+ .value = u"pre-filled",
+ .autocomplete_attribute = "section-billing cc-number"}}});
+ FormsSeen({form});
+
+ AutofillProfile profile = test::GetFullProfile();
+ FillAutofillFormData(form, form.fields.front(), &profile);
+ // Expect that this test doesn't cause a crash.
+}
+
// The following Refill Tests ensure that Autofill can handle the situation
// where it fills a credit card form with an expiration date like 04/2999
// and the website tries to reformat the input with whitespaces around the
