Merge pull request #16088 from brave/brave_vpn_win_improve

Improve vpn connection logic on Windows

components/brave_vpn/brave_vpn_os_connection_api_win.cc

@@ -19,13 +19,7 @@
 // ([email protected])'s work (https://github.com/bsclifton/winvpntool).
 
 using brave_vpn::internal::CheckConnectionResult;
-using brave_vpn::internal::CloseEventHandleForConnectFailed;
-using brave_vpn::internal::CloseEventHandleForConnecting;
-using brave_vpn::internal::CloseEventHandleForDisconnecting;
 using brave_vpn::internal::CreateEntry;
-using brave_vpn::internal::GetEventHandleForConnectFailed;
-using brave_vpn::internal::GetEventHandleForConnecting;
-using brave_vpn::internal::GetEventHandleForDisconnecting;
 using brave_vpn::internal::GetPhonebookPath;
 using brave_vpn::internal::PrintRasError;
 using brave_vpn::internal::RemoveEntry;
@@ -34,8 +28,8 @@ namespace brave_vpn {
 
 namespace {
 
-void ConnectEntry(const std::wstring& name) {
-  brave_vpn::internal::ConnectEntry(name);
+bool ConnectEntry(const std::wstring& name) {
+  return brave_vpn::internal::ConnectEntry(name);
 }
 
 void DisconnectEntry(const std::wstring& name) {
@@ -55,11 +49,7 @@ BraveVPNOSConnectionAPIWin::BraveVPNOSConnectionAPIWin() {
 }
 
 BraveVPNOSConnectionAPIWin::~BraveVPNOSConnectionAPIWin() {
-  CloseHandle(event_handle_for_connected_);
-  CloseHandle(event_handle_for_disconnected_);
-  CloseEventHandleForConnecting();
-  CloseEventHandleForDisconnecting();
-  CloseEventHandleForConnectFailed();
+  CloseHandle(event_handle_for_connected_disconnected_);
 }
 
 void BraveVPNOSConnectionAPIWin::CreateVPNConnectionImpl(
@@ -76,9 +66,11 @@ void BraveVPNOSConnectionAPIWin::CreateVPNConnectionImpl(
 
 void BraveVPNOSConnectionAPIWin::ConnectImpl(const std::string& name) {
   // Connection state update from this call will be done by monitoring.
-  base::ThreadPool::PostTask(
+  base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&ConnectEntry, base::UTF8ToWide(name)));
+      base::BindOnce(&ConnectEntry, base::UTF8ToWide(name)),
+      base::BindOnce(&BraveVPNOSConnectionAPIWin::OnConnected,
+                     weak_factory_.GetWeakPtr()));
 }
 
 void BraveVPNOSConnectionAPIWin::DisconnectImpl(const std::string& name) {
@@ -108,30 +100,21 @@ void BraveVPNOSConnectionAPIWin::CheckConnectionImpl(const std::string& name) {
 void BraveVPNOSConnectionAPIWin::OnObjectSignaled(HANDLE object) {
   DCHECK(!target_vpn_entry_name().empty());
 
-  CheckConnectionResult result = CheckConnectionResult::DISCONNECTING;
-  if (object == GetEventHandleForConnecting()) {
-    result = CheckConnectionResult::CONNECTING;
-  } else if (object == GetEventHandleForConnectFailed()) {
-    result = CheckConnectionResult::CONNECT_FAILED;
-  } else if (object == GetEventHandleForDisconnecting()) {
-    result = CheckConnectionResult::DISCONNECTING;
-  } else if (object == event_handle_for_connected_) {
-    result = CheckConnectionResult::CONNECTED;
-  } else if (object == event_handle_for_disconnected_) {
-    result = CheckConnectionResult::DISCONNECTED;
-  } else {
-    NOTREACHED();
+  // Check connection state for BraveVPN entry again when connected or
+  // disconnected events are arrived because we can get both event from any os
+  // vpn entry. All other events are sent by our code at utils_win.cc.
+  if (object == event_handle_for_connected_disconnected_) {
+    CheckConnectionImpl(target_vpn_entry_name());
+    return;
   }
-
-  OnCheckConnection(target_vpn_entry_name(), result);
 }
 
 void BraveVPNOSConnectionAPIWin::OnCheckConnection(
     const std::string& name,
     CheckConnectionResult result) {
   switch (result) {
     case CheckConnectionResult::CONNECTED:
-      OnConnected();
+      BraveVPNOSConnectionAPI::OnConnected();
       break;
     case CheckConnectionResult::CONNECTING:
       OnIsConnecting();
@@ -160,33 +143,27 @@ void BraveVPNOSConnectionAPIWin::OnCreated(const std::string& name,
   BraveVPNOSConnectionAPI::OnCreated();
 }
 
+void BraveVPNOSConnectionAPIWin::OnConnected(bool success) {
+  success ? BraveVPNOSConnectionAPI::OnConnected()
+          : BraveVPNOSConnectionAPI::OnConnectFailed();
+}
+
 void BraveVPNOSConnectionAPIWin::OnRemoved(const std::string& name,
                                            bool success) {}
 
 void BraveVPNOSConnectionAPIWin::StartVPNConnectionChangeMonitoring() {
-  DCHECK(!event_handle_for_connected_ && !event_handle_for_disconnected_);
+  DCHECK(!event_handle_for_connected_disconnected_);
 
-  event_handle_for_connected_ = CreateEvent(NULL, false, false, NULL);
-  event_handle_for_disconnected_ = CreateEvent(NULL, false, false, NULL);
+  event_handle_for_connected_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);
+  // Ase we pass INVALID_HANDLE_VALUE, we can get connected or disconnected
+  // event from any os vpn entry. It's filtered by OnObjectSignaled().
   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);
-  connecting_event_watcher_.StartWatchingMultipleTimes(
-      GetEventHandleForConnecting(), this);
-  disconnecting_event_watcher_.StartWatchingMultipleTimes(
-      GetEventHandleForDisconnecting(), this);
-  connect_failed_event_watcher_.StartWatchingMultipleTimes(
-      GetEventHandleForConnectFailed(), this);
+                             event_handle_for_connected_disconnected_,
+                             RASCN_Connection | RASCN_Disconnection);
+  connected_disconnected_event_watcher_.StartWatchingMultipleTimes(
+      event_handle_for_connected_disconnected_, this);
 }
 
 }  // namespace brave_vpn

components/brave_vpn/brave_vpn_os_connection_api_win.h

@@ -44,19 +44,15 @@ class BraveVPNOSConnectionAPIWin : public BraveVPNOSConnectionAPI,
   void OnObjectSignaled(HANDLE object) override;
 
   void OnCreated(const std::string& name, bool success);
+  void OnConnected(bool success);
   void OnRemoved(const std::string& name, bool success);
   void OnCheckConnection(const std::string& name,
                          internal::CheckConnectionResult result);
 
   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_;
-  base::win::ObjectWatcher disconnecting_event_watcher_;
-  base::win::ObjectWatcher disconnected_event_watcher_;
+  HANDLE event_handle_for_connected_disconnected_ = NULL;
+  base::win::ObjectWatcher connected_disconnected_event_watcher_;
   base::WeakPtrFactory<BraveVPNOSConnectionAPIWin> weak_factory_{this};
 };