PasswordFormManager supports multiple drivers

PFM serves potentially multiple frames by providing (auto)fill
information for each. That happens through drivers, which are
recently one per frame. The PFM only supports one driver so far.

This CL makes it handle a queue of drivers until it retrieves
the credentials from the store, and then serve each of the
associated frames through the drivers.

BUG=440438

Review URL: https://codereview.chromium.org/795183002

Cr-Commit-Position: refs/heads/master@{#308065}

components/password_manager/core/browser/password_form_manager.cc

@@ -84,10 +84,10 @@ PasswordFormManager::PasswordFormManager(
       preferred_match_(NULL),
       state_(PRE_MATCHING_PHASE),
       client_(client),
-      driver_(driver),
       manager_action_(kManagerActionNone),
       user_action_(kUserActionNone),
       submit_result_(kSubmitResultNotSubmitted) {
+  drivers_.push_back(driver);
   if (observed_form_.origin.is_valid())
     base::SplitString(observed_form_.origin.path(), '/', &form_path_tokens_);
 }
@@ -462,8 +462,6 @@ void PasswordFormManager::OnRequestDone(
     preferred_match_ = logins_result[i]->preferred ? logins_result[i]
                                                    : preferred_match_;
   }
-  // We're done matching now.
-  state_ = POST_MATCHING_PHASE;
 
   client_->AutofillResultsComputed();
 
@@ -471,10 +469,6 @@ void PasswordFormManager::OnRequestDone(
   // be equivalent for the moment, but it's less clear and may not be
   // equivalent in the future.
   if (best_score <= 0) {
-    // If no saved forms can be used, then it isn't blacklisted and generation
-    // should be allowed.
-    if (driver_)
-      driver_->AllowPasswordGenerationForForm(observed_form_);
     if (logger)
       logger->LogNumber(Logger::STRING_BEST_SCORE, best_score);
     return;
@@ -505,23 +499,24 @@ void PasswordFormManager::OnRequestDone(
   if (preferred_match_->blacklisted_by_user) {
     client_->PasswordAutofillWasBlocked(best_matches_);
     manager_action_ = kManagerActionBlacklisted;
+  }
+}
+
+void PasswordFormManager::ProcessFrame(
+    const base::WeakPtr<PasswordManagerDriver>& driver) {
+  if (state_ != POST_MATCHING_PHASE) {
+    drivers_.push_back(driver);
     return;
   }
 
-  // If not blacklisted, inform the driver that password generation is allowed
-  // for |observed_form_|.
-  if (driver_)
-    driver_->AllowPasswordGenerationForForm(observed_form_);
+  if (!driver || manager_action_ == kManagerActionBlacklisted)
+    return;
 
-  MaybeTriggerAutofill();
-}
+  // Allow generation for any non-blacklisted form.
+  driver->AllowPasswordGenerationForForm(observed_form_);
 
-void PasswordFormManager::MaybeTriggerAutofill() {
-  DCHECK_EQ(POST_MATCHING_PHASE, state_);
-  if (!driver_ || best_matches_.empty() ||
-      manager_action_ == kManagerActionBlacklisted) {
+  if (best_matches_.empty())
     return;
-  }
 
   // Proceed to autofill.
   // Note that we provide the choices but don't actually prefill a value if:
@@ -535,7 +530,7 @@ void PasswordFormManager::MaybeTriggerAutofill() {
     manager_action_ = kManagerActionNone;
   else
     manager_action_ = kManagerActionAutofilled;
-  password_manager_->Autofill(driver_.get(), observed_form_, best_matches_,
+  password_manager_->Autofill(driver.get(), observed_form_, best_matches_,
                               *preferred_match_, wait_for_username);
 }
 
@@ -550,16 +545,15 @@ void PasswordFormManager::OnGetPasswordStoreResults(
     logger->LogNumber(Logger::STRING_NUMBER_RESULTS, results.size());
   }
 
-  if (results.empty()) {
-    state_ = POST_MATCHING_PHASE;
-    // No result means that we visit this site the first time so we don't need
-    // to check whether this site is blacklisted or not. Just send a message
-    // to allow password generation.
-    if (driver_)
-      driver_->AllowPasswordGenerationForForm(observed_form_);
-    return;
+  if (!results.empty())
+    OnRequestDone(results);
+  state_ = POST_MATCHING_PHASE;
+
+  if (manager_action_ != kManagerActionBlacklisted) {
+    for (auto const& driver : drivers_)
+      ProcessFrame(driver);
   }
-  OnRequestDone(results);
+  drivers_.clear();
 }
 
 bool PasswordFormManager::ShouldIgnoreResult(const PasswordForm& form) const {

components/password_manager/core/browser/password_form_manager.h

@@ -106,13 +106,11 @@ class PasswordFormManager : public PasswordStoreConsumer {
   bool HasGeneratedPassword();
   void SetHasGeneratedPassword();
 
-  // Determines if we need to autofill given the results of the query.
-  // Takes ownership of the elements in |result|.
-  void OnRequestDone(const std::vector<autofill::PasswordForm*>& result);
-
-  // If reasonable, non-blacklisted, candidates were found by the password
-  // store, this will trigger the autofill through PasswordManager.
-  void MaybeTriggerAutofill();
+  // Through |driver|, supply the associated frame with appropriate information
+  // (fill data, whether to allow password generation, etc.). If this is called
+  // before |this| has data from the PasswordStore, the execution will be
+  // delayed until the data arrives.
+  void ProcessFrame(const base::WeakPtr<PasswordManagerDriver>& driver);
 
   void OnGetPasswordStoreResults(
       const std::vector<autofill::PasswordForm*>& results) override;
@@ -217,6 +215,10 @@ class PasswordFormManager : public PasswordStoreConsumer {
   static const int kMaxNumActionsTaken = kManagerActionMax * kUserActionMax *
                                          kSubmitResultMax;
 
+  // Determines if we need to autofill given the results of the query.
+  // Takes ownership of the elements in |result|.
+  void OnRequestDone(const std::vector<autofill::PasswordForm*>& result);
+
   // Helper for OnGetPasswordStoreResults to determine whether or not
   // the given result form is worth scoring.
   bool ShouldIgnoreResult(const autofill::PasswordForm& form) const;
@@ -328,17 +330,14 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // The client which implements embedder-specific PasswordManager operations.
   PasswordManagerClient* client_;
 
-  // The driver for frame-specific operations. Note that while |driver_| lives
-  // only as long as its associated frame, |this| is kept alive at least until
-  // main frame navigation or even past that (if it carries a provisionally
-  // saved password). Normally, |this| requests stored credentials from
-  // PasswordStore, and only needs |driver_| to update the frame with those as
-  // soon as the store calls back with the results. But because the store
-  // retrieval involves UI->DB->UI thread communication, the frame (and
-  // |driver_|) might get deleted sooner than the store calls back. In that
-  // case, the updates of the frame are no longer needed, and calls to |driver_|
-  // should silently be omitted.
-  const base::WeakPtr<PasswordManagerDriver> driver_;
+  // When |this| is created, it is because a form has been found in a frame,
+  // represented by a driver. For that frame, and for all others with an
+  // equivalent form, the associated drivers are needed to perform
+  // frame-specific operations (filling etc.). While |this| waits for the
+  // matching credentials from the PasswordStore, the drivers for frames needing
+  // fill information are buffered in |drivers_|, and served once the results
+  // arrive, if the drivers are still valid.
+  std::vector<base::WeakPtr<PasswordManagerDriver>> drivers_;
 
   // These three fields record the "ActionsTaken" by the browser and
   // the user with this form, and the result. They are combined and

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -952,7 +952,7 @@ TEST_F(PasswordFormManagerTest, TestUpdateIncompleteCredentials) {
   // Feed the incomplete credentials to the manager.
   std::vector<PasswordForm*> results;
   results.push_back(incomplete_form);  // Takes ownership.
-  form_manager.OnRequestDone(results);
+  form_manager.OnGetPasswordStoreResults(results);
 
   form_manager.ProvisionallySave(
       complete_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
@@ -1344,7 +1344,7 @@ TEST_F(PasswordFormManagerTest, DriverDeletedBeforeStoreDone) {
 
   std::vector<PasswordForm*> results;
   results.push_back(form.release());
-  form_manager.OnRequestDone(results);
+  form_manager.OnGetPasswordStoreResults(results);
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/password_manager.cc

@@ -435,7 +435,7 @@ void PasswordManager::CreatePendingLoginManagers(
       }
       old_manager_found = true;
       if (old_manager->HasCompletedMatching())
-        old_manager->MaybeTriggerAutofill();
+        old_manager->ProcessFrame(driver->AsWeakPtr());
       break;
     }
     if (old_manager_found)

components/password_manager/core/browser/password_manager_unittest.cc

@@ -566,7 +566,7 @@ TEST_F(PasswordManagerTest, InitiallyInvisibleForm) {
   result.push_back(existing);
   EXPECT_CALL(driver_, FillPasswordForm(_));
   EXPECT_CALL(*store_.get(), GetLogins(_, _, _))
-      .WillRepeatedly(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
+      .WillOnce(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
@@ -603,7 +603,7 @@ TEST_F(PasswordManagerTest, FillPasswordsOnDisabledManager) {
   EXPECT_CALL(driver_, FillPasswordForm(_));
   EXPECT_CALL(*store_.get(),
               GetLogins(_, testing::Eq(PasswordStore::DISALLOW_PROMPT), _))
-      .WillRepeatedly(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
+      .WillOnce(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
   std::vector<PasswordForm> observed;
   PasswordForm form(MakeSimpleForm());
   observed.push_back(form);
@@ -891,4 +891,30 @@ TEST_F(PasswordManagerTest, FormSubmitWithOnlyPassowrdField) {
   form_to_save->Save();
 }
 
+TEST_F(PasswordManagerTest, FillPasswordOnManyFrames) {
+  // If one password form appears in more frames, it should be filled in all of
+  // them.
+  PasswordForm form(MakeSimpleForm());  // The observed and saved form.
+
+  // "Save" the form.
+  std::vector<PasswordForm*> result;
+  result.push_back(new PasswordForm(form));  // Calee owns the form.
+  EXPECT_CALL(*store_.get(),
+              GetLogins(_, testing::Eq(PasswordStore::DISALLOW_PROMPT), _))
+      .WillOnce(DoAll(WithArg<2>(InvokeConsumer(result)), Return()));
+
+  // The form will be seen the first time.
+  EXPECT_CALL(driver_, FillPasswordForm(_)).Times(1);
+  std::vector<PasswordForm> observed;
+  observed.push_back(form);
+  manager()->OnPasswordFormsParsed(&driver_, observed);
+
+  // Now the form will be seen the second time, in a different frame. The driver
+  // for that frame should be told to fill it, but the store should not be asked
+  // for it again.
+  MockPasswordManagerDriver driver_b;
+  EXPECT_CALL(driver_b, FillPasswordForm(_)).Times(1);
+  manager()->OnPasswordFormsParsed(&driver_b, observed);
+}
+
 }  // namespace password_manager