Fix crashes when starting a worker fails during dispatching fetch event

ServiceWorkerFetchDispatcher now dispatches a fetch event without
waiting for an ACK of start worker message when ServiceWorkerOnUI is
enabled. However, due to the change, ServiceWorkerFetchDispatcher can
invoke a callback twice when starting a worker fails - one for a failure
of start worker and the other for a failure of dispatched event, and
this caused crashes.

This CL added a branch to ignore the latter callback when starting a
worker fails because the error status for starting a worker is more
detailed.

Bug: 1106977
Change-Id: I4cc408a850f40fb3db101584e71eb2164a90c28f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2309525Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790308}

content/browser/service_worker/service_worker_browsertest.cc

@@ -1037,6 +1037,84 @@ IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest,
   EXPECT_EQ(301, fetch_response->status_code);
 }
 
+// Check if a fetch event can be failed without crashing if starting a service
+// worker fails. This is a regression test for https://crbug.com/1106977.
+IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest,
+                       DispatchFetchEventToBrokenWorker) {
+  // Setup the server so that the test doesn't crash when tearing down.
+  StartServerAndNavigateToSetup();
+  // This test is meaningful only when ServiceWorkerOnUI is enabled.
+  if (!ServiceWorkerContext::IsServiceWorkerOnUIEnabled())
+    return;
+
+  WorkerRunningStatusObserver observer(public_context());
+  EXPECT_TRUE(NavigateToURL(shell(),
+                            embedded_test_server()->GetURL(
+                                "/service_worker/create_service_worker.html")));
+  EXPECT_EQ("DONE", EvalJs(shell(), "register('fetch_event.js');"));
+  observer.WaitUntilRunning();
+
+  ASSERT_TRUE(
+      BrowserThread::CurrentlyOn(ServiceWorkerContext::GetCoreThreadId()));
+  scoped_refptr<ServiceWorkerVersion> version =
+      wrapper()->GetLiveVersion(observer.version_id());
+  EXPECT_EQ(EmbeddedWorkerStatus::RUNNING, version->running_status());
+
+  {
+    base::RunLoop loop;
+    version->StopWorker(loop.QuitClosure());
+    loop.Run();
+    EXPECT_EQ(EmbeddedWorkerStatus::STOPPED, version->running_status());
+  }
+
+  // Set a non-existent resource to the version.
+  std::vector<storage::mojom::ServiceWorkerResourceRecordPtr> resources;
+  resources.push_back(storage::mojom::ServiceWorkerResourceRecord::New(
+      123456789, version->script_url(), 100));
+  version->script_cache_map()->resource_map_.clear();
+  version->script_cache_map()->SetResources(resources);
+
+  bool is_prepare_callback_called = false;
+  base::RunLoop fetch_loop;
+  blink::ServiceWorkerStatusCode fetch_status;
+  ServiceWorkerFetchDispatcher::FetchEventResult fetch_result;
+
+  auto request = blink::mojom::FetchAPIRequest::New();
+  request->url = embedded_test_server()->GetURL("/service_worker/in-scope");
+  request->method = "GET";
+  request->is_main_resource_load = true;
+  auto dispatcher = std::make_unique<ServiceWorkerFetchDispatcher>(
+      std::move(request), blink::mojom::ResourceType::kMainFrame,
+      /*client_id=*/base::GenerateGUID(), version,
+      base::BindLambdaForTesting([&]() { is_prepare_callback_called = true; }),
+      base::BindLambdaForTesting(
+          [&](blink::ServiceWorkerStatusCode status,
+              ServiceWorkerFetchDispatcher::FetchEventResult result,
+              blink::mojom::FetchAPIResponsePtr response,
+              blink::mojom::ServiceWorkerStreamHandlePtr,
+              blink::mojom::ServiceWorkerFetchEventTimingPtr,
+              scoped_refptr<ServiceWorkerVersion>) {
+            fetch_status = status;
+            fetch_result = result;
+            fetch_loop.Quit();
+          }),
+      /*is_offline_capability_check=*/false);
+
+  // DispatchFetchEvent is called synchronously with dispatcher->Run() even if
+  // the worker is stopped.
+  dispatcher->Run();
+  EXPECT_TRUE(is_prepare_callback_called);
+
+  // Check if the fetch event fails due to error of reading the resource.
+  fetch_loop.Run();
+  EXPECT_EQ(blink::ServiceWorkerStatusCode::kErrorDiskCache, fetch_status);
+  EXPECT_EQ(ServiceWorkerFetchDispatcher::FetchEventResult::kShouldFallback,
+            fetch_result);
+
+  // Make sure that no crash happens in the remaining tasks.
+  base::RunLoop().RunUntilIdle();
+}
+
 class ServiceWorkerEagerCacheStorageSetupTest
     : public ServiceWorkerBrowserTest {
  public:

content/browser/service_worker/service_worker_fetch_dispatcher.cc

@@ -625,6 +625,10 @@ void ServiceWorkerFetchDispatcher::DispatchFetchEvent() {
 void ServiceWorkerFetchDispatcher::DidFailToDispatch(
     std::unique_ptr<ResponseCallback> response_callback,
     blink::ServiceWorkerStatusCode status) {
+  // Fetch dispatcher can be completed at this point due to a failure of
+  // starting up a worker. In that case, let's simply ignore it.
+  if (IsCompleted())
+    return;
   DidFail(status);
 }
 
@@ -772,6 +776,10 @@ bool ServiceWorkerFetchDispatcher::IsEventDispatched() const {
   return request_.is_null();
 }
 
+bool ServiceWorkerFetchDispatcher::IsCompleted() const {
+  return fetch_callback_.is_null();
+}
+
 // static
 void ServiceWorkerFetchDispatcher::OnFetchEventFinished(
     ServiceWorkerVersion* version,

content/browser/service_worker/service_worker_fetch_dispatcher.h

@@ -110,6 +110,7 @@ class CONTENT_EXPORT ServiceWorkerFetchDispatcher {
   ServiceWorkerMetrics::EventType GetEventType() const;
 
   bool IsEventDispatched() const;
+  bool IsCompleted() const;
 
   blink::mojom::FetchAPIRequestPtr request_;
   std::string client_id_;

content/browser/service_worker/service_worker_script_cache_map.h

@@ -74,6 +74,8 @@ class CONTENT_EXPORT ServiceWorkerScriptCacheMap {
   friend class ServiceWorkerVersion;
   friend class ServiceWorkerVersionBrowserTest;
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerReadFromCacheJobTest, ResourceNotFound);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerBrowserTest,
+                           DispatchFetchEventToBrokenWorker);
 
   ServiceWorkerScriptCacheMap(
       ServiceWorkerVersion* owner,