Reland "Don't record first filling result for sign-in forms"

This reverts commit c13cea62.

Reason for revert: Fixed memory problem

Original change's description:
> Revert "Don't record first filling result for sign-in forms"
>
> This reverts commit 34d69265.
>
> Reason for revert: breaks msan
> https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20MSan%20Tests/14500
>
> Original change's description:
> > Don't record first filling result for sign-in forms
> >
> > The new form parser may decide that a form has no current-password element, but
> > only new-password / confirmation elements. In this case, the parser sets the
> > unique_renderer_id to 'undefined'. The credentials will be sent to the renderer
> > for filling via manual fallback but the renderer won't ever fill (as there is
> > no password element defined for filling.
> >
> > This CL ensures that this case does not get recorded as a failure to fill. This
> > is important because we record two metrics about the first fill attempt:
> > PasswordManager.FirstWaitForUsernameReason and
> > PasswordManager.FirstRendererFillingResult For websites that have a sign-up
> > form followed by a sign-in form, we don't want to record the failure to fill on
> > the sign-up form.
> >
> > Bug: 918846
> > Change-Id: I79e9a73ae3573a81e121eaa483e0ecef0889184f
> > Reviewed-on: https://chromium-review.googlesource.com/c/1422018
> > Commit-Queue: Dominic Battré <battre@chromium.org>
> > Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#625178}
>
> TBR=battre@chromium.org,dvadym@chromium.org
>
> Change-Id: I3c68004b67b5fcc65511c611c2d205297596e782
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 918846
> Reviewed-on: https://chromium-review.googlesource.com/c/1430581
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#625227}

TBR=dvadym@chromium.org,jam@chromium.org

Bug: 918846
Change-Id: I671bc4674f1660920eae0c73d5b1b01b49311383
Reviewed-on: https://chromium-review.googlesource.com/c/1431172Reviewed-by: Dominic Battré <battre@chromium.org>
Commit-Queue: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625375}

chrome/renderer/autofill/password_autofill_agent_browsertest.cc

@@ -121,7 +121,8 @@ const char kNonDisplayedFormHTML[] =
     "</body>";
 
 const char kSignupFormHTML[] =
-    "<FORM name='LoginTestForm' action='http://www.bidule.com'>"
+    "<FORM id='LoginTestForm' name='LoginTestForm' "
+    "    action='http://www.bidule.com'>"
     "  <INPUT type='text' id='random_info'/>"
     "  <INPUT type='password' id='new_password'/>"
     "  <INPUT type='password' id='confirm_password'/>"
@@ -450,8 +451,8 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
   }
 
   void SimulateSuggestionChoice(WebInputElement& username_input) {
-    base::string16 username(base::ASCIIToUTF16(kAliceUsername));
-    base::string16 password(base::ASCIIToUTF16(kAlicePassword));
+    base::string16 username(ASCIIToUTF16(kAliceUsername));
+    base::string16 password(ASCIIToUTF16(kAlicePassword));
     SimulateSuggestionChoiceOfUsernameAndPassword(username_input, username,
                                                   password);
   }
@@ -894,6 +895,38 @@ TEST_F(PasswordAutofillAgentTest,
                                               UTF16ToUTF8(password3_), true);
 }
 
+// Credentials are sent to the renderer even for sign-up forms as these may be
+// eligible for filling via manual fall back. In this case, the password_field
+// is not set. This test verifies that no failures are recorded in
+// PasswordManager.FirstRendererFillingResult.
+TEST_F(PasswordAutofillAgentTest, NoFillingOnSignupForm_NoMetrics) {
+  LoadHTML(kSignupFormHTML);
+
+  WebDocument document = GetMainFrame()->GetDocument();
+  WebElement element =
+      document.GetElementById(WebString::FromUTF8("random_info"));
+  ASSERT_FALSE(element.IsNull());
+  username_element_ = element.To<WebInputElement>();
+
+  fill_data_.has_renderer_ids = true;
+
+  fill_data_.username_field.name = ASCIIToUTF16("random_info");
+  fill_data_.username_field.unique_renderer_id =
+      username_element_.UniqueRendererFormControlId();
+
+  fill_data_.password_field.name = base::string16();
+  fill_data_.password_field.unique_renderer_id =
+      FormFieldData::kNotSetFormControlRendererId;
+
+  WebFormElement form_element =
+      document.GetElementById("LoginTestForm").To<WebFormElement>();
+  fill_data_.form_renderer_id = form_element.UniqueRendererFormId();
+
+  SimulateOnFillPasswordForm(fill_data_);
+  histogram_tester_.ExpectTotalCount(
+      "PasswordManager.FirstRendererFillingResult", 0);
+}
+
 // Do not fill a password field if the stored username is a prefix without @
 // of username in read-only field.
 TEST_F(PasswordAutofillAgentTest,
@@ -1355,7 +1388,7 @@ TEST_F(PasswordAutofillAgentTest,
   // the matching autofill from the dropdown.
   SimulateUsernameTyping("a");
   // Since the username element has focus, blur event will be not triggered.
-  base::Erase(event_checkers, base::ASCIIToUTF16("username_blur_event"));
+  base::Erase(event_checkers, ASCIIToUTF16("username_blur_event"));
   SimulateSuggestionChoice(username_element_);
 
   // The username and password should now have been autocompleted.
@@ -2555,7 +2588,7 @@ TEST_F(PasswordAutofillAgentTest,
   SetAccountCreationFormsDetectedMessage(password_generation_,
                                          GetMainFrame()->GetDocument(), 0, 2);
 
-  base::string16 password = base::ASCIIToUTF16("NewPass22");
+  base::string16 password = ASCIIToUTF16("NewPass22");
   EXPECT_CALL(fake_pw_client_,
               PresaveGeneratedPassword(testing::Field(
                   &autofill::PasswordForm::password_value, password)));
@@ -2572,7 +2605,7 @@ TEST_F(PasswordAutofillAgentTest,
   SetNotBlacklistedMessage(password_generation_, kFormHTML);
   SetAccountCreationFormsDetectedMessage(password_generation_,
                                          GetMainFrame()->GetDocument(), 0, 2);
-  base::string16 password = base::ASCIIToUTF16("NewPass22");
+  base::string16 password = ASCIIToUTF16("NewPass22");
   EXPECT_CALL(fake_pw_client_,
               PresaveGeneratedPassword(testing::Field(
                   &autofill::PasswordForm::password_value, password)));
@@ -2614,7 +2647,7 @@ TEST_F(PasswordAutofillAgentTest, PasswordGenerationSupersedesAutofill) {
   // show UI when the password field is focused.
   fill_data_.wait_for_username = true;
   fill_data_.username_field = FormFieldData();
-  fill_data_.password_field.name = base::ASCIIToUTF16("new_password");
+  fill_data_.password_field.name = ASCIIToUTF16("new_password");
   UpdateOriginForHTML(kSignupFormHTML);
   SimulateOnFillPasswordForm(fill_data_);
 

components/autofill/content/renderer/password_autofill_agent.cc

@@ -1415,6 +1415,13 @@ void PasswordAutofillAgent::FillUsingRendererIDs(
       FindUsernamePasswordElements(form_data);
   if (password_element.IsNull()) {
     MaybeStoreFallbackData(form_data);
+    if (form_data.password_field.unique_renderer_id ==
+        FormFieldData::kNotSetFormControlRendererId) {
+      // If the password_field.unique_renderer_id was not set, this was never
+      // meant as an honest attempt to fill the form. Therefore, don't log it as
+      // such.
+      return;
+    }
     LogFirstFillingResult(form_data, FillingResult::kNoPasswordElement);
     return;
   }

components/password_manager/core/browser/password_form_filling.cc

@@ -143,6 +143,15 @@ LikelyFormFilling SendFillInformationToRenderer(
   const bool form_good_for_filling =
       new_parsing_enabled || !observed_form.IsPossibleChangePasswordForm();
 
+  // If the parser of the NewPasswordFormManager decides that there is no
+  // current password field, no filling attempt will be made. In this case the
+  // renderer won't treat this as the "first filling" and won't record metrics
+  // accordingly. The browser should not do that either.
+  const bool no_sign_in_form =
+      new_parsing_enabled &&
+      observed_form.password_element_renderer_id ==
+          autofill::FormFieldData::kNotSetFormControlRendererId;
+
   // Wait for the username before filling passwords in case the
   // FillOnAccountSelectHttp feature is active and the main frame is
   // insecure.
@@ -168,10 +177,19 @@ LikelyFormFilling SendFillInformationToRenderer(
     wait_for_username_reason = WaitForUsernameReason::kPublicSuffixMatch;
   } else if (!form_good_for_filling) {
     wait_for_username_reason = WaitForUsernameReason::kFormNotGoodForFilling;
+  } else if (no_sign_in_form) {
+    // If the parser did not find a current password element, don't fill.
+    wait_for_username_reason = WaitForUsernameReason::kFormNotGoodForFilling;
   } else if (enable_foas_on_http) {
     wait_for_username_reason = WaitForUsernameReason::kFoasOnHTTP;
   }
-  metrics_recorder->RecordFirstWaitForUsernameReason(wait_for_username_reason);
+
+  // Record no "FirstWaitForUsernameReason" metrics for a form that is not meant
+  // for filling. The renderer won't record a "FirstFillingResult" either.
+  if (!no_sign_in_form) {
+    metrics_recorder->RecordFirstWaitForUsernameReason(
+        wait_for_username_reason);
+  }
 
   bool wait_for_username =
       wait_for_username_reason != WaitForUsernameReason::kDontWait;

components/password_manager/core/browser/password_form_filling_unittest.cc

@@ -181,10 +181,14 @@ TEST_F(PasswordFormFillingTest, TestFillOnLoadSuggestion) {
     best_matches[saved_match_.username_value] = &saved_match_;
 
     PasswordForm observed_form = observed_form_;
+    observed_form.password_element_renderer_id = 123;
     if (!test_case.new_password_present)
       observed_form.new_password_element = ASCIIToUTF16("New Passwd");
-    if (!test_case.current_password_present)
+    if (!test_case.current_password_present) {
       observed_form.password_element.clear();
+      observed_form.password_element_renderer_id =
+          autofill::FormFieldData::kNotSetFormControlRendererId;
+    }
 
     PasswordFormFillData fill_data;
     EXPECT_CALL(driver_, FillPasswordForm(_)).WillOnce(SaveArg<0>(&fill_data));
@@ -194,9 +198,13 @@ TEST_F(PasswordFormFillingTest, TestFillOnLoadSuggestion) {
         client_, &driver_, false, observed_form, best_matches,
         federated_matches_, &saved_match_, metrics_recorder_.get());
 
-    // In all cases, fill on load should not be prevented. If there is no
-    // current-password field, the renderer will not fill anyway.
-    EXPECT_EQ(LikelyFormFilling::kFillOnPageLoad, likely_form_filling);
+    // In all cases where a current password exists, fill on load should be
+    // permitted. Otherwise, the renderer will not fill anyway and return
+    // kFillOnAccountSelect.
+    if (test_case.current_password_present)
+      EXPECT_EQ(LikelyFormFilling::kFillOnPageLoad, likely_form_filling);
+    else
+      EXPECT_EQ(LikelyFormFilling::kFillOnAccountSelect, likely_form_filling);
   }
 }