Make service worker's idle timer more precise to take arbitrary delay

The service worker's idle timeout is triggered when the service worker
is idle for a certain period of time. Previously, the idleness is
checked periodically on UpdateStatus() in ServiceWorkerEventQueue.
However, the periodic check prevents us from introducing arbitrary delay
of idle timeout because the delay is quantized by the interval of the
periodic check. This CL re-implements the idleness check by using
delayed cancellable callback. In the new implementation, the idle
callback is scheduled immediately after the worker has no inflight
event. If we get another event before calling the scheduled callback,
it's cancelled.

Bug: 1043845
Change-Id: I0d8950ced2e2465ef3d1bd88c7c499fb2c78f609
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2019142
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735793}

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

@@ -28,7 +28,7 @@ int NextEventId() {
 }  // namespace
 
 // static
-constexpr base::TimeDelta ServiceWorkerEventQueue::kIdleDelay;
+constexpr base::TimeDelta ServiceWorkerEventQueue::kDefaultIdleDelay;
 constexpr base::TimeDelta ServiceWorkerEventQueue::kEventTimeout;
 constexpr base::TimeDelta ServiceWorkerEventQueue::kUpdateInterval;
 
@@ -36,6 +36,7 @@ ServiceWorkerEventQueue::StayAwakeToken::StayAwakeToken(
     base::WeakPtr<ServiceWorkerEventQueue> event_queue)
     : event_queue_(std::move(event_queue)) {
   DCHECK(event_queue_);
+  event_queue_->ResetIdleTimeout();
   event_queue_->num_of_stay_awake_tokens_++;
 }
 
@@ -53,16 +54,20 @@ ServiceWorkerEventQueue::StayAwakeToken::~StayAwakeToken() {
 
 ServiceWorkerEventQueue::ServiceWorkerEventQueue(
     BeforeStartEventCallback before_start_event_callback,
-    base::RepeatingClosure idle_callback)
+    base::RepeatingClosure idle_callback,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
     : ServiceWorkerEventQueue(std::move(before_start_event_callback),
                               std::move(idle_callback),
+                              std::move(task_runner),
                               base::DefaultTickClock::GetInstance()) {}
 
 ServiceWorkerEventQueue::ServiceWorkerEventQueue(
     BeforeStartEventCallback before_start_event_callback,
     base::RepeatingClosure idle_callback,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
     const base::TickClock* tick_clock)
-    : before_start_event_callback_(std::move(before_start_event_callback)),
+    : task_runner_(std::move(task_runner)),
+      before_start_event_callback_(std::move(before_start_event_callback)),
       idle_callback_(std::move(idle_callback)),
       tick_clock_(tick_clock) {}
 
@@ -77,9 +82,10 @@ ServiceWorkerEventQueue::~ServiceWorkerEventQueue() {
 
 void ServiceWorkerEventQueue::Start() {
   DCHECK(!timer_.IsRunning());
-  // |idle_callback_| will be invoked if no event happens in |kIdleDelay|.
-  if (!HasInflightEvent() && idle_time_.is_null())
-    idle_time_ = tick_clock_->NowTicks() + kIdleDelay;
+  if (!HasInflightEvent() && !HasScheduledIdleCallback()) {
+    // If no event happens until Start(), the idle callback should be scheduled.
+    OnNoInflightEvent();
+  }
   timer_.Start(FROM_HERE, kUpdateInterval,
                WTF::BindRepeating(&ServiceWorkerEventQueue::UpdateStatus,
                                   WTF::Unretained(this)));
@@ -125,12 +131,11 @@ void ServiceWorkerEventQueue::EnqueueEvent(std::unique_ptr<Event> event) {
   bool can_start_processing_events =
       !processing_events_ && event->type != Event::Type::Pending;
   queue_.emplace_back(std::move(event));
+
   if (!can_start_processing_events)
     return;
-  if (did_idle_timeout()) {
-    idle_time_ = base::TimeTicks();
-    did_idle_timeout_ = false;
-  }
+
+  ResetIdleTimeout();
   ProcessEvents();
 }
 
@@ -153,7 +158,6 @@ void ServiceWorkerEventQueue::ProcessEvents() {
 void ServiceWorkerEventQueue::StartEvent(std::unique_ptr<Event> event) {
   DCHECK(CanStartEvent(*event));
   running_offline_events_ = event->type == Event::Type::Offline;
-  idle_time_ = base::TimeTicks();
   const int event_id = NextEventId();
   DCHECK(!HasEvent(event_id));
   id_event_map_.insert(
@@ -186,10 +190,38 @@ ServiceWorkerEventQueue::CreateStayAwakeToken() {
       weak_factory_.GetWeakPtr());
 }
 
-void ServiceWorkerEventQueue::SetIdleTimerDelayToZero() {
-  zero_idle_timer_delay_ = true;
-  if (!HasInflightEvent())
-    MaybeTriggerIdleTimer();
+void ServiceWorkerEventQueue::SetIdleDelay(base::TimeDelta idle_delay) {
+  idle_delay_ = idle_delay;
+
+  if (HasInflightEvent())
+    return;
+
+  if (did_idle_timeout()) {
+    // The idle callback has already been called. It should not be called again
+    // until this worker becomes active.
+    return;
+  }
+
+  // There should be a scheduled idle callback because this is now in the idle
+  // delay. The idle callback will be rescheduled based on the new idle delay.
+  DCHECK(HasScheduledIdleCallback());
+  idle_callback_handle_.Cancel();
+
+  // Calculate the updated time of when the |idle_callback_| should be invoked.
+  DCHECK(!last_no_inflight_event_.is_null());
+  auto new_idle_callback_time = last_no_inflight_event_ + idle_delay;
+  base::TimeDelta delta_until_idle =
+      new_idle_callback_time - tick_clock_->NowTicks();
+
+  if (delta_until_idle <= base::TimeDelta::FromSeconds(0)) {
+    // The new idle delay is shorter than the previous idle delay, and the idle
+    // time has been already passed. Let's run the idle callback immediately.
+    TriggerIdleCallback();
+    return;
+  }
+
+  // Let's schedule the idle callback in |delta_until_idle|.
+  ScheduleIdleCallback(delta_until_idle);
 }
 
 void ServiceWorkerEventQueue::UpdateStatus() {
@@ -197,6 +229,7 @@ void ServiceWorkerEventQueue::UpdateStatus() {
 
   HashMap<int /* event_id */, std::unique_ptr<EventInfo>> new_id_event_map;
 
+  bool should_idle_delay_to_be_zero = false;
   // Abort all events exceeding |kEventTimeout|.
   for (auto& it : id_event_map_) {
     auto& event_info = it.value;
@@ -206,33 +239,41 @@ void ServiceWorkerEventQueue::UpdateStatus() {
     }
     std::move(event_info->abort_callback)
         .Run(blink::mojom::ServiceWorkerEventStatus::TIMEOUT);
-    // Shut down the worker as soon as possible since the worker may have gone
-    // into bad state.
-    zero_idle_timer_delay_ = true;
+    should_idle_delay_to_be_zero = true;
   }
   id_event_map_.swap(new_id_event_map);
-
-  // If the worker is now idle, set the |idle_time_| and possibly trigger the
-  // idle callback.
-  if (!HasInflightEvent() && idle_time_.is_null()) {
+  if (should_idle_delay_to_be_zero) {
+    // Inflight events might be timed out and there might be no inflight event
+    // at this point.
+    if (!HasInflightEvent()) {
       OnNoInflightEvent();
-    return;
     }
-
-  if (!idle_time_.is_null() && idle_time_ < now) {
-    did_idle_timeout_ = true;
-    idle_callback_.Run();
+    // Shut down the worker as soon as possible since the worker may have gone
+    // into bad state.
+    SetIdleDelay(base::TimeDelta::FromSeconds(0));
   }
 }
 
-bool ServiceWorkerEventQueue::MaybeTriggerIdleTimer() {
+void ServiceWorkerEventQueue::ScheduleIdleCallback(base::TimeDelta delay) {
   DCHECK(!HasInflightEvent());
-  if (!zero_idle_timer_delay_)
-    return false;
+  DCHECK(!HasScheduledIdleCallback());
+
+  // WTF::Unretained() is safe because the task runner will be destroyed
+  // before |this| is destroyed at ServiceWorkerGlobalScope::Dispose().
+  idle_callback_handle_ = PostDelayedCancellableTask(
+      *task_runner_, FROM_HERE,
+      WTF::Bind(&ServiceWorkerEventQueue::TriggerIdleCallback,
+                WTF::Unretained(this)),
+      delay);
+}
+
+void ServiceWorkerEventQueue::TriggerIdleCallback() {
+  DCHECK(!HasInflightEvent());
+  DCHECK(!HasScheduledIdleCallback());
+  DCHECK(!did_idle_timeout_);
 
   did_idle_timeout_ = true;
   idle_callback_.Run();
-  return true;
 }
 
 void ServiceWorkerEventQueue::OnNoInflightEvent() {
@@ -244,14 +285,24 @@ void ServiceWorkerEventQueue::OnNoInflightEvent() {
     ProcessEvents();
     return;
   }
-  idle_time_ = tick_clock_->NowTicks() + kIdleDelay;
-  MaybeTriggerIdleTimer();
+  last_no_inflight_event_ = tick_clock_->NowTicks();
+  ScheduleIdleCallback(idle_delay_);
 }
 
 bool ServiceWorkerEventQueue::HasInflightEvent() const {
   return !id_event_map_.IsEmpty() || num_of_stay_awake_tokens_ > 0;
 }
 
+void ServiceWorkerEventQueue::ResetIdleTimeout() {
+  last_no_inflight_event_ = base::TimeTicks();
+  idle_callback_handle_.Cancel();
+  did_idle_timeout_ = false;
+}
+
+bool ServiceWorkerEventQueue::HasScheduledIdleCallback() const {
+  return idle_callback_handle_.IsActive();
+}
+
 ServiceWorkerEventQueue::Event::Event(
     ServiceWorkerEventQueue::Event::Type type,
     StartCallback start_callback,

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

@@ -13,6 +13,7 @@
 #include "base/timer/timer.h"
 #include "third_party/blink/public/mojom/service_worker/service_worker_event_status.mojom-blink-forward.h"
 #include "third_party/blink/renderer/modules/modules_export.h"
+#include "third_party/blink/renderer/platform/scheduler/public/post_cancellable_task.h"
 #include "third_party/blink/renderer/platform/wtf/deque.h"
 #include "third_party/blink/renderer/platform/wtf/hash_map.h"
 
@@ -33,12 +34,14 @@ namespace blink {
 // 1) Event timeout: when an event starts, StartEvent() records the expiration
 // time of the event (kEventTimeout). If EndEvent() has not been called within
 // the timeout time, |abort_callback| passed to StartEvent() is called with
-// status TIMEOUT. Also, |zero_idle_timer_delay_| is set to true to shut down
+// status TIMEOUT. Additionally, the |idle_delay_| is set to zero to shut down
 // the worker as soon as possible since the worker may have gone into bad state.
-// 2) Idle timeout: when a certain time has passed (kIdleDelay) since all of
-// events have ended, ServiceWorkerEventQueue calls the |idle_callback|.
-// |idle_callback| will be continuously called at a certain interval
-// (kUpdateInterval) until the next event starts.
+// 2) Idle timeout: when a certain time has passed (|idle_delay_|) since all of
+// events have ended, ServiceWorkerEventQueue calls the |idle_callback_|.
+// Idle callbacks are called on the |task_runner| passed by the constructor.
+// Note that the implementation assumes the task runner is provided by
+// ServiceWorkerGlobalScope, and the lifetime of the task runner is shorter than
+// |this|.
 //
 // The lifetime of ServiceWorkerEventQueue is the same with the worker
 // thread. If ServiceWorkerEventQueue is destructed while there are inflight
@@ -64,10 +67,12 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
       base::RepeatingCallback<void(/*is_offline_event=*/bool)>;
 
   ServiceWorkerEventQueue(BeforeStartEventCallback before_start_event_callback,
-                          base::RepeatingClosure idle_callback);
+                          base::RepeatingClosure idle_callback,
+                          scoped_refptr<base::SequencedTaskRunner> task_runner);
   // For testing.
   ServiceWorkerEventQueue(BeforeStartEventCallback before_start_event_callback,
                           base::RepeatingClosure idle_callback,
+                          scoped_refptr<base::SequencedTaskRunner> task_runner,
                           const base::TickClock* tick_clock);
   ~ServiceWorkerEventQueue();
 
@@ -101,22 +106,21 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
   // Returns true if |event_id| was started and hasn't ended.
   bool HasEvent(int event_id) const;
 
-  // Creates a StayAwakeToken to ensure that the idle timer won't be triggered
-  // while any of these are alive.
+  // Creates a StayAwakeToken to ensure that the idle callback won't be
+  // triggered while any of these are alive.
   std::unique_ptr<StayAwakeToken> CreateStayAwakeToken();
 
-  // 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().
-  void SetIdleTimerDelayToZero();
+  // Sets the |idle_delay_| to the new value, and re-schedule the idle callback
+  // based on the new delay if it's now pending.
+  void SetIdleDelay(base::TimeDelta idle_delay);
 
-  // Returns true if the timer thinks no events ran for a while, and has
-  // triggered the |idle_callback| passed to the constructor. It'll be reset to
+  // Returns true if the event queue thinks no events ran for a while, and has
+  // triggered the |idle_callback_| passed to the constructor. It'll be reset to
   // false again when StartEvent() is called.
   bool did_idle_timeout() const { return did_idle_timeout_; }
 
   // Idle timeout duration since the last event has finished.
-  static constexpr base::TimeDelta kIdleDelay =
+  static constexpr base::TimeDelta kDefaultIdleDelay =
       base::TimeDelta::FromSeconds(30);
   // Duration of the long standing event timeout since StartEvent() has been
   // called.
@@ -176,12 +180,15 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
   // Starts a single event.
   void StartEvent(std::unique_ptr<Event> event);
 
-  // Updates the internal states and fires timeout callbacks if any.
+  // Updates the internal states and fires the event timeout callbacks if any.
+  // TODO(shimazu): re-implement it by delayed tasks and cancelable callbacks.
   void UpdateStatus();
 
-  // Triggers idle timer if |zero_idle_timer_delay_| is true. Returns true if
-  // the idle callback is called.
-  bool MaybeTriggerIdleTimer();
+  // Schedules the idle callback in |delay|.
+  void ScheduleIdleCallback(base::TimeDelta delay);
+
+  // Runs the idle callback and updates the state accordingly.
+  void TriggerIdleCallback();
 
   // Sets the |idle_time_| and maybe calls |idle_callback_| immediately if the
   // timeout delay is set to zero.
@@ -193,6 +200,12 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
   // Processes all events in |queue_|.
   void ProcessEvents();
 
+  // Resets the members for idle timeouts to cancel the idle callback.
+  void ResetIdleTimeout();
+
+  // True if the idle callback is scheduled to run.
+  bool HasScheduledIdleCallback() const;
+
   struct EventInfo {
     EventInfo(base::TimeTicks expiration_time,
               base::OnceCallback<void(mojom::blink::ServiceWorkerEventStatus)>
@@ -203,25 +216,31 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
         abort_callback;
   };
 
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
   // For long standing event timeouts. This is used to look up an EventInfo
   // by event id.
   HashMap<int /* event_id */, std::unique_ptr<EventInfo>> id_event_map_;
 
-  // For idle timeouts. The time the service worker started being considered
-  // idle. This time is null if there are any inflight events.
-  base::TimeTicks idle_time_;
-
-  // Set to true if the idle callback should be fired immediately after all
-  // inflight events finish.
-  bool zero_idle_timer_delay_ = false;
-
   // Callback which is run just before starting an event.
   BeforeStartEventCallback before_start_event_callback_;
 
-  // For idle timeouts. Invoked when UpdateStatus() is called after
-  // |idle_time_|.
+  // For idle timeouts. When there's no inflight event, this is scheduled to run
+  // in |idle_delay_|.
   base::RepeatingClosure idle_callback_;
 
+  // For idle timeouts. The handle of the scheduled |idle_callback_|. This can
+  // be canceled when a new event happens before the scheduled callback runs.
+  TaskHandle idle_callback_handle_;
+
+  // For idle timeouts. The time when there's no inflight event. Set to null if
+  // there are inflight events.
+  base::TimeTicks last_no_inflight_event_;
+
+  // For idle timeouts. The delay until the worker is identified as idle after
+  // all inflight events are completed.
+  base::TimeDelta idle_delay_ = kDefaultIdleDelay;
+
   // Set to true once |idle_callback_| has been invoked. Set to false when
   // StartEvent() is called.
   bool did_idle_timeout_ = false;

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

@@ -220,11 +220,19 @@ ServiceWorkerGlobalScope::ServiceWorkerGlobalScope(
       cache_storage_remote_(std::move(cache_storage_remote)) {
   // Create the event queue. At this point its timer is not started. It will be
   // started by DidEvaluateScript().
+  //
+  // We are using TaskType::kInternalDefault for the idle callback, and it can
+  // be paused or throttled. This should work for now because we don't throttle
+  // or pause service worker threads, while it may cause not calling idle
+  // callback. We need to revisit this once we want to implement pausing
+  // service workers, but basically that won't be big problem because we have
+  // ping-pong timer and that will kill paused service workers.
   event_queue_ = std::make_unique<ServiceWorkerEventQueue>(
       WTF::BindRepeating(&ServiceWorkerGlobalScope::OnBeforeStartEvent,
                          WrapWeakPersistent(this)),
       WTF::BindRepeating(&ServiceWorkerGlobalScope::OnIdleTimeout,
-                         WrapWeakPersistent(this)));
+                         WrapWeakPersistent(this)),
+      GetTaskRunner(TaskType::kInternalDefault));
 }
 
 ServiceWorkerGlobalScope::~ServiceWorkerGlobalScope() = default;
@@ -2303,7 +2311,7 @@ void ServiceWorkerGlobalScope::Ping(PingCallback callback) {
 void ServiceWorkerGlobalScope::SetIdleTimerDelayToZero() {
   DCHECK(IsContextThread());
   DCHECK(event_queue_);
-  event_queue_->SetIdleTimerDelayToZero();
+  event_queue_->SetIdleDelay(base::TimeDelta::FromSeconds(0));
 }
 
 void ServiceWorkerGlobalScope::AddMessageToConsole(