Fixed region change failure while connected on Windows

I suspect that this failure comes from vpn disconnect before starting
re-connect to new region. When failure happens, vpn service can't fetch
new credentials from guardian. Maybe disconnect could make network
unavailalbe shortly.

issue: brave/brave-browser#25789

components/brave_vpn/brave_vpn_os_connection_api_win.cc

@@ -21,10 +21,12 @@
 using brave_vpn::internal::CheckConnectionResult;
 using brave_vpn::internal::CloseEventHandleForConnectFailed;
 using brave_vpn::internal::CloseEventHandleForConnecting;
+using brave_vpn::internal::CloseEventHandleForDisconnected;
 using brave_vpn::internal::CloseEventHandleForDisconnecting;
 using brave_vpn::internal::CreateEntry;
 using brave_vpn::internal::GetEventHandleForConnectFailed;
 using brave_vpn::internal::GetEventHandleForConnecting;
+using brave_vpn::internal::GetEventHandleForDisconnected;
 using brave_vpn::internal::GetEventHandleForDisconnecting;
 using brave_vpn::internal::GetPhonebookPath;
 using brave_vpn::internal::PrintRasError;
@@ -56,9 +58,9 @@ BraveVPNOSConnectionAPIWin::BraveVPNOSConnectionAPIWin() {
 
 BraveVPNOSConnectionAPIWin::~BraveVPNOSConnectionAPIWin() {
   CloseHandle(event_handle_for_connected_);
-  CloseHandle(event_handle_for_disconnected_);
   CloseEventHandleForConnecting();
   CloseEventHandleForDisconnecting();
+  CloseEventHandleForDisconnected();
   CloseEventHandleForConnectFailed();
 }
 
@@ -121,7 +123,7 @@ void BraveVPNOSConnectionAPIWin::OnObjectSignaled(HANDLE object) {
     result = CheckConnectionResult::DISCONNECTING;
   } else if (object == event_handle_for_connected_) {
     result = CheckConnectionResult::CONNECTED;
-  } else if (object == event_handle_for_disconnected_) {
+  } else if (object == GetEventHandleForDisconnected()) {
     result = CheckConnectionResult::DISCONNECTED;
   } else {
     NOTREACHED();
@@ -178,23 +180,19 @@ void BraveVPNOSConnectionAPIWin::OnRemoved(const std::string& name,
 }
 
 void BraveVPNOSConnectionAPIWin::StartVPNConnectionChangeMonitoring() {
-  DCHECK(!event_handle_for_connected_ && !event_handle_for_disconnected_);
+  DCHECK(!event_handle_for_connected_);
 
   event_handle_for_connected_ = CreateEvent(NULL, false, false, NULL);
-  event_handle_for_disconnected_ = CreateEvent(NULL, false, false, NULL);
 
   // We don't need to check current connection state again if monitor each event
   // separately.
   RasConnectionNotificationW(static_cast<HRASCONN>(INVALID_HANDLE_VALUE),
                              event_handle_for_connected_, RASCN_Connection);
-  RasConnectionNotificationW(static_cast<HRASCONN>(INVALID_HANDLE_VALUE),
-                             event_handle_for_disconnected_,
-                             RASCN_Disconnection);
 
   connected_event_watcher_.StartWatchingMultipleTimes(
       event_handle_for_connected_, this);
   disconnected_event_watcher_.StartWatchingMultipleTimes(
-      event_handle_for_disconnected_, this);
+      GetEventHandleForDisconnected(), this);
   connecting_event_watcher_.StartWatchingMultipleTimes(
       GetEventHandleForConnecting(), this);
   disconnecting_event_watcher_.StartWatchingMultipleTimes(

components/brave_vpn/brave_vpn_os_connection_api_win.h

@@ -52,7 +52,6 @@ class BraveVPNOSConnectionAPIWin : public BraveVPNOSConnectionAPI,
   void StartVPNConnectionChangeMonitoring();
 
   HANDLE event_handle_for_connected_ = NULL;
-  HANDLE event_handle_for_disconnected_ = NULL;
   base::win::ObjectWatcher connected_event_watcher_;
   base::win::ObjectWatcher connect_failed_event_watcher_;
   base::win::ObjectWatcher connecting_event_watcher_;

components/brave_vpn/brave_vpn_service.cc

@@ -13,6 +13,7 @@
 #include "base/json/json_writer.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
+#include "brave/components/brave_vpn/brave_vpn_service_helper.h"
 #include "brave/components/brave_vpn/brave_vpn_utils.h"
 #include "brave/components/brave_vpn/pref_names.h"
 #include "brave/components/p3a_utils/feature_usage.h"
@@ -24,7 +25,6 @@
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "url/url_util.h"
 
-#include "brave/components/brave_vpn/brave_vpn_service_helper.h"
 #if !BUILDFLAG(IS_ANDROID)
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -325,7 +325,7 @@ void BraveVpnService::CreateVPNConnection() {
   GetBraveVPNConnectionAPI()->CreateVPNConnection(GetConnectionInfo());
 }
 
-void BraveVpnService::RemoveVPNConnnection() {
+void BraveVpnService::RemoveVPNConnection() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   VLOG(2) << __func__;
   GetBraveVPNConnectionAPI()->RemoveVPNConnection(kBraveVPNEntryName);
@@ -345,10 +345,12 @@ void BraveVpnService::Connect() {
 
   // User can ask connect again when user want to change region.
   if (connection_state_ == ConnectionState::CONNECTED) {
+#if !BUILDFLAG(IS_WIN)
     // Disconnect first and then create again to setup for new region.
     needs_connect_ = true;
     Disconnect();
     return;
+#endif
   }
 
   VLOG(2) << __func__ << " : start connecting!";
@@ -873,6 +875,7 @@ void BraveVpnService::OnCreateSupportTicket(
           << "\nresponse_code=" << api_request_result.response_code();
   std::move(callback).Run(success, api_request_result.body());
 }
+
 void BraveVpnService::OnSuspend() {
   // Set reconnection state in case if computer/laptop is going to sleep.
   // The disconnection event will be fired after waking up and we want to

components/brave_vpn/brave_vpn_service.h

@@ -89,7 +89,7 @@ class BraveVpnService :
 
 #if !BUILDFLAG(IS_ANDROID)
   void ToggleConnection();
-  void RemoveVPNConnnection();
+  void RemoveVPNConnection();
   bool is_connected() const {
     return connection_state_ == mojom::ConnectionState::CONNECTED;
   }

components/brave_vpn/brave_vpn_unittest.cc

@@ -764,9 +764,11 @@ TEST_F(BraveVPNServiceTest, NeedsConnectTest) {
   // Handle connect after disconnect current connection.
   connection_state() = ConnectionState::CONNECTED;
   Connect();
+#if !BUILDFLAG(IS_WIN)
   EXPECT_TRUE(needs_connect());
   EXPECT_EQ(ConnectionState::DISCONNECTING, connection_state());
   OnDisconnected();
+#endif
   EXPECT_FALSE(needs_connect());
   EXPECT_EQ(ConnectionState::CONNECTING, connection_state());
 

components/brave_vpn/utils_win.cc

@@ -23,6 +23,7 @@ namespace {
 HANDLE g_connecting_event_handle = NULL;
 HANDLE g_connect_failed_event_handle = NULL;
 HANDLE g_disconnecting_event_handle = NULL;
+HANDLE g_disconnected_event_handle = NULL;
 
 void WINAPI RasDialFunc(UINT, RASCONNSTATE rasconnstate, DWORD error) {
   if (error) {
@@ -120,6 +121,19 @@ void CloseEventHandleForDisconnecting() {
   }
 }
 
+HANDLE GetEventHandleForDisconnected() {
+  if (!g_disconnected_event_handle)
+    g_disconnected_event_handle = CreateEvent(NULL, false, false, NULL);
+  return g_disconnected_event_handle;
+}
+
+void CloseEventHandleForDisconnected() {
+  if (g_disconnected_event_handle) {
+    CloseHandle(g_disconnected_event_handle);
+    g_disconnected_event_handle = NULL;
+  }
+}
+
 // https://docs.microsoft.com/en-us/windows/win32/api/ras/nf-ras-rasgeterrorstringa
 void PrintRasError(DWORD error) {
   constexpr DWORD kBufSize = 512;
@@ -172,7 +186,7 @@ std::wstring GetPhonebookPath() {
 }
 
 // https://docs.microsoft.com/en-us/windows/win32/api/ras/nf-ras-rasenumconnectionsa
-bool DisconnectEntry(const std::wstring& entry_name) {
+bool DisconnectEntry(const std::wstring& entry_name, bool notify) {
   DWORD dw_cb = 0;
   DWORD dw_ret = ERROR_SUCCESS;
   DWORD dw_connections = 0;
@@ -208,12 +222,15 @@ bool DisconnectEntry(const std::wstring& entry_name) {
         VLOG(2) << __func__ << " : " << name << ", " << type;
         if (name.compare(entry_name) == 0 && type.compare(L"VPN") == 0) {
           VLOG(2) << __func__ << " : Disconnect... " << entry_name;
-          SetEvent(g_disconnecting_event_handle);
+          if (notify)
+            SetEvent(g_disconnecting_event_handle);
           dw_ret = RasHangUpA(lp_ras_conn[i].hrasconn);
           break;
         }
       }
     }
+    if (dw_ret == ERROR_SUCCESS && notify)
+      SetEvent(g_disconnected_event_handle);
     // Deallocate memory for the connection buffer
     HeapFree(GetProcessHeap(), 0, lp_ras_conn);
     lp_ras_conn = NULL;
@@ -233,6 +250,8 @@ bool DisconnectEntry(const std::wstring& entry_name) {
 
 // https://docs.microsoft.com/en-us/windows/win32/api/ras/nf-ras-rasdiala
 bool ConnectEntry(const std::wstring& entry_name) {
+  DisconnectEntry(entry_name, false);
+
   LPRASDIALPARAMS lp_ras_dial_params = NULL;
   DWORD cb = sizeof(RASDIALPARAMS);
 

components/brave_vpn/utils_win.h

@@ -30,7 +30,7 @@ bool CreateEntry(const std::wstring& entry_name,
                  const std::wstring& username,
                  const std::wstring& password);
 bool RemoveEntry(const std::wstring& entry_name);
-bool DisconnectEntry(const std::wstring& entry_name);
+bool DisconnectEntry(const std::wstring& entry_name, bool notify = true);
 bool ConnectEntry(const std::wstring& entry_name);
 // Don't cache returned HANDLE. It could be invalidated.
 HANDLE GetEventHandleForConnecting();
@@ -39,6 +39,8 @@ HANDLE GetEventHandleForConnectFailed();
 void CloseEventHandleForConnectFailed();
 HANDLE GetEventHandleForDisconnecting();
 void CloseEventHandleForDisconnecting();
+HANDLE GetEventHandleForDisconnected();
+void CloseEventHandleForDisconnected();
 
 CheckConnectionResult CheckConnection(const std::wstring& entry_name);