Waiting for passwords server predictions before filling.

In order to support filling with using server-side predictions
NewPasswordFormManager should wait for them. This CL introduces waiting
for filling up to 500 ms, if for this time no server predictions
received NewPasswordFormManager fills without server support.

Bug: 831123
Change-Id: I7031ba03ab0229ed41de7869a1379a83ccd82259
Reviewed-on: https://chromium-review.googlesource.com/1091056
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567226}

components/password_manager/core/browser/new_password_form_manager.cc

@@ -17,6 +17,7 @@
 using autofill::FormData;
 using autofill::FormSignature;
 using autofill::FormStructure;
+using base::TimeDelta;
 
 using Logger = autofill::SavePasswordProgressLogger;
 
@@ -24,6 +25,9 @@ namespace password_manager {
 
 namespace {
 
+constexpr TimeDelta kMaxFillingDelayForServerPerdictions =
+    TimeDelta::FromMilliseconds(500);
+
 // Helper function for calling form parsing and logging results if logging is
 // active.
 std::unique_ptr<autofill::PasswordForm> ParseFormAndMakeLogging(
@@ -61,7 +65,8 @@ NewPasswordFormManager::NewPasswordFormManager(
                              client_,
                              true /* should_migrate_http_passwords */,
                              true /* should_query_suppressed_https_forms */)),
-      form_fetcher_(form_fetcher ? form_fetcher : owned_form_fetcher_.get()) {
+      form_fetcher_(form_fetcher ? form_fetcher : owned_form_fetcher_.get()),
+      weak_ptr_factory_(this) {
   metrics_recorder_ = base::MakeRefCounted<PasswordFormMetricsRecorder>(
       client_->IsMainFrameSecure(), client_->GetUkmSourceId());
   metrics_recorder_->RecordFormSignature(CalculateFormSignature(observed_form));
@@ -168,10 +173,17 @@ void NewPasswordFormManager::ProcessMatches(
   std::vector<const autofill::PasswordForm*> not_best_matches;
   password_manager_util::FindBestMatches(non_federated, &best_matches_,
                                          &not_best_matches, &preferred_match_);
+  filled_ = false;
 
-  // TODO(https://crbug.com/831123). Implement waiting for server-side
-  // predictions.
-  Fill();
+  if (predictions_)
+    Fill();
+  else {
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&NewPasswordFormManager::Fill,
+                       weak_ptr_factory_.GetWeakPtr()),
+        kMaxFillingDelayForServerPerdictions);
+  }
 }
 
 bool NewPasswordFormManager::SetSubmittedFormIfIsManaged(
@@ -192,8 +204,6 @@ void NewPasswordFormManager::ProcessServerPredictions(
     if (form_predictions->form_signature() != observed_form_signature)
       continue;
     predictions_ = ConvertToFormPredictions(observed_form_, *form_predictions);
-    // TODO(https://crbug.com/831123). Implement checking whether it was already
-    // filled.
     Fill();
     break;
   }
@@ -212,16 +222,17 @@ 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_ || best_matches_.empty())
+  if (!driver_ || best_matches_.empty() || filled_)
     return;
 
   // TODO(https://crbug.com/831123). Implement correct treating of federated
   // matches.
   std::vector<const autofill::PasswordForm*> federated_matches;
   SendFillInformationToRenderer(
-      *client_, driver_.get(), false /* is_blaclisted */,
+      *client_, driver_.get(), false /* is_blacklisted */,
       *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

@@ -145,11 +145,16 @@ class NewPasswordFormManager : public PasswordFormManagerForUI,
 
   base::Optional<FormPredictions> predictions_;
 
+  // True when the managed form was already filled.
+  bool filled_ = false;
+
   // Used for comparison metrics.
   // TODO(https://crbug.com/831123): Remove it when the old form parsing is
   // removed.
   autofill::PasswordForm old_parsing_result_;
 
+  base::WeakPtrFactory<NewPasswordFormManager> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NewPasswordFormManager);
 };
 

components/password_manager/core/browser/new_password_form_manager_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/password_manager/core/browser/new_password_form_manager.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "components/autofill/core/browser/form_structure.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/password_form.h"
@@ -21,7 +22,9 @@ using autofill::FormFieldData;
 using autofill::PasswordForm;
 using autofill::PasswordFormFillData;
 using base::ASCIIToUTF16;
+using base::TestMockTimeTaskRunner;
 using testing::_;
+using testing::Mock;
 using testing::SaveArg;
 
 namespace password_manager {
@@ -41,7 +44,7 @@ class MockPasswordManagerDriver : public StubPasswordManagerDriver {
 
 class NewPasswordFormManagerTest : public testing::Test {
  public:
-  NewPasswordFormManagerTest() {
+  NewPasswordFormManagerTest() : task_runner_(new TestMockTimeTaskRunner) {
     GURL origin = GURL("http://accounts.google.com/a/ServiceLoginAuth");
     GURL action = GURL("http://accounts.google.com/a/ServiceLogin");
 
@@ -76,6 +79,7 @@ class NewPasswordFormManagerTest : public testing::Test {
   PasswordForm saved_match_;
   StubPasswordManagerClient client_;
   MockPasswordManagerDriver driver_;
+  scoped_refptr<TestMockTimeTaskRunner> task_runner_;
 };
 
 TEST_F(NewPasswordFormManagerTest, DoesManage) {
@@ -110,6 +114,7 @@ TEST_F(NewPasswordFormManagerTest, DoesManageNoFormTag) {
 }
 
 TEST_F(NewPasswordFormManagerTest, Autofill) {
+  TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
   FakeFormFetcher fetcher;
   fetcher.Fetch();
   EXPECT_CALL(driver_, AllowPasswordGenerationForForm(_));
@@ -119,6 +124,8 @@ TEST_F(NewPasswordFormManagerTest, Autofill) {
                                       observed_form_, &fetcher);
   fetcher.SetNonFederated({&saved_match_}, 0u);
 
+  task_runner_->FastForwardUntilNoTasksRemain();
+
   EXPECT_EQ(observed_form_.origin, fill_data.origin);
   EXPECT_FALSE(fill_data.wait_for_username);
   EXPECT_EQ(observed_form_.fields[1].name, fill_data.username_field.name);
@@ -159,23 +166,75 @@ TEST_F(NewPasswordFormManagerTest, SetSubmitted) {
   EXPECT_FALSE(form_manager.is_submitted());
 }
 
-TEST_F(NewPasswordFormManagerTest, ProcessServerPredictions) {
+// Tests that when NewPasswordFormManager receives saved matches it waits for
+// server predictions and fills on receving them.
+TEST_F(NewPasswordFormManagerTest, ServerPredictionsWithinDelay) {
+  TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
+  FakeFormFetcher fetcher;
+  fetcher.Fetch();
+
+  // Expects no filling on save matches receiving.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(0);
+  NewPasswordFormManager form_manager(&client_, driver_.AsWeakPtr(),
+                                      observed_form_, &fetcher);
+  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};
+
+  // Expect filling without delay on receiving server predictions.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(1);
+  form_manager.ProcessServerPredictions(predictions);
+}
+
+// Tests that NewPasswordFormManager fills after some delay even without
+// server predictions.
+TEST_F(NewPasswordFormManagerTest, ServerPredictionsAfterDelay) {
+  TestMockTimeTaskRunner::ScopedContext scoped_context(task_runner_.get());
   FakeFormFetcher fetcher;
   fetcher.Fetch();
-  // Expects 2 fills: one when results from |fetcher| received and another when
-  // server predictions received.
-  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(2);
+
   NewPasswordFormManager form_manager(&client_, driver_.AsWeakPtr(),
                                       observed_form_, &fetcher);
   fetcher.SetNonFederated({&saved_match_}, 0u);
+  // Expect filling after passing filling delay.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(1);
+  // Simulate passing filling delay.
+  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};
 
+  // Expect no filling on receiving server predictions because it was already
+  // done.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(0);
+  form_manager.ProcessServerPredictions(predictions);
+  task_runner_->FastForwardUntilNoTasksRemain();
+}
+
+// Tests that filling happens immediately if server predictions are received
+// before saved matches.
+TEST_F(NewPasswordFormManagerTest, ServerPredictionsBeforeFetcher) {
+  FakeFormFetcher fetcher;
+  fetcher.Fetch();
+  // Expect no filling after receiving saved matches from |fetcher|, since
+  // |form_manager| is waiting for server-side predictions.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(0);
+  NewPasswordFormManager form_manager(&client_, driver_.AsWeakPtr(),
+                                      observed_form_, &fetcher);
+  FormStructure form_structure(observed_form_);
+  form_structure.field(2)->set_server_type(autofill::PASSWORD);
+  std::vector<FormStructure*> predictions{&form_structure};
   form_manager.ProcessServerPredictions(predictions);
-  // TODO(https://crbug.com/831123): Extend testing when form parsing based on
-  // server predictions is implemented.
+  Mock::VerifyAndClearExpectations(&driver_);
+
+  // Expect filling without delay on receiving server predictions.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(1);
+  fetcher.SetNonFederated({&saved_match_}, 0u);
 }
 
 }  // namespace  password_manager

components/password_manager/core/browser/password_manager_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/metrics/user_action_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "build/build_config.h"
 #include "components/password_manager/core/browser/form_fetcher_impl.h"
 #include "components/password_manager/core/browser/mock_password_store.h"
@@ -37,6 +38,7 @@
 
 using autofill::PasswordForm;
 using base::ASCIIToUTF16;
+using base::TestMockTimeTaskRunner;
 using testing::_;
 using testing::AnyNumber;
 using testing::Return;
@@ -147,7 +149,9 @@ ACTION_P(SaveToScopedPtr, scoped) { scoped->reset(arg0); }
 
 class PasswordManagerTest : public testing::Test {
  public:
-  PasswordManagerTest() : test_url_("https://www.example.com") {}
+  PasswordManagerTest()
+      : test_url_("https://www.example.com"),
+        task_runner_(new TestMockTimeTaskRunner) {}
   ~PasswordManagerTest() override = default;
 
  protected:
@@ -283,6 +287,7 @@ class PasswordManagerTest : public testing::Test {
   std::unique_ptr<PasswordAutofillManager> password_autofill_manager_;
   std::unique_ptr<PasswordManager> manager_;
   PasswordForm submitted_form_;
+  scoped_refptr<TestMockTimeTaskRunner> task_runner_;
 };
 
 MATCHER_P(FormMatches, form, "") {
@@ -1184,6 +1189,10 @@ TEST_F(PasswordManagerTest, FormSubmitWithOnlyPasswordField) {
 // ID. The test checks that nevertheless each of them gets assigned its own
 // NewPasswordFormManager and filled as expected.
 TEST_F(PasswordManagerTest, FillPasswordOnManyFrames_SameId) {
+  // Setting task runner is required since NewPasswordFormManager uses
+  // PostDelayTask for making filling.
+  TestMockTimeTaskRunner::ScopedContext scoped_context_(task_runner_.get());
+
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(features::kNewPasswordFormParsing);
 
@@ -1233,6 +1242,7 @@ TEST_F(PasswordManagerTest, FillPasswordOnManyFrames_SameId) {
       .WillOnce(WithArg<1>(InvokeConsumer(second_form)));
   EXPECT_CALL(driver_b, FillPasswordForm(_));
   manager()->OnPasswordFormsParsed(&driver_b, {second_form});
+  task_runner_->FastForwardUntilNoTasksRemain();
 }
 
 // If kNewPasswordFormParsing is disabled, "similar" is governed by