service worker: Return a status for the *ExternalRequest methods.

The caller-side in extensions may want to take different actions
depending on how things failed. Per discussion at:
https://chromium-review.googlesource.com/c/chromium/src/+/1763560

There is no behavioral change yet except that the Stop* function no
longer fails if the worker is STOPPING. There's no reason to fail there,
we can just finish the request as requested.

Bug: 999027, 824858
Change-Id: Ib04cd671e99af6afd082562d13c9677772894c26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1792518
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695382}

content/browser/service_worker/service_worker_context_wrapper.cc

@@ -475,29 +475,31 @@ void ServiceWorkerContextWrapper::UnregisterServiceWorker(
       base::BindOnce(&FinishUnregistrationOnCoreThread, std::move(callback)));
 }
 
-bool ServiceWorkerContextWrapper::StartingExternalRequest(
+ServiceWorkerExternalRequestResult
+ServiceWorkerContextWrapper::StartingExternalRequest(
     int64_t service_worker_version_id,
     const std::string& request_uuid) {
   DCHECK_CURRENTLY_ON(GetCoreThreadId());
   if (!context())
-    return false;
+    return ServiceWorkerExternalRequestResult::kNullContext;
   ServiceWorkerVersion* version =
       context()->GetLiveVersion(service_worker_version_id);
   if (!version)
-    return false;
+    return ServiceWorkerExternalRequestResult::kWorkerNotFound;
   return version->StartExternalRequest(request_uuid);
 }
 
-bool ServiceWorkerContextWrapper::FinishedExternalRequest(
+ServiceWorkerExternalRequestResult
+ServiceWorkerContextWrapper::FinishedExternalRequest(
     int64_t service_worker_version_id,
     const std::string& request_uuid) {
   DCHECK_CURRENTLY_ON(GetCoreThreadId());
   if (!context())
-    return false;
+    return ServiceWorkerExternalRequestResult::kNullContext;
   ServiceWorkerVersion* version =
       context()->GetLiveVersion(service_worker_version_id);
   if (!version)
-    return false;
+    return ServiceWorkerExternalRequestResult::kWorkerNotFound;
   return version->FinishExternalRequest(request_uuid);
 }
 

content/browser/service_worker/service_worker_context_wrapper.h

@@ -136,10 +136,12 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
       ResultCallback callback) override;
   void UnregisterServiceWorker(const GURL& scope,
                                ResultCallback callback) override;
-  bool StartingExternalRequest(int64_t service_worker_version_id,
-                               const std::string& request_uuid) override;
-  bool FinishedExternalRequest(int64_t service_worker_version_id,
-                               const std::string& request_uuid) override;
+  ServiceWorkerExternalRequestResult StartingExternalRequest(
+      int64_t service_worker_version_id,
+      const std::string& request_uuid) override;
+  ServiceWorkerExternalRequestResult FinishedExternalRequest(
+      int64_t service_worker_version_id,
+      const std::string& request_uuid) override;
   void CountExternalRequestsForTest(
       const GURL& url,
       CountExternalRequestsCallback callback) override;

content/browser/service_worker/service_worker_version.cc

@@ -37,6 +37,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/page_navigator.h"
+#include "content/public/browser/service_worker_external_request_result.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/result_codes.h"
 #include "net/http/http_response_headers.h"
@@ -347,9 +348,9 @@ void ServiceWorkerVersion::SetStatus(Status status) {
   for (auto& callback : callbacks)
     std::move(callback).Run();
 
-  if (status == INSTALLED)
+  if (status == INSTALLED) {
     embedded_worker_->OnWorkerVersionInstalled();
-  else if (status == REDUNDANT) {
+  } else if (status == REDUNDANT) {
     embedded_worker_->OnWorkerVersionDoomed();
 
     // TODO(crbug.com/951571): Remove this once we figured out the cause of
@@ -621,25 +622,28 @@ int ServiceWorkerVersion::StartRequestWithCustomTimeout(
   return request_id;
 }
 
-bool ServiceWorkerVersion::StartExternalRequest(
+ServiceWorkerExternalRequestResult ServiceWorkerVersion::StartExternalRequest(
     const std::string& request_uuid) {
-  if (running_status() == EmbeddedWorkerStatus::STARTING)
-    return pending_external_requests_.insert(request_uuid).second;
+  if (running_status() == EmbeddedWorkerStatus::STARTING) {
+    return pending_external_requests_.insert(request_uuid).second
+               ? ServiceWorkerExternalRequestResult::kOk
+               : ServiceWorkerExternalRequestResult::kBadRequestId;
+  }
 
-  // It's possible that the renderer is lying or the version started stopping
-  // right around the time of the IPC.
-  if (running_status() != EmbeddedWorkerStatus::RUNNING)
-    return false;
+  if (running_status() == EmbeddedWorkerStatus::STOPPING ||
+      running_status() == EmbeddedWorkerStatus::STOPPED) {
+    return ServiceWorkerExternalRequestResult::kWorkerNotRunning;
+  }
 
   if (external_request_uuid_to_request_id_.count(request_uuid) > 0u)
-    return false;
+    return ServiceWorkerExternalRequestResult::kBadRequestId;
 
   int request_id =
       StartRequest(ServiceWorkerMetrics::EventType::EXTERNAL_REQUEST,
                    base::BindOnce(&ServiceWorkerVersion::CleanUpExternalRequest,
                                   this, request_uuid));
   external_request_uuid_to_request_id_[request_uuid] = request_id;
-  return true;
+  return ServiceWorkerExternalRequestResult::kOk;
 }
 
 bool ServiceWorkerVersion::FinishRequest(int request_id, bool was_handled) {
@@ -660,27 +664,34 @@ bool ServiceWorkerVersion::FinishRequest(int request_id, bool was_handled) {
   return true;
 }
 
-bool ServiceWorkerVersion::FinishExternalRequest(
+ServiceWorkerExternalRequestResult ServiceWorkerVersion::FinishExternalRequest(
     const std::string& request_uuid) {
   if (running_status() == EmbeddedWorkerStatus::STARTING)
-    return pending_external_requests_.erase(request_uuid) > 0u;
+    return pending_external_requests_.erase(request_uuid) > 0u
+               ? ServiceWorkerExternalRequestResult::kOk
+               : ServiceWorkerExternalRequestResult::kBadRequestId;
 
-  // It's possible that the renderer is lying or the version started stopping
-  // right around the time of the IPC.
-  if (running_status() != EmbeddedWorkerStatus::RUNNING)
-    return false;
+  // If it's STOPPED, there is no request to finish. We could just consider this
+  // a success, but the caller may want to know about it. (If it's STOPPING,
+  // proceed with finishing the request as normal.)
+  if (running_status() == EmbeddedWorkerStatus::STOPPED)
+    return ServiceWorkerExternalRequestResult::kWorkerNotRunning;
 
   auto iter = external_request_uuid_to_request_id_.find(request_uuid);
   if (iter != external_request_uuid_to_request_id_.end()) {
     int request_id = iter->second;
     external_request_uuid_to_request_id_.erase(iter);
-    return FinishRequest(request_id, true);
+    return FinishRequest(request_id, true)
+               ? ServiceWorkerExternalRequestResult::kOk
+               : ServiceWorkerExternalRequestResult::kBadRequestId;
   }
 
   // It is possible that the request was cancelled or timed out before and we
-  // won't find it in |external_request_uuid_to_request_id_|.
-  // Return true so we don't kill the process.
-  return true;
+  // won't find it in |external_request_uuid_to_request_id_|. Just return
+  // kOk.
+  // TODO(falken): Consider keeping track of these so we can return
+  // kBadRequestId for invalid requests ids.
+  return ServiceWorkerExternalRequestResult::kOk;
 }
 
 ServiceWorkerVersion::SimpleEventCallback

content/browser/service_worker/service_worker_version.h

@@ -311,7 +311,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
   // Provides a mechanism to external clients to keep the worker running.
   // |request_uuid| is a GUID for clients to identify the request.
   // Returns true if the request was successfully scheduled to starrt.
-  bool StartExternalRequest(const std::string& request_uuid);
+  ServiceWorkerExternalRequestResult StartExternalRequest(
+      const std::string& request_uuid);
 
   // Informs ServiceWorkerVersion that an event has finished being dispatched.
   // Returns false if no inflight requests with the provided id exist, for
@@ -322,9 +323,8 @@ class CONTENT_EXPORT ServiceWorkerVersion
   bool FinishRequest(int request_id, bool was_handled);
 
   // Finishes an external request that was started by StartExternalRequest().
-  // Returns false if there was an error finishing the request: e.g. the request
-  // was not found or the worker already terminated.
-  bool FinishExternalRequest(const std::string& request_uuid);
+  ServiceWorkerExternalRequestResult FinishExternalRequest(
+      const std::string& request_uuid);
 
   // Creates a callback that is to be used for marking simple events dispatched
   // through blink::mojom::ServiceWorker as finished for the |request_id|.

content/public/browser/BUILD.gn

@@ -302,6 +302,7 @@ jumbo_source_set("browser_sources") {
     "service_process_info.h",
     "service_worker_context.h",
     "service_worker_context_observer.h",
+    "service_worker_external_request_result.h",
     "service_worker_running_info.h",
     "session_storage_namespace.h",
     "session_storage_usage_info.h",

content/public/browser/service_worker_context.h

@@ -12,6 +12,7 @@
 #include "base/callback_forward.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/service_worker_external_request_result.h"
 #include "content/public/browser/service_worker_running_info.h"
 #include "third_party/blink/public/mojom/service_worker/service_worker_registration.mojom-forward.h"
 #include "url/gurl.h"
@@ -131,18 +132,21 @@ class CONTENT_EXPORT ServiceWorkerContext {
 
   // Mechanism for embedder to increment/decrement ref count of a service
   // worker.
+  //
   // Embedders can call StartingExternalRequest() while it is performing some
   // work with the worker. The worker is considered to be working until embedder
   // calls FinishedExternalRequest(). This ensures that content/ does not
   // shut the worker down while embedder is expecting the worker to be kept
   // alive.
   //
-  // Must be called from the core thread. Returns whether or not changing the
-  // ref count succeeded.
-  virtual bool StartingExternalRequest(int64_t service_worker_version_id,
-                                       const std::string& request_uuid) = 0;
-  virtual bool FinishedExternalRequest(int64_t service_worker_version_id,
-                                       const std::string& request_uuid) = 0;
+  // Must be called from the core thread.
+  virtual ServiceWorkerExternalRequestResult StartingExternalRequest(
+      int64_t service_worker_version_id,
+      const std::string& request_uuid) = 0;
+  virtual ServiceWorkerExternalRequestResult FinishedExternalRequest(
+      int64_t service_worker_version_id,
+      const std::string& request_uuid) = 0;
+
   // Returns the pending external request count for the worker with the
   // specified |origin| via |callback|. Must be called from the UI thread. The
   // callback is called on the UI thread.

content/public/browser/service_worker_external_request_result.h

+// Copyright 2019 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.
+
+#ifndef CONTENT_PUBLIC_BROWSER_SERVICE_WORKER_EXTERNAL_REQUEST_RESULT_H_
+#define CONTENT_PUBLIC_BROWSER_SERVICE_WORKER_EXTERNAL_REQUEST_RESULT_H_
+
+namespace content {
+
+// The result of adjusting the ref count of a service worker by an embedder,
+// which keeps the service worker running.
+enum class ServiceWorkerExternalRequestResult {
+  // The ref count was adjusted successfully.
+  kOk,
+
+  // Error cases (the ref count did not change):
+  // There is already an outstanding request with the given id (if
+  // incrementing), or there was not an outstanding request with that id (if
+  // decrementing).
+  kBadRequestId,
+  // The worker is already stopping or stopped (if incrementing), or
+  // is already stopped (if decrementing).
+  kWorkerNotRunning,
+  // The worker with the given version id doesn't exist or is not live.
+  kWorkerNotFound,
+  // The core context inside ServiceWorkerContext has been destroyed.
+  // This can happen during init or shutdown or a fatal storage error
+  // occurred and the context is being reinitialized.
+  kNullContext,
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_SERVICE_WORKER_EXTERNAL_REQUEST_RESULT_H_

content/public/test/fake_service_worker_context.cc

@@ -3,10 +3,12 @@
 // found in the LICENSE file.
 
 #include "content/public/test/fake_service_worker_context.h"
-#include "content/public/browser/service_worker_context_observer.h"
+
+#include <utility>
 
 #include "base/callback.h"
 #include "base/logging.h"
+#include "content/public/browser/service_worker_context_observer.h"
 #include "third_party/blink/public/common/messaging/transferable_message.h"
 
 namespace content {
@@ -33,17 +35,19 @@ void FakeServiceWorkerContext::UnregisterServiceWorker(
     ResultCallback callback) {
   NOTREACHED();
 }
-bool FakeServiceWorkerContext::StartingExternalRequest(
+ServiceWorkerExternalRequestResult
+FakeServiceWorkerContext::StartingExternalRequest(
     int64_t service_worker_version_id,
     const std::string& request_uuid) {
   NOTREACHED();
-  return false;
+  return ServiceWorkerExternalRequestResult::kWorkerNotFound;
 }
-bool FakeServiceWorkerContext::FinishedExternalRequest(
+ServiceWorkerExternalRequestResult
+FakeServiceWorkerContext::FinishedExternalRequest(
     int64_t service_worker_version_id,
     const std::string& request_uuid) {
   NOTREACHED();
-  return false;
+  return ServiceWorkerExternalRequestResult::kWorkerNotFound;
 }
 void FakeServiceWorkerContext::CountExternalRequestsForTest(
     const GURL& url,

content/public/test/fake_service_worker_context.h

@@ -7,6 +7,7 @@
 
 #include <string>
 #include <tuple>
+#include <vector>
 
 #include "base/callback_forward.h"
 #include "base/observer_list.h"
@@ -38,10 +39,12 @@ class FakeServiceWorkerContext : public ServiceWorkerContext {
       ResultCallback callback) override;
   void UnregisterServiceWorker(const GURL& scope,
                                ResultCallback callback) override;
-  bool StartingExternalRequest(int64_t service_worker_version_id,
-                               const std::string& request_uuid) override;
-  bool FinishedExternalRequest(int64_t service_worker_version_id,
-                               const std::string& request_uuid) override;
+  ServiceWorkerExternalRequestResult StartingExternalRequest(
+      int64_t service_worker_version_id,
+      const std::string& request_uuid) override;
+  ServiceWorkerExternalRequestResult FinishedExternalRequest(
+      int64_t service_worker_version_id,
+      const std::string& request_uuid) override;
   void CountExternalRequestsForTest(
       const GURL& url,
       CountExternalRequestsCallback callback) override;

extensions/browser/events/event_ack_data.cc

@@ -4,6 +4,9 @@
 
 #include "extensions/browser/events/event_ack_data.h"
 
+#include <string>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/guid.h"
@@ -11,6 +14,7 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/service_worker_context.h"
+#include "content/public/browser/service_worker_external_request_result.h"
 
 namespace extensions {
 
@@ -51,8 +55,10 @@ void EventAckData::StartExternalRequestOnIO(
 
   std::string request_uuid = base::GenerateGUID();
 
-  if (!context->StartingExternalRequest(version_id, request_uuid)) {
-    LOG(ERROR) << "StartExternalRequest failed";
+  content::ServiceWorkerExternalRequestResult result =
+      context->StartingExternalRequest(version_id, request_uuid);
+  if (result != content::ServiceWorkerExternalRequestResult::kOk) {
+    LOG(ERROR) << "StartExternalRequest failed: " << static_cast<int>(result);
     return;
   }
 
@@ -85,8 +91,12 @@ void EventAckData::FinishExternalRequestOnIO(
   std::string request_uuid = std::move(request_info_iter->second.first);
   unacked_events_map.erase(request_info_iter);
 
-  if (!context->FinishedExternalRequest(version_id, request_uuid))
+  content::ServiceWorkerExternalRequestResult result =
+      context->FinishedExternalRequest(version_id, request_uuid);
+  if (result != content::ServiceWorkerExternalRequestResult::kOk) {
+    LOG(ERROR) << "FinishExternalRequest failed: " << static_cast<int>(result);
     std::move(failure_callback).Run();
+  }
 }
 
 EventAckData::EventAckData()

extensions/browser/extension_service_worker_message_filter.cc

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "content/public/browser/service_worker_context.h"
+#include "content/public/browser/service_worker_external_request_result.h"
 #include "extensions/browser/bad_message.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/events/event_ack_data.h"
@@ -85,10 +86,13 @@ void ExtensionServiceWorkerMessageFilter::OnDecrementServiceWorkerActivity(
     int64_t service_worker_version_id,
     const std::string& request_uuid) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  bool status = service_worker_context_->FinishedExternalRequest(
-      service_worker_version_id, request_uuid);
-  if (!status)
-    LOG(ERROR) << "ServiceWorkerContext::FinishedExternalRequest failed.";
+  content::ServiceWorkerExternalRequestResult result =
+      service_worker_context_->FinishedExternalRequest(
+          service_worker_version_id, request_uuid);
+  if (result != content::ServiceWorkerExternalRequestResult::kOk) {
+    LOG(ERROR) << "ServiceWorkerContext::FinishedExternalRequest failed: "
+               << static_cast<int>(result);
+  }
   bool erased = active_request_uuids_.erase(request_uuid) == 1;
   // The worker may have already stopped before we got here, so only report
   // a bad message if we didn't have an increment for the UUID.

extensions/browser/extension_service_worker_message_filter.h

@@ -5,6 +5,7 @@
 #ifndef EXTENSIONS_BROWSER_EXTENSION_SERVICE_WORKER_MESSAGE_FILTER_H_
 #define EXTENSIONS_BROWSER_EXTENSION_SERVICE_WORKER_MESSAGE_FILTER_H_
 
+#include <string>
 #include <unordered_set>
 
 #include "base/macros.h"

extensions/browser/process_manager.cc

@@ -29,6 +29,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/service_worker_context.h"
+#include "content/public/browser/service_worker_external_request_result.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
@@ -136,9 +137,9 @@ void FinishServiceWorkerExternalRequest(content::ServiceWorkerContext* context,
                                         int64_t service_worker_version_id,
                                         const std::string& request_uuid) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  bool status =
+  content::ServiceWorkerExternalRequestResult result =
       context->FinishedExternalRequest(service_worker_version_id, request_uuid);
-  DCHECK(status);
+  DCHECK_EQ(result, content::ServiceWorkerExternalRequestResult::kOk);
 }
 
 }  // namespace