Fill multiples time with NewPasswordFormManager.

PasswordFormManager fills multiple times on different events from the
Renderer. That's not optimal from performance point of view, but it adds
robustness. This CL implements the same behaviour in
NewPasswordFormManager as in PasswordFormManager: to fill till 5 times.

This is quick fix, that will be merged in M-69. The better more
robust filling is more complicated and will be implemented later.

Bug: 868948, 831123
Change-Id: I44c93c5358b0e6707c545001f5f1a1fb7b60eec3
Reviewed-on: https://chromium-review.googlesource.com/1155116
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579777}

components/password_manager/core/browser/new_password_form_manager.cc

@@ -215,7 +215,7 @@ void NewPasswordFormManager::ProcessMatches(
         return form->blacklisted_by_user && !form->is_public_suffix_match;
       });
 
-  filled_ = false;
+  autofills_left_ = kMaxTimesAutofill;
 
   if (predictions_) {
     ReportTimeBetweenStoreAndServerUMA();
@@ -255,6 +255,10 @@ void NewPasswordFormManager::ProcessServerPredictions(
 }
 
 void NewPasswordFormManager::Fill() {
+  if (autofills_left_ <= 0)
+    return;
+  autofills_left_--;
+
   // There are additional signals (server-side data) and parse results in
   // filling and saving mode might be different so it is better not to cache
   // parse result, but to parse each time again.
@@ -268,7 +272,7 @@ void NewPasswordFormManager::Fill() {
 
   // TODO(https://crbug.com/831123). Move this lines to the beginning of the
   // function when the old parsing is removed.
-  if (!driver_ || filled_)
+  if (!driver_)
     return;
 
   // TODO(https://crbug.com/831123). Implement correct treating of federated
@@ -278,7 +282,6 @@ void NewPasswordFormManager::Fill() {
                                 *observed_password_form.get(), best_matches_,
                                 federated_matches, preferred_match_,
                                 metrics_recorder_.get());
-  filled_ = true;
 }
 
 void NewPasswordFormManager::RecordMetricOnCompareParsingResult(

components/password_manager/core/browser/new_password_form_manager.h

@@ -49,6 +49,10 @@ class NewPasswordFormManager : public PasswordFormManagerForUI,
 
   ~NewPasswordFormManager() override;
 
+  // The upper limit on how many times Chrome will try to autofill the same
+  // form.
+  static constexpr int kMaxTimesAutofill = 5;
+
   // Compares |observed_form_| with |form| and returns true if they are the
   // same and if |driver| is the same as |driver_|.
   bool DoesManage(const autofill::FormData& form,
@@ -73,6 +77,9 @@ class NewPasswordFormManager : public PasswordFormManagerForUI,
   void ProcessServerPredictions(
       const std::vector<autofill::FormStructure*>& predictions);
 
+  // Sends fill data to the renderer.
+  void Fill();
+
   // PasswordFormManagerForUI:
   FormFetcher* GetFormFetcher() override;
   const GURL& GetOrigin() const override;
@@ -105,9 +112,6 @@ class NewPasswordFormManager : public PasswordFormManagerForUI,
       size_t filtered_count) override;
 
  private:
-  // Sends fill data to the renderer.
-  void Fill();
-
   // Compares |parsed_form| with |old_parsing_result_| and records UKM metric.
   // TODO(https://crbug.com/831123): Remove it when the old form parsing is
   // removed.
@@ -223,8 +227,11 @@ class NewPasswordFormManager : public PasswordFormManagerForUI,
 
   base::Optional<FormPredictions> predictions_;
 
-  // True when the managed form was already filled.
-  bool filled_ = false;
+  // If Chrome has already autofilled a few times, it is probable that autofill
+  // is triggered by programmatic changes in the page. We set a maximum number
+  // of times that Chrome will autofill to avoid being stuck in an infinite
+  // loop.
+  int autofills_left_ = kMaxTimesAutofill;
 
   // Used for comparison metrics.
   // TODO(https://crbug.com/831123): Remove it when the old form parsing is

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -186,6 +186,29 @@ TEST_F(NewPasswordFormManagerTest, Autofill) {
   EXPECT_EQ(saved_match_.password_value, fill_data.password_field.value);
 }
 
+TEST_F(NewPasswordFormManagerTest, AutofillNotMoreThan5Times) {
+  TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
+  FakeFormFetcher fetcher;
+  fetcher.Fetch();
+  PasswordFormFillData fill_data;
+  EXPECT_CALL(driver_, FillPasswordForm(_));
+  NewPasswordFormManager form_manager(&client_, driver_.AsWeakPtr(),
+                                      observed_form_, &fetcher);
+  fetcher.SetNonFederated({&saved_match_}, 0u);
+
+  task_runner_->FastForwardUntilNoTasksRemain();
+  Mock::VerifyAndClearExpectations(&driver_);
+
+  for (size_t i = 0; i < NewPasswordFormManager::kMaxTimesAutofill - 1; ++i) {
+    EXPECT_CALL(driver_, FillPasswordForm(_));
+    form_manager.Fill();
+    Mock::VerifyAndClearExpectations(&driver_);
+  }
+
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(0);
+  form_manager.Fill();
+}
+
 // NewPasswordFormManager should always send fill data to renderer, even for
 // sign-up forms (no "current-password" field, i.e., no password field to fill
 // into). However, for sign-up forms, no particular password field should be
@@ -304,9 +327,9 @@ TEST_F(NewPasswordFormManagerTest, ServerPredictionsAfterDelay) {
   form_structure.field(2)->set_server_type(autofill::PASSWORD);
   std::vector<FormStructure*> predictions{&form_structure};
 
-  // Expect no filling on receiving server predictions because it was already
-  // done.
-  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(0);
+  // Expect filling on receiving server predictions because it was less than
+  // kMaxTimesAutofill attempts to fill.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(1);
   form_manager.ProcessServerPredictions(predictions);
   task_runner_->FastForwardUntilNoTasksRemain();
 }

components/password_manager/core/browser/password_manager.cc

@@ -710,13 +710,21 @@ void PasswordManager::CreateFormManagers(
   // Find new forms.
   std::vector<const PasswordForm*> new_forms;
   for (const PasswordForm& form : forms) {
-    bool form_manager_exists =
-        std::any_of(form_managers_.begin(), form_managers_.end(),
-                    [&form, driver](const auto& form_manager) {
-                      return form_manager->DoesManage(form.form_data, driver);
-                    });
-    if (!form_manager_exists)
+    auto form_it =
+        std::find_if(form_managers_.begin(), form_managers_.end(),
+                     [&form, driver](const auto& form_manager) {
+                       return form_manager->DoesManage(form.form_data, driver);
+                     });
+    if (form_it == form_managers_.end()) {
       new_forms.push_back(&form);
+    } else {
+      // This extra filling is just duplicating redundancy that was in
+      // PasswordFormManager, that helps to fix cases when the site overrides
+      // filled values.
+      // TODO(https://crbug.com/831123): Implement more robust filling and
+      // remove the next line.
+      (*form_it)->Fill();
+    }
   }
 
   // Create form manager for new forms.