Don't delay password requirements fetching after autofill responds

Chrome used to fetch password requirements specifications once the Autofill
server responded. This means that two network round trips were necessary until
the data was available.

With this change, the requirements are fetched every time the renderer reports
that password forms were found, which moves the lookup to an earlier point.

As this reporting can happen multiple times through the life-cycle of a site,
the CL also introduces an extra in-memory cache lookup that prevents further
processing if the requirements have been fetched already.

Bug: 873653, 846694
Change-Id: Id1bdd575fa659fcf05b40a937cb6aa00206e8965
Reviewed-on: https://chromium-review.googlesource.com/1172972Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582912}

components/password_manager/core/browser/password_generation_manager.cc

@@ -38,8 +38,7 @@ PasswordGenerationManager::PasswordGenerationManager(
 PasswordGenerationManager::~PasswordGenerationManager() {
 }
 
-void PasswordGenerationManager::ProcessPasswordRequirements(
-    const std::vector<autofill::FormStructure*>& forms) {
+void PasswordGenerationManager::PrefetchSpec(const GURL& origin) {
   // IsGenerationEnabled is called multiple times and it is sufficient to
   // log debug data once.
   if (!IsGenerationEnabled(/*log_debug_data=*/false))
@@ -53,10 +52,23 @@ void PasswordGenerationManager::ProcessPasswordRequirements(
     return;
 
   // Fetch password requirements for the domain.
-  if (IsRequirementsFetchingEnabled()) {
-    password_requirements_service->PrefetchSpec(
-        client_->GetLastCommittedEntryURL().GetOrigin());
-  }
+  if (IsRequirementsFetchingEnabled())
+    password_requirements_service->PrefetchSpec(origin);
+}
+
+void PasswordGenerationManager::ProcessPasswordRequirements(
+    const std::vector<autofill::FormStructure*>& forms) {
+  // IsGenerationEnabled is called multiple times and it is sufficient to
+  // log debug data once.
+  if (!IsGenerationEnabled(/*log_debug_data=*/false))
+    return;
+
+  // It is legit to have no PasswordRequirementsService on some platforms where
+  // it has not been implemented.
+  PasswordRequirementsService* password_requirements_service =
+      client_->GetPasswordRequirementsService();
+  if (!password_requirements_service)
+    return;
 
   // Store password requirements from the autofill server.
   for (const autofill::FormStructure* form : forms) {

components/password_manager/core/browser/password_generation_manager.h

@@ -40,6 +40,11 @@ class PasswordGenerationManager {
                             PasswordManagerDriver* driver);
   virtual ~PasswordGenerationManager();
 
+  // Instructs the PasswordRequirementsService to fetch requirements for
+  // |origin|. This needs to be called to enable domain-wide password
+  // requirements overrides.
+  void PrefetchSpec(const GURL& origin);
+
   // Stores password requirements received from the autofill server for the
   // |forms| and fetches domain-wide requirements.
   void ProcessPasswordRequirements(

components/password_manager/core/browser/password_generation_manager_unittest.cc

@@ -333,10 +333,11 @@ TEST_F(PasswordGenerationManagerTest, ProcessPasswordRequirements) {
                                             ? SYNCING_NORMAL_ENCRYPTION
                                             : NOT_SYNCING));
 
+    GetGenerationManager()->PrefetchSpec(origin.GetOrigin());
+
     // Processs the password requirements with expected side effects of
     // either storing the requirements from the AutofillQueryResponseContents)
-    // in the PasswordRequirementsService or triggering a lookup for the
-    // domain that is stored in the PasswordRequirementsService.
+    // in the PasswordRequirementsService.
     GetGenerationManager()->ProcessPasswordRequirements(forms);
 
     // Validate the result.

components/password_manager/core/browser/password_manager.cc

@@ -27,6 +27,7 @@
 #include "components/password_manager/core/browser/new_password_form_manager.h"
 #include "components/password_manager/core/browser/password_autofill_manager.h"
 #include "components/password_manager/core/browser/password_form_manager.h"
+#include "components/password_manager/core/browser/password_generation_manager.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
@@ -586,6 +587,13 @@ void PasswordManager::OnPasswordFormsParsed(
     password_manager::PasswordManagerDriver* driver,
     const std::vector<PasswordForm>& forms) {
   CreatePendingLoginManagers(driver, forms);
+
+  PasswordGenerationManager* password_generation_manager =
+      driver ? driver->GetPasswordGenerationManager() : nullptr;
+  if (password_generation_manager) {
+    password_generation_manager->PrefetchSpec(
+        client_->GetLastCommittedEntryURL().GetOrigin());
+  }
 }
 
 void PasswordManager::CreatePendingLoginManagers(

components/password_manager/core/browser/password_requirements_service.cc

@@ -69,6 +69,12 @@ void PasswordRequirementsService::PrefetchSpec(const GURL& main_frame_domain) {
     return;
   }
 
+  // No need to fetch the same data multiple times.
+  if (specs_for_domains_.Get(main_frame_domain) != specs_for_domains_.end()) {
+    VLOG(1) << "PasswordRequirementsService::PrefetchSpec has an entry already";
+    return;
+  }
+
   // Using base::Unretained(this) is safe here because the
   // PasswordRequirementsService owns fetcher_. If |this| is deleted, so is
   // the |fetcher_|, and no callback can happen.
@@ -96,4 +102,9 @@ void PasswordRequirementsService::AddSpec(
                             spec);
 }
 
+void PasswordRequirementsService::ClearDataForTestingImpl() {
+  specs_for_domains_.Clear();
+  specs_for_signatures_.Clear();
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/password_requirements_service.h

@@ -9,6 +9,7 @@
 #include <utility>
 
 #include "base/containers/mru_cache.h"
+#include "base/macros.h"
 #include "components/autofill/core/browser/password_requirements_spec_fetcher.h"
 #include "components/autofill/core/browser/proto/password_requirements.pb.h"
 #include "components/autofill/core/common/signatures_util.h"
@@ -52,9 +53,17 @@ class PasswordRequirementsService : public KeyedService {
                autofill::FieldSignature field_signature,
                const autofill::PasswordRequirementsSpec& spec);
 
+#if defined(UNIT_TEST)
+  // Wipes MRU cached data to ensure that it gets fetched again.
+  // This style of delegation is used because UNIT_TEST is only available in
+  // header files as per presubmit checks.
+  void ClearDataForTesting() { ClearDataForTestingImpl(); }
+#endif
+
  private:
   void OnFetchedRequirements(const GURL& main_frame_domain,
                              const autofill::PasswordRequirementsSpec& spec);
+  void ClearDataForTestingImpl();
 
   using FullSignature =
       std::pair<autofill::FormSignature, autofill::FieldSignature>;

components/password_manager/core/browser/password_requirements_service_unittest.cc

@@ -127,6 +127,8 @@ TEST_F(PasswordRequirementsServiceTest, ExerciseEverything) {
   for (const auto& test : tests) {
     SCOPED_TRACE(test.test_name);
 
+    service_.ClearDataForTesting();
+
     // Populate the service with data.
     if (test.spec_for_domain) {
       fetcher_ptr_->SetDataToReturn(test_origin_, *(test.spec_for_domain));