Introduce PushTask() to unify StartEvent() and PushPendingTask()

This is a pure refactoring, as a preparation for introducing a concept
of 'offline' tasks later.

Background: The WIP CL, https://crrev.com/c/1900803/, introduced a
concept of a *Task* in ServiceWorkerTimeoutTimer, however, there are
still two different queues in the timer, |pending_tasks_| (for normal
tasks) and |pending_after_idle_timeout_tasks_| (for pending
after-idle-timeout tasks). That are confusing and error-prone.

So before proceeding further, I've decided to refactor the current
ServiceWorkerTimeoutTimer, merging two different interfaces,
StartEvent() and PushPendingTask(), into one interface, PushTask().

Now all callers should use PushTask(). The current StartEvent(),
StartEventWithCustomCustomTimeout(), and PushPendingTask() are no
longer available from outside. StartEvent() is now an internal
function, and others were just removed.

After this CL, I'll introduce a new kind of a task, OFFLINE.

Bug: 965802
Change-Id: Ide270432f486484c1ac2127cff59e0258f88a835
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906452
Commit-Queue: Hayato Ito <hayato@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720386}

third_party/blink/renderer/modules/service_worker/service_worker_global_scope.h

@@ -354,13 +354,6 @@ class MODULES_EXPORT ServiceWorkerGlobalScope final
 
   using DispatchFetchEventInternalCallback =
       base::OnceCallback<void(mojom::blink::ServiceWorkerEventStatus)>;
-  void DispatchFetchEventInternal(
-      mojom::blink::DispatchFetchEventParamsPtr params,
-      network::mojom::blink::CrossOriginEmbedderPolicy requestor_coep,
-      mojo::PendingRemote<mojom::blink::ServiceWorkerFetchResponseCallback>
-          response_callback,
-      DispatchFetchEventInternalCallback callback);
-
   void SetFetchHandlerExistence(FetchHandlerExistence fetch_handler_existence);
 
   // Implements mojom::blink::ControllerServiceWorker.
@@ -464,6 +457,96 @@ class MODULES_EXPORT ServiceWorkerGlobalScope final
   void NoteNewFetchEvent(const KURL& request_url);
   void NoteRespondedToFetchEvent(const KURL& request_url);
 
+  // Dispatches the event synchronously. Enqueued by Dispatch*Event methods to
+  // the timeout timer, and executed immediately or sometimes later.
+  void StartFetchEvent(
+      mojom::blink::DispatchFetchEventParamsPtr params,
+      network::mojom::blink::CrossOriginEmbedderPolicy requestor_coep,
+      mojo::PendingRemote<mojom::blink::ServiceWorkerFetchResponseCallback>
+          response_callback,
+      DispatchFetchEventInternalCallback callback,
+      int event_id);
+  void StartInstallEvent(DispatchInstallEventCallback callback, int event_id);
+  void StartActivateEvent(DispatchActivateEventCallback callback, int event_id);
+  void StartBackgroundFetchAbortEvent(
+      mojom::blink::BackgroundFetchRegistrationPtr registration,
+      DispatchBackgroundFetchAbortEventCallback callback,
+      int event_id);
+  void StartBackgroundFetchClickEvent(
+      mojom::blink::BackgroundFetchRegistrationPtr registration,
+      DispatchBackgroundFetchClickEventCallback callback,
+      int event_id);
+  void StartBackgroundFetchFailEvent(
+      mojom::blink::BackgroundFetchRegistrationPtr registration,
+      DispatchBackgroundFetchFailEventCallback callback,
+      int event_id);
+  void StartBackgroundFetchSuccessEvent(
+      mojom::blink::BackgroundFetchRegistrationPtr registration,
+      DispatchBackgroundFetchSuccessEventCallback callback,
+      int event_id);
+  void StartExtendableMessageEvent(
+      mojom::blink::ExtendableMessageEventPtr event,
+      DispatchExtendableMessageEventCallback callback,
+      int event_id);
+  void StartFetchEventForMainResource(
+      mojom::blink::DispatchFetchEventParamsPtr params,
+      mojo::PendingRemote<mojom::blink::ServiceWorkerFetchResponseCallback>
+          response_callback,
+      int event_id);
+  void StartNotificationClickEvent(
+      String notification_id,
+      mojom::blink::NotificationDataPtr notification_data,
+      int action_index,
+      String reply,
+      DispatchNotificationClickEventCallback callback,
+      int event_id);
+  void StartNotificationCloseEvent(
+      String notification_id,
+      mojom::blink::NotificationDataPtr notification_data,
+      DispatchNotificationCloseEventCallback callback,
+      int event_id);
+  void StartPushEvent(String payload,
+                      DispatchPushEventCallback callback,
+                      int event_id);
+  void StartPushSubscriptionChangeEvent(
+      mojom::blink::PushSubscriptionPtr old_subscription,
+      mojom::blink::PushSubscriptionPtr new_subscription,
+      DispatchPushSubscriptionChangeEventCallback callback,
+      int event_id);
+  void StartSyncEvent(String tag,
+                      bool last_chance,
+                      DispatchSyncEventCallback callback,
+                      int event_id);
+  void StartPeriodicSyncEvent(String tag,
+                              DispatchPeriodicSyncEventCallback callback,
+                              int event_id);
+  void StartAbortPaymentEvent(
+      mojo::PendingRemote<
+          payments::mojom::blink::PaymentHandlerResponseCallback>
+          response_callback,
+      DispatchAbortPaymentEventCallback callback,
+      int event_id);
+  void StartCanMakePaymentEvent(
+      payments::mojom::blink::CanMakePaymentEventDataPtr event_data,
+      mojo::PendingRemote<
+          payments::mojom::blink::PaymentHandlerResponseCallback>
+          response_callback,
+      DispatchCanMakePaymentEventCallback callback,
+      int event_id);
+  void StartPaymentRequestEvent(
+      payments::mojom::blink::PaymentRequestEventDataPtr event_data,
+      mojo::PendingRemote<
+          payments::mojom::blink::PaymentHandlerResponseCallback>
+          response_callback,
+      DispatchPaymentRequestEventCallback callback,
+      int event_id);
+  void StartCookieChangeEvent(network::mojom::blink::CookieChangeInfoPtr change,
+                              DispatchCookieChangeEventCallback callback,
+                              int event_id);
+  void StartContentDeleteEvent(String id,
+                               DispatchContentDeleteEventCallback callback,
+                               int event_id);
+
   Member<ServiceWorkerClients> clients_;
   Member<ServiceWorkerRegistration> registration_;
   Member<::blink::ServiceWorker> service_worker_;

third_party/blink/renderer/modules/service_worker/service_worker_timeout_timer.cc

@@ -80,25 +80,44 @@ void ServiceWorkerTimeoutTimer::Start() {
                                   WTF::Unretained(this)));
 }
 
-int ServiceWorkerTimeoutTimer::StartEvent(AbortCallback abort_callback) {
-  return StartEventWithCustomTimeout(std::move(abort_callback), kEventTimeout);
-}
-
-int ServiceWorkerTimeoutTimer::StartEventWithCustomTimeout(
-    AbortCallback abort_callback,
-    base::TimeDelta timeout) {
+void ServiceWorkerTimeoutTimer::PushTask(std::unique_ptr<Task> task) {
+  DCHECK(task->type != Task::Type::Pending || did_idle_timeout());
+  bool can_start_processing_tasks =
+      !processing_tasks_ && task->type != Task::Type::Pending;
+  task_queue_.emplace_back(std::move(task));
+  if (!can_start_processing_tasks)
+    return;
   if (did_idle_timeout()) {
-    DCHECK(!running_pending_tasks_);
     idle_time_ = base::TimeTicks();
     did_idle_timeout_ = false;
+  }
+  ProcessTasks();
+}
 
-    running_pending_tasks_ = true;
-    while (!pending_tasks_.IsEmpty()) {
-      pending_tasks_.TakeFirst().Run();
-    }
-    running_pending_tasks_ = false;
+void ServiceWorkerTimeoutTimer::ProcessTasks() {
+  DCHECK(!processing_tasks_);
+  processing_tasks_ = true;
+  while (!task_queue_.IsEmpty()) {
+    StartTask(task_queue_.TakeFirst());
   }
+  processing_tasks_ = false;
 
+  // We have to check HasInflightEvent() and may trigger
+  // OnNoInflightEvent() here because StartTask() can call EndEvent()
+  // synchronously, and EndEvent() never triggers OnNoInflightEvent()
+  // while ProcessTasks() is running.
+  if (!HasInflightEvent())
+    OnNoInflightEvent();
+}
+
+void ServiceWorkerTimeoutTimer::StartTask(std::unique_ptr<Task> task) {
+  int event_id = StartEvent(std::move(task->abort_callback),
+                            task->custom_timeout.value_or(kEventTimeout));
+  std::move(task->start_callback).Run(event_id);
+}
+
+int ServiceWorkerTimeoutTimer::StartEvent(AbortCallback abort_callback,
+                                          base::TimeDelta timeout) {
   idle_time_ = base::TimeTicks();
   const int event_id = NextEventId();
   auto add_result = id_event_map_.insert(
@@ -112,7 +131,10 @@ int ServiceWorkerTimeoutTimer::StartEventWithCustomTimeout(
 void ServiceWorkerTimeoutTimer::EndEvent(int event_id) {
   DCHECK(HasEvent(event_id));
   id_event_map_.erase(event_id);
-  if (!HasInflightEvent())
+  // Check |processing_tasks_| here because EndEvent() can be called
+  // synchronously in StartTask(). We don't want to trigger
+  // OnNoInflightEvent() while ProcessTasks() is running.
+  if (!processing_tasks_ && !HasInflightEvent())
     OnNoInflightEvent();
 }
 
@@ -126,12 +148,6 @@ ServiceWorkerTimeoutTimer::CreateStayAwakeToken() {
       weak_factory_.GetWeakPtr());
 }
 
-void ServiceWorkerTimeoutTimer::PushPendingTask(
-    base::OnceClosure pending_task) {
-  DCHECK(did_idle_timeout());
-  pending_tasks_.emplace_back(std::move(pending_task));
-}
-
 void ServiceWorkerTimeoutTimer::SetIdleTimerDelayToZero() {
   zero_idle_timer_delay_ = true;
   if (!HasInflightEvent())
@@ -188,10 +204,21 @@ void ServiceWorkerTimeoutTimer::OnNoInflightEvent() {
 }
 
 bool ServiceWorkerTimeoutTimer::HasInflightEvent() const {
-  return !id_event_map_.IsEmpty() || running_pending_tasks_ ||
-         num_of_stay_awake_tokens_ > 0;
+  return !id_event_map_.IsEmpty() || num_of_stay_awake_tokens_ > 0;
 }
 
+ServiceWorkerTimeoutTimer::Task::Task(
+    ServiceWorkerTimeoutTimer::Task::Type type,
+    StartCallback start_callback,
+    AbortCallback abort_callback,
+    base::Optional<base::TimeDelta> custom_timeout)
+    : type(type),
+      start_callback(std::move(start_callback)),
+      abort_callback(std::move(abort_callback)),
+      custom_timeout(custom_timeout) {}
+
+ServiceWorkerTimeoutTimer::Task::~Task() = default;
+
 ServiceWorkerTimeoutTimer::EventInfo::EventInfo(
     base::TimeTicks expiration_time,
     base::OnceCallback<void(blink::mojom::ServiceWorkerEventStatus)>

third_party/blink/renderer/modules/service_worker/service_worker_timeout_timer.h

@@ -68,20 +68,48 @@ class MODULES_EXPORT ServiceWorkerTimeoutTimer {
   // on the timer before.
   void Start();
 
-  // StartEvent() should be called at the beginning of an event. It returns an
-  // event id, which is unique among threads in the same process.
-  // The event id should be passed to EndEvent() when the event has finished.
-  // If there are pending tasks queued by PushPendingTask(), they will
-  // run in order synchronouslly in StartEvent().
-  // See the class comment to know when |abort_callback| runs.
-  int StartEvent(AbortCallback abort_callback);
-  // This is basically the same as StartEvent, but you can customize the
-  // timeout time until |abort_callback| runs by |timeout|.
-  int StartEventWithCustomTimeout(AbortCallback abort_callback,
-                                  base::TimeDelta timeout);
+  using StartCallback = base::OnceCallback<void(int /* event_id */)>;
+
+  // Represents an event dispatch task, which can be queued into |task_queue_|.
+  struct MODULES_EXPORT Task {
+    enum class Type {
+      // A normal task is pushed to the end of the task queue and triggers
+      // processing of the queue.
+      Normal,
+      // A pending task, which does not start until a normal task is
+      // pushed. A pending task should be used if the idle timeout
+      // occurred (did_idle_timeout() returns true). In practice, the
+      // caller pushes pending tasks after the service worker
+      // requested the browser to terminate it due to idleness. These
+      // tasks run once PushTask() is called due to a new event from
+      // the browser, signalling that the browser decided not to
+      // terminate the worker.
+      Pending,
+    };
+
+    Task(Type type,
+         StartCallback start_callback,
+         AbortCallback abort_callback,
+         base::Optional<base::TimeDelta> custom_timeout);
+    ~Task();
+    Type type;
+    // Callback which is run when the timer starts this task. The
+    // callback receives |event_id|, which is the result of
+    // StartEvent(). When an event finishes, EndEvent() should be
+    // called with the given |event_id|.
+    StartCallback start_callback;
+    // Callback which is run when the timer aborts a started task.
+    AbortCallback abort_callback;
+    // The custom timeout value.
+    base::Optional<base::TimeDelta> custom_timeout;
+  };
 
   void EndEvent(int event_id);
 
+  // Push the task to the task queue in the timer and tasks in the queue can run
+  // synchronously. See also Task's comment.
+  void PushTask(std::unique_ptr<Task> task);
+
   // Returns true if |event_id| was started and hasn't ended.
   bool HasEvent(int event_id) const;
 
@@ -89,12 +117,6 @@ class MODULES_EXPORT ServiceWorkerTimeoutTimer {
   // while any of these are alive.
   std::unique_ptr<StayAwakeToken> CreateStayAwakeToken();
 
-  // Pushes a task which is expected to run after any event starts again to a
-  // pending task queue. The tasks will run at the next StartEvent() call.
-  // PushPendingTask() should be called if the idle timeout occurred
-  // (did_idle_timeout() returns true).
-  void PushPendingTask(base::OnceClosure pending_task);
-
   // Sets the |zero_idle_timer_delay_| to true and triggers the idle callback if
   // there are not inflight events. If there are, the callback will be called
   // next time when the set of inflight events becomes empty in EndEvent().
@@ -118,6 +140,11 @@ class MODULES_EXPORT ServiceWorkerTimeoutTimer {
       base::TimeDelta::FromSeconds(30);
 
  private:
+  // StartEvent() should be called at the beginning of an event. It returns an
+  // event id, which is unique among threads in the same process.
+  // The event id should be passed to EndEvent() when the event has finished.
+  int StartEvent(AbortCallback abort_callback, base::TimeDelta timeout);
+
   // Updates the internal states and fires timeout callbacks if any.
   void UpdateStatus();
 
@@ -132,6 +159,12 @@ class MODULES_EXPORT ServiceWorkerTimeoutTimer {
   // Returns true if there are running events.
   bool HasInflightEvent() const;
 
+  // Processes all tasks in |task_queue_|.
+  void ProcessTasks();
+
+  // Start a single task.
+  void StartTask(std::unique_ptr<Task> task);
+
   struct EventInfo {
     EventInfo(base::TimeTicks expiration_time,
               base::OnceCallback<void(mojom::blink::ServiceWorkerEventStatus)>
@@ -162,16 +195,13 @@ class MODULES_EXPORT ServiceWorkerTimeoutTimer {
   // StartEvent() is called.
   bool did_idle_timeout_ = false;
 
-  // Tasks that are to be run after the next StartEvent() call. In practice, the
-  // caller adds pending tasks after the service worker requested the browser to
-  // terminate it due to idleness. These tasks run once StartEvent() is called
-  // due to a new event from the browser, signalling that the browser decided
-  // not to terminate the worker.
-  Deque<base::OnceClosure> pending_tasks_;
+  // Tasks that are to be run in the next ProcessTasks() call.
+  Deque<std::unique_ptr<Task>> task_queue_;
 
-  // Set to true during running |pending_tasks_|. This is used for avoiding to
-  // invoke |idle_callback_| when running |pending_tasks_|.
-  bool running_pending_tasks_ = false;
+  // Set to true during running ProcessTasks(). This is used for avoiding to
+  // invoke |idle_callback_| or to re-enter ProcessTasks() when calling
+  // ProcessTasks().
+  bool processing_tasks_ = false;
 
   // The number of the living StayAwakeToken. See also class comments.
   int num_of_stay_awake_tokens_ = 0;