Add support for multiple requests to AffiliationService.

This CL adds a vector of pending fetchers that enable handling parallel requests.
It uses a new struct FetchInfo to group together the fetcher, dedicated  callback and requested origins, which facilitates processing each response.


Bug: 1117045
Change-Id: I0f7529d02266f2cc20e8e0e4f4c77638483ce1b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436329
Commit-Queue: Nela Kaczmarek <nelakaczmarek@google.com>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812639}

components/password_manager/core/browser/site_affiliation/affiliation_service_impl.cc

@@ -7,6 +7,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/ranges/algorithm.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "components/password_manager/core/browser/android_affiliation/affiliation_fetcher.h"
 #include "components/password_manager/core/browser/password_store_factory_util.h"
 #include "components/sync/driver/sync_service.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -17,6 +18,10 @@ namespace password_manager {
 
 namespace {
 
+void LogFetchResult(metrics_util::GetChangePasswordUrlMetric result) {
+  base::UmaHistogramEnumeration(kGetChangePasswordURLMetricName, result);
+}
+
 // Creates a look-up (Facet URI : change password URL) map for facets from
 // requested |groupings|. If a facet does not have change password URL it gets
 // paired with another facet's URL, which belongs to the same group. In case
@@ -54,6 +59,33 @@ CreateFacetUriToChangePasswordUrlMap(
 const char kGetChangePasswordURLMetricName[] =
     "PasswordManager.AffiliationService.GetChangePasswordUsage";
 
+struct AffiliationServiceImpl::FetchInfo {
+  FetchInfo(std::unique_ptr<AffiliationFetcherInterface> pending_fetcher,
+            std::vector<url::SchemeHostPort> tuple_origins,
+            base::OnceClosure result_callback)
+      : fetcher(std::move(pending_fetcher)),
+        requested_tuple_origins(std::move(tuple_origins)),
+        callback(std::move(result_callback)) {}
+
+  FetchInfo(FetchInfo&& other) = default;
+
+  FetchInfo& operator=(FetchInfo&& other) = default;
+
+  ~FetchInfo() {
+    // The check is essential here, because emplace_back calls move constructor
+    // and destructor, respectively. Therefore, the check is necessary to
+    // prevent accessing already moved object.
+    if (callback)
+      std::move(callback).Run();
+  }
+
+  std::unique_ptr<AffiliationFetcherInterface> fetcher;
+  std::vector<url::SchemeHostPort> requested_tuple_origins;
+  // Callback is passed in PrefetchChangePasswordURLs and is run to indicate the
+  // prefetch has finished or got canceled.
+  base::OnceClosure callback;
+};
+
 AffiliationServiceImpl::AffiliationServiceImpl(
     syncer::SyncService* sync_service,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
@@ -66,18 +98,17 @@ AffiliationServiceImpl::~AffiliationServiceImpl() = default;
 void AffiliationServiceImpl::PrefetchChangePasswordURLs(
     const std::vector<GURL>& urls,
     base::OnceClosure callback) {
-  result_callback_ = std::move(callback);
   if (ShouldAffiliationBasedMatchingBeActive(sync_service_)) {
-    RequestFacetsAffiliations(ConvertMissingURLsToFacets(urls),
-                              {.change_password_info = true});
+    RequestFacetsAffiliations(urls, {.change_password_info = true},
+                              std::move(callback));
   } else {
-    base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, std::move(result_callback_));
+    base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                     std::move(callback));
   }
 }
 
 void AffiliationServiceImpl::Clear() {
-  fetcher_.reset();
+  pending_fetches_.clear();
   change_password_urls_.clear();
 }
 
@@ -85,23 +116,22 @@ GURL AffiliationServiceImpl::GetChangePasswordURL(const GURL& url) const {
   auto it = change_password_urls_.find(url::SchemeHostPort(url));
   if (it != change_password_urls_.end()) {
     if (it->second.group_url_override) {
-      base::UmaHistogramEnumeration(
-          kGetChangePasswordURLMetricName,
+      LogFetchResult(
           metrics_util::GetChangePasswordUrlMetric::kGroupUrlOverrideUsed);
     } else {
-      base::UmaHistogramEnumeration(
-          kGetChangePasswordURLMetricName,
+      LogFetchResult(
           metrics_util::GetChangePasswordUrlMetric::kUrlOverrideUsed);
     }
     return it->second.change_password_url;
   }
-  if (base::Contains(requested_tuple_origins_, url::SchemeHostPort(url))) {
-    base::UmaHistogramEnumeration(
-        kGetChangePasswordURLMetricName,
-        metrics_util::GetChangePasswordUrlMetric::kNotFetchedYet);
+
+  url::SchemeHostPort tuple(url);
+  if (base::ranges::any_of(pending_fetches_, [&tuple](const auto& info) {
+        return base::Contains(info.requested_tuple_origins, tuple);
+      })) {
+    LogFetchResult(metrics_util::GetChangePasswordUrlMetric::kNotFetchedYet);
   } else {
-    base::UmaHistogramEnumeration(
-        kGetChangePasswordURLMetricName,
+    LogFetchResult(
         metrics_util::GetChangePasswordUrlMetric::kNoUrlOverrideAvailable);
   }
   return GURL();
@@ -110,10 +140,15 @@ GURL AffiliationServiceImpl::GetChangePasswordURL(const GURL& url) const {
 void AffiliationServiceImpl::OnFetchSucceeded(
     AffiliationFetcherInterface* fetcher,
     std::unique_ptr<AffiliationFetcherDelegate::Result> result) {
-  fetcher_.reset();
+  auto processed_fetch =
+      base::ranges::find(pending_fetches_, fetcher,
+                         [](const auto& info) { return info.fetcher.get(); });
+  if (processed_fetch == pending_fetches_.end())
+    return;
+
   std::map<FacetURI, AffiliationServiceImpl::ChangePasswordUrlMatch>
       uri_to_url = CreateFacetUriToChangePasswordUrlMap(result->groupings);
-  for (const url::SchemeHostPort& requested_tuple : requested_tuple_origins_) {
+  for (const auto& requested_tuple : processed_fetch->requested_tuple_origins) {
     auto it = uri_to_url.find(
         FacetURI::FromPotentiallyInvalidSpec(requested_tuple.Serialize()));
     if (it != uri_to_url.end()) {
@@ -121,48 +156,44 @@ void AffiliationServiceImpl::OnFetchSucceeded(
     }
   }
 
-  requested_tuple_origins_.clear();
-  std::move(result_callback_).Run();
+  pending_fetches_.erase(processed_fetch);
 }
 
 void AffiliationServiceImpl::OnFetchFailed(
     AffiliationFetcherInterface* fetcher) {
-  fetcher_.reset();
-  requested_tuple_origins_.clear();
-  std::move(result_callback_).Run();
+  base::EraseIf(pending_fetches_, [fetcher](const auto& info) {
+    return info.fetcher.get() == fetcher;
+  });
 }
 
 void AffiliationServiceImpl::OnMalformedResponse(
     AffiliationFetcherInterface* fetcher) {
-  fetcher_.reset();
-  requested_tuple_origins_.clear();
-  std::move(result_callback_).Run();
+  base::EraseIf(pending_fetches_, [fetcher](const auto& info) {
+    return info.fetcher.get() == fetcher;
+  });
 }
 
-std::vector<FacetURI> AffiliationServiceImpl::ConvertMissingURLsToFacets(
-    const std::vector<GURL>& urls) {
+void AffiliationServiceImpl::RequestFacetsAffiliations(
+    const std::vector<GURL>& urls,
+    const AffiliationFetcherInterface::RequestInfo request_info,
+    base::OnceClosure callback) {
   std::vector<FacetURI> facets;
+  std::vector<url::SchemeHostPort> tuple_origins;
   for (const auto& url : urls) {
     if (url.is_valid()) {
       url::SchemeHostPort scheme_host_port(url);
       if (!base::Contains(change_password_urls_, scheme_host_port)) {
         facets.push_back(
             FacetURI::FromCanonicalSpec(scheme_host_port.Serialize()));
-        requested_tuple_origins_.push_back(std::move(scheme_host_port));
+        tuple_origins.push_back(std::move(scheme_host_port));
       }
     }
   }
-  return facets;
-}
-
-// TODO(crbug.com/1117045): New request resets the pointer to
-// AffiliationFetcher, therefore the previous request gets canceled.
-void AffiliationServiceImpl::RequestFacetsAffiliations(
-    const std::vector<FacetURI>& facets,
-    const AffiliationFetcherInterface::RequestInfo request_info) {
   if (!facets.empty()) {
-    fetcher_ = fetcher_factory_->CreateInstance(url_loader_factory_, this);
-    fetcher_->StartRequest(facets, request_info);
+    auto fetcher = fetcher_factory_->CreateInstance(url_loader_factory_, this);
+    fetcher->StartRequest(facets, request_info);
+    pending_fetches_.emplace_back(std::move(fetcher), tuple_origins,
+                                  std::move(callback));
   }
 }
 

components/password_manager/core/browser/site_affiliation/affiliation_service_impl.h

@@ -48,19 +48,18 @@ class AffiliationServiceImpl : public AffiliationService,
 
   // Prefetches change password URLs and saves them to |change_password_urls_|
   // map. The verification if affiliation based matching is enabled must be
-  // performed. Assigns the callback to |result_callback_|.
+  // performed. Creates a unique fetcher and appends it to |pending_fetches_|
+  // along with |urls| and |callback|.
   void PrefetchChangePasswordURLs(const std::vector<GURL>& urls,
                                   base::OnceClosure callback) override;
 
-  // Clears the |change_password_urls_| map and cancels prefetch if still
-  // running.
+  // Clears the |change_password_urls_| map and cancels prefetch requests if
+  // still running.
   void Clear() override;
 
   // In case no valid URL was found, a method returns an empty URL.
   GURL GetChangePasswordURL(const GURL& url) const override;
 
-  AffiliationFetcherInterface* GetFetcherForTesting() { return fetcher_.get(); }
-
   void SetURLLoaderFactoryForTesting(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
     url_loader_factory_ = std::move(url_loader_factory);
@@ -76,6 +75,8 @@ class AffiliationServiceImpl : public AffiliationService,
   }
 
  private:
+  struct FetchInfo;
+
   // AffiliationFetcherDelegate:
   void OnFetchSucceeded(
       AffiliationFetcherInterface* fetcher,
@@ -83,27 +84,20 @@ class AffiliationServiceImpl : public AffiliationService,
   void OnFetchFailed(AffiliationFetcherInterface* fetcher) override;
   void OnMalformedResponse(AffiliationFetcherInterface* fetcher) override;
 
-  // Converts new |urls| to facets and inserts them to the
-  // |change_password_urls_|.
-  std::vector<FacetURI> ConvertMissingURLsToFacets(
-      const std::vector<GURL>& urls);
-
-  // Calls Affiliation Fetcher and starts a request for |facets| affiliations.
+  // Creates AffiliationFetcher and starts a request to retrieve affiliations
+  // for given |urls|. |Request_info| defines what info should be requested.
+  // When prefetch is finished or a fetcher gets destroyed as a result of
+  // Clear() a callback is run.
   void RequestFacetsAffiliations(
-      const std::vector<FacetURI>& facets,
-      const AffiliationFetcherInterface::RequestInfo request_info);
+      const std::vector<GURL>& urls,
+      const AffiliationFetcherInterface::RequestInfo request_info,
+      base::OnceClosure callback);
 
   syncer::SyncService* sync_service_;
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
-  std::vector<url::SchemeHostPort> requested_tuple_origins_;
   std::map<url::SchemeHostPort, ChangePasswordUrlMatch> change_password_urls_;
-  // TODO(crbug.com/1117045): A vector of pending fetchers to be created.
-  std::unique_ptr<AffiliationFetcherInterface> fetcher_;
+  std::vector<FetchInfo> pending_fetches_;
   std::unique_ptr<AffiliationFetcherFactory> fetcher_factory_;
-  // Callback is passed in PrefetchChangePasswordURLs and is run in
-  // OnFetchSucceeded, OnFetchMalformed, OnFetchFailed to indicate the prefetch
-  // has finished.
-  base::OnceClosure result_callback_;
 };
 
 }  // namespace password_manager

components/password_manager/core/browser/site_affiliation/affiliation_service_impl_unittest.cc

@@ -35,6 +35,7 @@ constexpr char kOneExampleURL[] = "https://one.example.com";
 constexpr char kOneExampleChangePasswordURL[] =
     "https://one.example.com/settings/passwords";
 constexpr char k2ExampleURL[] = "https://2.example.com";
+constexpr char k2ExampleChangePasswordURL[] = "https://2.example.com/pwd";
 constexpr char k3ExampleURL[] = "https://3.example.com";
 constexpr char k4ExampleURL[] = "https://4.example.com";
 constexpr char k5ExampleURL[] = "https://5.example.com";
@@ -88,7 +89,7 @@ TEST_F(AffiliationServiceImplTest, GetChangePasswordURLReturnsEmpty) {
   EXPECT_EQ(GURL(), service()->GetChangePasswordURL(GURL(k1ExampleURL)));
 }
 
-TEST_F(AffiliationServiceImplTest, ClearStopsOngoingAffiliationFetcherRequest) {
+TEST_F(AffiliationServiceImplTest, ClearStopsOngoingRequest) {
   const std::vector<GURL> origins = {GURL(k1ExampleURL), GURL(k2ExampleURL)};
   auto mock_fetcher = std::make_unique<MockAffiliationFetcher>();
 
@@ -99,11 +100,11 @@ TEST_F(AffiliationServiceImplTest, ClearStopsOngoingAffiliationFetcherRequest) {
   EXPECT_CALL(mock_fetcher_factory(), CreateInstance)
       .WillOnce(Return(ByMove(std::move(mock_fetcher))));
 
-  service()->PrefetchChangePasswordURLs(origins, base::DoNothing());
-  EXPECT_NE(nullptr, service()->GetFetcherForTesting());
+  base::MockOnceClosure callback;
+  service()->PrefetchChangePasswordURLs(origins, callback.Get());
 
+  EXPECT_CALL(callback, Run());
   service()->Clear();
-  EXPECT_EQ(nullptr, service()->GetFetcherForTesting());
 }
 
 TEST_F(AffiliationServiceImplTest,
@@ -210,12 +211,12 @@ TEST_F(AffiliationServiceImplTest, OnFetchFailedResetsFetcher) {
   EXPECT_CALL(mock_fetcher_factory(), CreateInstance)
       .WillOnce(Return(ByMove(std::move(mock_fetcher))));
 
-  service()->PrefetchChangePasswordURLs(origins, base::DoNothing());
-  EXPECT_NE(nullptr, service()->GetFetcherForTesting());
+  base::MockOnceClosure callback;
+  service()->PrefetchChangePasswordURLs(origins, callback.Get());
 
+  EXPECT_CALL(callback, Run());
   static_cast<AffiliationFetcherDelegate*>(service())->OnFetchFailed(
       raw_mock_fetcher);
-  EXPECT_EQ(nullptr, service()->GetFetcherForTesting());
 }
 
 TEST_F(AffiliationServiceImplTest, OnMalformedResponseResetsFetcher) {
@@ -230,12 +231,12 @@ TEST_F(AffiliationServiceImplTest, OnMalformedResponseResetsFetcher) {
   EXPECT_CALL(mock_fetcher_factory(), CreateInstance)
       .WillOnce(Return(ByMove(std::move(mock_fetcher))));
 
-  service()->PrefetchChangePasswordURLs(origins, base::DoNothing());
-  EXPECT_NE(nullptr, service()->GetFetcherForTesting());
+  base::MockOnceClosure callback;
+  service()->PrefetchChangePasswordURLs(origins, callback.Get());
 
+  EXPECT_CALL(callback, Run());
   static_cast<AffiliationFetcherDelegate*>(service())->OnMalformedResponse(
       raw_mock_fetcher);
-  EXPECT_EQ(nullptr, service()->GetFetcherForTesting());
 }
 
 TEST_F(AffiliationServiceImplTest,
@@ -415,4 +416,49 @@ TEST_F(AffiliationServiceImplTest, OnFetchSuccedeedRunsCallback) {
       raw_mock_fetcher, std::make_unique<AffiliationFetcherDelegate::Result>());
 }
 
+TEST_F(AffiliationServiceImplTest, SupportForMultipleRequests) {
+  const GURL origin1(k1ExampleURL);
+  const GURL origin2(k2ExampleURL);
+  const std::vector<GURL> origins_1 = {origin1};
+  const std::vector<GURL> origins_2 = {origin2};
+
+  auto mock_fetcher = std::make_unique<MockAffiliationFetcher>();
+  auto* raw_mock_fetcher = mock_fetcher.get();
+  auto new_mock_fetcher = std::make_unique<MockAffiliationFetcher>();
+  auto* new_raw_mock_fetcher = new_mock_fetcher.get();
+
+  AffiliationFetcher::RequestInfo request_info{.change_password_info = true};
+
+  EXPECT_CALL(*mock_fetcher,
+              StartRequest(ToFacetsURIs(origins_1), request_info));
+  EXPECT_CALL(*new_mock_fetcher,
+              StartRequest(ToFacetsURIs(origins_2), request_info));
+  EXPECT_CALL(mock_fetcher_factory(), CreateInstance)
+      .WillOnce(Return(ByMove(std::move(mock_fetcher))))
+      .WillOnce(Return(ByMove(std::move(new_mock_fetcher))));
+
+  service()->PrefetchChangePasswordURLs(origins_1, base::DoNothing());
+  service()->PrefetchChangePasswordURLs(origins_2, base::DoNothing());
+
+  const GroupedFacets group1 = {
+      {.uri = FacetURI::FromPotentiallyInvalidSpec(k1ExampleURL),
+       .change_password_url = GURL(k1ExampleChangePasswordURL)}};
+  auto test_result1 = std::make_unique<AffiliationFetcherDelegate::Result>();
+  test_result1->groupings.push_back(group1);
+  static_cast<AffiliationFetcherDelegate*>(service())->OnFetchSucceeded(
+      raw_mock_fetcher, std::move(test_result1));
+  EXPECT_EQ(GURL(k1ExampleChangePasswordURL),
+            service()->GetChangePasswordURL(origin1));
+
+  const GroupedFacets group2 = {
+      {.uri = FacetURI::FromPotentiallyInvalidSpec(k2ExampleURL),
+       .change_password_url = GURL(k2ExampleChangePasswordURL)}};
+  auto test_result2 = std::make_unique<AffiliationFetcherDelegate::Result>();
+  test_result2->groupings.push_back(group2);
+  static_cast<AffiliationFetcherDelegate*>(service())->OnFetchSucceeded(
+      new_raw_mock_fetcher, std::move(test_result2));
+  EXPECT_EQ(GURL(k2ExampleChangePasswordURL),
+            service()->GetChangePasswordURL(origin2));
+}
+
 }  // namespace password_manager

components/password_manager/core/browser/well_known_change_password_state_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/timer/mock_timer.h"
 #include "components/password_manager/core/browser/android_affiliation/mock_affiliation_fetcher.h"
 #include "components/password_manager/core/browser/site_affiliation/affiliation_service_impl.h"
+#include "components/password_manager/core/browser/site_affiliation/mock_affiliation_fetcher_factory.h"
 #include "components/password_manager/core/browser/well_known_change_password_util.h"
 #include "components/sync/driver/test_sync_service.h"
 #include "net/base/isolation_info.h"
@@ -230,11 +231,22 @@ TEST_P(WellKnownChangePasswordStateTest, TimeoutTriggersOnProcessingFinished) {
 
 TEST_P(WellKnownChangePasswordStateTest,
        PrefetchCallbackTriggersOnProcessingFinished) {
+  auto mock_fetcher = std::make_unique<MockAffiliationFetcher>();
+  auto* raw_mock_fetcher = mock_fetcher.get();
+  auto mock_fetcher_factory = std::make_unique<MockAffiliationFetcherFactory>();
+  EXPECT_CALL(*(mock_fetcher_factory.get()), CreateInstance)
+      .WillOnce(testing::Return(testing::ByMove(std::move(mock_fetcher))));
+
   syncer::TestSyncService test_sync_service;
+  test_sync_service.SetFirstSetupComplete(true);
+  test_sync_service.SetIsUsingSecondaryPassphrase(false);
   AffiliationServiceImpl affiliation_service(&test_sync_service,
                                              test_shared_loader_factory());
+  affiliation_service.SetFetcherFactoryForTesting(
+      std::move(mock_fetcher_factory));
 
-  state()->PrefetchChangePasswordURLs(&affiliation_service, {});
+  state()->PrefetchChangePasswordURLs(&affiliation_service,
+                                      {GURL("https://example.com")});
 
   ResponseDelayParams params = GetParam();
   RespondeToChangePasswordRequest(net::HTTP_NOT_FOUND,
@@ -245,10 +257,9 @@ TEST_P(WellKnownChangePasswordStateTest,
   FastForwardBy(base::TimeDelta::FromMilliseconds(ms_to_forward));
 
   EXPECT_CALL(*delegate(), OnProcessingFinished(false));
-  auto mock_fetcher = std::make_unique<MockAffiliationFetcher>();
   static_cast<AffiliationFetcherDelegate*>(&affiliation_service)
       ->OnFetchSucceeded(
-          mock_fetcher.get(),
+          raw_mock_fetcher,
           std::make_unique<AffiliationFetcherDelegate::Result>());
 }