Fix server predictions race conditions in NewPasswordFormManager.

If server predicitons are cached, then often PasswordManager receives
them earlier than learns about a corresponding form in DOM. So
NewPasswordFormManager is created later. And predictions are lost.
This CL fixes this, by caching predictions in PasswordManager.

Bug: 905982, 831123


Change-Id: I833fe675c3c621edbae31d5b449e274c7d5a33e6
Reviewed-on: https://chromium-review.googlesource.com/c/1338101
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608750}

components/password_manager/core/browser/new_password_form_manager.cc

@@ -517,21 +517,24 @@ bool NewPasswordFormManager::SetSubmittedFormIfIsManaged(
 }
 
 void NewPasswordFormManager::ProcessServerPredictions(
-    const std::vector<FormStructure*>& predictions) {
+    const std::map<FormSignature, FormPredictions>& predictions) {
   FormSignature observed_form_signature =
       CalculateFormSignature(observed_form_);
-  for (const FormStructure* form_predictions : predictions) {
-    if (form_predictions->form_signature() != observed_form_signature)
-      continue;
-    ReportTimeBetweenStoreAndServerUMA();
-    parser_.set_predictions(ConvertToFormPredictions(*form_predictions));
-    Fill();
-    break;
-  }
+  auto it = predictions.find(observed_form_signature);
+  if (it == predictions.end())
+    return;
+
+  ReportTimeBetweenStoreAndServerUMA();
+  parser_.set_predictions(it->second);
+  Fill();
 }
 
 void NewPasswordFormManager::Fill() {
   waiting_for_server_predictions_ = false;
+
+  if (form_fetcher_->GetState() == FormFetcher::State::WAITING)
+    return;
+
   if (autofills_left_ <= 0)
     return;
   autofills_left_--;

components/password_manager/core/browser/new_password_form_manager.h

@@ -14,6 +14,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "components/autofill/core/common/form_data.h"
+#include "components/autofill/core/common/signatures_util.h"
 #include "components/password_manager/core/browser/form_fetcher.h"
 #include "components/password_manager/core/browser/form_parsing/form_parser.h"
 #include "components/password_manager/core/browser/form_parsing/password_field_prediction.h"
@@ -22,10 +23,6 @@
 #include "components/password_manager/core/browser/password_form_user_action.h"
 #include "components/password_manager/core/browser/votes_uploader.h"
 
-namespace autofill {
-class FormStructure;
-}
-
 namespace password_manager {
 
 class FormSaver;
@@ -87,7 +84,7 @@ class NewPasswordFormManager : public PasswordFormManagerInterface,
   // |observed_form_|, initiates filling and stores predictions in
   // |predictions_|.
   void ProcessServerPredictions(
-      const std::vector<autofill::FormStructure*>& predictions);
+      const std::map<autofill::FormSignature, FormPredictions>& predictions);
 
   // Sends fill data to the renderer.
   void Fill();

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "build/build_config.h"
+#include "components/autofill/core/browser/field_types.h"
 #include "components/autofill/core/browser/form_structure.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/form_field_data.h"
@@ -23,10 +24,12 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using autofill::FormData;
+using autofill::FormSignature;
 using autofill::FormStructure;
 using autofill::FormFieldData;
 using autofill::PasswordForm;
 using autofill::PasswordFormFillData;
+using autofill::ServerFieldType;
 using base::ASCIIToUTF16;
 using base::TestMockTimeTaskRunner;
 using testing::_;
@@ -111,6 +114,21 @@ void CheckPasswordGenerationUKM(const ukm::TestAutoSetUkmRecorder& recorder,
       ukm::builders::PasswordForm::kGeneration_GeneratedPasswordModifiedName);
 }
 
+// Create predictions for |form| using field predictions |field_predictions|.
+std::map<FormSignature, FormPredictions> CreatePredictions(
+    const FormData& form,
+    std::vector<std::pair<int, ServerFieldType>> field_predictions) {
+  FormPredictions predictions;
+  for (const auto& index_prediction : field_predictions) {
+    uint32_t renderer_id =
+        form.fields[index_prediction.first].unique_renderer_id;
+    ServerFieldType server_type = index_prediction.second;
+    predictions[renderer_id] = PasswordFieldPrediction{.type = server_type};
+  }
+  FormSignature form_signature = CalculateFormSignature(form);
+  return {{form_signature, predictions}};
+}
+
 class MockFormSaver : public StubFormSaver {
  public:
   MockFormSaver() = default;
@@ -442,9 +460,8 @@ TEST_F(NewPasswordFormManagerTest, ServerPredictionsWithinDelay) {
   fetcher_->SetNonFederated({&saved_match_}, 0u);
   Mock::VerifyAndClearExpectations(&driver_);
 
-  FormStructure form_structure(observed_form_);
-  form_structure.field(2)->set_server_type(autofill::PASSWORD);
-  std::vector<FormStructure*> predictions{&form_structure};
+  std::map<FormSignature, FormPredictions> predictions = CreatePredictions(
+      observed_form_, {std::make_pair(2, autofill::PASSWORD)});
 
   // Expect filling without delay on receiving server predictions.
   EXPECT_CALL(driver_, FillPasswordForm(_)).Times(1);
@@ -464,9 +481,8 @@ TEST_F(NewPasswordFormManagerTest, ServerPredictionsAfterDelay) {
   task_runner_->FastForwardUntilNoTasksRemain();
   Mock::VerifyAndClearExpectations(&driver_);
 
-  FormStructure form_structure(observed_form_);
-  form_structure.field(2)->set_server_type(autofill::PASSWORD);
-  std::vector<FormStructure*> predictions{&form_structure};
+  std::map<FormSignature, FormPredictions> predictions = CreatePredictions(
+      observed_form_, {std::make_pair(2, autofill::PASSWORD)});
 
   // Expect filling on receiving server predictions because it was less than
   // kMaxTimesAutofill attempts to fill.
@@ -483,9 +499,9 @@ TEST_F(NewPasswordFormManagerTest, ServerPredictionsBeforeFetcher) {
   // |form_manager| is waiting for server-side predictions.
   EXPECT_CALL(driver_, FillPasswordForm(_)).Times(0);
   CreateFormManager(observed_form_);
-  FormStructure form_structure(observed_form_);
-  form_structure.field(2)->set_server_type(autofill::PASSWORD);
-  std::vector<FormStructure*> predictions{&form_structure};
+
+  std::map<FormSignature, FormPredictions> predictions = CreatePredictions(
+      observed_form_, {std::make_pair(2, autofill::PASSWORD)});
   form_manager_->ProcessServerPredictions(predictions);
   Mock::VerifyAndClearExpectations(&driver_);
 
@@ -623,9 +639,9 @@ TEST_F(NewPasswordFormManagerTest, CreatePendingCredentialsEmptyName) {
   anonymous_signup.fields[2].name.clear();
   anonymous_signup.fields[2].value = ASCIIToUTF16("a password");
   // Mark the password field as new-password.
-  FormStructure form_structure(observed_form_);
-  form_structure.field(2)->set_server_type(autofill::ACCOUNT_CREATION_PASSWORD);
-  std::vector<FormStructure*> predictions{&form_structure};
+  std::map<FormSignature, FormPredictions> predictions = CreatePredictions(
+      observed_form_, {std::make_pair(2, autofill::ACCOUNT_CREATION_PASSWORD)});
+
   form_manager_->ProcessServerPredictions(predictions);
 
   EXPECT_TRUE(

components/password_manager/core/browser/password_manager.cc

@@ -609,6 +609,7 @@ void PasswordManager::DropFormManagers() {
   owned_submitted_form_manager_.reset();
   provisional_save_manager_.reset();
   all_visible_forms_.clear();
+  predictions_.clear();
 }
 
 bool PasswordManager::IsPasswordFieldDetectedOnPage() {
@@ -646,6 +647,7 @@ void PasswordManager::DidNavigateMainFrame() {
   }
 
   form_managers_.clear();
+  predictions_.clear();
 }
 
 void PasswordManager::OnPasswordFormSubmitted(
@@ -885,6 +887,7 @@ void PasswordManager::CreateFormManagers(
         new_form->form_data, nullptr,
         std::make_unique<FormSaverImpl>(client_->GetPasswordStore()), nullptr));
     form_managers_.back()->set_old_parsing_result(*new_form);
+    form_managers_.back()->ProcessServerPredictions(predictions_);
   }
 }
 
@@ -1252,8 +1255,10 @@ void PasswordManager::ProcessAutofillPredictions(
 
   if (base::FeatureList::IsEnabled(
           password_manager::features::kNewPasswordFormParsing)) {
+    for (const autofill::FormStructure* form : forms)
+      predictions_[form->form_signature()] = ConvertToFormPredictions(*form);
     for (auto& manager : form_managers_)
-      manager->ProcessServerPredictions(forms);
+      manager->ProcessServerPredictions(predictions_);
   }
 
   // Leave only forms that contain fields that are useful for password manager.

components/password_manager/core/browser/password_manager.h

@@ -18,6 +18,8 @@
 #include "build/build_config.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/password_form_fill_data.h"
+#include "components/autofill/core/common/signatures_util.h"
+#include "components/password_manager/core/browser/form_parsing/password_field_prediction.h"
 #include "components/password_manager/core/browser/form_submission_observer.h"
 #include "components/password_manager/core/browser/login_model.h"
 #include "components/password_manager/core/browser/password_form_manager.h"
@@ -346,6 +348,9 @@ class PasswordManager : public LoginModel, public FormSubmissionObserver {
   // (to see if the login was a failure), and clears the vector.
   std::vector<autofill::PasswordForm> all_visible_forms_;
 
+  // Server predictions for the forms on the page.
+  std::map<autofill::FormSignature, FormPredictions> predictions_;
+
   // The user-visible URL from the last time a password was provisionally saved.
   GURL main_frame_url_;
 

components/password_manager/core/browser/password_manager_unittest.cc

@@ -2898,4 +2898,37 @@ TEST_F(PasswordManagerTest, NoSavePromptWhenPasswordManagerDisabled) {
   manager()->OnPasswordFormSubmittedNoChecks(&driver_, submitted_form);
 }
 
+// Check that when autofill predictions are received before a form is found then
+// server predictions are not ignored and used for filling.
+TEST_F(PasswordManagerTest, AutofillPredictionBeforeFormParsed) {
+  TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kNewPasswordFormParsing);
+
+  EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
+      .WillRepeatedly(Return(true));
+
+  PasswordForm form(MakeSimpleForm());
+  // Simulate that the form is incorrectly marked as sign-up, which means it can
+  // not be filled without server predictions.
+  form.form_data.fields[1].autocomplete_attribute = "new-password";
+
+  // Server predictions says that this is a sign-in form. Since they have higher
+  // priority than autocomplete attributes then the form should be filled.
+  FormStructure form_structure(form.form_data);
+  form_structure.field(1)->set_server_type(autofill::PASSWORD);
+  manager()->ProcessAutofillPredictions(&driver_, {&form_structure});
+
+  EXPECT_CALL(*store_, GetLogins(_, _))
+      .WillRepeatedly(WithArg<1>(InvokeConsumer(form)));
+  // There are 2 fills, the first when the server predictions are received, the
+  // second when the filling delayed task is executed. In production code a
+  // delayed task is not posted since receiving results from the store is
+  // asynchronous in contrast to test code.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(2);
+
+  manager()->OnPasswordFormsParsed(&driver_, {form});
+  task_runner_->FastForwardUntilNoTasksRemain();
+}
+
 }  // namespace password_manager