Call FinishRequest for ResponseCallback on timeout of fetch event

Previously ResponseCallback was not discarded even if the waitUntil()
hits the timeout. This ends up with keeping a request stored in
`ServiceWorkerVersion::inflight_requests_` until respondWith() is
settled even if the waitUntil() hits the timeout. This CL changes the
behavior by closing the Mojo endpoint for ResponseCallback on timeout.

In addition, another bug was revealed by the fix where
`is_endpoint_ready_` was not updated correctly. This caused an issue of
dispatching a fetch event while it's still in stopping state. This CL
updates the flag in OnStopping() which is called synchronously from
EmbeddedWorkerInstance::Stop().

Bug: 1156091, 1156081, 1156626, 1152793
Change-Id: Iaa227b0dbfa7cd6046e441a1c73c18e15757172a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2586645Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Asami Doi <asamidoi@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838601}

content/browser/service_worker/service_worker_fetch_dispatcher.cc

@@ -354,7 +354,10 @@ class ServiceWorkerFetchDispatcher::ResponseCallback
       ServiceWorkerVersion* version)
       : receiver_(this, std::move(receiver)),
         fetch_dispatcher_(fetch_dispatcher),
-        version_(version) {}
+        version_(version) {
+    receiver_.set_disconnect_handler(base::BindOnce(
+        &ResponseCallback::OnDisconnected, base::Unretained(this)));
+  }
 
   ~ResponseCallback() override { DCHECK(fetch_event_id_.has_value()); }
 
@@ -363,6 +366,15 @@ class ServiceWorkerFetchDispatcher::ResponseCallback
     fetch_event_id_ = id;
   }
 
+  void OnDisconnected() {
+    version_->FinishRequest(fetch_event_id_.value(), /*was_handled=*/false);
+    // HandleResponse() is not needed to be called here because
+    // OnFetchEventFinished() should be called with an error code when
+    // disconnecting without response, and it lets the request failed.
+
+    // Do not add code here because FinishRequest() removes `this`.
+  }
+
   // Implements blink::mojom::ServiceWorkerFetchResponseCallback.
   void OnResponse(
       blink::mojom::FetchAPIResponsePtr response,

content/browser/service_worker/service_worker_fetch_dispatcher_browsertest.cc

@@ -241,9 +241,8 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerFetchDispatcherBrowserTest, FetchEvent) {
 // This is the timeout case that the lifetime of a fetch event is shorter than
 // the response finishes. ServiceWorkerFetchDispatcher::OnFetchEventFinished is
 // called first.
-// Disabled due to flakes; see https://crbug.com/1156091.
 IN_PROC_BROWSER_TEST_F(ServiceWorkerFetchDispatcherBrowserTest,
-                       DISABLED_FetchEventTimeout) {
+                       FetchEventTimeout) {
   StartServerAndNavigateToSetup();
   ServiceWorkerVersion* version = CreateVersion();
 

content/browser/service_worker/service_worker_offline_capability_check_browsertest.cc

@@ -647,16 +647,9 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerOfflineCapabilityCheckBrowserTest,
 
 // Sites with a service worker are identified as supporting offline capability
 // only when it returns a valid response in the offline mode.
-#if defined(OS_WIN) || defined(OS_MAC)
-// Flaky on Win7: https://crbug.com/1156081
-// Flaky on Mac: https://crbug.com/1156626
-#define MAYBE_CheckOfflineCapability DISABLED_CheckOfflineCapability
-#else
-#define MAYBE_CheckOfflineCapability CheckOfflineCapability
-#endif
 
 IN_PROC_BROWSER_TEST_F(ServiceWorkerOfflineCapabilityCheckBrowserTest,
-                       MAYBE_CheckOfflineCapability) {
+                       CheckOfflineCapability) {
   EXPECT_TRUE(NavigateToURL(shell(),
                             embedded_test_server()->GetURL(
                                 "/service_worker/create_service_worker.html")));

content/browser/service_worker/service_worker_version.cc

@@ -483,12 +483,6 @@ void ServiceWorkerVersion::StopWorker(base::OnceClosure callback) {
   switch (running_status()) {
     case EmbeddedWorkerStatus::STARTING:
     case EmbeddedWorkerStatus::RUNNING: {
-      // Endpoint isn't available after calling StopWorker(). This needs to be
-      // set here without waiting until the worker is actually stopped because
-      // subsequent StartWorker() may read the flag to decide whether an event
-      // can be dispatched or not.
-      is_endpoint_ready_ = false;
-
       // EmbeddedWorkerInstance::Stop() may synchronously call
       // ServiceWorkerVersion::OnStopped() and destroy |this|. This protection
       // avoids it.
@@ -1223,6 +1217,12 @@ void ServiceWorkerVersion::OnStopping() {
                            script_url_.spec(), "Version Status",
                            VersionStatusToString(status_));
 
+  // Endpoint isn't available after calling EmbeddedWorkerInstance::Stop().
+  // This needs to be set here without waiting until the worker is actually
+  // stopped because subsequent StartWorker() may read the flag to decide
+  // whether an event can be dispatched or not.
+  is_endpoint_ready_ = false;
+
   // Shorten the interval so stalling in stopped can be fixed quickly. Once the
   // worker stops, the timer is disabled. The interval will be reset to normal
   // when the worker starts up again.

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

@@ -1006,7 +1006,10 @@ void ServiceWorkerGlobalScope::RespondToFetchEventWithNoResponse(
       TRACE_ID_WITH_SCOPE(kServiceWorkerGlobalScopeTraceScope,
                           TRACE_ID_LOCAL(fetch_event_id)),
       TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
-  DCHECK(fetch_response_callbacks_.Contains(fetch_event_id));
+  // `fetch_response_callbacks_` does not have the entry when the event timed
+  // out.
+  if (!fetch_response_callbacks_.Contains(fetch_event_id))
+    return;
   mojom::blink::ServiceWorkerFetchResponseCallback* response_callback =
       fetch_response_callbacks_.Take(fetch_event_id)->Value().get();
 
@@ -1031,7 +1034,10 @@ void ServiceWorkerGlobalScope::RespondToFetchEvent(
       TRACE_ID_WITH_SCOPE(kServiceWorkerGlobalScopeTraceScope,
                           TRACE_ID_LOCAL(fetch_event_id)),
       TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
-  DCHECK(fetch_response_callbacks_.Contains(fetch_event_id));
+  // `fetch_response_callbacks_` does not have the entry when the event timed
+  // out.
+  if (!fetch_response_callbacks_.Contains(fetch_event_id))
+    return;
 
   mojom::blink::ServiceWorkerFetchResponseCallback* response_callback =
       fetch_response_callbacks_.Take(fetch_event_id)->Value().get();
@@ -1059,7 +1065,10 @@ void ServiceWorkerGlobalScope::RespondToFetchEventWithResponseStream(
       TRACE_ID_WITH_SCOPE(kServiceWorkerGlobalScopeTraceScope,
                           TRACE_ID_LOCAL(fetch_event_id)),
       TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT);
-  DCHECK(fetch_response_callbacks_.Contains(fetch_event_id));
+  // `fetch_response_callbacks_` does not have the entry when the event timed
+  // out.
+  if (!fetch_response_callbacks_.Contains(fetch_event_id))
+    return;
   mojom::blink::ServiceWorkerFetchResponseCallback* response_callback =
       fetch_response_callbacks_.Take(fetch_event_id)->Value().get();
 
@@ -1472,11 +1481,25 @@ void ServiceWorkerGlobalScope::DispatchExtendableMessageEventInternal(
   DispatchExtendableEvent(event_to_dispatch, observer);
 }
 
+void ServiceWorkerGlobalScope::AbortCallbackForFetchEvent(
+    int event_id,
+    mojom::blink::ServiceWorkerEventStatus status) {
+  // Discard a callback for an inflight respondWith() if it still exists.
+  auto response_callback_iter = fetch_response_callbacks_.find(event_id);
+  if (response_callback_iter != fetch_response_callbacks_.end()) {
+    response_callback_iter->value->TakeValue().reset();
+    fetch_response_callbacks_.erase(response_callback_iter);
+  }
+
+  // Run the event callback with the error code.
+  auto event_callback_iter = fetch_event_callbacks_.find(event_id);
+  std::move(event_callback_iter->value).Run(status);
+  fetch_event_callbacks_.erase(event_callback_iter);
+}
+
 void ServiceWorkerGlobalScope::StartFetchEvent(
     mojom::blink::DispatchFetchEventParamsPtr params,
     base::WeakPtr<CrossOriginResourcePolicyChecker> corp_checker,
-    mojo::PendingRemote<mojom::blink::ServiceWorkerFetchResponseCallback>
-        response_callback,
     base::Optional<base::TimeTicks> created_time,
     int event_id) {
   DCHECK(IsContextThread());
@@ -1484,13 +1507,6 @@ void ServiceWorkerGlobalScope::StartFetchEvent(
     RecordQueuingTime(created_time.value());
   }
 
-  HeapMojoRemote<mojom::blink::ServiceWorkerFetchResponseCallback,
-                 HeapMojoWrapperMode::kWithoutContextObserver>
-      remote(this);
-  remote.Bind(std::move(response_callback),
-              GetThread()->GetTaskRunner(TaskType::kNetworking));
-  fetch_response_callbacks_.Set(event_id, WrapDisallowNew(std::move(remote)));
-
   // This TRACE_EVENT is used for perf benchmark to confirm if all of fetch
   // events have completed. (crbug.com/736697)
   TRACE_EVENT_WITH_FLOW1(
@@ -1574,23 +1590,31 @@ void ServiceWorkerGlobalScope::DispatchFetchEventForSubresource(
 
   const int event_id = event_queue_->NextEventId();
   fetch_event_callbacks_.Set(event_id, std::move(callback));
+  HeapMojoRemote<mojom::blink::ServiceWorkerFetchResponseCallback,
+                 HeapMojoWrapperMode::kWithoutContextObserver>
+      remote(this);
+  remote.Bind(std::move(response_callback),
+              GetThread()->GetTaskRunner(TaskType::kNetworking));
+  fetch_response_callbacks_.Set(event_id, WrapDisallowNew(std::move(remote)));
 
   if (RequestedTermination()) {
     event_queue_->EnqueuePending(
         event_id,
         WTF::Bind(&ServiceWorkerGlobalScope::StartFetchEvent,
                   WrapWeakPersistent(this), std::move(params),
-                  std::move(corp_checker), std::move(response_callback),
-                  base::TimeTicks::Now()),
-        CreateAbortCallback(&fetch_event_callbacks_), base::nullopt);
+                  std::move(corp_checker), base::TimeTicks::Now()),
+        WTF::Bind(&ServiceWorkerGlobalScope::AbortCallbackForFetchEvent,
+                  WrapWeakPersistent(this)),
+        base::nullopt);
   } else {
     event_queue_->EnqueueNormal(
         event_id,
         WTF::Bind(&ServiceWorkerGlobalScope::StartFetchEvent,
                   WrapWeakPersistent(this), std::move(params),
-                  std::move(corp_checker), std::move(response_callback),
-                  base::TimeTicks::Now()),
-        CreateAbortCallback(&fetch_event_callbacks_), base::nullopt);
+                  std::move(corp_checker), base::TimeTicks::Now()),
+        WTF::Bind(&ServiceWorkerGlobalScope::AbortCallbackForFetchEvent,
+                  WrapWeakPersistent(this)),
+        base::nullopt);
   }
 }
 
@@ -1960,6 +1984,13 @@ void ServiceWorkerGlobalScope::DispatchFetchEventForMainResource(
   const int event_id = event_queue_->NextEventId();
   fetch_event_callbacks_.Set(event_id, std::move(callback));
 
+  HeapMojoRemote<mojom::blink::ServiceWorkerFetchResponseCallback,
+                 HeapMojoWrapperMode::kWithoutContextObserver>
+      remote(this);
+  remote.Bind(std::move(response_callback),
+              GetThread()->GetTaskRunner(TaskType::kNetworking));
+  fetch_response_callbacks_.Set(event_id, WrapDisallowNew(std::move(remote)));
+
   // We can use nullptr as a |corp_checker| for the main resource because it
   // must be the same origin.
   if (params->is_offline_capability_check) {
@@ -1967,18 +1998,19 @@ void ServiceWorkerGlobalScope::DispatchFetchEventForMainResource(
         event_id,
         WTF::Bind(&ServiceWorkerGlobalScope::StartFetchEvent,
                   WrapWeakPersistent(this), std::move(params),
-                  /*corp_checker=*/nullptr, std::move(response_callback),
-                  base::nullopt),
-        CreateAbortCallback(&fetch_event_callbacks_),
+                  /*corp_checker=*/nullptr, base::nullopt),
+        WTF::Bind(&ServiceWorkerGlobalScope::AbortCallbackForFetchEvent,
+                  WrapWeakPersistent(this)),
         base::TimeDelta::FromSeconds(kCustomTimeoutForOfflineEvent.Get()));
   } else {
     event_queue_->EnqueueNormal(
         event_id,
         WTF::Bind(&ServiceWorkerGlobalScope::StartFetchEvent,
                   WrapWeakPersistent(this), std::move(params),
-                  /*corp_checker=*/nullptr, std::move(response_callback),
-                  base::TimeTicks::Now()),
-        CreateAbortCallback(&fetch_event_callbacks_), base::nullopt);
+                  /*corp_checker=*/nullptr, base::TimeTicks::Now()),
+        WTF::Bind(&ServiceWorkerGlobalScope::AbortCallbackForFetchEvent,
+                  WrapWeakPersistent(this)),
+        base::nullopt);
   }
 }
 

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

@@ -499,13 +499,15 @@ class MODULES_EXPORT ServiceWorkerGlobalScope final
   void NoteNewFetchEvent(const KURL& request_url);
   void NoteRespondedToFetchEvent(const KURL& request_url);
 
+  void AbortCallbackForFetchEvent(
+      int event_id,
+      mojom::blink::ServiceWorkerEventStatus status);
+
   // Dispatches the event synchronously. Enqueued by Dispatch*Event methods to
   // the event queue, and executed immediately or sometimes later.
   void StartFetchEvent(
       mojom::blink::DispatchFetchEventParamsPtr params,
       base::WeakPtr<CrossOriginResourcePolicyChecker> corp_checker,
-      mojo::PendingRemote<mojom::blink::ServiceWorkerFetchResponseCallback>
-          response_callback,
       base::Optional<base::TimeTicks> created_time,
       int event_id);
   void StartInstallEvent(int event_id);