ServiceWorker: Consider sites that take more than the timeout as offline capable.

When a timeout happens in ServiceWorkerEventQueue, the error status
transfers to ServiceWorkerFetchDispatcher::OnFetchEventFinished(), but
the callback of an offline capability checker
(ServiceWorkerOfflineCapabilityChecker::OnFetchResult) can't catch the
error status.

This CL set a flag `is_timeout_event_` when the timeout happens and if
the flag is set, call DidFail() with the error status instead of
DidFinish() which is called in a successful case.

Bug: 965802
Change-Id: I9b38c8332f3bfc539d16b0f101143e7af3db66f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2437081
Commit-Queue: Asami Doi <asamidoi@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823905}

content/browser/service_worker/service_worker_fetch_dispatcher.cc

@@ -623,6 +623,7 @@ void ServiceWorkerFetchDispatcher::DispatchFetchEvent() {
   version_->endpoint()->DispatchFetchEventForMainResource(
       std::move(params), std::move(pending_response_callback),
       base::BindOnce(&ServiceWorkerFetchDispatcher::OnFetchEventFinished,
+                     weak_factory_.GetWeakPtr(),
                      base::Unretained(version_.get()), event_finish_id,
                      url_loader_assets_));
 }
@@ -781,12 +782,14 @@ bool ServiceWorkerFetchDispatcher::IsEventDispatched() const {
   return request_.is_null();
 }
 
-// static
 void ServiceWorkerFetchDispatcher::OnFetchEventFinished(
     ServiceWorkerVersion* version,
     int event_finish_id,
     scoped_refptr<URLLoaderAssets> url_loader_assets,
     blink::mojom::ServiceWorkerEventStatus status) {
+  if (status == blink::mojom::ServiceWorkerEventStatus::TIMEOUT) {
+    DidFail(blink::ServiceWorkerStatusCode::kErrorTimeout);
+  }
   version->FinishRequest(
       event_finish_id,
       status != blink::mojom::ServiceWorkerEventStatus::ABORTED);

content/browser/service_worker/service_worker_fetch_dispatcher.h

@@ -101,11 +101,10 @@ class CONTENT_EXPORT ServiceWorkerFetchDispatcher {
   // are settled. This function is called once the renderer signals that
   // happened. |fetch_callback_| can run before this, once respondWith() is
   // settled.
-  static void OnFetchEventFinished(
-      ServiceWorkerVersion* version,
-      int event_finish_id,
-      scoped_refptr<URLLoaderAssets> url_loader_assets,
-      blink::mojom::ServiceWorkerEventStatus status);
+  void OnFetchEventFinished(ServiceWorkerVersion* version,
+                            int event_finish_id,
+                            scoped_refptr<URLLoaderAssets> url_loader_assets,
+                            blink::mojom::ServiceWorkerEventStatus status);
 
   ServiceWorkerMetrics::EventType GetEventType() const;
 

content/browser/service_worker/service_worker_offline_capability_check_browsertest.cc

@@ -677,6 +677,24 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerOfflineCapabilityCheckBrowserTest,
 
   EXPECT_EQ(OfflineCapability::kUnsupported,
             CheckOfflineCapability("/service_worker/empty.html?cache_add"));
+
+  EXPECT_EQ(
+      OfflineCapability::kSupported,
+      CheckOfflineCapability("/service_worker/empty.html?sleep_then_offline"));
+
+  // Site that takes more than the timeout is considered as "offline capable".
+  EXPECT_EQ(OfflineCapability::kSupported,
+            CheckOfflineCapability(
+                "/service_worker/empty.html?sleep_then_offline&sleep=20000"));
+
+  EXPECT_EQ(
+      OfflineCapability::kUnsupported,
+      CheckOfflineCapability("/service_worker/empty.html?sleep_then_fetch"));
+
+  // Site that takes more than the timeout is considered as "offline capable".
+  EXPECT_EQ(OfflineCapability::kSupported,
+            CheckOfflineCapability(
+                "/service_worker/empty.html?sleep_then_fetch&sleep=20000"));
 }
 
 // Sites with a service worker which is not activated yet are identified as

content/browser/service_worker/service_worker_offline_capability_checker.cc

@@ -130,15 +130,18 @@ void ServiceWorkerOfflineCapabilityChecker::OnFetchResult(
     blink::mojom::ServiceWorkerStreamHandlePtr /* stream */,
     blink::mojom::ServiceWorkerFetchEventTimingPtr /* timing */,
     scoped_refptr<ServiceWorkerVersion> version) {
-  if (status == blink::ServiceWorkerStatusCode::kOk &&
-      result == ServiceWorkerFetchDispatcher::FetchEventResult::kGotResponse &&
-      // TODO(hayato): Investigate whether any 2xx should be accepted or not.
-      response->status_code == 200) {
+  // The sites are considered as "offline capable" when the response finished
+  // successfully and returns 200 or the timeout happens.
+  if ((status == blink::ServiceWorkerStatusCode::kOk &&
+       result == ServiceWorkerFetchDispatcher::FetchEventResult::kGotResponse &&
+       // TODO(hayato): Investigate whether any 2xx should be accepted or not.
+       response->status_code == 200) ||
+      status == blink::ServiceWorkerStatusCode::kErrorTimeout) {
     std::move(callback_).Run(OfflineCapability::kSupported,
                              version->registration_id());
   } else {
     // TODO(hayato): At present, we return kUnsupported even if the detection
-    // failed due to internal errors (disk fialures, timeout, etc). In the
+    // failed due to internal errors except timeout (disk fialures, etc). In the
     // future, we might want to return another enum value so that the callside
     // can know whether internal errors happened or not.
     std::move(callback_).Run(OfflineCapability::kUnsupported,

content/test/data/service_worker/maybe_offline_support.js

@@ -33,13 +33,13 @@ self.addEventListener("fetch", event => {
     );
   } else if (param.has("sleep_then_fetch")) {
     event.respondWith(
-      sleep(param.get("sleep") || 0, event.request.url).then(() => {
+      sleep(param.get("sleep") || 0).then(() => {
         return fetch(event.request.url);
       })
     );
   } else if (param.has("sleep_then_offline")) {
     event.respondWith(
-      sleep(param.get("sleep") || 0, event.request.url).then(() => {
+      sleep(param.get("sleep") || 0).then(() => {
         return new Response("Hello Offline page");
       })
     );

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

@@ -1952,9 +1952,9 @@ void ServiceWorkerGlobalScope::DispatchFetchEventForMainResource(
   DCHECK(IsContextThread());
 
   // The timeout for offline events in a service worker. The default value is
-  // the same as the default timeout (5 mins) of all events.
+  // the same as the update interval value in the event queue.
   static const base::FeatureParam<int> kCustomTimeoutForOfflineEvent{
-      &features::kCheckOfflineCapability, "timeout_second", 5 * 60};
+      &features::kCheckOfflineCapability, "timeout_second", 10};
 
   const int event_id = event_queue_->NextEventId();
   fetch_event_callbacks_.Set(event_id, std::move(callback));