[SHv2] Split up update tasks in a background and UI task

This CL changes the functionality of the Safety Hub service that allows
splitting up the update task in one for the background thread (where
the most expensive computations can take place), and one on the UI
thread. The former will be a static function to ensure that it is
thread-safe. The unused site permissions service is updated using this
new structure accordingly.

Change-Id: I9f16bf892dd04b3aae6e5842ce5d327d82989a59
Bug: 1443466
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4703064
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Commit-Queue: Tom Van Goethem <tov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1187212}

chrome/browser/ui/safety_hub/README.md

@@ -1,6 +1,57 @@
+## Services
+
 This directory contains the services related to Safety Hub. Each specific
 service inherits from `SafetyHubService`, as defined in `safety_hub_service.h`.
-The services override the `UpdateOnBackgroundThread()` and
-`GetRepeatedUpdateInterval()` methods. The former contains the functionality
-that will be periodically executed, whereas the latter determines the interval
-in which the function is executed.
+The service will be periodically updated, depending on the frequency determined
+by `GetRepeatedUpdateInterval()`. Additionally, an update will be launched every
+time that the service is started (whenever the browser is started). Hence, it's
+important to note that the update can be called more frequently than the
+interval returned by `GetRepeatedUpdateInterval()`.
+
+The update process consists of two stages, a background task and a UI thread
+task.
+
+The background task will be run on a separate thread in the background. The
+function that needs to be run in the background should contain the functionality
+of the update that is the most computation-heavy, to prevent blocking the UI
+thread and possibly freezing the browser. The background task can return an
+intermediate result, that will be passed along to the UI thread task. Ideally,
+the background task should be a static function that will be returned by
+`GetBackgroundTask()`. As this will be run in a thread other than the one the
+service runs in, any arguments that are bound to the function should be
+thread-safe. As the browser shuts down, references to these objects might be
+destroyed, possibly leading to memory issues. For instance, a reference to the
+service itself should NOT be bound to the function. This will result in crashes
+of other tests, and could cause the browser to crash when one profile is shut
+down.
+
+The UI thread task needs to be defined in `UpdateOnUIThread()`. It will be
+passed the intermediate result (a unique pointer to `SafetyHubService::Result`)
+that was returned by the background task. This method will be run synchronously
+on the UI thread after the background task has completed. The result by this UI
+thread task will be the final result that the observers will be notified of.
+`SafetyHubService::Result` will thus be used for 1) passing the intermediate
+result of `GetBackgroundTask()` to `UpdateOnUIThread()`, and 2) the final result
+that follows from running the update process of the service. To reduce
+unnecessary overhead, it is suggested that the final result does not contain any
+of the intermediate results, e.g. by creating a new `SafetyHubService::Result`
+in `UpdateOnUIThread()`.
+
+In summary, each Safety Hub service should implement the following functions:
+
+ - `GetRepeatedUpdateInterval()`: returns a `base::TimeDelta` that determines
+   the minimal frequency of how often the service is updated.
+ - `GetBackgroundTask()`: returns a bound (static) function that will be
+   executed in the background, containing the computation-heavy part of the
+   service's update.
+ - `UpdateOnUIThread()`: will be run synchronously on the UI thread, and will
+   further process the intermediate result of the background task.
+ - `GetAsWeakRef()`: returns a weak pointer to the service, from the
+   `WeakPtrFactory` of the derived service.
+
+## Testing
+
+As updating the service will run a task both in the background as well as on the
+UI thread, it is advised to use the helper function
+`UpdateSafetyHubServiceAsync(service)`, defined in `safety_hub_test_util.h` to
+trigger an update of the service in tests.

chrome/browser/ui/safety_hub/safety_hub_service.cc

@@ -26,28 +26,34 @@ void SafetyHubService::Shutdown() {
 
 void SafetyHubService::StartRepeatedUpdates() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // TODO(crbug.com/1443466): the 10 minute delay is a workaround of the task
-  // being posted to a different thread. This should be removed.
-  delay_timer_.Start(
-      FROM_HERE, base::Minutes(10),
-      base::BindOnce(&SafetyHubService::UpdateAsync, GetAsWeakRef()));
+  UpdateAsync();
   update_timer_.Start(FROM_HERE, GetRepeatedUpdateInterval(),
                       base::BindRepeating(&SafetyHubService::UpdateAsync,
                                           base::Unretained(this)));
 }
 
 void SafetyHubService::UpdateAsync() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (pending_updates_++) {
+    return;
+  }
+  UpdateAsyncInternal();
+}
+
+void SafetyHubService::UpdateAsyncInternal() {
   base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::TaskPriority::BEST_EFFORT},
-      base::BindOnce(&SafetyHubService::UpdateOnBackgroundThread,
-                     base::Unretained(this)),
+      FROM_HERE, {base::TaskPriority::BEST_EFFORT}, GetBackgroundTask(),
       base::BindOnce(&SafetyHubService::OnUpdateFinished, GetAsWeakRef()));
 }
 
-void SafetyHubService::OnUpdateFinished(std::unique_ptr<Result> result) {
+void SafetyHubService::OnUpdateFinished(
+    std::unique_ptr<SafetyHubService::Result> result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  NotifyObservers(result.get());
+  std::unique_ptr<Result> final_result = UpdateOnUIThread(std::move(result));
+  NotifyObservers(final_result.get());
+  if (--pending_updates_) {
+    UpdateAsyncInternal();
+  }
 }
 
 void SafetyHubService::AddObserver(Observer* observer) {
@@ -64,7 +70,6 @@ void SafetyHubService::NotifyObservers(Result* result) {
   }
 }
 
-std::unique_ptr<SafetyHubService::Result>
-SafetyHubService::UpdateOnBackgroundThreadForTesting() {
-  return UpdateOnBackgroundThread();
+bool SafetyHubService::IsUpdateRunning() {
+  return pending_updates_ > 0;
 }

chrome/browser/ui/safety_hub/safety_hub_service.h

@@ -16,20 +16,22 @@
 #include "base/timer/timer.h"
 #include "components/keyed_service/core/keyed_service.h"
 
-// Base class for Safety Hub services. The Update() function of the derived
-// classes will be executed periodically, according to the time delta interval
-// returned by GetRepeatedUpdateInterval().
+// Base class for Safety Hub services. The background and UI tasks of the
+// derived classes will be executed periodically, according to the time delta
+// interval returned by GetRepeatedUpdateInterval().
 class SafetyHubService : public KeyedService,
                          public base::SupportsWeakPtr<SafetyHubService> {
  public:
   // Base class for results returned after the periodic execution of the Safety
   // Hub service. Each service should implement a derived class that captures
-  // the specific information that is retrieved.
+  // the specific information that is retrieved. Any intermediate data that is
+  // required for the background task, or that needs to passed through to the UI
+  // thread task should be included as well.
   class Result {
    public:
     explicit Result(base::TimeTicks timestamp = base::TimeTicks::Now());
 
-    Result(const Result&) = delete;
+    Result(const Result&) = default;
     Result& operator=(const Result&) = delete;
     virtual ~Result() = default;
 
@@ -52,8 +54,8 @@ class SafetyHubService : public KeyedService,
 
   ~SafetyHubService() override;
 
-  // Makes an asynchronous call to the Update function, and will call the
-  // callback function upon completion.
+  // Makes an asynchronous call to the background task, which will be followed
+  // by the UI task.
   void UpdateAsync();
 
   // Adds an observer to be notified when a new result is available.
@@ -62,12 +64,12 @@ class SafetyHubService : public KeyedService,
   // Removes an observer from the observer list.
   void RemoveObserver(Observer* observer);
 
+  // Indicates whether the update process is currently running.
+  bool IsUpdateRunning();
+
   // KeyedService implementation.
   void Shutdown() override;
 
-  // Public version of UpdateOnBackgroundThread for testing purposes.
-  std::unique_ptr<Result> UpdateOnBackgroundThreadForTesting();
-
  protected:
   // Triggers the repeated update task that updates the state of the Safety Hub
   // service.
@@ -79,9 +81,20 @@ class SafetyHubService : public KeyedService,
   // the Update function will be called.
   virtual base::TimeDelta GetRepeatedUpdateInterval() = 0;
 
-  // Contains the actual implementation to make updates to the Safety Hub
-  // service.
-  virtual std::unique_ptr<Result> UpdateOnBackgroundThread() = 0;
+  // Should return the background task that will be executed, containing the
+  // computation-heavy part of the update process. This task should be static
+  // and not be bound to the service, as it will be executed on a separate
+  // background thread. As such, only thread-safe parameters should be bound.
+  // The returned Result will be passed along to the UpdateOnUIThread function.
+  virtual base::OnceCallback<std::unique_ptr<Result>()> GetBackgroundTask() = 0;
+
+  // This function contains the part of the update task that will be executed
+  // synchronously on the UI thread. Hence, it should not be computation-heavy
+  // to avoid freezing the browser. It will be passed the intermediate result
+  // that was produced by the background task. The result returned by this UI
+  // task will be the final result that will be sent to the observers.
+  virtual std::unique_ptr<Result> UpdateOnUIThread(
+      std::unique_ptr<Result> result) = 0;
 
   virtual base::WeakPtr<SafetyHubService> GetAsWeakRef() = 0;
 
@@ -95,14 +108,17 @@ class SafetyHubService : public KeyedService,
   // Notifies each of the added observers that a new result is available.
   void NotifyObservers(Result* result);
 
+  // Posts the background task on a background thread.
+  void UpdateAsyncInternal();
+
   // Repeating timer that runs the recurring tasks.
   base::RepeatingTimer update_timer_;
 
-  // Timer used to delay the execution of the initial task with several minutes.
-  base::OneShotTimer delay_timer_;
-
   // List of observers that have to be notified when a new result is available.
   base::ObserverList<Observer> observers_;
+
+  // Indicator of how many requested updates are still pending.
+  int pending_updates_ = 0;
 };
 
 #endif  // CHROME_BROWSER_UI_SAFETY_HUB_SAFETY_HUB_SERVICE_H_

chrome/browser/ui/safety_hub/safety_hub_service_unittest.cc

@@ -8,7 +8,9 @@
 #include <utility>
 
 #include "base/observer_list_types.h"
+#include "base/run_loop.h"
 #include "base/time/time.h"
+#include "chrome/browser/ui/safety_hub/safety_hub_test_util.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -20,13 +22,24 @@ constexpr base::TimeDelta kUpdateIntervalForTest = base::Days(7);
 class MockSafetyHubResult : public SafetyHubService::Result {
  public:
   ~MockSafetyHubResult() override = default;
+
+  int GetVal() { return val_; }
+
+  void IncreaseVal() { ++val_; }
+
+ private:
+  int val_ = 0;
 };
 
 class MockSafetyHubService : public SafetyHubService {
  public:
   // Returns the number of times that the UpdateOnBackgroundThread function was
   // called.
-  int GetNumUpdates() const { return num_updates_; }
+  int GetNumBackgroundUpdates() const { return num_updates_background_; }
+
+  // Returns the number of times that the UpdateOnBackgroundThread function was
+  // called.
+  int GetNumUIUpdates() const { return num_updates_ui_; }
 
  protected:
   // For testing purposes, the UpdateOnBackgroundThread function will be
@@ -35,8 +48,23 @@ class MockSafetyHubService : public SafetyHubService {
     return kUpdateIntervalForTest;
   }
 
-  std::unique_ptr<Result> UpdateOnBackgroundThread() override {
-    ++num_updates_;
+  base::OnceCallback<std::unique_ptr<Result>()> GetBackgroundTask() override {
+    auto init_result = std::make_unique<MockSafetyHubResult>();
+    return base::BindOnce(&UpdateOnBackgroundThread, std::move(init_result));
+  }
+
+  static std::unique_ptr<Result> UpdateOnBackgroundThread(
+      std::unique_ptr<Result> result) {
+    auto background_result = std::make_unique<MockSafetyHubResult>();
+    background_result->IncreaseVal();
+    return background_result;
+  }
+
+  std::unique_ptr<SafetyHubService::Result> UpdateOnUIThread(
+      std::unique_ptr<Result> result) override {
+    num_updates_background_ +=
+        static_cast<MockSafetyHubResult*>(result.get())->GetVal();
+    ++num_updates_ui_;
     return std::make_unique<MockSafetyHubResult>();
   }
 
@@ -45,7 +73,8 @@ class MockSafetyHubService : public SafetyHubService {
   }
 
  private:
-  int num_updates_ = 0;
+  int num_updates_background_ = 0;
+  int num_updates_ui_ = 0;
 
   base::WeakPtrFactory<MockSafetyHubService> weak_factory_{this};
 };
@@ -54,13 +83,21 @@ class MockObserver : public SafetyHubService::Observer {
  public:
   void OnResultAvailable(const SafetyHubService::Result* result) override {
     ++num_calls_;
+    if (callback_) {
+      callback_.Run();
+    }
+  }
+
+  void SetCallback(const base::RepeatingClosure& callback) {
+    callback_ = callback;
   }
 
   // Returns the number of times that the observer was notified.
   int GetNumCalls() const { return num_calls_; }
 
  private:
   int num_calls_ = 0;
+  base::RepeatingClosure callback_;
 };
 
 }  // namespace
@@ -79,8 +116,6 @@ class SafetyHubServiceTest : public testing::Test {
     task_environment_.FastForwardBy(delta);
   }
 
-  void RunUntilIdle() { task_environment_.RunUntilIdle(); }
-
   MockSafetyHubService* service() { return service_.get(); }
 
  private:
@@ -116,21 +151,28 @@ TEST_F(SafetyHubServiceTest, ManageObservers) {
 }
 
 TEST_F(SafetyHubServiceTest, UpdateOnBackgroundThread) {
-  EXPECT_EQ(service()->GetNumUpdates(), 0);
+  EXPECT_EQ(service()->GetNumUIUpdates(), 0);
+  EXPECT_EQ(service()->GetNumBackgroundUpdates(), 0);
   // As long as StartRepeatedUpdates() has not been called, no updates should
   // be made.
   FastForwardBy(kUpdateIntervalForTest);
-  EXPECT_EQ(service()->GetNumUpdates(), 0);
+  EXPECT_EQ(service()->GetNumUIUpdates(), 0);
+  EXPECT_EQ(service()->GetNumBackgroundUpdates(), 0);
   // The update will be run asynchronously as soon as StartRepeatedUpdates() is
   // called.
+  base::RunLoop loop;
+  auto observer = std::make_shared<MockObserver>();
+  observer->SetCallback(loop.QuitClosure());
+  service()->AddObserver(observer.get());
   service()->StartRepeatedUpdates();
-  // TODO(tov): When we remove the delay for running the repeated updates, this
-  // should be removed.
-  FastForwardBy(base::Minutes(15));
-  RunUntilIdle();
-  EXPECT_EQ(service()->GetNumUpdates(), 1);
+  loop.Run();
+  EXPECT_EQ(service()->GetNumUIUpdates(), 1);
+  EXPECT_EQ(service()->GetNumBackgroundUpdates(), 1);
   // Move forward a full update interval, which will trigger another update.
+  base::RunLoop loop2;
+  observer->SetCallback(loop2.QuitClosure());
   FastForwardBy(kUpdateIntervalForTest);
-  RunUntilIdle();
-  EXPECT_EQ(service()->GetNumUpdates(), 2);
+  loop2.Run();
+  EXPECT_EQ(service()->GetNumUIUpdates(), 2);
+  EXPECT_EQ(service()->GetNumBackgroundUpdates(), 2);
 }