Move UpdateEngagedSites(...) to worker pool

This CL is moving the computation of engaged sites to the worker pool.

Move the worker model to a single static function to avoid any
incorrect fields accesses. The worker compute the sites and
pass it to the main thread.

The callbacks are queued and executed when at the completion of the
background computation.

Bug: 1157596
Change-Id: I4021d87fde51d673f78154b6cd5fc94fa71356d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2627758Reviewed-by: Joe DeBlasio <jdeblasio@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843572}

chrome/browser/lookalikes/lookalike_url_service.cc

@@ -30,7 +30,8 @@
 
 namespace {
 
-constexpr uint32_t kEngagedSiteUpdateIntervalInSeconds = 60;
+constexpr base::TimeDelta kEngagedSiteUpdateInterval =
+    base::TimeDelta::FromSeconds(60);
 
 class LookalikeUrlServiceFactory : public BrowserContextKeyedServiceFactory {
  public:
@@ -70,14 +71,40 @@ class LookalikeUrlServiceFactory : public BrowserContextKeyedServiceFactory {
   DISALLOW_COPY_AND_ASSIGN(LookalikeUrlServiceFactory);
 };
 
+// static
+std::vector<DomainInfo> UpdateEngagedSitesOnWorkerThread(
+    base::Time now,
+    scoped_refptr<HostContentSettingsMap> map) {
+  std::vector<DomainInfo> new_engaged_sites;
+
+  auto details =
+      site_engagement::SiteEngagementService::GetAllDetailsInBackground(now,
+                                                                        map);
+  for (const site_engagement::mojom::SiteEngagementDetails& detail : details) {
+    if (!detail.origin.SchemeIsHTTPOrHTTPS()) {
+      continue;
+    }
+    // Ignore sites with an engagement score below threshold.
+    if (detail.total_score <
+        site_engagement::SiteEngagementScore::GetMediumEngagementBoundary()) {
+      continue;
+    }
+    const DomainInfo domain_info = GetDomainInfo(detail.origin);
+    if (domain_info.domain_and_registry.empty()) {
+      continue;
+    }
+    new_engaged_sites.push_back(domain_info);
+  }
+
+  return new_engaged_sites;
+}
+
 }  // namespace
 
 LookalikeUrlService::LookalikeUrlService(Profile* profile)
-    : profile_(profile),
-      clock_(base::DefaultClock::GetInstance()),
-      update_in_progress_(false) {}
+    : profile_(profile), clock_(base::DefaultClock::GetInstance()) {}
 
-LookalikeUrlService::~LookalikeUrlService() {}
+LookalikeUrlService::~LookalikeUrlService() = default;
 
 // static
 LookalikeUrlService* LookalikeUrlService::Get(Profile* profile) {
@@ -85,39 +112,40 @@ LookalikeUrlService* LookalikeUrlService::Get(Profile* profile) {
 }
 
 bool LookalikeUrlService::EngagedSitesNeedUpdating() const {
-  if (!last_engagement_fetch_time_.is_null()) {
-    const base::TimeDelta elapsed = clock_->Now() - last_engagement_fetch_time_;
-    return (elapsed >=
-            base::TimeDelta::FromSeconds(kEngagedSiteUpdateIntervalInSeconds));
-  }
-  return true;
+  if (last_engagement_fetch_time_.is_null())
+    return true;
+  const base::TimeDelta elapsed = clock_->Now() - last_engagement_fetch_time_;
+  return elapsed >= kEngagedSiteUpdateInterval;
 }
 
 void LookalikeUrlService::ForceUpdateEngagedSites(
     EngagedSitesCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // Queue an update if necessary.
+  // Queue an update on a worker thread if necessary.
   if (!update_in_progress_) {
     update_in_progress_ = true;
-    base::SequencedTaskRunnerHandle::Get()->PostTask(
+
+    base::ThreadPool::PostTaskAndReplyWithResult(
         FROM_HERE,
+        {base::TaskPriority::USER_BLOCKING,
+         base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
         base::BindOnce(
-            &LookalikeUrlService::UpdateEngagedSitesInBackground,
-            weak_factory_.GetWeakPtr(),
+            &UpdateEngagedSitesOnWorkerThread,
+            clock_->Now(),
             base::WrapRefCounted(
-                HostContentSettingsMapFactory::GetForProfile(profile_))));
+                HostContentSettingsMapFactory::GetForProfile(profile_))),
+        base::BindOnce(&LookalikeUrlService::OnUpdateEngagedSitesCompleted,
+                       weak_factory_.GetWeakPtr()));
   }
 
-  // Post the callback to the same sequenced task runner, which will run it
-  // after the update is complete.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::BindOnce(std::move(callback), std::cref(engaged_sites_)));
+  // Postpone the execution of the callback after the update is completed.
+  pending_update_complete_callbacks_.push_back(std::move(callback));
 }
 
 const std::vector<DomainInfo> LookalikeUrlService::GetLatestEngagedSites()
     const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return engaged_sites_;
 }
 
@@ -125,37 +153,17 @@ void LookalikeUrlService::SetClockForTesting(base::Clock* clock) {
   clock_ = clock;
 }
 
-void LookalikeUrlService::UpdateEngagedSitesInBackground(
-    scoped_refptr<HostContentSettingsMap> map) {
+void LookalikeUrlService::OnUpdateEngagedSitesCompleted(
+    std::vector<DomainInfo> new_engaged_sites) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(update_in_progress_);
-
-  // Bail if another update has occurred since this update was scheduled.
-  if (!EngagedSitesNeedUpdating()) {
-    return;
-  }
-
-  auto details =
-      site_engagement::SiteEngagementService::GetAllDetailsInBackground(
-          clock_->Now(), map);
 
-  std::vector<DomainInfo> new_engaged_sites;
-  for (const site_engagement::mojom::SiteEngagementDetails& detail : details) {
-    if (!detail.origin.SchemeIsHTTPOrHTTPS()) {
-      continue;
-    }
-    // Ignore sites with an engagement score below threshold.
-    if (detail.total_score <
-        site_engagement::SiteEngagementScore::GetMediumEngagementBoundary()) {
-      continue;
-    }
-    const DomainInfo domain_info = GetDomainInfo(detail.origin);
-    if (domain_info.domain_and_registry.empty()) {
-      continue;
-    }
-    new_engaged_sites.push_back(domain_info);
-  }
   engaged_sites_.swap(new_engaged_sites);
   last_engagement_fetch_time_ = clock_->Now();
   update_in_progress_ = false;
+
+  // Call pending callbacks.
+  std::vector<EngagedSitesCallback> callbacks;
+  callbacks.swap(pending_update_complete_callbacks_);
+  for (auto&& callback : callbacks)
+    std::move(callback).Run(engaged_sites_);
 }

chrome/browser/lookalikes/lookalike_url_service.h

@@ -17,7 +17,6 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/lookalikes/core/lookalike_url_util.h"
-#include "components/url_formatter/url_formatter.h"
 
 class Profile;
 
@@ -54,18 +53,17 @@ class LookalikeUrlService : public KeyedService {
   void SetClockForTesting(base::Clock* clock);
 
  private:
-  void UpdateEngagedSitesInBackground(
-      scoped_refptr<HostContentSettingsMap> map);
+  void OnUpdateEngagedSitesCompleted(std::vector<DomainInfo> new_engaged_sites);
 
   Profile* profile_;
   base::Clock* clock_;
   base::Time last_engagement_fetch_time_;
-  std::vector<DomainInfo> engaged_sites_;
+  std::vector<DomainInfo> engaged_sites_ GUARDED_BY_CONTEXT(sequence_checker_);
 
   // Indicates that an update to the engaged sites list has been queued. Serves
   // to prevent enqueuing excessive updates.
-  bool update_in_progress_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  bool update_in_progress_ = false;
+  std::vector<EngagedSitesCallback> pending_update_complete_callbacks_;
 
   SEQUENCE_CHECKER(sequence_checker_);