From 858ee83ea6e4a0ae9c05bd536e23eb9c5c1cac4e Mon Sep 17 00:00:00 2001
From: Greg Thompson <grt@chromium.org>
Date: Wed, 10 Jun 2026 09:02:22 -0700
Subject: [PATCH] [etw tracing] Fix shutdown race

Destruction of a SystemTracingSession by its last ref being released
could race with detection of client process termination by
ProcessWatcher. To avoid a data race, ProcessWatcher's destructor must
wait for its watch task to complete before allowing destruction to
complete. This ensures that the SystemTracingSession instance that owns
the watcher (by way of a ScopedSession) is not destroyed while
SessionRegistry::OnClientTerminated is handling termination of the
client.

Fixed: 520426287
Change-Id: Ia6d27379ec566ee1ff7e2454fd9fc9c1810f2ec7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7918051
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1644684}
---

diff --git a/chrome/windows_services/elevated_tracing_service/process_watcher.cc b/chrome/windows_services/elevated_tracing_service/process_watcher.cc
index 38d2dea0..0e35b9ac 100644
--- a/chrome/windows_services/elevated_tracing_service/process_watcher.cc
+++ b/chrome/windows_services/elevated_tracing_service/process_watcher.cc
@@ -7,6 +7,7 @@
 #include <windows.h>
 
 #include <array>
+#include <functional>
 #include <utility>
 
 #include "base/check.h"
@@ -27,11 +28,17 @@
 
 // Waits for either the watched process to terminate or for the shutdown event
 // to be signaled. In the former case, the `on_terminated` closure is
-// run before exiting.
+// run before exiting. `completed_event` is signaled when the task is complete.
 void WatchInThreadPool(base::Process process,
                        base::OnceClosure on_terminated,
                        HANDLE startup_event,
-                       base::win::ScopedHandle shutdown_event) {
+                       base::win::ScopedHandle shutdown_event,
+                       base::win::ScopedHandle completed_event,
+                       base::PlatformThreadId& watch_thread_id) {
+  // Give the ID of this thread to the ProcessWatcher so that it can avoid
+  // waiting on itself during destruction.
+  watch_thread_id = base::PlatformThread::CurrentId();
+
   // Signal that the task is ready to watch.
   ::SetEvent(std::exchange(startup_event, nullptr));
 
@@ -48,6 +55,8 @@
   if (result == WAIT_OBJECT_0) {
     std::move(on_terminated).Run();
   }  // else the shutdown event was signaled.
+
+  ::SetEvent(completed_event.get());
 }
 
 }  // namespace
@@ -66,13 +75,22 @@
                           /*dwDesiredAccess=*/0,
                           /*bInheritHandle=*/FALSE, DUPLICATE_SAME_ACCESS));
 
+  // Prepare a dup of the completed event for the task to signal.
+  HANDLE completed_event = nullptr;
+  CHECK(::DuplicateHandle(::GetCurrentProcess(), completed_event_.handle(),
+                          ::GetCurrentProcess(), &completed_event,
+                          /*dwDesiredAccess=*/0,
+                          /*bInheritHandle=*/FALSE, DUPLICATE_SAME_ACCESS));
+
   base::ThreadPool::CreateTaskRunner(
       {base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN, base::MayBlock()})
       ->PostTask(
           FROM_HERE,
           base::BindOnce(&WatchInThreadPool, std::move(process),
                          std::move(on_terminated), startup_event.handle(),
-                         base::win::ScopedHandle(shutdown_event)));
+                         base::win::ScopedHandle(shutdown_event),
+                         base::win::ScopedHandle(completed_event),
+                         std::ref(watch_thread_id_)));
 
   // Wait for the watch task to signal that it is ready.
   startup_event.Wait();
@@ -81,6 +99,13 @@
 ProcessWatcher::~ProcessWatcher() {
   // Signal that the watch task should exit if it is still watching the process.
   shutdown_event_.Signal();
+
+  // Wait for the watch task to complete before continuing with destruction,
+  // unless destruction is taking place on the watch thread itself. This can
+  // happen if client termination synchronously triggers destruction via COM.
+  if (base::PlatformThread::CurrentId() != watch_thread_id_) {
+    completed_event_.Wait();
+  }
 }
 
 }  // namespace elevated_tracing_service
diff --git a/chrome/windows_services/elevated_tracing_service/process_watcher.h b/chrome/windows_services/elevated_tracing_service/process_watcher.h
index d31cfa4..49ee70f 100644
--- a/chrome/windows_services/elevated_tracing_service/process_watcher.h
+++ b/chrome/windows_services/elevated_tracing_service/process_watcher.h
@@ -8,6 +8,7 @@
 #include "base/functional/callback_forward.h"
 #include "base/process/process.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/threading/platform_thread.h"
 
 namespace elevated_tracing_service {
 
@@ -22,10 +23,14 @@
   ~ProcessWatcher();
 
  private:
-  class ThreadDelegate;
-
   // An event that is signaled at destruction to cancel the watch.
   base::WaitableEvent shutdown_event_;
+
+  // An event that is signaled when the watch task has completed.
+  base::WaitableEvent completed_event_;
+
+  // The TID of the thread servicing the watch task.
+  base::PlatformThreadId watch_thread_id_ = base::kInvalidThreadId;
 };
 
 }  // namespace elevated_tracing_service
diff --git a/chrome/windows_services/elevated_tracing_service/session_registry.cc b/chrome/windows_services/elevated_tracing_service/session_registry.cc
index f9610391..bd30ed0 100644
--- a/chrome/windows_services/elevated_tracing_service/session_registry.cc
+++ b/chrome/windows_services/elevated_tracing_service/session_registry.cc
@@ -6,6 +6,8 @@
 
 #include <objbase.h>
 
+#include <wrl/client.h>
+
 #include <utility>
 
 #include "base/check_deref.h"
@@ -108,6 +110,9 @@
   // still holds the session's IUnknown pointer (meaning that the ScopedSession
   // has yet to be destroyed), tell COM to force a disconnect.
   if (IUnknown* unknown = core->release(); unknown != nullptr) {
+    // Keep a strong reference locally to prevent the object from being
+    // synchronously destroyed (and deallocated) during CoDisconnectObject.
+    Microsoft::WRL::ComPtr<IUnknown> keep_alive(unknown);
     ::CoDisconnectObject(unknown, /*dwReserved=*/0);
 
     // This task is handling client termination before the session is destroyed,