[Background Sync] Wait for all syncs to complete.

We currently wake the browser up and call FireReadyEvents() to fire any
ready sync events, and then call the passed |callback|. Before this
change, we were invoking the callback once all sync events had been
fired. This CL waits till all those sync events have completed before
running the callback.

This will have the effect of keeping the browser awake until all sync
events complete, and will add a few minutes to how long we hold the
wakelock. It will give the browser a chance to finish processing
(Periodic) Background Sync registrations and reduce the number of times
we need to wake the browser up.

This CL adds a new parameter to BackgroundSyncParameters to enable this
functionality, so that we can run experiments using the existing
'BackgroundSync' field trial before it's enabled everywhere.

A unit test has also been added to verify the functionality.

Bug: 933849, 932591
Change-Id: Ica68f6bdb262dea2365334296ae825d91126c65b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807194
Commit-Queue: Mugdha Lakhani <nator@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704489}

chrome/browser/background_sync/background_sync_controller_impl.cc

@@ -35,6 +35,8 @@ const char BackgroundSyncControllerImpl::kDisabledParameterName[] = "disabled";
 const char BackgroundSyncControllerImpl::kRelyOnAndroidNetworkDetection[] =
     "rely_on_android_network_detection";
 #endif
+const char BackgroundSyncControllerImpl::kKeepBrowserAwakeParameterName[] =
+    "keep_browser_awake_till_events_complete";
 const char BackgroundSyncControllerImpl::kMaxAttemptsParameterName[] =
     "max_sync_attempts";
 const char BackgroundSyncControllerImpl::
@@ -112,6 +114,11 @@ void BackgroundSyncControllerImpl::GetParameterOverrides(
     parameters->disable = true;
   }
 
+  if (base::LowerCaseEqualsASCII(field_params[kKeepBrowserAwakeParameterName],
+                                 "true")) {
+    parameters->keep_browser_awake_till_events_complete = true;
+  }
+
   if (base::Contains(field_params,
                      kMaxAttemptsWithNotificationPermissionParameterName)) {
     int max_attempts;

chrome/browser/background_sync/background_sync_controller_impl.h

@@ -42,6 +42,7 @@ class BackgroundSyncControllerImpl : public content::BackgroundSyncController,
  public:
   static const char kFieldTrialName[];
   static const char kDisabledParameterName[];
+  static const char kKeepBrowserAwakeParameterName[];
   static const char kMaxAttemptsParameterName[];
   static const char kRelyOnAndroidNetworkDetection[];
   static const char kMaxAttemptsWithNotificationPermissionParameterName[];

content/browser/background_sync/background_sync_manager.cc

@@ -1859,23 +1859,23 @@ void BackgroundSyncManager::FireReadyEvents(
   op_scheduler_.ScheduleOperation(
       id, CacheStorageSchedulerMode::kExclusive,
       CacheStorageSchedulerOp::kBackgroundSync,
-      base::BindOnce(
-          &BackgroundSyncManager::FireReadyEventsImpl,
-          weak_ptr_factory_.GetWeakPtr(), sync_type, reschedule,
-          op_scheduler_.WrapCallbackToRunNext(id, std::move(callback)),
-          std::move(keepalive)));
+      base::BindOnce(&BackgroundSyncManager::FireReadyEventsImpl,
+                     weak_ptr_factory_.GetWeakPtr(), sync_type, reschedule, id,
+                     std::move(callback), std::move(keepalive)));
 }
 
 void BackgroundSyncManager::FireReadyEventsImpl(
     blink::mojom::BackgroundSyncType sync_type,
     bool reschedule,
+    int scheduler_id,
     base::OnceClosure callback,
     std::unique_ptr<BackgroundSyncEventKeepAlive> keepalive) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
 
   if (disabled_) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                  std::move(callback));
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        op_scheduler_.WrapCallbackToRunNext(scheduler_id, std::move(callback)));
     return;
   }
 
@@ -1917,27 +1917,46 @@ void BackgroundSyncManager::FireReadyEventsImpl(
     // called from a wakeup task.
     if (reschedule)
       ScheduleOrCancelDelayedProcessing(sync_type);
-    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                  std::move(callback));
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        op_scheduler_.WrapCallbackToRunNext(scheduler_id, std::move(callback)));
     return;
   }
 
   base::TimeTicks start_time = base::TimeTicks::Now();
 
-  // Fire the sync event of the ready registrations and run |callback| once
-  // they're all done.
+  // If we've been called from a wake up task, potentially keep the browser
+  // awake till all events have completed. If not, we only do so until all
+  // events have been fired.
+  // To allow the |op_scheduler_| to process other tasks after sync events
+  // have been fired, mark this task complete after firing events.
+  base::OnceClosure events_fired_callback, events_completed_callback;
+  bool keep_browser_awake_till_events_complete =
+      !reschedule && parameters_->keep_browser_awake_till_events_complete;
+  if (keep_browser_awake_till_events_complete) {
+    events_fired_callback = MakeEmptyCompletion(scheduler_id);
+    events_completed_callback = std::move(callback);
+  } else {
+    events_fired_callback =
+        op_scheduler_.WrapCallbackToRunNext(scheduler_id, std::move(callback));
+    events_completed_callback = base::DoNothing::Once();
+  }
+
+  // Fire the sync event of the ready registrations and run
+  // |events_fired_closure| once they're all done.
   base::RepeatingClosure events_fired_barrier_closure = base::BarrierClosure(
       to_fire.size(),
       base::BindOnce(&BackgroundSyncManager::FireReadyEventsAllEventsFiring,
                      weak_ptr_factory_.GetWeakPtr(), sync_type, reschedule,
-                     std::move(callback)));
+                     std::move(events_fired_callback)));
 
   // Record the total time taken after all events have run to completion.
   base::RepeatingClosure events_completed_barrier_closure =
       base::BarrierClosure(
           to_fire.size(),
-          base::BindOnce(&OnAllSyncEventsCompleted, sync_type, start_time,
-                         !reschedule, to_fire.size()));
+          base::BindOnce(&BackgroundSyncManager::OnAllSyncEventsCompleted,
+                         sync_type, start_time, !reschedule, to_fire.size(),
+                         std::move(events_completed_callback)));
 
   for (auto& registration_info : to_fire) {
     const BackgroundSyncRegistration* registration =
@@ -2036,6 +2055,7 @@ void BackgroundSyncManager::FireReadyEventsAllEventsFiring(
 
   if (reschedule)
     ScheduleOrCancelDelayedProcessing(sync_type);
+
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(callback));
 }
 
@@ -2277,11 +2297,13 @@ void BackgroundSyncManager::OnAllSyncEventsCompleted(
     BackgroundSyncType sync_type,
     const base::TimeTicks& start_time,
     bool from_wakeup_task,
-    int number_of_batched_sync_events) {
+    int number_of_batched_sync_events,
+    base::OnceClosure callback) {
   // Record the combined time taken by all sync events.
   BackgroundSyncMetrics::RecordBatchSyncEventComplete(
       sync_type, base::TimeTicks::Now() - start_time, from_wakeup_task,
       number_of_batched_sync_events);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(callback));
 }
 
 void BackgroundSyncManager::OnRegistrationDeletedImpl(

content/browser/background_sync/background_sync_manager.h

@@ -147,7 +147,7 @@ class CONTENT_EXPORT BackgroundSyncManager
   // Scans the list of available events and fires those of type |sync_type| that
   // are ready to fire. For those that can't yet be fired, wakeup alarms are
   // set. Once all of this is done, invokes |callback|.
-  void FireReadyEvents(
+  virtual void FireReadyEvents(
       blink::mojom::BackgroundSyncType sync_type,
       bool reschedule,
       base::OnceClosure callback,
@@ -339,9 +339,18 @@ class CONTENT_EXPORT BackgroundSyncManager
   void ScheduleDelayedProcessingOfRegistrations(
       blink::mojom::BackgroundSyncType sync_type);
 
+  // Fires ready events for |sync_type|.
+  // |reschedule| is true when it's ok to schedule background processing from
+  // this method, false otherwise.
+  // |scheduler_id| is an id unique to the |op_scheduler_| task. It's passed to
+  // correctly mark this operation as finished with the |op_scheduler_| and run
+  // the next operation scheduled.
+  // |keepalive| is used to keep the browser alive until the first attempt to
+  // fire a sync event has been made.
   void FireReadyEventsImpl(
       blink::mojom::BackgroundSyncType sync_type,
       bool reschedule,
+      int scheduler_id,
       base::OnceClosure callback,
       std::unique_ptr<BackgroundSyncEventKeepAlive> keepalive);
 
@@ -386,7 +395,8 @@ class CONTENT_EXPORT BackgroundSyncManager
       blink::mojom::BackgroundSyncType sync_type,
       const base::TimeTicks& start_time,
       bool from_wakeup_task,
-      int number_of_batched_sync_events);
+      int number_of_batched_sync_events,
+      base::OnceClosure callback);
 
   // OnRegistrationDeleted callbacks
   void OnRegistrationDeletedImpl(int64_t sw_registration_id,

content/browser/background_sync/background_sync_manager_unittest.cc

@@ -672,6 +672,15 @@ class BackgroundSyncManagerTest
     SetupBackgroundSyncManager();
   }
 
+  void SetKeepBrowserAwakeTillEventsCompleteAndRestartManager(
+      bool keep_browser_awake_till_events_complete) {
+    BackgroundSyncParameters* parameters =
+        GetController()->background_sync_parameters();
+    parameters->keep_browser_awake_till_events_complete =
+        keep_browser_awake_till_events_complete;
+    SetupBackgroundSyncManager();
+  }
+
   void FireReadyEvents() { test_background_sync_manager()->OnNetworkChanged(); }
 
   bool AreOptionConditionsMet() {
@@ -1384,6 +1393,33 @@ TEST_F(BackgroundSyncManagerTest, ReregisterMidSyncFirstAttemptFails) {
   EXPECT_FALSE(GetRegistration(sync_options_1_));
 }
 
+TEST_F(BackgroundSyncManagerTest, RunCallbackAfterEventsComplete) {
+  SetKeepBrowserAwakeTillEventsCompleteAndRestartManager(
+      /* keep_browser_awake_till_events_complete= */ true);
+  InitDelayedSyncEventTest();
+
+  // This ensures other invocations of FireReadyEvents won't complete the
+  // registration.
+  test_background_sync_manager()->SuspendFiringEvents();
+
+  EXPECT_TRUE(Register(sync_options_1_));
+
+  bool callback_called = false;
+  test_background_sync_manager()->ResumeFiringEvents();
+  test_background_sync_manager()->FireReadyEvents(
+      blink::mojom::BackgroundSyncType::ONE_SHOT,
+      /* reschedule= */ false,
+      base::BindOnce([](bool* callback_called) { *callback_called = true; },
+                     &callback_called));
+
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_FALSE(callback_called);
+  std::move(sync_fired_callback_).Run(blink::ServiceWorkerStatusCode::kOk);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(callback_called);
+}
+
 TEST_F(BackgroundSyncManagerTest, ReregisterMidSyncFirstAttemptSucceeds) {
   InitDelayedSyncEventTest();
   RegisterAndVerifySyncEventDelayed(sync_options_1_);

content/public/browser/background_sync_parameters.cc

@@ -23,6 +23,7 @@ BackgroundSyncParameters::BackgroundSyncParameters()
 #if defined(OS_ANDROID)
       rely_on_android_network_detection(false),
 #endif
+      keep_browser_awake_till_events_complete(false),
       max_sync_attempts(kMaxSyncAttempts),
       max_sync_attempts_with_notification_permission(kMaxSyncAttempts),
       initial_retry_delay(kInitialRetryDelay),
@@ -32,6 +33,9 @@ BackgroundSyncParameters::BackgroundSyncParameters()
       min_periodic_sync_events_interval(kMinPeriodicSyncEventsInterval) {
 }
 
+BackgroundSyncParameters::BackgroundSyncParameters(
+    const BackgroundSyncParameters& other) = default;
+
 bool BackgroundSyncParameters::operator==(
     const BackgroundSyncParameters& other) const {
   return disable == other.disable &&
@@ -39,6 +43,8 @@ bool BackgroundSyncParameters::operator==(
          rely_on_android_network_detection ==
              other.rely_on_android_network_detection &&
 #endif
+         keep_browser_awake_till_events_complete ==
+             other.keep_browser_awake_till_events_complete &&
          max_sync_attempts == other.max_sync_attempts &&
          max_sync_attempts_with_notification_permission ==
              other.max_sync_attempts_with_notification_permission &&

content/public/browser/background_sync_parameters.h

@@ -15,6 +15,7 @@ namespace content {
 
 struct CONTENT_EXPORT BackgroundSyncParameters {
   BackgroundSyncParameters();
+  BackgroundSyncParameters(const BackgroundSyncParameters& other);
   bool operator==(const BackgroundSyncParameters& other) const;
 
   // True if the manager should be disabled and registration attempts should
@@ -26,6 +27,11 @@ struct CONTENT_EXPORT BackgroundSyncParameters {
   bool rely_on_android_network_detection;
 #endif
 
+  // If true, we keep the browser awake till all (periodic)sync events fired
+  // have completed. If false, we only keep the browser awake till all ready
+  // (periodic)sync events have been fired.
+  bool keep_browser_awake_till_events_complete;
+
   // The number of attempts the BackgroundSyncManager will make to fire an
   // event before giving up.
   int max_sync_attempts;

content/test/test_background_sync_manager.cc

@@ -100,6 +100,20 @@ void TestBackgroundSyncManager::HasMainFrameProviderHost(
   std::move(callback).Run(has_main_frame_provider_host_);
 }
 
+void TestBackgroundSyncManager::FireReadyEvents(
+    blink::mojom::BackgroundSyncType sync_type,
+    bool reschedule,
+    base::OnceClosure callback,
+    std::unique_ptr<BackgroundSyncEventKeepAlive> keepalive) {
+  if (dont_fire_sync_events_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  std::move(callback));
+  } else {
+    BackgroundSyncManager::FireReadyEvents(
+        sync_type, reschedule, std::move(callback), std::move(keepalive));
+  }
+}
+
 void TestBackgroundSyncManager::StoreDataInBackendContinue(
     int64_t sw_registration_id,
     const url::Origin& origin,

content/test/test_background_sync_manager.h

@@ -99,6 +99,17 @@ class TestBackgroundSyncManager : public BackgroundSyncManager {
       blink::mojom::BackgroundSyncType sync_type,
       base::Time last_browser_wakeup_for_periodic_sync) override;
 
+  // Override to do not fire any sync events when firing is disabled.
+  void FireReadyEvents(blink::mojom::BackgroundSyncType sync_type,
+                       bool reschedule,
+                       base::OnceClosure callback,
+                       std::unique_ptr<BackgroundSyncEventKeepAlive> keepalive =
+                           nullptr) override;
+
+  void SuspendFiringEvents() { dont_fire_sync_events_ = true; }
+
+  void ResumeFiringEvents() { dont_fire_sync_events_ = false; }
+
  protected:
   // Override to allow delays to be injected by tests.
   void StoreDataInBackend(
@@ -147,6 +158,7 @@ class TestBackgroundSyncManager : public BackgroundSyncManager {
   bool delay_backend_ = false;
   bool has_main_frame_provider_host_ = true;
   bool last_chance_ = false;
+  bool dont_fire_sync_events_ = false;
   base::OnceClosure continuation_;
   DispatchSyncCallback dispatch_sync_callback_;
   DispatchSyncCallback dispatch_periodic_sync_callback_;