[M126-LTS] Protect ServiceWorkerVersion during ReleaseProcess

This CL avoid use-after-free around
`EmbeddedWorkerInstance::ReleaseProcess()`:

1. During `ReleaseProcess()`:
This CL protects `ServiceWorkerVersion` and its
`EmbeddedWorkerInstance` from deletion.

2. In the direct callers of `ReleaseProcess()`:
This CL early returns if `this` is deleted during `ReleaseProcess()`.
Skipping `listener_list_` should be fine because `listener_list_`
is the `owner_version_` (at least in non-test) that is already
deleted if `this` is deleted.
This CL also adds explicit comments that the methods calling
`ReleaseProcess()` may delete `this`.

3. In the callers of 2.:
As far as I checked, the callers should work even in the case of
deletion of `ServiceWorkerVersion`.

(cherry picked from commit 76ce1fe9)

Bug: 350407902
Change-Id: If59e8354ae9832009b62408a8f0058cb6f6e803f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5685760


Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1324689}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5806117


Auto-Submit: Roger Felipe Zanoni da Silva (xWF) <rzanoni@google.com>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Shunya Shishido <sisidovski@chromium.org>
Commit-Queue: Roger Felipe Zanoni da Silva (xWF) <rzanoni@google.com>
Owners-Override: Artem Sumaneev <asumaneev@google.com>
Reviewed-by: Artem Sumaneev <asumaneev@google.com>
Cr-Commit-Position: refs/branch-heads/6478@{#1956}
Cr-Branched-From: e6143acc-refs/heads/main@{#1300313}

content/browser/service_worker/embedded_worker_instance.cc

@@ -224,6 +224,7 @@ struct EmbeddedWorkerInstance::StartInfo {
 
 EmbeddedWorkerInstance::~EmbeddedWorkerInstance() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  in_dtor_ = true;
   ReleaseProcess();
 }
 
@@ -468,7 +469,12 @@ void EmbeddedWorkerInstance::Stop() {
   // been sent.
   if (status_ == blink::EmbeddedWorkerStatus::kStarting &&
       !HasSentStartWorker(starting_phase())) {
+    base::WeakPtr<EmbeddedWorkerInstance> weak_this =
+        weak_factory_.GetWeakPtr();
     ReleaseProcess();
+    if (!weak_this) {
+      return;
+    }
     for (auto& observer : listener_list_)
       observer.OnStopped(
           blink::EmbeddedWorkerStatus::kStarting /* old_status */);
@@ -702,7 +708,11 @@ void EmbeddedWorkerInstance::OnStarted(
 void EmbeddedWorkerInstance::OnStopped() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   blink::EmbeddedWorkerStatus old_status = status_;
+  base::WeakPtr<EmbeddedWorkerInstance> weak_this = weak_factory_.GetWeakPtr();
   ReleaseProcess();
+  if (!weak_this) {
+    return;
+  }
   for (auto& observer : listener_list_)
     observer.OnStopped(old_status);
 }
@@ -714,7 +724,11 @@ void EmbeddedWorkerInstance::Detach() {
   }
 
   blink::EmbeddedWorkerStatus old_status = status_;
+  base::WeakPtr<EmbeddedWorkerInstance> weak_this = weak_factory_.GetWeakPtr();
   ReleaseProcess();
+  if (!weak_this) {
+    return;
+  }
   for (auto& observer : listener_list_)
     observer.OnDetached(old_status);
 }
@@ -991,6 +1005,13 @@ void EmbeddedWorkerInstance::OnNetworkAccessedForScriptLoad() {
 }
 
 void EmbeddedWorkerInstance::ReleaseProcess() {
+  // Keeps alive `owner_version_` and `this` during the method.
+  // We don't have to protect `owner_version_` during the destruction because
+  // there should no remaining `scoped_refptr` to `*owner_version_` that could
+  // trigger re-entering `~EmbeddedWorkerInstance()`.
+  scoped_refptr<ServiceWorkerVersion> protect =
+      !in_dtor_ ? owner_version_.get() : nullptr;
+
   // Abort an inflight start task.
   inflight_start_info_.reset();
   // NotifyForegroundServiceWorkerRemoved() may trigger a call to
@@ -1018,8 +1039,8 @@ void EmbeddedWorkerInstance::OnSetupFailed(
     StatusCallback callback,
     blink::ServiceWorkerStatusCode status) {
   blink::EmbeddedWorkerStatus old_status = status_;
-  ReleaseProcess();
   base::WeakPtr<EmbeddedWorkerInstance> weak_this = weak_factory_.GetWeakPtr();
+  ReleaseProcess();
   std::move(callback).Run(status);
   if (weak_this && old_status != blink::EmbeddedWorkerStatus::kStopped) {
     for (auto& observer : weak_this->listener_list_)

content/browser/service_worker/embedded_worker_instance.h

@@ -161,11 +161,15 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
   // synchronously complete if this instance is STARTING but the Start IPC
   // message has not yet been sent. In that case, the start procedure is
   // aborted, and this instance enters STOPPED status.
+  //
+  // May destroy `this`.
   void Stop();
 
   // Stops the worker if the worker is not being debugged (i.e. devtools is
   // not attached). This method is called by a stop-worker timer to kill
   // idle workers.
+  //
+  // May destroy `this`.
   void StopIfNotAttachedToDevTools();
 
   int embedded_worker_id() const { return embedded_worker_id_; }
@@ -220,6 +224,7 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
   // renderer is unresponsive.  Essentially, it throws away any information
   // about the renderer-side worker, and frees this instance up to start a new
   // worker.
+  // May destroy `this`.
   void Detach();
 
   // Examine the current state of the worker in order to determine if it should
@@ -306,6 +311,7 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
       blink::mojom::EmbeddedWorkerStartTimingPtr start_timing) override;
   // Resets the embedded worker instance to the initial state. Changes
   // the internal status from STARTING or RUNNING to STOPPED.
+  // May destroy `this`.
   void OnStopped() override;
   void OnReportException(const std::u16string& error_message,
                          int line_number,
@@ -319,6 +325,7 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
 
   // Resets all running state. After this function is called, |status_| is
   // kStopped.
+  // May destroy `this`.
   void ReleaseProcess();
 
   // Called back from StartTask when the startup sequence failed. Calls
@@ -416,6 +423,8 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
   // thread, and |coep_reporter_| has the ownership of the impl instance.
   std::unique_ptr<CrossOriginEmbedderPolicyReporter> coep_reporter_;
 
+  bool in_dtor_{false};
+
   base::WeakPtrFactory<EmbeddedWorkerInstance> weak_factory_{this};
 };