From b600e4a963272a207b2fa9c6de23a4d35bd59378 Mon Sep 17 00:00:00 2001
From: Yuwei Huang <yuweih@chromium.org>
Date: Fri, 24 Jul 2026 10:23:39 -0700
Subject: [PATCH] CRD: Keep WtsSessionProcessDelegate::Core alive until job empty

WtsSessionProcessDelegate::Core registers itself as the completion-port
handler for its job object, so the kernel holds a raw pointer to it.
Stop() previously closed the job handle and released |self_| after a
single task round-trip to the I/O thread, but TerminateJobObject() does
not wait for the worker process to exit, so
JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO can be posted to the completion port
after Core has been destroyed.

Track whether a process has been assigned to the job and defer closing
the job handle and releasing |self_| until OnActiveProcessZero() runs on
the caller thread, which only happens after the I/O thread has dequeued
the terminal job notification. Once that notification has been delivered
the job contains no processes, closing the handle destroys the kernel
job object, and the existing drain round-trip can safely release the
final reference.

Move wts_session_process_delegate.cc into the :win source set so that
remoting_unittests can link it, drop the unused host_main.h include, and
add testing hooks plus a unit test that exercises the job-object
shutdown path with a real child process.

Bug: 515473074
Change-Id: I521d5738648f0d0241b6e3042f3182ff3e625d59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8142304
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1667967}
---

diff --git a/remoting/host/win/BUILD.gn b/remoting/host/win/BUILD.gn
index c5aca2b..78dd098 100644
--- a/remoting/host/win/BUILD.gn
+++ b/remoting/host/win/BUILD.gn
@@ -179,6 +179,7 @@
     "windows_process_delegate.h",
     "wts_session_change_observer.cc",
     "wts_session_change_observer.h",
+    "wts_session_process_delegate.cc",
     "wts_session_process_delegate.h",
     "wts_terminal_monitor.cc",
     "wts_terminal_monitor.h",
@@ -243,6 +244,7 @@
     "mouse_cursor_monitor_win_unittest.cc",
     "rdp_client_unittest.cc",
     "unprivileged_process_delegate_unittest.cc",
+    "wts_session_process_delegate_unittest.cc",
   ]
 
   deps = [
@@ -427,7 +429,6 @@
     "host_service.cc",
     "rdp_desktop_session.cc",
     "rdp_desktop_session.h",
-    "wts_session_process_delegate.cc",
   ]
   deps = [
     ":acl_util",
diff --git a/remoting/host/win/wts_session_process_delegate.cc b/remoting/host/win/wts_session_process_delegate.cc
index e8f0204a..e54b8ab 100644
--- a/remoting/host/win/wts_session_process_delegate.cc
+++ b/remoting/host/win/wts_session_process_delegate.cc
@@ -15,6 +15,7 @@
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/functional/bind.h"
+#include "base/functional/callback.h"
 #include "base/logging.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/scoped_refptr.h"
@@ -37,7 +38,6 @@
 #include "mojo/public/cpp/platform/platform_handle.h"
 #include "mojo/public/cpp/system/invitation.h"
 #include "remoting/host/base/switches.h"
-#include "remoting/host/host_main.h"
 #include "remoting/host/ipc_constants.h"
 #include "remoting/host/mojom/desktop_session.mojom.h"
 #include "remoting/host/win/launch_process_with_token.h"
@@ -46,13 +46,13 @@
 #include "remoting/host/worker_process_ipc_delegate.h"
 #include "remoting/host/worker_process_launcher.h"
 
-    using base::win::ScopedHandle;
-
 // Name of the default session desktop.
 const char kDefaultDesktopName[] = "winsta0\\default";
 
 namespace remoting {
 
+using base::win::ScopedHandle;
+
 // A private class actually implementing the functionality provided by
 // |WtsSessionProcessDelegate|. This class is ref-counted and implements
 // asynchronous fire-and-forget shutdown.
@@ -90,6 +90,9 @@
   void CrashProcess(const base::Location& location);
   void KillProcess();
 
+  bool AssignProcessToJobForTesting(base::ProcessHandle process);
+  void SetCoreDeletedCallbackForTesting(base::OnceClosure callback);
+
  private:
   friend class base::RefCountedThreadSafe<Core>;
   ~Core() override;
@@ -188,6 +191,13 @@
 
   // True if Stop() has been called.
   bool stopped_ = false;
+
+  // True if a process has been assigned to the job object and the resulting
+  // JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO notification has not yet been delivered
+  // by the I/O thread.
+  bool job_process_assigned_ = false;
+
+  base::OnceClosure deleted_callback_for_testing_;
 };
 
 WtsSessionProcessDelegate::Core::Core(
@@ -320,10 +330,28 @@
   worker_process_.Close();
 }
 
+bool WtsSessionProcessDelegate::Core::AssignProcessToJobForTesting(
+    base::ProcessHandle process) {
+  DCHECK(caller_task_runner_->BelongsToCurrentThread());
+  if (!job_.is_valid() || !::AssignProcessToJobObject(job_.Get(), process)) {
+    return false;
+  }
+  job_process_assigned_ = true;
+  return true;
+}
+
+void WtsSessionProcessDelegate::Core::SetCoreDeletedCallbackForTesting(
+    base::OnceClosure callback) {
+  deleted_callback_for_testing_ = std::move(callback);
+}
+
 WtsSessionProcessDelegate::Core::~Core() {
   DCHECK(!channel_);
   DCHECK(!event_handler_);
   DCHECK(!worker_process_.is_valid());
+  if (deleted_callback_for_testing_) {
+    std::move(deleted_callback_for_testing_).Run();
+  }
 }
 
 void WtsSessionProcessDelegate::Core::OnIOCompleted(
@@ -477,6 +505,7 @@
       ReportFatalError();
       return;
     }
+    job_process_assigned_ = true;
   }
 
   if (!ResumeThread(worker_thread.Get())) {
@@ -514,6 +543,15 @@
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   if (job_.is_valid()) {
+    // Closing the last handle to the job destroys the kernel job object only
+    // once every process in the job has terminated. TerminateJobObject() does
+    // not wait for the processes to exit, so defer closing the handle until
+    // OnActiveProcessZero() has run so that no further notifications will be
+    // posted to the completion port after the handle is closed.
+    if (job_process_assigned_) {
+      return;
+    }
+
     job_.Close();
 
     // Drain the completion queue to make sure all job object notifications have
@@ -571,6 +609,26 @@
 void WtsSessionProcessDelegate::Core::OnActiveProcessZero() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
+  if (job_.is_valid()) {
+    JOBOBJECT_BASIC_ACCOUNTING_INFORMATION info;
+    if (QueryInformationJobObject(job_.Get(),
+                                  JobObjectBasicAccountingInformation, &info,
+                                  sizeof(info), nullptr)) {
+      if (info.ActiveProcesses > 0) {
+        return;
+      }
+    }
+  }
+
+  job_process_assigned_ = false;
+
+  if (stopped_) {
+    // The I/O thread has now processed all pending job object notifications,
+    // so it is safe to close the job handle and release |self_|.
+    DrainJobNotificationsCompleted();
+    return;
+  }
+
   if (launch_pending_) {

diff --git a/remoting/host/win/wts_session_process_delegate_unittest.cc b/remoting/host/win/wts_session_process_delegate_unittest.cc
new file mode 100644
index 0000000..484ef86
--- /dev/null
+++ b/remoting/host/win/wts_session_process_delegate_unittest.cc
@@ -0,0 +1,164 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/win/wts_session_process_delegate.h"
+
+#include <windows.h>
+
+#include <memory>
+#include <tuple>
+
+#include "base/command_line.h"
+#include "base/functional/callback_helpers.h"
+#include "base/message_loop/message_pump_type.h"
+#include "base/process/process.h"
+#include "base/run_loop.h"
+#include "base/task/single_thread_task_runner.h"
+#include "base/test/bind.h"
+#include "base/test/multiprocess_test.h"
+#include "base/test/task_environment.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/thread.h"
+#include "base/win/scoped_handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/multiprocess_func_list.h"
+
+namespace remoting {
+
+class WtsSessionProcessDelegateTest : public testing::Test {
+ public:
+  WtsSessionProcessDelegateTest()
+      : task_environment_(base::test::TaskEnvironment::MainThreadType::UI),
+        io_thread_("WtsSessionProcessDelegateTest IO") {}
+
+  void SetUp() override {
+    base::Thread::Options options;
+    options.message_pump_type = base::MessagePumpType::IO;
+    ASSERT_TRUE(io_thread_.StartWithOptions(std::move(options)));
+  }
+
+  void TearDown() override { io_thread_.Stop(); }
+
+ protected:
+  void FlushIoThread() {
+    base::RunLoop run_loop;
+    io_thread_.task_runner()->PostTaskAndReply(FROM_HERE, base::DoNothing(),
+                                               run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  base::test::TaskEnvironment task_environment_;
+  base::Thread io_thread_;
+};
+
+// Verifies that the delegate's internal Core stays alive while a worker process
+// is still assigned to the job object, and is only released once the I/O thread
+// has reported that the job has no remaining active processes.
+TEST_F(WtsSessionProcessDelegateTest, CoreOutlivesJobNotifications) {
+  auto target_command =
+      std::make_unique<base::CommandLine>(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<WtsSessionProcessDelegate>(
+      io_thread_.task_runner(), std::move(target_command),
+      /*launch_elevated=*/true,
+      /*channel_security=*/std::string());
+
+  // Initialize() creates the job object and registers it with the I/O thread's
+  // completion port, but is expected to fail to create a session token when the
+  // test is not running with the required privileges. The job-object plumbing
+  // is set up regardless of the return value, which is what this test relies
+  // on, so the result is intentionally ignored.
+  std::ignore = delegate->Initialize(WTSGetActiveConsoleSessionId());
+
+  // Allow the asynchronous job-object initialization to complete.
+  FlushIoThread();
+
+  // Launch a child process and assign it to the delegate's job object so that
+  // the job will post completion-port notifications when the child exits.
+  base::Process child = base::SpawnMultiProcessTestChild(
+      "WtsSessionProcessDelegateTestChild",
+      base::GetMultiProcessTestChildBaseCommandLine(), {});
+  ASSERT_TRUE(child.IsValid());
+  ASSERT_TRUE(delegate->AssignProcessToJobForTesting(child.Handle()));
+
+  bool core_deleted = false;
+  base::RunLoop run_loop;
+  delegate->SetCoreDeletedCallbackForTesting(base::BindLambdaForTesting([&]() {
+    core_deleted = true;
+    run_loop.Quit();
+  }));
+
+  // Destroying the delegate calls Stop(), which terminates the job. The Core
+  // must remain alive until the I/O thread has delivered the final job
+  // notifications.
+  delegate.reset();
+  EXPECT_FALSE(core_deleted);
+
+  // Wait for the I/O thread to deliver all job-object notifications and for the
+  // Core to be released. Any premature release of Core would result in
+  // OnIOCompleted() being invoked on a freed object.
+  run_loop.Run();
+  EXPECT_TRUE(core_deleted);
+
+  // The job object terminates the child as part of Stop().
+  int exit_code = 0;
+  EXPECT_TRUE(
+      child.WaitForExitWithTimeout(TestTimeouts::action_timeout(), &exit_code));
+}
+
+TEST_F(WtsSessionProcessDelegateTest,
+       StaleJobNotificationIgnoredIfNewProcessAssigned) {
+  auto target_command =
+      std::make_unique<base::CommandLine>(base::CommandLine::NO_PROGRAM);
+  auto delegate = std::make_unique<WtsSessionProcessDelegate>(
+      io_thread_.task_runner(), std::move(target_command),
+      /*launch_elevated=*/true,
+      /*channel_security=*/std::string());
+
+  std::ignore = delegate->Initialize(WTSGetActiveConsoleSessionId());
+  FlushIoThread();
+
+  base::Process child1 = base::SpawnMultiProcessTestChild(
+      "WtsSessionProcessDelegateTestChild",
+      base::GetMultiProcessTestChildBaseCommandLine(), {});
+  ASSERT_TRUE(child1.IsValid());
+  ASSERT_TRUE(delegate->AssignProcessToJobForTesting(child1.Handle()));
+
+  base::Process child2 = base::SpawnMultiProcessTestChild(
+      "WtsSessionProcessDelegateTestChild",
+      base::GetMultiProcessTestChildBaseCommandLine(), {});
+  ASSERT_TRUE(child2.IsValid());
+  ASSERT_TRUE(delegate->AssignProcessToJobForTesting(child2.Handle()));
+
+  // Terminate child1 so a JOB_OBJECT_MSG_EXIT_PROCESS / ACTIVE_PROCESS_ZERO is
+  // generated, but child2 remains active in the job object.
+  child1.Terminate(0, false);
+  FlushIoThread();
+
+  bool core_deleted = false;
+  base::RunLoop run_loop;
+  delegate->SetCoreDeletedCallbackForTesting(base::BindLambdaForTesting([&]() {
+    core_deleted = true;
+    run_loop.Quit();
+  }));
+
+  // Destroying delegate stops the job. Since child2 is still in the job, Core
+  // must stay alive until child2 is terminated.
+  delegate.reset();
+  EXPECT_FALSE(core_deleted);
+
+  run_loop.Run();
+  EXPECT_TRUE(core_deleted);
+
+  int exit_code = 0;
+  EXPECT_TRUE(child2.WaitForExitWithTimeout(TestTimeouts::action_timeout(),
+                                            &exit_code));
+}
+
+MULTIPROCESS_TEST_MAIN(WtsSessionProcessDelegateTestChild) {
+  // Block until the parent terminates this process via the job object.
+  ::Sleep(INFINITE);
+  return 0;
+}
+
+}  // namespace remoting