[Lookalikes] Ensure only one site engagement update occurs at once

This CL changes the way async callbacks to the lookalike service are
handled. Previously, if the list of engaged sites was out of date, a
background task would be spawned to update them. This could result in
a spike of near-simultaneous background tasks that would contend for the
same lock. This contributes to jank.

This CL changes the process to ensure that only one update is in flight
at a time, and simply pushes the on-updated callbacks onto the end of
the task sequence after any existing update completes.

Bug: 1157596,1135279
Change-Id: I0629f523491d8e4b8e966fa3b65f670c901e7169
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623005
Commit-Queue: Joe DeBlasio <jdeblasio@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842821}

chrome/browser/lookalikes/lookalike_url_service.cc

@@ -12,7 +12,6 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/singleton.h"
 #include "base/task/post_task.h"
-#include "base/task/thread_pool.h"
 #include "base/time/default_clock.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/engagement/site_engagement_service_factory.h"
@@ -74,7 +73,9 @@ class LookalikeUrlServiceFactory : public BrowserContextKeyedServiceFactory {
 }  // namespace
 
 LookalikeUrlService::LookalikeUrlService(Profile* profile)
-    : profile_(profile), clock_(base::DefaultClock::GetInstance()) {}
+    : profile_(profile),
+      clock_(base::DefaultClock::GetInstance()),
+      update_in_progress_(false) {}
 
 LookalikeUrlService::~LookalikeUrlService() {}
 
@@ -83,30 +84,36 @@ LookalikeUrlService* LookalikeUrlService::Get(Profile* profile) {
   return LookalikeUrlServiceFactory::GetForProfile(profile);
 }
 
-bool LookalikeUrlService::EngagedSitesNeedUpdating() {
+bool LookalikeUrlService::EngagedSitesNeedUpdating() const {
   if (!last_engagement_fetch_time_.is_null()) {
     const base::TimeDelta elapsed = clock_->Now() - last_engagement_fetch_time_;
-    if (elapsed <
-        base::TimeDelta::FromSeconds(kEngagedSiteUpdateIntervalInSeconds)) {
-      return false;
-    }
+    return (elapsed >=
+            base::TimeDelta::FromSeconds(kEngagedSiteUpdateIntervalInSeconds));
   }
   return true;
 }
 
 void LookalikeUrlService::ForceUpdateEngagedSites(
     EngagedSitesCallback callback) {
-  base::ThreadPool::PostTaskAndReplyWithResult(
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Queue an update if necessary.
+  if (!update_in_progress_) {
+    update_in_progress_ = true;
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &LookalikeUrlService::UpdateEngagedSitesInBackground,
+            weak_factory_.GetWeakPtr(),
+            base::WrapRefCounted(
+                HostContentSettingsMapFactory::GetForProfile(profile_))));
+  }
+
+  // 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::TaskPriority::USER_BLOCKING,
-       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
-      base::BindOnce(
-          &site_engagement::SiteEngagementService::GetAllDetailsInBackground,
-          clock_->Now(),
-          base::WrapRefCounted(
-              HostContentSettingsMapFactory::GetForProfile(profile_))),
-      base::BindOnce(&LookalikeUrlService::OnFetchEngagedSites,
-                     weak_factory_.GetWeakPtr(), std::move(callback)));
+      base::BindOnce(std::move(callback), std::cref(engaged_sites_)));
 }
 
 const std::vector<DomainInfo> LookalikeUrlService::GetLatestEngagedSites()
@@ -118,10 +125,21 @@ void LookalikeUrlService::SetClockForTesting(base::Clock* clock) {
   clock_ = clock;
 }
 
-void LookalikeUrlService::OnFetchEngagedSites(
-    EngagedSitesCallback callback,
-    std::vector<site_engagement::mojom::SiteEngagementDetails> details) {
-  engaged_sites_.clear();
+void LookalikeUrlService::UpdateEngagedSitesInBackground(
+    scoped_refptr<HostContentSettingsMap> map) {
+  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;
@@ -135,8 +153,9 @@ void LookalikeUrlService::OnFetchEngagedSites(
     if (domain_info.domain_and_registry.empty()) {
       continue;
     }
-    engaged_sites_.push_back(domain_info);
+    new_engaged_sites.push_back(domain_info);
   }
+  engaged_sites_.swap(new_engaged_sites);
   last_engagement_fetch_time_ = clock_->Now();
-  std::move(callback).Run(engaged_sites_);
+  update_in_progress_ = false;
 }

chrome/browser/lookalikes/lookalike_url_service.h

@@ -11,10 +11,12 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
+#include "base/task/thread_pool.h"
 #include "base/time/time.h"
+#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/site_engagement/core/mojom/site_engagement_details.mojom-forward.h"
 #include "components/url_formatter/url_formatter.h"
 
 class Profile;
@@ -37,11 +39,12 @@ class LookalikeUrlService : public KeyedService {
 
   static LookalikeUrlService* Get(Profile* profile);
 
-  // Returns whether the engaged site list is recently updated.
-  bool EngagedSitesNeedUpdating();
+  // Returns whether the engaged site list is recently updated. Returns true
+  // even when an update has already been queued or is in progress.
+  bool EngagedSitesNeedUpdating() const;
 
-  // Triggers an update to the engaged sites list and calls |callback| with the
-  // new list once available.
+  // Triggers an update to the engaged site list if one is not already inflight,
+  // then schedules |callback| to be called with the new list once available.
   void ForceUpdateEngagedSites(EngagedSitesCallback callback);
 
   // Returns the _current_ list of engaged sites, without updating them if
@@ -51,16 +54,22 @@ class LookalikeUrlService : public KeyedService {
   void SetClockForTesting(base::Clock* clock);
 
  private:
-  void OnFetchEngagedSites(
-      EngagedSitesCallback callback,
-      std::vector<site_engagement::mojom::SiteEngagementDetails> details);
+  void UpdateEngagedSitesInBackground(
+      scoped_refptr<HostContentSettingsMap> map);
 
   Profile* profile_;
   base::Clock* clock_;
   base::Time last_engagement_fetch_time_;
   std::vector<DomainInfo> engaged_sites_;
-  base::WeakPtrFactory<LookalikeUrlService> weak_factory_{this};
 
+  // 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_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  base::WeakPtrFactory<LookalikeUrlService> weak_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(LookalikeUrlService);
 };