From d19e2df40048b0031919e698917d19abae20d856 Mon Sep 17 00:00:00 2001
From: Patrick Meenan <pmeenan@chromium.org>
Date: Tue, 21 Jul 2026 07:04:49 -0700
Subject: [PATCH] Maintain adapter priority order for Windows WPAD

DhcpPacFileFetcherWin previously stored candidate network adapter names
in a std::set<std::string>. This sorted the adapter GUID strings
alphabetically, losing the route metric preference order returned by
GetAdaptersAddresses().

This change replaces the set with a vector to maintain the adapter
priority order.

Bug: 533408915
Change-Id: Ied4a9b255f62feed0a4823b0bde2c673ba1f339e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8087629
Reviewed-by: Nidhi Jaju <nidhijaju@chromium.org>
Commit-Queue: Patrick Meenan <pmeenan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1665448}
---

diff --git a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.cc b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.cc
index 866efaf7..cbd8341c 100644
--- a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.cc
+++ b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.cc
@@ -8,6 +8,7 @@
 
 #include <iphlpapi.h>
 
+#include <algorithm>
 #include <memory>
 #include <vector>
 
@@ -370,7 +371,7 @@
 
   state_ = STATE_NO_RESULTS;
 
-  const std::set<std::string>& adapter_names = query->adapter_names();
+  const std::vector<std::string>& adapter_names = query->adapter_names();
 
   if (adapter_names.empty()) {
     TransitionToDone();
@@ -525,7 +526,7 @@
 }
 
 bool DhcpPacFileFetcherWin::GetCandidateAdapterNames(
-    std::set<std::string>* adapter_names,
+    std::vector<std::string>* adapter_names,
     DhcpAdapterNamesLoggingInfo* info) {
   DCHECK(adapter_names);
   adapter_names->clear();
@@ -567,7 +568,10 @@
   for (adapter = adapters.get(); adapter; adapter = adapter->Next) {
     if (IsDhcpCapableAdapter(adapter)) {
       DCHECK(adapter->AdapterName);
-      adapter_names->insert(adapter->AdapterName);
+      if (std::ranges::find(*adapter_names, adapter->AdapterName) ==
+          adapter_names->end()) {
+        adapter_names->push_back(adapter->AdapterName);
+      }
     }
   }
 
@@ -591,13 +595,13 @@
   logging_info_->worker_thread_end_time = base::TimeTicks::Now();
 }
 
-const std::set<std::string>&
+const std::vector<std::string>&
 DhcpPacFileFetcherWin::AdapterQuery::adapter_names() const {
   return adapter_names_;
 }
 
 bool DhcpPacFileFetcherWin::AdapterQuery::ImplGetCandidateAdapterNames(
-    std::set<std::string>* adapter_names,
+    std::vector<std::string>* adapter_names,
     DhcpAdapterNamesLoggingInfo* info) {
   return DhcpPacFileFetcherWin::GetCandidateAdapterNames(adapter_names,
                                                          info);
diff --git a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.h b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.h
index 73a953f..adba861 100644
--- a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.h
+++ b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win.h
@@ -60,7 +60,7 @@
   // this machine that has DHCP enabled and is not a loop-back adapter. May
   // optionally update |info| (if non-null) with information for logging.
   // Returns false on error.
-  static bool GetCandidateAdapterNames(std::set<std::string>* adapter_names,
+  static bool GetCandidateAdapterNames(std::vector<std::string>* adapter_names,
                                        DhcpAdapterNamesLoggingInfo* info);
 
  protected:
@@ -84,16 +84,16 @@
     // This is the method that runs on the worker pool thread.
     void GetCandidateAdapterNames();
 
-    // This set is valid after GetCandidateAdapterNames has
+    // This vector is valid after GetCandidateAdapterNames has
     // been run. Its lifetime is scoped by this object.
-    const std::set<std::string>& adapter_names() const;
+    const std::vector<std::string>& adapter_names() const;
 
     DhcpAdapterNamesLoggingInfo* logging_info() { return logging_info_.get(); }
 
    protected:
     // Virtual method introduced to allow unit testing.
     virtual bool ImplGetCandidateAdapterNames(
-        std::set<std::string>* adapter_names,
+        std::vector<std::string>* adapter_names,
         DhcpAdapterNamesLoggingInfo* info);
 
     friend class base::RefCountedThreadSafe<AdapterQuery>;
@@ -103,7 +103,7 @@
     // These are constructed on the originating thread, then used on the
     // worker thread, then used again on the originating thread only when
     // the task has completed on the worker thread. No locking required.
-    std::set<std::string> adapter_names_;
+    std::vector<std::string> adapter_names_;
     std::unique_ptr<DhcpAdapterNamesLoggingInfo> logging_info_;
   };
 
diff --git a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc
index 44bf5c7..76cf2ca 100644
--- a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc
+++ b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc
@@ -9,6 +9,7 @@
 
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
+#include "base/memory/raw_ptr.h"
 #include "base/rand_util.h"
 #include "base/run_loop.h"
 #include "base/test/task_environment.h"
@@ -41,7 +42,7 @@
   // running in, so it just exercises the code to make sure there
   // is no crash and no error returned, but does not assert on the number
   // of interfaces or the information returned via DHCP.
-  std::set<std::string> adapter_names;
+  std::vector<std::string> adapter_names;
   DhcpPacFileFetcherWin::GetCandidateAdapterNames(&adapter_names, nullptr);
   for (const std::string& adapter_name : adapter_names) {
     DhcpPacFileAdapterFetcher::GetPacURLFromDhcp(adapter_name);
@@ -222,6 +223,9 @@
   void Fetch(const std::string& adapter_name,
              CompletionOnceCallback callback,
              const NetworkTrafficAnnotationTag traffic_annotation) override {
+    if (fetched_adapter_names_) {
+      fetched_adapter_names_->push_back(adapter_name);
+    }
     callback_ = std::move(callback);
     timer_.Start(FROM_HERE, base::Milliseconds(fetch_delay_ms_), this,
                  &DummyDhcpPacFileAdapterFetcher::OnTimer);
@@ -246,11 +250,13 @@
   void Configure(bool did_finish,
                  int result,
                  std::u16string pac_script,
-                 int fetch_delay_ms) {
+                 int fetch_delay_ms,
+                 std::vector<std::string>* fetched_adapter_names = nullptr) {
     did_finish_ = did_finish;
     result_ = result;
     pac_script_ = pac_script;
     fetch_delay_ms_ = fetch_delay_ms;
+    fetched_adapter_names_ = fetched_adapter_names;
   }
 
  private:
@@ -258,6 +264,7 @@
   int result_ = OK;
   std::u16string pac_script_;
   int fetch_delay_ms_ = 1;
+  raw_ptr<std::vector<std::string>> fetched_adapter_names_ = nullptr;
   CompletionOnceCallback callback_;
   base::OneShotTimer timer_;
 };
@@ -270,9 +277,9 @@
     }
 
     bool ImplGetCandidateAdapterNames(
-        std::set<std::string>* adapter_names,
+        std::vector<std::string>* adapter_names,
         DhcpAdapterNamesLoggingInfo* logging) override {
-      adapter_names->insert(mock_adapter_names_.begin(),
+      adapter_names->insert(adapter_names->end(), mock_adapter_names_.begin(),
                             mock_adapter_names_.end());
       return true;
     }
@@ -311,8 +318,9 @@
                                    base::TimeDelta fetch_delay) {
     auto adapter_fetcher = std::make_unique<DummyDhcpPacFileAdapterFetcher>(
         url_request_context(), GetTaskRunner());
-    adapter_fetcher->Configure(
-        did_finish, result, pac_script, fetch_delay.InMilliseconds());
+    adapter_fetcher->Configure(did_finish, result, pac_script,
+                               fetch_delay.InMilliseconds(),
+                               &fetched_adapter_names_);
     PushBackAdapter(adapter_name, std::move(adapter_fetcher));
   }
 
@@ -336,9 +344,12 @@

diff --git a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc
index 44bf5c7..76cf2ca 100644
--- a/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc
+++ b/net/proxy_resolution/win/dhcp_pac_file_fetcher_win_unittest.cc
@@ -9,6 +9,7 @@
 
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
+#include "base/memory/raw_ptr.h"
 #include "base/rand_util.h"
 #include "base/run_loop.h"
 #include "base/test/task_environment.h"
@@ -41,7 +42,7 @@
   // running in, so it just exercises the code to make sure there
   // is no crash and no error returned, but does not assert on the number
   // of interfaces or the information returned via DHCP.
-  std::set<std::string> adapter_names;
+  std::vector<std::string> adapter_names;
   DhcpPacFileFetcherWin::GetCandidateAdapterNames(&adapter_names, nullptr);
   for (const std::string& adapter_name : adapter_names) {
     DhcpPacFileAdapterFetcher::GetPacURLFromDhcp(adapter_name);
@@ -222,6 +223,9 @@
   void Fetch(const std::string& adapter_name,
              CompletionOnceCallback callback,
              const NetworkTrafficAnnotationTag traffic_annotation) override {
+    if (fetched_adapter_names_) {
+      fetched_adapter_names_->push_back(adapter_name);
+    }
     callback_ = std::move(callback);
     timer_.Start(FROM_HERE, base::Milliseconds(fetch_delay_ms_), this,
                  &DummyDhcpPacFileAdapterFetcher::OnTimer);
@@ -246,11 +250,13 @@
   void Configure(bool did_finish,
                  int result,
                  std::u16string pac_script,
-                 int fetch_delay_ms) {
+                 int fetch_delay_ms,
+                 std::vector<std::string>* fetched_adapter_names = nullptr) {
     did_finish_ = did_finish;
     result_ = result;
     pac_script_ = pac_script;
     fetch_delay_ms_ = fetch_delay_ms;
+    fetched_adapter_names_ = fetched_adapter_names;
   }
 
  private:
@@ -258,6 +264,7 @@
   int result_ = OK;
   std::u16string pac_script_;
   int fetch_delay_ms_ = 1;
+  raw_ptr<std::vector<std::string>> fetched_adapter_names_ = nullptr;
   CompletionOnceCallback callback_;
   base::OneShotTimer timer_;
 };
@@ -270,9 +277,9 @@
     }
 
     bool ImplGetCandidateAdapterNames(
-        std::set<std::string>* adapter_names,
+        std::vector<std::string>* adapter_names,
         DhcpAdapterNamesLoggingInfo* logging) override {
-      adapter_names->insert(mock_adapter_names_.begin(),
+      adapter_names->insert(adapter_names->end(), mock_adapter_names_.begin(),
                             mock_adapter_names_.end());
       return true;
     }
@@ -311,8 +318,9 @@
                                    base::TimeDelta fetch_delay) {
     auto adapter_fetcher = std::make_unique<DummyDhcpPacFileAdapterFetcher>(
         url_request_context(), GetTaskRunner());
-    adapter_fetcher->Configure(
-        did_finish, result, pac_script, fetch_delay.InMilliseconds());
+    adapter_fetcher->Configure(did_finish, result, pac_script,
+                               fetch_delay.InMilliseconds(),
+                               &fetched_adapter_names_);
     PushBackAdapter(adapter_name, std::move(adapter_fetcher));
   }
 
@@ -336,9 +344,12 @@
   }
 
   void ResetTestState() {
+    Cancel();
+    worker_finished_event_.Reset();
     next_adapter_fetcher_index_ = 0;
     num_fetchers_created_ = 0;
     adapter_fetchers_.clear();
+    fetched_adapter_names_.clear();
     adapter_query_ = base::MakeRefCounted<MockAdapterQuery>();
     max_wait_ = TestTimeouts::tiny_timeout();
   }
@@ -349,6 +360,9 @@
 
   int next_adapter_fetcher_index_;
 
+  // Must outlive adapter fetchers, which may hold pointers to this vector.
+  std::vector<std::string> fetched_adapter_names_;
+
   // Ownership gets transferred to the implementation class via
   // ImplCreateAdapterFetcher, but any objects not handed out are
   // deleted on destruction.
@@ -590,6 +604,26 @@
             timer.Elapsed());
 }
 
+void TestPreservesCandidateAdapterOrder(FetcherClient* client) {
+  client->fetcher_.ConfigureAndPushBackAdapter(
+      "z_primary", true, OK, u"primary", base::Milliseconds(1));
+  client->fetcher_.ConfigureAndPushBackAdapter(
+      "a_secondary", true, OK, u"secondary", base::Milliseconds(1));
+  client->RunTest();
+  client->RunMessageLoopUntilComplete();
+  ASSERT_EQ(2u, client->fetcher_.fetched_adapter_names_.size());
+  EXPECT_EQ("z_primary", client->fetcher_.fetched_adapter_names_[0]);
+  EXPECT_EQ("a_secondary", client->fetcher_.fetched_adapter_names_[1]);
+  EXPECT_EQ(u"primary", client->pac_text_);
+}
+
+TEST(DhcpPacFileFetcherWin, PreservesCandidateAdapterOrder) {
+  base::test::TaskEnvironment task_environment;
+
+  FetcherClient client;
+  TestPreservesCandidateAdapterOrder(&client);
+}
+
 TEST(DhcpPacFileFetcherWin, ShortCircuitLessPreferredAdapters) {
   base::test::TaskEnvironment task_environment;
 
@@ -640,6 +674,7 @@
   test_functions.push_back(TestFailureCaseNoDhcpAdapters);
   test_functions.push_back(TestShortCircuitLessPreferredAdapters);
   test_functions.push_back(TestImmediateCancel);
+  test_functions.push_back(TestPreservesCandidateAdapterOrder);
 
   base::RandomShuffle(test_functions.begin(), test_functions.end());
   for (TestVector::const_iterator it = test_functions.begin();