From e015f284b9aebc418adcc3d5695efd5fc3ab722a Mon Sep 17 00:00:00 2001
From: Etienne Bergeron <etienneb@google.com>
Date: Thu, 16 Jul 2026 11:01:33 -0700
Subject: [PATCH] [net] Defer NetworkChangeNotifier registration in ProxyConfigServiceWin

ProxyConfigServiceWin was registering itself as a NetworkChangeObserver
during construction. In some Windows environments (e.g., Chrome Remote
Desktop host binaries), the service is constructed on the UI/controller
thread but is subsequently used and destroyed on the Network thread.
Because the observer was registered during construction on the UI
thread, NetworkChangeNotifier dispatched OnNetworkChanged callbacks back
to the UI thread. On the first network change, OnNetworkChanged called
CheckForChangesNow(), which lazily bound PollingProxyConfigService's
Core origin task runner to the UI thread instead of the Network thread.

Subsequent operations (such as AddObserver) called on the Network thread
would then fail sequence checks or cause concurrent thread access
(races) on the non-thread-safe observer list, leading to memory
corruption or Use-After-Free crashes during shutdown.

This CL resolves the issue by:

1. Deferring NetworkChangeNotifier registration until the first call to
   AddObserver(), which runs on the correct Network thread.
2. Adding a SEQUENCE_CHECKER and detaching it on construction to enforce
   proper sequence affinity on first use.
3. Adding a new unit test ThreadMismatchRegistration to verify
   lazy thread binding.

Bug: 533418127
Test: net_unittests --gtest_filter=ProxyConfigServiceWinTest.ThreadMismatchRegistration
Change-Id: Iddd14cdc6e4a007838e99cef80de4e2a8a1a4859
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8083986
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1663303}
---
diff --git a/net/proxy_resolution/win/proxy_config_service_win.cc b/net/proxy_resolution/win/proxy_config_service_win.cc
index fc238b5e..4004f97 100644
--- a/net/proxy_resolution/win/proxy_config_service_win.cc
+++ b/net/proxy_resolution/win/proxy_config_service_win.cc
@@ -48,11 +48,16 @@
           base::Seconds(kPollIntervalSec),
           base::BindRepeating(&ProxyConfigServiceWin::GetCurrentProxyConfig),
           traffic_annotation) {
-  NetworkChangeNotifier::AddNetworkChangeObserver(this);
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 ProxyConfigServiceWin::~ProxyConfigServiceWin() {
-  NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (registered_as_network_change_observer_) {
+    NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+  }
+
   // The registry functions below will end up going to disk.  TODO: Do this on
   // another thread to avoid slowing the current thread.  http://crbug.com/61453
   base::ScopedAllowBlocking scoped_allow_blocking;
@@ -60,15 +65,25 @@
 }
 
 void ProxyConfigServiceWin::AddObserver(Observer* observer) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // Lazily-initialize our registry watcher.
   StartWatchingRegistryForChanges();
 
+  // Lazily-register as network change observer on the correct thread.
+  if (!registered_as_network_change_observer_) {
+    NetworkChangeNotifier::AddNetworkChangeObserver(this);
+    registered_as_network_change_observer_ = true;
+  }
+
   // Let the super-class do its work now.
   PollingProxyConfigService::AddObserver(observer);
 }
 
 void ProxyConfigServiceWin::OnNetworkChanged(
     NetworkChangeNotifier::ConnectionType type) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // Proxy settings on Windows may change when the active connection changes.
   // For instance, after connecting to a VPN, the proxy settings for the active
   // connection will be that for the VPN. (And ProxyConfigService only reports
@@ -83,6 +98,8 @@
 }
 
 void ProxyConfigServiceWin::StartWatchingRegistryForChanges() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!keys_to_watch_.empty()) {
     return;  // Already initialized.
   }
@@ -135,6 +152,8 @@
 }
 
 void ProxyConfigServiceWin::OnObjectSignaled(base::win::RegKey* key) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // Figure out which registry key signalled this change.
   auto it = std::ranges::find(keys_to_watch_, key,
                               &std::unique_ptr<base::win::RegKey>::get);
diff --git a/net/proxy_resolution/win/proxy_config_service_win.h b/net/proxy_resolution/win/proxy_config_service_win.h
index 7597fe6..79fa43ca 100644
--- a/net/proxy_resolution/win/proxy_config_service_win.h
+++ b/net/proxy_resolution/win/proxy_config_service_win.h
@@ -14,6 +14,7 @@
 
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
+#include "base/sequence_checker.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 #include "net/proxy_resolution/polling_proxy_config_service.h"
@@ -83,6 +84,10 @@
       const WINHTTP_CURRENT_USER_IE_PROXY_CONFIG& ie_config);
 
   std::vector<std::unique_ptr<base::win::RegKey>> keys_to_watch_;
+
+  bool registered_as_network_change_observer_ = false;
+
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 }  // namespace net
diff --git a/net/proxy_resolution/win/proxy_config_service_win_unittest.cc b/net/proxy_resolution/win/proxy_config_service_win_unittest.cc
index 40e68003..033b4341 100644
--- a/net/proxy_resolution/win/proxy_config_service_win_unittest.cc
+++ b/net/proxy_resolution/win/proxy_config_service_win_unittest.cc
@@ -6,9 +6,15 @@
 
 #include <array>
 
+#include "base/run_loop.h"
+#include "base/test/task_environment.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/thread.h"
 #include "net/base/net_errors.h"
+#include "net/base/network_change_notifier.h"
 #include "net/proxy_resolution/proxy_config.h"
 #include "net/proxy_resolution/proxy_config_service_common_unittest.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -224,4 +230,83 @@
   }
 }
 
+// An observer that quits a RunLoop when notified of a config change.
+class TestProxyConfigObserver : public ProxyConfigService::Observer {
+ public:
+  TestProxyConfigObserver(base::OnceClosure quit_closure,
+                          base::PlatformThreadId expected_thread_id)
+      : quit_closure_(std::move(quit_closure)),
+        expected_thread_id_(expected_thread_id) {}
+  void OnProxyConfigChanged(
+      const ProxyConfigWithAnnotation& config,
+      ProxyConfigService::ConfigAvailability availability) override {
+    EXPECT_EQ(base::PlatformThread::CurrentId(), expected_thread_id_);
+    if (quit_closure_) {
+      std::move(quit_closure_).Run();
+    }
+  }
+
+ private:
+  base::OnceClosure quit_closure_;
+  base::PlatformThreadId expected_thread_id_;
+};
+
+TEST(ProxyConfigServiceWinTest, ThreadMismatchRegistration) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::UI);
+
+  // Initialize a mock NetworkChangeNotifier so we can dispatch test events.
+  std::unique_ptr<NetworkChangeNotifier> ncn(
+      NetworkChangeNotifier::CreateMockIfNeeded());
+
+  // Simulating Thread A (UI/Constructor thread) and Thread B (Network thread).
+  base::Thread network_thread("NetworkThread");
+  ASSERT_TRUE(network_thread.Start());
+  std::unique_ptr<ProxyConfigServiceWin> service;
+
+  // 1. Construct the ProxyConfigServiceWin on the UI thread (Thread A).
+  service =
+      std::make_unique<ProxyConfigServiceWin>(TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  // Create a run loop to wait for the network change notification.
+  base::RunLoop network_change_run_loop;
+  TestProxyConfigObserver observer(network_change_run_loop.QuitClosure(),
+                                   network_thread.GetThreadId());
+
+  // 2. Add the observer on the Network thread (Thread B).
+  base::RunLoop init_run_loop;
+  network_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](ProxyConfigServiceWin* service, TestProxyConfigObserver* observer,
+             base::OnceClosure quit_closure) {
+            service->AddObserver(observer);
+            std::move(quit_closure).Run();
+          },
+          base::Unretained(service.get()), base::Unretained(&observer),
+          init_run_loop.QuitClosure()));
+  init_run_loop.Run();
+
+  // 3. Trigger a network change event (CONNECTION_NONE) on the UI thread.
+  NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+      NetworkChangeNotifier::CONNECTION_NONE);
+
+  // 4. Wait for the observer to be notified on the Network thread (Thread B).
+  // The quit closure is thread-safe and will wake up this loop on the UI
+  // thread.
+  network_change_run_loop.Run();
+
+  // 5. Cleanup the service on the Network thread.
+  base::RunLoop destruct_run_loop;
+  network_thread.task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(
+                     [](std::unique_ptr<ProxyConfigServiceWin> service,
+                        base::OnceClosure quit_closure) {
+                       service.reset();
+                       std::move(quit_closure).Run();
+                     },
+                     std::move(service), destruct_run_loop.QuitClosure()));
+  destruct_run_loop.Run();
+}
+
 }  // namespace net

diff --git a/net/proxy_resolution/win/proxy_config_service_win_unittest.cc b/net/proxy_resolution/win/proxy_config_service_win_unittest.cc
index 40e68003..033b4341 100644
--- a/net/proxy_resolution/win/proxy_config_service_win_unittest.cc
+++ b/net/proxy_resolution/win/proxy_config_service_win_unittest.cc
@@ -6,9 +6,15 @@
 
 #include <array>
 
+#include "base/run_loop.h"
+#include "base/test/task_environment.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/thread.h"
 #include "net/base/net_errors.h"
+#include "net/base/network_change_notifier.h"
 #include "net/proxy_resolution/proxy_config.h"
 #include "net/proxy_resolution/proxy_config_service_common_unittest.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -224,4 +230,83 @@
   }
 }
 
+// An observer that quits a RunLoop when notified of a config change.
+class TestProxyConfigObserver : public ProxyConfigService::Observer {
+ public:
+  TestProxyConfigObserver(base::OnceClosure quit_closure,
+                          base::PlatformThreadId expected_thread_id)
+      : quit_closure_(std::move(quit_closure)),
+        expected_thread_id_(expected_thread_id) {}
+  void OnProxyConfigChanged(
+      const ProxyConfigWithAnnotation& config,
+      ProxyConfigService::ConfigAvailability availability) override {
+    EXPECT_EQ(base::PlatformThread::CurrentId(), expected_thread_id_);
+    if (quit_closure_) {
+      std::move(quit_closure_).Run();
+    }
+  }
+
+ private:
+  base::OnceClosure quit_closure_;
+  base::PlatformThreadId expected_thread_id_;
+};
+
+TEST(ProxyConfigServiceWinTest, ThreadMismatchRegistration) {
+  base::test::TaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::UI);
+
+  // Initialize a mock NetworkChangeNotifier so we can dispatch test events.
+  std::unique_ptr<NetworkChangeNotifier> ncn(
+      NetworkChangeNotifier::CreateMockIfNeeded());
+
+  // Simulating Thread A (UI/Constructor thread) and Thread B (Network thread).
+  base::Thread network_thread("NetworkThread");
+  ASSERT_TRUE(network_thread.Start());
+  std::unique_ptr<ProxyConfigServiceWin> service;
+
+  // 1. Construct the ProxyConfigServiceWin on the UI thread (Thread A).
+  service =
+      std::make_unique<ProxyConfigServiceWin>(TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  // Create a run loop to wait for the network change notification.
+  base::RunLoop network_change_run_loop;
+  TestProxyConfigObserver observer(network_change_run_loop.QuitClosure(),
+                                   network_thread.GetThreadId());
+
+  // 2. Add the observer on the Network thread (Thread B).
+  base::RunLoop init_run_loop;
+  network_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](ProxyConfigServiceWin* service, TestProxyConfigObserver* observer,
+             base::OnceClosure quit_closure) {
+            service->AddObserver(observer);
+            std::move(quit_closure).Run();
+          },
+          base::Unretained(service.get()), base::Unretained(&observer),
+          init_run_loop.QuitClosure()));
+  init_run_loop.Run();
+
+  // 3. Trigger a network change event (CONNECTION_NONE) on the UI thread.
+  NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+      NetworkChangeNotifier::CONNECTION_NONE);
+
+  // 4. Wait for the observer to be notified on the Network thread (Thread B).
+  // The quit closure is thread-safe and will wake up this loop on the UI
+  // thread.
+  network_change_run_loop.Run();
+
+  // 5. Cleanup the service on the Network thread.
+  base::RunLoop destruct_run_loop;
+  network_thread.task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(
+                     [](std::unique_ptr<ProxyConfigServiceWin> service,
+                        base::OnceClosure quit_closure) {
+                       service.reset();
+                       std::move(quit_closure).Run();
+                     },
+                     std::move(service), destruct_run_loop.QuitClosure()));
+  destruct_run_loop.Run();
+}
+
 }  // namespace net