[Extensions] Simplify ServiceWorker keepalive counts for API calls

When an extension API is called from an ephemeral context like a service worker (or in MV2, an event page), we increment the keepalive count for the ephemeral context. We then decrement that keepalive count when the API call is complete. For event page contexts, we handle this entirely in the browser context in response to the API call coming in. For service workers, we currently have two separate IPC messages: one for the API call and a second to increment the keepalive. We similarly have an "ack" in the renderer that responds to the API call completion and sends the message to decrement the keepalive. At the time, this was done because it was simpler than incorporating the keepalive logic into the API call message (as it is for the event page case); this was in part due to service worker management previously running on the IO thread. This is no longer the case, and its simpler now to simply tie this into the browser receiving the API call. Do this, and have the browser handle incrementing and decrementing the keepalive count directly in response to the extension API call. This has the following advantages: - Significantly less code overall - Fewer IPC messsages - More similar code paths for event pages + service workers - The service worker keepalive count now uses the (more common) ProcessManager::[In|De]crementServiceWorkerKeepaliveCount() methods, which provide more information in debugging and make it easier to audit callsites. As part of this, we also add a UUID to ExtensionFunctions. This is used to track the request in the service worker layer. In the future, we can use this in lieu of the existing integer request ID. Bug: 1423190, 1444561 Change-Id: I249a4de1a85b74a125f9321cbaf32fc85b55a3ac Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4518520 Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: David Bertoni <dbertoni@chromium.org> Commit-Queue: Devlin Cronin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/main@{#1154102}
chromium · Jun 6, 2023 · bac8ba6 · bac8ba6
1 parent 16ffa83
commit bac8ba6
Show file tree

Hide file tree

Showing 15 changed files with 77 additions and 170 deletions.
diff --git a/extensions/browser/bad_message.h b/extensions/browser/bad_message.h
@@ -45,8 +45,7 @@ enum BadMessageReason {
   AVG_BAD_INST_ID = 4,
   AVG_BAD_EXT_ID = 5,
   OBSOLETE_AVG_NULL_AVG = 6,
-  // Invalid decrement of an Extensions SW ref count.
-  ESWMF_INVALID_DECREMENT_ACTIVITY = 7,
+  // DEPRECATED_ESWMF_INVALID_DECREMENT_ACTIVITY = 7,
   EFD_BAD_MESSAGE = 8,
   EFD_BAD_MESSAGE_WORKER = 9,
   WVG_PARTITION_ID_NOT_UTF8 = 10,

diff --git a/extensions/browser/extension_function.cc b/extensions/browser/extension_function.cc
@@ -363,8 +363,7 @@ ExtensionFunction::~ExtensionFunction() {
         name());
   }
   if (dispatcher() && (render_frame_host() || is_from_service_worker())) {
-    dispatcher()->OnExtensionFunctionCompleted(
-        extension(), is_from_service_worker(), name());
+    dispatcher()->OnExtensionFunctionCompleted(*this);
   }
 
 // The extension function should always respond to avoid leaks in the

diff --git a/extensions/browser/extension_function.h b/extensions/browser/extension_function.h
@@ -23,6 +23,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_function_histogram_value.h"
 #include "extensions/browser/quota_service.h"
+#include "extensions/browser/service_worker/worker_id.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/context_data.h"
 #include "extensions/common/error_utils.h"
@@ -301,6 +302,9 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
   void set_request_id(int request_id) { request_id_ = request_id; }
   int request_id() { return request_id_; }
 
+  void set_request_uuid(std::string uuid) { request_uuid_ = std::move(uuid); }
+  const std::string& request_uuid() const { return request_uuid_; }
+
   void set_source_url(const GURL& source_url) { source_url_ = source_url; }
   const GURL& source_url() const { return source_url_; }
 
@@ -343,18 +347,20 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
     return source_process_id_;
   }
 
-  void set_service_worker_version_id(int64_t service_worker_version_id) {
-    service_worker_version_id_ = service_worker_version_id;
+  void set_worker_id(extensions::WorkerId worker_id) {
+    worker_id_ = std::move(worker_id);
   }
-  int64_t service_worker_version_id() const {
-    return service_worker_version_id_;
+  const absl::optional<extensions::WorkerId>& worker_id() const {
+    return worker_id_;
   }
 
-  bool is_from_service_worker() const {
-    return service_worker_version_id_ !=
-           blink::mojom::kInvalidServiceWorkerVersionId;
+  int64_t service_worker_version_id() const {
+    return worker_id_ ? worker_id_->version_id
+                      : blink::mojom::kInvalidServiceWorkerVersionId;
   }
 
+  bool is_from_service_worker() const { return worker_id_.has_value(); }
+
   ResponseType* response_type() const { return response_type_.get(); }
 
   bool did_respond() const { return did_respond_; }
@@ -380,10 +386,9 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
     return dispatcher_.get();
   }
 
-  void set_worker_thread_id(int worker_thread_id) {
-    worker_thread_id_ = worker_thread_id;
+  int worker_thread_id() const {
+    return worker_id_ ? worker_id_->thread_id : extensions::kMainThreadId;
   }
-  int worker_thread_id() const { return worker_thread_id_; }
 
   // Returns the web contents associated with the sending |render_frame_host_|.
   // This can be null.
@@ -580,6 +585,11 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
   // Id of this request, used to map the response back to the caller.
   int request_id_ = -1;
 
+  // UUID for this request. Currently only set for worker calls.
+  // TODO(crbug.com/1444561): Make this a base::Uuid and replace
+  // `request_id_` with this.
+  std::string request_uuid_;
+
   // The name of this function.
   const char* name_ = nullptr;
 
@@ -625,10 +635,9 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
   // if unknown.
   int source_process_id_ = -1;
 
-  // If this ExtensionFunction was called by an extension Service Worker, then
-  // this contains the worker's version id.
-  int64_t service_worker_version_id_ =
-      blink::mojom::kInvalidServiceWorkerVersionId;
+  // Set to the ID of the calling worker if this function was invoked by an
+  // extension service worker context.
+  absl::optional<extensions::WorkerId> worker_id_;
 
   // The response type of the function, if the response has been sent.
   std::unique_ptr<ResponseType> response_type_;
@@ -668,8 +677,6 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
 
   // The blobs transferred to the renderer process.
   std::vector<blink::mojom::SerializedBlobPtr> transferred_blobs_;
-
-  int worker_thread_id_ = -1;
 };
 
 #endif  // EXTENSIONS_BROWSER_EXTENSION_FUNCTION_H_
diff --git a/extensions/browser/extension_function_dispatcher.cc b/extensions/browser/extension_function_dispatcher.cc
@@ -634,9 +634,16 @@ void ExtensionFunctionDispatcher::DispatchWithCallbackInternal(
   if (!registry->enabled_extensions().GetByID(params.extension_id))
     return;
 
-  if (!IsRequestFromServiceWorker(params)) {
-    // Increment ref count for non-service worker extension API. Ref count for
-    // service worker extension API is handled separately on IO thread via IPC.
+  // Increment the keepalive to ensure the extension doesn't shut down while
+  // it's executing an API function.
+  if (IsRequestFromServiceWorker(params)) {
+    CHECK(function->worker_id());
+    std::string uuid = process_manager->IncrementServiceWorkerKeepaliveCount(
+        *function->worker_id(),
+        content::ServiceWorkerExternalRequestTimeoutType::kDefault,
+        Activity::API_FUNCTION, function->name());
+    function->set_request_uuid(std::move(uuid));
+  } else {
     process_manager->IncrementLazyKeepaliveCount(
         function->extension(), Activity::API_FUNCTION, function->name());
   }
@@ -656,15 +663,38 @@ void ExtensionFunctionDispatcher::RemoveWorkerCallbacksForProcess(
 }
 
 void ExtensionFunctionDispatcher::OnExtensionFunctionCompleted(
-    const Extension* extension,
-    bool is_from_service_worker,
-    const char* name) {
-  if (extension && !is_from_service_worker) {
-    // Decrement ref count for non-service worker extension API. Service
-    // worker extension API ref counts are handled separately on IO thread
-    // directly via IPC.
-    ProcessManager::Get(browser_context_)
-        ->DecrementLazyKeepaliveCount(extension, Activity::API_FUNCTION, name);
+    const ExtensionFunction& extension_function) {
+  if (!extension_function.extension()) {
+    // The function had no associated extension; nothing to clean up.
+    return;
+  }
+
+  if (!extension_function.browser_context()) {
+    // The ExtensionFunction's browser context is null'ed out when the browser
+    // context is being shut down. If this happens, there's nothing to clean up.
+    return;
+  }
+
+  if (!ExtensionRegistry::Get(browser_context_)
+           ->enabled_extensions()
+           .GetByID(extension_function.extension()->id())) {
+    // The extension may have been unloaded (the ExtensionFunction holds a
+    // reference to it, so it's still safe to access). If so, there's nothing to
+    // // clean up.
+    return;
+  }
+
+  ProcessManager* process_manager = ProcessManager::Get(browser_context_);
+  if (extension_function.is_from_service_worker()) {
+    CHECK(!extension_function.request_uuid().empty());
+    CHECK(extension_function.worker_id());
+    process_manager->DecrementServiceWorkerKeepaliveCount(
+        *extension_function.worker_id(), extension_function.request_uuid(),
+        Activity::API_FUNCTION, extension_function.name());
+  } else {
+    process_manager->DecrementLazyKeepaliveCount(extension_function.extension(),
+                                                 Activity::API_FUNCTION,
+                                                 extension_function.name());
   }
 }
 
@@ -753,9 +783,14 @@ ExtensionFunctionDispatcher::CreateExtensionFunction(
   function->set_response_callback(std::move(callback));
   function->set_source_context_type(context_type);
   function->set_source_process_id(requesting_process_id);
-  function->set_worker_thread_id(params.worker_thread_id);
   if (is_worker_request) {
-    function->set_service_worker_version_id(params.service_worker_version_id);
+    CHECK(extension);
+    WorkerId worker_id;
+    worker_id.thread_id = params.worker_thread_id;
+    worker_id.version_id = params.service_worker_version_id;
+    worker_id.render_process_id = requesting_process_id;
+    worker_id.extension_id = extension->id();
+    function->set_worker_id(std::move(worker_id));
   } else {
     function->SetRenderFrameHost(render_frame_host);
   }

diff --git a/extensions/browser/extension_function_dispatcher.h b/extensions/browser/extension_function_dispatcher.h
@@ -87,9 +87,8 @@ class ExtensionFunctionDispatcher {
 
   // Called when an ExtensionFunction is done executing, after it has sent
   // a response (if any) to the extension.
-  void OnExtensionFunctionCompleted(const Extension* extension,
-                                    bool is_from_service_worker,
-                                    const char* name);
+  void OnExtensionFunctionCompleted(
+      const ExtensionFunction& extension_function);
 
   // See the Delegate class for documentation on these methods.
   // TODO(devlin): None of these belong here. We should kill

diff --git a/extensions/browser/service_worker/service_worker_host.cc b/extensions/browser/service_worker/service_worker_host.cc
@@ -151,53 +151,6 @@ void ServiceWorkerHost::DidStopServiceWorkerContext(
           service_worker_scope, service_worker_version_id, worker_thread_id);
 }
 
-void ServiceWorkerHost::IncrementServiceWorkerActivity(
-    int64_t service_worker_version_id,
-    const std::string& request_uuid) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (!GetBrowserContext()) {
-    return;
-  }
-
-  active_request_uuids_.insert(request_uuid);
-  // The worker might have already stopped before we got here, so the increment
-  // below might fail legitimately. Therefore, we do not send bad_message to the
-  // worker even if it fails.
-  render_process_host_->GetStoragePartition()
-      ->GetServiceWorkerContext()
-      ->StartingExternalRequest(
-          service_worker_version_id,
-          content::ServiceWorkerExternalRequestTimeoutType::kDefault,
-          request_uuid);
-}
-
-void ServiceWorkerHost::DecrementServiceWorkerActivity(
-    int64_t service_worker_version_id,
-    const std::string& request_uuid) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (!GetBrowserContext()) {
-    return;
-  }
-
-  content::ServiceWorkerExternalRequestResult result =
-      render_process_host_->GetStoragePartition()
-          ->GetServiceWorkerContext()
-          ->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.
-  if (!erased) {
-    bad_message::ReceivedBadMessage(
-        render_process_host_->GetID(),
-        bad_message::ESWMF_INVALID_DECREMENT_ACTIVITY);
-  }
-}
-
 void ServiceWorkerHost::RequestWorker(mojom::RequestParamsPtr params) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!GetBrowserContext()) {

diff --git a/extensions/browser/service_worker/service_worker_host.h b/extensions/browser/service_worker/service_worker_host.h
@@ -63,10 +63,6 @@ class ServiceWorkerHost : public base::SupportsUserData::Data,
       const GURL& service_worker_scope,
       int64_t service_worker_version_id,
       int worker_thread_id) override;
-  void IncrementServiceWorkerActivity(int64_t service_worker_version_id,
-                                      const std::string& request_uuid) override;
-  void DecrementServiceWorkerActivity(int64_t service_worker_version_id,
-                                      const std::string& request_uuid) override;
   void RequestWorker(mojom::RequestParamsPtr params) override;
   void WorkerResponseAck(int request_id,
                          int64_t service_worker_version_id) override;
@@ -80,9 +76,6 @@ class ServiceWorkerHost : public base::SupportsUserData::Data,
   // RenderProcessHost.
   const raw_ptr<content::RenderProcessHost> render_process_host_;
 
-  // This set is maintained by `(In|De)crementServiceWorkerActivity`.
-  std::unordered_set<std::string> active_request_uuids_;
-
   std::unique_ptr<ExtensionFunctionDispatcher> dispatcher_;
 
   mojo::AssociatedReceiver<mojom::ServiceWorkerHost> receiver_{this};

diff --git a/extensions/common/mojom/service_worker_host.mojom b/extensions/common/mojom/service_worker_host.mojom
@@ -56,25 +56,6 @@ interface ServiceWorkerHost {
       int64 service_worker_version_id,
       int32 worker_thread_id);
 
-  // Asks the browser to increment the pending activity count for
-  // the worker with version id `service_worker_version_id`.
-  // Each request to increment must use unique `request_uuid`. If a request with
-  // `request_uuid` is already in progress (due to race condition or renderer
-  // compromise), browser process ignores the IPC.
-  // TODO(crbug.com/1423190): Investigate simpifying this message because the
-  // browser process should already know that a request is active or not.
-  IncrementServiceWorkerActivity(int64 service_worker_version_id,
-                                 string request_uuid);
-
-  // Asks the browser to decrement the pending activity count for
-  // the worker with version id `service_worker_version_id`.
-  // `request_uuid` must match the GUID of a previous request, otherwise the
-  // browser process ignores the IPC.
-  // TODO(crbug.com/1423190): Investigate simpifying this message because the
-  // browser process should already know that a request is active or not.
-  DecrementServiceWorkerActivity(int64 service_worker_version_id,
-                                 string request_uuid);
-
   // A service worker thread sends this message when an extension service worker
   // starts an API request. We use [UnlimitedSize] here because `params` may be
   // large with some extension function (ex. Storage API).

diff --git a/extensions/renderer/ipc_message_sender.cc b/extensions/renderer/ipc_message_sender.cc
@@ -4,12 +4,10 @@
 
 #include "extensions/renderer/ipc_message_sender.h"
 
-#include <map>
 #include <utility>
 
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/uuid.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/worker_thread.h"
@@ -62,8 +60,6 @@ class MainThreadIPCMessageSender : public IPCMessageSender {
                        weak_ptr_factory_.GetWeakPtr(), request_id));
   }
 
-  void SendOnRequestResponseReceivedIPC(int request_id) override {}
-
   mojom::EventListenerParamPtr GetEventListenerParam(ScriptContext* context) {
     return !context->GetExtensionID().empty()
                ? mojom::EventListenerParam::NewExtensionId(
@@ -311,24 +307,9 @@ class WorkerThreadIPCMessageSender : public IPCMessageSender {
     params->worker_thread_id = worker_thread_id;
     params->service_worker_version_id = service_worker_version_id_;
 
-    std::string guid = base::Uuid::GenerateRandomV4().AsLowercaseString();
-    request_id_to_guid_[params->request_id] = guid;
-
-    // Keeps the worker alive during extension function call. Balanced in
-    // `SendOnRequestResponseReceivedIPC()`.
-    dispatcher_->IncrementServiceWorkerActivity(service_worker_version_id_,
-                                                guid);
     dispatcher_->RequestWorker(std::move(params));
   }
 
-  void SendOnRequestResponseReceivedIPC(int request_id) override {
-    auto iter = request_id_to_guid_.find(request_id);
-    DCHECK(iter != request_id_to_guid_.end());
-    dispatcher_->DecrementServiceWorkerActivity(service_worker_version_id_,
-                                                iter->second);
-    request_id_to_guid_.erase(iter);
-  }
-
   void SendAddUnfilteredEventListenerIPC(
       ScriptContext* context,
       const std::string& event_name) override {
@@ -520,9 +501,6 @@ class WorkerThreadIPCMessageSender : public IPCMessageSender {
   WorkerThreadDispatcher* const dispatcher_;
   const int64_t service_worker_version_id_;
   absl::optional<ExtensionId> extension_id_;
-
-  // request id -> GUID map for each outstanding requests.
-  std::map<int, std::string> request_id_to_guid_;
 };
 
 }  // namespace

diff --git a/extensions/renderer/ipc_message_sender.h b/extensions/renderer/ipc_message_sender.h
@@ -38,10 +38,6 @@ class IPCMessageSender {
   virtual void SendRequestIPC(ScriptContext* context,
                               mojom::RequestParamsPtr params) = 0;
 
-  // Handles sending any additional messages required after receiving a response
-  // to a request.
-  virtual void SendOnRequestResponseReceivedIPC(int request_id) = 0;
-
   // Sends a message to add/remove an unfiltered listener.
   virtual void SendAddUnfilteredEventListenerIPC(
       ScriptContext* context,

diff --git a/extensions/renderer/native_extension_bindings_system.cc b/extensions/renderer/native_extension_bindings_system.cc
@@ -653,7 +653,6 @@ void NativeExtensionBindingsSystem::HandleResponse(
       request_id, response,
       !success && error.empty() ? "Unknown error." : error,
       std::move(extra_data));
-  ipc_message_sender_->SendOnRequestResponseReceivedIPC(request_id);
 }
 
 IPCMessageSender* NativeExtensionBindingsSystem::GetIPCMessageSender() {

diff --git a/extensions/renderer/native_extension_bindings_system_test_base.h b/extensions/renderer/native_extension_bindings_system_test_base.h
@@ -53,7 +53,6 @@ class TestIPCMessageSender : public IPCMessageSender {
   // IPCMessageSender:
   void SendRequestIPC(ScriptContext* context,
                       mojom::RequestParamsPtr params) override;
-  void SendOnRequestResponseReceivedIPC(int request_id) override {}
   // The event listener methods are less of a pain to mock (since they don't
   // have complex parameters like mojom::RequestParams).
   MOCK_METHOD2(SendAddUnfilteredEventListenerIPC,