S13nServiceWorker: postpone fetch events for subresources when idle

This patch implements a part of idle timer's logic. Fetch events coming through
mojom::ControllerServiceWorker after the worker has requested termination to the
browser should be queued until the worker receives the next event or StopWorker
message from the browser. If the worker receives the next event from the
browser, the queued events should run before the event from the browser,
otherwise the order of the events would be messed up. If the worker receives
StopWorker message instead, the worker shouldn't invoke the callback for the
fetch event, and silently should disconnect the pipe to the clients.

Bug: 774374
Change-Id: I6f3c90405ff037fe07df0310e2234cb81cb2ead9
Reviewed-on: https://chromium-review.googlesource.com/816458
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523677}

content/renderer/service_worker/controller_service_worker_impl.cc

@@ -29,9 +29,9 @@ void ControllerServiceWorkerImpl::DispatchFetchEvent(
     mojom::ServiceWorkerFetchResponseCallbackPtr response_callback,
     DispatchFetchEventCallback callback) {
   DCHECK(context_client_);
-  context_client_->DispatchFetchEvent(request, nullptr /* preload_handle */,
-                                      std::move(response_callback),
-                                      std::move(callback));
+  context_client_->DispatchOrQueueFetchEvent(
+      request, nullptr /* preload_handle */, std::move(response_callback),
+      std::move(callback));
 }
 
 }  // namespace content

content/renderer/service_worker/service_worker_context_client.cc

@@ -448,14 +448,18 @@ struct ServiceWorkerContextClient::WorkerContextData {
   // Inflight navigation preload requests.
   base::IDMap<std::unique_ptr<NavigationPreloadRequest>> preload_requests;
 
-  // S13nServiceWorker
-  std::unique_ptr<ControllerServiceWorkerImpl> controller_impl;
-
   // S13nServiceWorker
   // Timer triggered when the service worker considers it should be stopped or
   // an event should be aborted.
   std::unique_ptr<ServiceWorkerTimeoutTimer> timeout_timer;
 
+  // S13nServiceWorker
+  // |controller_impl| should be destroyed before |timeout_timer| since the
+  // pipe needs to be disconnected before callbacks passed by
+  // DispatchSomeEvent() get destructed, which may be stored in |timeout_timer|
+  // as parameters of pending tasks added after a termination request.
+  std::unique_ptr<ControllerServiceWorkerImpl> controller_impl;
+
   base::ThreadChecker thread_checker;
   base::WeakPtrFactory<ServiceWorkerContextClient> weak_factory;
   base::WeakPtrFactory<blink::WebServiceWorkerContextProxy> proxy_weak_factory;
@@ -1267,6 +1271,22 @@ void ServiceWorkerContextClient::Claim(
                          base::Unretained(this), std::move(callbacks)));
 }
 
+void ServiceWorkerContextClient::DispatchOrQueueFetchEvent(
+    const ResourceRequest& request,
+    mojom::FetchEventPreloadHandlePtr preload_handle,
+    mojom::ServiceWorkerFetchResponseCallbackPtr response_callback,
+    DispatchFetchEventCallback callback) {
+  if (RequestedTermination()) {
+    context_->timeout_timer->PushPendingTask(
+        base::BindOnce(&ServiceWorkerContextClient::DispatchFetchEvent,
+                       GetWeakPtr(), request, std::move(preload_handle),
+                       std::move(response_callback), std::move(callback)));
+    return;
+  }
+  DispatchFetchEvent(request, std::move(preload_handle),
+                     std::move(response_callback), std::move(callback));
+}
+
 void ServiceWorkerContextClient::DispatchSyncEvent(
     const std::string& tag,
     blink::mojom::BackgroundSyncEventLastChance last_chance,
@@ -1358,7 +1378,7 @@ void ServiceWorkerContextClient::SendWorkerStarted() {
   // Start the idle timer.
   context_->timeout_timer =
       std::make_unique<ServiceWorkerTimeoutTimer>(base::BindRepeating(
-          &ServiceWorkerContextClient::OnIdle, base::Unretained(this)));
+          &ServiceWorkerContextClient::OnIdleTimeout, base::Unretained(this)));
 }
 
 void ServiceWorkerContextClient::DispatchActivateEvent(
@@ -1828,12 +1848,18 @@ void ServiceWorkerContextClient::SetupNavigationPreload(
                                        fetch_event_id);
 }
 
-void ServiceWorkerContextClient::OnIdle() {
-  // TODO(crbug.com/774374): Ignore events from clients after this point until
-  // an event from the browser process or StopWorker() is received.
+void ServiceWorkerContextClient::OnIdleTimeout() {
+  // ServiceWorkerTimeoutTimer::did_idle_timeout() returns true if
+  // ServiceWorkerContextClient::OnIdleTiemout() has been called. It means
+  // termination has been requested.
+  DCHECK(RequestedTermination());
   (*instance_host_)->RequestTermination();
 }
 
+bool ServiceWorkerContextClient::RequestedTermination() const {
+  return context_->timeout_timer->did_idle_timeout();
+}
+
 base::WeakPtr<ServiceWorkerContextClient>
 ServiceWorkerContextClient::GetWeakPtr() {
   DCHECK(worker_task_runner_->RunsTasksInCurrentSequence());
@@ -1846,4 +1872,10 @@ void ServiceWorkerContextClient::ResetThreadSpecificInstanceForTesting() {
   g_worker_client_tls.Pointer()->Set(nullptr);
 }
 
+void ServiceWorkerContextClient::SetTimeoutTimerForTesting(
+    std::unique_ptr<ServiceWorkerTimeoutTimer> timeout_timer) {
+  DCHECK(context_);
+  context_->timeout_timer = std::move(timeout_timer);
+}
+
 }  // namespace content

content/renderer/service_worker/service_worker_context_client.h

@@ -61,10 +61,11 @@ namespace content {
 struct PlatformNotificationData;
 struct PushEventPayload;
 struct ServiceWorkerClientInfo;
+class EmbeddedWorkerInstanceClientImpl;
 class ServiceWorkerNetworkProvider;
 class ServiceWorkerProviderContext;
+class ServiceWorkerTimeoutTimer;
 class ThreadSafeSender;
-class EmbeddedWorkerInstanceClientImpl;
 class WebWorkerFetchContext;
 
 // ServiceWorkerContextClient is a "client" of a service worker execution
@@ -252,11 +253,33 @@ class CONTENT_EXPORT ServiceWorkerContextClient
   void Claim(std::unique_ptr<blink::WebServiceWorkerClientsClaimCallbacks>
                  callbacks) override;
 
+  // Dispatches the fetch event if the worker is running normally, and queues it
+  // instead if the worker has already requested to be terminated by the
+  // browser. If queued, the event will be dispatched once the worker resumes
+  // normal operation (if the browser decides not to terminate it, and instead
+  // starts another event), or else is dropped if the worker is terminated.
+  //
+  // This method needs to be used only if the event comes directly from a
+  // client, which means it is coming through the ControllerServiceWorkerImpl.
+  void DispatchOrQueueFetchEvent(
+      const ResourceRequest& request,
+      mojom::FetchEventPreloadHandlePtr preload_handle,
+      mojom::ServiceWorkerFetchResponseCallbackPtr response_callback,
+      DispatchFetchEventCallback callback);
+
  private:
   struct WorkerContextData;
   class NavigationPreloadRequest;
   friend class ControllerServiceWorkerImpl;
   friend class ServiceWorkerContextClientTest;
+  FRIEND_TEST_ALL_PREFIXES(
+      ServiceWorkerContextClientTest,
+      DispatchOrQueueFetchEvent_RequestedTerminationAndDie);
+  FRIEND_TEST_ALL_PREFIXES(
+      ServiceWorkerContextClientTest,
+      DispatchOrQueueFetchEvent_RequestedTerminationAndWakeUp);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerContextClientTest,
+                           DispatchOrQueueFetchEvent_NotRequestedTermination);
 
   // Get routing_id for sending message to the ServiceWorkerVersion
   // in the browser process.
@@ -380,12 +403,19 @@ class CONTENT_EXPORT ServiceWorkerContextClient
                               const GURL& url,
                               mojom::FetchEventPreloadHandlePtr preload_handle);
 
-  // Called when a certain time has passed since the last task finished.
-  void OnIdle();
+  // Called by ServiceWorkerTimeoutTimer when a certain time has passed since
+  // the last task finished.
+  void OnIdleTimeout();
+
+  // Returns true if the worker has requested to be terminated by the browser
+  // process. It does this due to idle timeout.
+  bool RequestedTermination() const;
 
   base::WeakPtr<ServiceWorkerContextClient> GetWeakPtr();
 
   static void ResetThreadSpecificInstanceForTesting();
+  void SetTimeoutTimerForTesting(
+      std::unique_ptr<ServiceWorkerTimeoutTimer> timeout_timer);
 
   const int embedded_worker_id_;
   const int64_t service_worker_version_id_;

content/renderer/service_worker/service_worker_timeout_timer.cc

@@ -53,6 +53,15 @@ ServiceWorkerTimeoutTimer::~ServiceWorkerTimeoutTimer() {
 
 int ServiceWorkerTimeoutTimer::StartEvent(
     base::OnceCallback<void(int /* event_id */)> abort_callback) {
+  if (did_idle_timeout()) {
+    idle_time_ = base::TimeTicks();
+    did_idle_timeout_ = false;
+    while (!pending_tasks_.empty()) {
+      std::move(pending_tasks_.front()).Run();
+      pending_tasks_.pop();
+    }
+  }
+
   idle_time_ = base::TimeTicks();
   const int event_id = NextEventId();
   std::set<EventInfo>::iterator iter;
@@ -74,6 +83,13 @@ void ServiceWorkerTimeoutTimer::EndEvent(int event_id) {
     idle_time_ = tick_clock_->NowTicks() + kIdleDelay;
 }
 
+void ServiceWorkerTimeoutTimer::PushPendingTask(
+    base::OnceClosure pending_task) {
+  DCHECK(ServiceWorkerUtils::IsServicificationEnabled());
+  DCHECK(did_idle_timeout());
+  pending_tasks_.emplace(std::move(pending_task));
+}
+
 void ServiceWorkerTimeoutTimer::UpdateStatus() {
   if (!ServiceWorkerUtils::IsServicificationEnabled())
     return;
@@ -94,8 +110,10 @@ void ServiceWorkerTimeoutTimer::UpdateStatus() {
   if (inflight_events_.empty() && idle_time_.is_null())
     idle_time_ = tick_clock_->NowTicks() + kIdleDelay;
 
-  if (!idle_time_.is_null() && idle_time_ < now)
+  if (!idle_time_.is_null() && idle_time_ < now) {
+    did_idle_timeout_ = true;
     idle_callback_.Run();
+  }
 }
 
 ServiceWorkerTimeoutTimer::EventInfo::EventInfo(

content/renderer/service_worker/service_worker_timeout_timer.h

@@ -50,11 +50,23 @@ class CONTENT_EXPORT ServiceWorkerTimeoutTimer {
 
   // StartEvent() should be called at the beginning of an event. It returns an
   // event id. The event id should be passed to EndEvent() when the event has
-  // finished.
+  // 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(base::OnceCallback<void(int /* event_id */)> abort_callback);
   void EndEvent(int event_id);
 
+  // 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);
+
+  // 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
+  // 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 =
       base::TimeDelta::FromSeconds(30);
@@ -99,6 +111,14 @@ class CONTENT_EXPORT ServiceWorkerTimeoutTimer {
   // |idle_time_|.
   base::RepeatingClosure idle_callback_;
 
+  // Set to true once |idle_callback_| has been invoked. Set to false when
+  // StartEvent() is called.
+  bool did_idle_timeout_ = false;
+
+  // Tasks waiting for the timer getting the next request to start an event
+  // by StartEvent().
+  base::queue<base::OnceClosure> pending_tasks_;
+
   // |timer_| invokes UpdateEventStatus() periodically.
   base::RepeatingTimer timer_;
 

content/renderer/service_worker/service_worker_timeout_timer_unittest.cc

@@ -5,6 +5,7 @@
 #include "content/renderer/service_worker/service_worker_timeout_timer.h"
 
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_mock_time_task_runner.h"
@@ -19,9 +20,11 @@ namespace {
 
 class MockEvent {
  public:
+  MockEvent() : weak_factory_(this) {}
+
   base::OnceCallback<void(int)> CreateAbortCallback() {
     EXPECT_FALSE(has_aborted_);
-    return base::BindOnce(&MockEvent::Abort, base::Unretained(this));
+    return base::BindOnce(&MockEvent::Abort, weak_factory_.GetWeakPtr());
   }
 
   int event_id() const { return event_id_; }
@@ -36,8 +39,14 @@ class MockEvent {
 
   bool has_aborted_ = false;
   int event_id_ = 0;
+  base::WeakPtrFactory<MockEvent> weak_factory_;
 };
 
+base::RepeatingClosure CreateReceiverWithCalledFlag(bool* out_is_called) {
+  return base::BindRepeating([](bool* out_is_called) { *out_is_called = true; },
+                             out_is_called);
+}
+
 }  // namespace
 
 class ServiceWorkerTimeoutTimerTest : public testing::Test {
@@ -74,10 +83,8 @@ TEST_F(ServiceWorkerTimeoutTimerTest, IdleTimer) {
       base::BindRepeating([](int) {});
 
   bool is_idle = false;
-  ServiceWorkerTimeoutTimer timer(
-      base::BindRepeating([](bool* out_is_idle) { *out_is_idle = true; },
-                          &is_idle),
-      task_runner()->GetMockTickClock());
+  ServiceWorkerTimeoutTimer timer(CreateReceiverWithCalledFlag(&is_idle),
+                                  task_runner()->GetMockTickClock());
   task_runner()->FastForwardBy(kIdleInterval);
   // |idle_callback| should be fired since there is no event.
   EXPECT_TRUE(is_idle);
@@ -132,10 +139,8 @@ TEST_F(ServiceWorkerTimeoutTimerTest, BecomeIdleAfterAbort) {
   EnableServicification();
 
   bool is_idle = false;
-  ServiceWorkerTimeoutTimer timer(
-      base::BindRepeating([](bool* out_is_idle) { *out_is_idle = true; },
-                          &is_idle),
-      task_runner()->GetMockTickClock());
+  ServiceWorkerTimeoutTimer timer(CreateReceiverWithCalledFlag(&is_idle),
+                                  task_runner()->GetMockTickClock());
 
   MockEvent event;
   int event_id = timer.StartEvent(event.CreateAbortCallback());
@@ -175,6 +180,26 @@ TEST_F(ServiceWorkerTimeoutTimerTest, AbortAllOnDestruction) {
   EXPECT_TRUE(event2.has_aborted());
 }
 
+TEST_F(ServiceWorkerTimeoutTimerTest, PushPendingTask) {
+  EnableServicification();
+  ServiceWorkerTimeoutTimer timer(base::BindRepeating(&base::DoNothing),
+                                  task_runner()->GetMockTickClock());
+  task_runner()->FastForwardBy(ServiceWorkerTimeoutTimer::kIdleDelay +
+                               ServiceWorkerTimeoutTimer::kUpdateInterval +
+                               base::TimeDelta::FromSeconds(1));
+  EXPECT_TRUE(timer.did_idle_timeout());
+
+  bool did_task_run = false;
+  timer.PushPendingTask(CreateReceiverWithCalledFlag(&did_task_run));
+
+  // Start a new event. StartEvent() should run the pending tasks.
+  MockEvent event;
+  const int event_id = timer.StartEvent(event.CreateAbortCallback());
+  event.set_event_id(event_id);
+  EXPECT_FALSE(timer.did_idle_timeout());
+  EXPECT_TRUE(did_task_run);
+}
+
 TEST_F(ServiceWorkerTimeoutTimerTest, NonS13nServiceWorker) {
   ASSERT_FALSE(ServiceWorkerUtils::IsServicificationEnabled());