Make service worker check for fetch handle existence at the end of

initial script evaluation.

This patch makes service worker check for fetch handle existence at the
end of initial script evaluation, which is more consistent with spec
(https://w3c.github.io/ServiceWorker/#should-skip-event).

Bug: 1005060
Change-Id: I3e389e5a641971398857fc40e6e5fcc79e074cb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1995062Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Wei4 Wang <wei4.wang@intel.com>
Cr-Commit-Position: refs/heads/master@{#748508}

content/browser/service_worker/embedded_worker_instance.cc

@@ -928,6 +928,7 @@ void EmbeddedWorkerInstance::OnScriptEvaluationStart() {
 
 void EmbeddedWorkerInstance::OnStarted(
     blink::mojom::ServiceWorkerStartStatus start_status,
+    bool has_fetch_handler,
     int thread_id,
     blink::mojom::EmbeddedWorkerStartTimingPtr start_timing) {
   TRACE_EVENT0("ServiceWorker", "EmbeddedWorkerInstance::OnStarted");
@@ -971,7 +972,7 @@ void EmbeddedWorkerInstance::OnStarted(
   thread_id_ = thread_id;
   inflight_start_task_.reset();
   for (auto& observer : listener_list_) {
-    observer.OnStarted(start_status);
+    observer.OnStarted(start_status, has_fetch_handler);
     // |this| may be destroyed here. Fortunately we know there is only one
     // observer in production code.
   }

content/browser/service_worker/embedded_worker_instance.h

@@ -92,7 +92,8 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
     virtual void OnRegisteredToDevToolsManager() {}
     virtual void OnStartWorkerMessageSent() {}
     virtual void OnScriptEvaluationStart() {}
-    virtual void OnStarted(blink::mojom::ServiceWorkerStartStatus status) {}
+    virtual void OnStarted(blink::mojom::ServiceWorkerStartStatus status,
+                           bool has_fetch_handler) {}
 
     // Called when status changed to STOPPING. The renderer has been sent a Stop
     // IPC message and OnStopped() will be called upon successful completion.
@@ -269,6 +270,7 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
   // Changes the internal worker status from STARTING to RUNNING.
   void OnStarted(
       blink::mojom::ServiceWorkerStartStatus status,
+      bool has_fetch_handler,
       int thread_id,
       blink::mojom::EmbeddedWorkerStartTimingPtr start_timing) override;
   // Resets the embedded worker instance to the initial state. Changes

content/browser/service_worker/embedded_worker_instance_unittest.cc

@@ -90,7 +90,9 @@ class EmbeddedWorkerInstanceTest : public testing::Test,
   void OnStartWorkerMessageSent() override {
     RecordEvent(START_WORKER_MESSAGE_SENT);
   }
-  void OnStarted(blink::mojom::ServiceWorkerStartStatus status) override {
+  void OnStarted(blink::mojom::ServiceWorkerStartStatus status,
+                 bool has_fetch_handler) override {
+    has_fetch_handler_ = has_fetch_handler;
     RecordEvent(STARTED, base::nullopt, status);
   }
   void OnStopped(EmbeddedWorkerStatus old_status) override {
@@ -219,6 +221,7 @@ class EmbeddedWorkerInstanceTest : public testing::Test,
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
   std::vector<EventLog> events_;
   base::test::ScopedFeatureList scoped_feature_list_;
+  bool has_fetch_handler_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(EmbeddedWorkerInstanceTest);
@@ -651,7 +654,7 @@ class AbruptCompletionInstanceClient : public FakeEmbeddedWorkerInstanceClient {
   void EvaluateScript() override {
     host()->OnScriptEvaluationStart();
     host()->OnStarted(blink::mojom::ServiceWorkerStartStatus::kAbruptCompletion,
-                      helper()->GetNextThreadId(),
+                      /*has_fetch_handler=*/false, helper()->GetNextThreadId(),
                       blink::mojom::EmbeddedWorkerStartTiming::New());
   }
 };
@@ -679,6 +682,62 @@ TEST_F(EmbeddedWorkerInstanceTest, AbruptCompletion) {
   worker->Stop();
 }
 
+// A fake instance client for toggling whether a fetch event handler exists.
+class FetchHandlerInstanceClient : public FakeEmbeddedWorkerInstanceClient {
+ public:
+  explicit FetchHandlerInstanceClient(EmbeddedWorkerTestHelper* helper)
+      : FakeEmbeddedWorkerInstanceClient(helper) {}
+  ~FetchHandlerInstanceClient() override = default;
+
+  void set_has_fetch_handler(bool has_fetch_handler) {
+    has_fetch_handler_ = has_fetch_handler;
+  }
+
+ protected:
+  void EvaluateScript() override {
+    host()->OnScriptEvaluationStart();
+    host()->OnStarted(blink::mojom::ServiceWorkerStartStatus::kNormalCompletion,
+                      has_fetch_handler_, helper()->GetNextThreadId(),
+                      blink::mojom::EmbeddedWorkerStartTiming::New());
+  }
+
+ private:
+  bool has_fetch_handler_ = false;
+};
+
+// Tests that whether a fetch event handler exists.
+TEST_F(EmbeddedWorkerInstanceTest, HasFetchHandler) {
+  const GURL scope("http://example.com/");
+  const GURL url("http://example.com/worker.js");
+  RegistrationAndVersionPair pair1 = PrepareRegistrationAndVersion(scope, url);
+  auto worker1 = std::make_unique<EmbeddedWorkerInstance>(pair1.second.get());
+  EXPECT_EQ(EmbeddedWorkerStatus::STOPPED, worker1->status());
+  worker1->AddObserver(this);
+
+  auto* fetch_handler_worker =
+      helper_->AddNewPendingInstanceClient<FetchHandlerInstanceClient>(
+          helper_.get());
+  fetch_handler_worker->set_has_fetch_handler(true);
+  StartWorker(worker1.get(), CreateStartParams(pair1.second));
+
+  EXPECT_TRUE(has_fetch_handler_);
+  worker1->Stop();
+
+  RegistrationAndVersionPair pair2 = PrepareRegistrationAndVersion(scope, url);
+  auto worker2 = std::make_unique<EmbeddedWorkerInstance>(pair2.second.get());
+  EXPECT_EQ(EmbeddedWorkerStatus::STOPPED, worker2->status());
+  worker2->AddObserver(this);
+
+  auto* no_fetch_handler_worker =
+      helper_->AddNewPendingInstanceClient<FetchHandlerInstanceClient>(
+          helper_.get());
+  no_fetch_handler_worker->set_has_fetch_handler(false);
+  StartWorker(worker2.get(), CreateStartParams(pair2.second));
+
+  EXPECT_FALSE(has_fetch_handler_);
+  worker2->Stop();
+}
+
 // Tests recording the lifetime UMA.
 TEST_F(EmbeddedWorkerInstanceTest, Lifetime) {
   const GURL scope("http://example.com/");

content/browser/service_worker/fake_embedded_worker_instance_client.cc

@@ -126,7 +126,7 @@ void FakeEmbeddedWorkerInstanceClient::CallOnConnectionError() {
 void FakeEmbeddedWorkerInstanceClient::EvaluateScript() {
   host_->OnScriptEvaluationStart();
   host_->OnStarted(blink::mojom::ServiceWorkerStartStatus::kNormalCompletion,
-                   helper_->GetNextThreadId(),
+                   true /* has_fetch_handler */, helper_->GetNextThreadId(),
                    blink::mojom::EmbeddedWorkerStartTiming::New());
 }
 

content/browser/service_worker/fake_service_worker.cc

@@ -77,8 +77,7 @@ void FakeServiceWorker::InitializeGlobalScope(
 
 void FakeServiceWorker::DispatchInstallEvent(
     DispatchInstallEventCallback callback) {
-  std::move(callback).Run(blink::mojom::ServiceWorkerEventStatus::COMPLETED,
-                          true /* has_fetch_handler */);
+  std::move(callback).Run(blink::mojom::ServiceWorkerEventStatus::COMPLETED);
 }
 
 void FakeServiceWorker::DispatchActivateEvent(

content/browser/service_worker/service_worker_context_unittest.cc

@@ -117,8 +117,7 @@ class InstallActivateWorker : public FakeServiceWorker {
     events_.emplace_back(ServiceWorkerMetrics::EventType::INSTALL);
     std::move(callback).Run(
         reject_install_ ? blink::mojom::ServiceWorkerEventStatus::REJECTED
-                        : blink::mojom::ServiceWorkerEventStatus::COMPLETED,
-        true /* has_fetch_handler */);
+                        : blink::mojom::ServiceWorkerEventStatus::COMPLETED);
   }
 
   void DispatchActivateEvent(

content/browser/service_worker/service_worker_job_unittest.cc

@@ -816,8 +816,8 @@ TEST_F(ServiceWorkerJobTest, UnregisterWaitingSetsRedundant) {
   ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk,
             StartServiceWorker(version.get()));
 
-  version->set_fetch_handler_existence(
-      ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
+  EXPECT_EQ(version->fetch_handler_existence(),
+            ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
   version->SetStatus(ServiceWorkerVersion::INSTALLED);
   registration->SetWaitingVersion(version);
   EXPECT_EQ(EmbeddedWorkerStatus::RUNNING, version->running_status());
@@ -1065,22 +1065,23 @@ TEST_F(ServiceWorkerJobTest, RegisterSameScriptMultipleTimesWhileUninstalling) {
   EXPECT_EQ(ServiceWorkerVersion::ACTIVATED, new_version->status());
 }
 
-// A fake service worker for toggling whether a fetch event handler exists.
-class FetchHandlerWorker : public FakeServiceWorker {
+// A fake instance client for toggling whether a fetch event handler exists.
+class FetchHandlerInstanceClient : public FakeEmbeddedWorkerInstanceClient {
  public:
-  FetchHandlerWorker(EmbeddedWorkerTestHelper* helper)
-      : FakeServiceWorker(helper) {}
-  ~FetchHandlerWorker() override = default;
+  explicit FetchHandlerInstanceClient(EmbeddedWorkerTestHelper* helper)
+      : FakeEmbeddedWorkerInstanceClient(helper) {}
+  ~FetchHandlerInstanceClient() override = default;
 
   void set_has_fetch_handler(bool has_fetch_handler) {
     has_fetch_handler_ = has_fetch_handler;
   }
 
-  void DispatchInstallEvent(
-      blink::mojom::ServiceWorker::DispatchInstallEventCallback callback)
-      override {
-    std::move(callback).Run(blink::mojom::ServiceWorkerEventStatus::COMPLETED,
-                            has_fetch_handler_);
+ protected:
+  void EvaluateScript() override {
+    host()->OnScriptEvaluationStart();
+    host()->OnStarted(blink::mojom::ServiceWorkerStartStatus::kNormalCompletion,
+                      has_fetch_handler_, helper()->GetNextThreadId(),
+                      blink::mojom::EmbeddedWorkerStartTiming::New());
   }
 
  private:
@@ -1094,7 +1095,8 @@ TEST_F(ServiceWorkerJobTest, HasFetchHandler) {
   scoped_refptr<ServiceWorkerRegistration> registration;
 
   auto* fetch_handler_worker =
-      helper_->AddNewPendingServiceWorker<FetchHandlerWorker>(helper_.get());
+      helper_->AddNewPendingInstanceClient<FetchHandlerInstanceClient>(
+          helper_.get());
   fetch_handler_worker->set_has_fetch_handler(true);
   RunRegisterJob(script, options);
   // Wait until the worker becomes active.
@@ -1105,7 +1107,8 @@ TEST_F(ServiceWorkerJobTest, HasFetchHandler) {
   RunUnregisterJob(options.scope);
 
   auto* no_fetch_handler_worker =
-      helper_->AddNewPendingServiceWorker<FetchHandlerWorker>(helper_.get());
+      helper_->AddNewPendingInstanceClient<FetchHandlerInstanceClient>(
+          helper_.get());
   no_fetch_handler_worker->set_has_fetch_handler(false);
   RunRegisterJob(script, options);
   // Wait until the worker becomes active.
@@ -1258,7 +1261,7 @@ class UpdateJobTestHelper : public EmbeddedWorkerTestHelper,
       host()->OnScriptEvaluationStart();
       host()->OnStarted(
           blink::mojom::ServiceWorkerStartStatus::kAbruptCompletion,
-          helper()->GetNextThreadId(),
+          /*has_fetch_handler=*/false, helper()->GetNextThreadId(),
           blink::mojom::EmbeddedWorkerStartTiming::New());
     }
 

content/browser/service_worker/service_worker_register_job.cc

@@ -612,8 +612,7 @@ void ServiceWorkerRegisterJob::DispatchInstallEvent(
 
 void ServiceWorkerRegisterJob::OnInstallFinished(
     int request_id,
-    blink::mojom::ServiceWorkerEventStatus event_status,
-    bool has_fetch_handler) {
+    blink::mojom::ServiceWorkerEventStatus event_status) {
   bool succeeded =
       event_status == blink::mojom::ServiceWorkerEventStatus::COMPLETED;
   new_version()->FinishRequest(request_id, succeeded);
@@ -629,10 +628,6 @@ void ServiceWorkerRegisterJob::OnInstallFinished(
 
   SetPhase(STORE);
   DCHECK(!registration()->last_update_check().is_null());
-  new_version()->set_fetch_handler_existence(
-      has_fetch_handler
-          ? ServiceWorkerVersion::FetchHandlerExistence::EXISTS
-          : ServiceWorkerVersion::FetchHandlerExistence::DOES_NOT_EXIST);
   context_->registry()->StoreRegistration(
       registration(), new_version(),
       base::BindOnce(&ServiceWorkerRegisterJob::OnStoreRegistrationComplete,

content/browser/service_worker/service_worker_register_job.h

@@ -146,8 +146,7 @@ class ServiceWorkerRegisterJob : public ServiceWorkerRegisterJobBase {
   void InstallAndContinue();
   void DispatchInstallEvent(blink::ServiceWorkerStatusCode start_worker_status);
   void OnInstallFinished(int request_id,
-                         blink::mojom::ServiceWorkerEventStatus event_status,
-                         bool has_fetch_handler);
+                         blink::mojom::ServiceWorkerEventStatus event_status);
   void OnInstallFailed(blink::ServiceWorkerStatusCode status);
   void Complete(blink::ServiceWorkerStatusCode status);
   void Complete(blink::ServiceWorkerStatusCode status,

content/browser/service_worker/service_worker_version.cc

@@ -1172,7 +1172,8 @@ void ServiceWorkerVersion::OnStarting() {
 }
 
 void ServiceWorkerVersion::OnStarted(
-    blink::mojom::ServiceWorkerStartStatus start_status) {
+    blink::mojom::ServiceWorkerStartStatus start_status,
+    bool has_fetch_handler) {
   DCHECK_EQ(EmbeddedWorkerStatus::RUNNING, running_status());
 
   // TODO(falken): This maps kAbruptCompletion to kErrorScriptEvaluated, which
@@ -1183,6 +1184,12 @@ void ServiceWorkerVersion::OnStarted(
   blink::ServiceWorkerStatusCode status =
       mojo::ConvertTo<blink::ServiceWorkerStatusCode>(start_status);
 
+  if (status == blink::ServiceWorkerStatusCode::kOk &&
+      fetch_handler_existence_ == FetchHandlerExistence::UNKNOWN)
+    set_fetch_handler_existence(has_fetch_handler
+                                    ? FetchHandlerExistence::EXISTS
+                                    : FetchHandlerExistence::DOES_NOT_EXIST);
+
   // Fire all start callbacks.
   scoped_refptr<ServiceWorkerVersion> protect(this);
   FinishStartWorker(status);

content/browser/service_worker/service_worker_version.h

@@ -731,7 +731,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
   // EmbeddedWorkerInstance::Listener overrides:
   void OnScriptEvaluationStart() override;
   void OnStarting() override;
-  void OnStarted(blink::mojom::ServiceWorkerStartStatus status) override;
+  void OnStarted(blink::mojom::ServiceWorkerStartStatus status,
+                 bool has_fetch_handler) override;
   void OnStopping() override;
   void OnStopped(EmbeddedWorkerStatus old_status) override;
   void OnDetached(EmbeddedWorkerStatus old_status) override;

content/browser/service_worker/service_worker_version_browsertest.cc

@@ -628,15 +628,10 @@ class ServiceWorkerVersionBrowserTest : public ContentBrowserTest {
       base::OnceClosure done,
       base::Optional<blink::ServiceWorkerStatusCode>* out_result,
       int request_id,
-      blink::mojom::ServiceWorkerEventStatus status,
-      bool has_fetch_handler) {
+      blink::mojom::ServiceWorkerEventStatus status) {
     version_->FinishRequest(
         request_id,
         status == blink::mojom::ServiceWorkerEventStatus::COMPLETED);
-    version_->set_fetch_handler_existence(
-        has_fetch_handler
-            ? ServiceWorkerVersion::FetchHandlerExistence::EXISTS
-            : ServiceWorkerVersion::FetchHandlerExistence::DOES_NOT_EXIST);
 
     *out_result = mojo::ConvertTo<blink::ServiceWorkerStatusCode>(status);
     if (!done.is_null())
@@ -1091,6 +1086,17 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest,
             version_->fetch_handler_existence());
 }
 
+// Check that fetch event handler added in the install event should result in a
+// service worker that doesn't count as having a fetch event handler.
+IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest,
+                       FetchHandlerSetInInstallEvent) {
+  StartServerAndNavigateToSetup();
+  InstallTestHelper("/service_worker/fetch_event_set_in_install_event.js",
+                    blink::ServiceWorkerStatusCode::kOk);
+  EXPECT_EQ(ServiceWorkerVersion::FetchHandlerExistence::DOES_NOT_EXIST,
+            version_->fetch_handler_existence());
+}
+
 // Check that ServiceWorker script requests set a "Service-Worker: script"
 // header.
 IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest,

content/renderer/service_worker/service_worker_context_client.cc

@@ -466,7 +466,8 @@ void ServiceWorkerContextClient::SendWorkerStarted(
   CHECK_LE(start_timing_->script_evaluation_start_time,
            start_timing_->script_evaluation_end_time);
 
-  instance_host_->OnStarted(status, WorkerThread::GetCurrentId(),
+  instance_host_->OnStarted(status, proxy_->HasFetchHandler(),
+                            WorkerThread::GetCurrentId(),
                             std::move(start_timing_));
 
   TRACE_EVENT_NESTABLE_ASYNC_END0("ServiceWorker", "ServiceWorkerContextClient",

content/test/data/service_worker/fetch_event_set_in_install_event.js

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+this.oninstall = function(event){
+    this.onfetch = function(fetchevent) {
+        var headers = new Headers;
+        headers.set('Content-Language', 'fi');
+        headers.set('Content-Type', 'text/html; charset=UTF-8');
+        var blob = new Blob(["This resource is gone. Gone, gone, gone."]);
+        var response = new Response(blob, {
+            status: 301,
+            statusText: 'Moved Permanently',
+            headers: headers
+        });
+
+        fetchevent.respondWith(response);
+    };
+};

content/test/data/service_worker/fetch_event_set_in_install_event.js.mock-http-headers

+HTTP/1.1 200 OK
+Content-Type: text/javascript

third_party/blink/public/mojom/service_worker/embedded_worker.mojom

@@ -186,8 +186,10 @@ interface EmbeddedWorkerInstanceHost {
   // for the browser process to start enforcing timeouts on script execution.
   OnScriptEvaluationStart();
   // Indicates that the worker has started. |thread_id| is the platform
-  // thread id the worker runs on.
+  // thread id the worker runs on. |has_fetch_handler| is true when the fetch
+  // event handler exists after initial script evaluation.
   OnStarted(ServiceWorkerStartStatus status,
+            bool has_fetch_handler,
             int32 thread_id,
             EmbeddedWorkerStartTiming start_timing);
 

third_party/blink/public/mojom/service_worker/service_worker.mojom

@@ -187,8 +187,10 @@ interface ServiceWorker {
       FetchHandlerExistence fetch_handler_existence,
       URLLoaderFactoryBundle? subresource_loader_factories);
 
+  // These methods dispatch InstallEvent and ActivateEvent to the
+  // ServiceWorkerGlobalScope.
   DispatchInstallEvent()
-      => (ServiceWorkerEventStatus status, bool has_fetch_handler);
+      => (ServiceWorkerEventStatus status);
   DispatchActivateEvent()
       => (ServiceWorkerEventStatus status);
 

third_party/blink/public/web/modules/service_worker/web_service_worker_context_proxy.h

@@ -71,6 +71,7 @@ class WebServiceWorkerContextProxy {
   virtual bool IsWindowInteractionAllowed() = 0;
   virtual void PauseEvaluation() = 0;
   virtual void ResumeEvaluation() = 0;
+  virtual bool HasFetchHandler() = 0;
 };
 
 }  // namespace blink

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

@@ -858,8 +858,7 @@ void ServiceWorkerGlobalScope::DidHandleInstallEvent(
                           TRACE_ID_LOCAL(install_event_id)),
       TRACE_EVENT_FLAG_FLOW_IN, "status", MojoEnumToString(status));
   RunEventCallback(&install_event_callbacks_, event_queue_.get(),
-                   install_event_id, status,
-                   HasEventListeners(event_type_names::kFetch));
+                   install_event_id, status);
 }
 
 void ServiceWorkerGlobalScope::DidHandleActivateEvent(
@@ -1583,9 +1582,7 @@ void ServiceWorkerGlobalScope::DispatchInstallEvent(
   event_queue_->EnqueueNormal(
       WTF::Bind(&ServiceWorkerGlobalScope::StartInstallEvent,
                 WrapWeakPersistent(this), std::move(callback)),
-      CreateAbortCallback(&install_event_callbacks_,
-                          false /* has_fetch_handler */),
-      base::nullopt);
+      CreateAbortCallback(&install_event_callbacks_), base::nullopt);
 }
 
 void ServiceWorkerGlobalScope::StartInstallEvent(

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

@@ -324,6 +324,10 @@ void ServiceWorkerGlobalScopeProxy::ResumeEvaluation() {
   WorkerGlobalScope()->ResumeEvaluation();
 }
 
+bool ServiceWorkerGlobalScopeProxy::HasFetchHandler() {
+  return WorkerGlobalScope()->HasEventListeners(event_type_names::kFetch);
+}
+
 WebServiceWorkerContextClient& ServiceWorkerGlobalScopeProxy::Client() const {
   DCHECK(client_);
   return *client_;

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

@@ -93,6 +93,7 @@ class ServiceWorkerGlobalScopeProxy final : public WebServiceWorkerContextProxy,
   bool IsWindowInteractionAllowed() override;
   void PauseEvaluation() override;
   void ResumeEvaluation() override;
+  bool HasFetchHandler() override;
 
   // WorkerReportingProxy overrides:
   void CountFeature(WebFeature) override;