Merge pull request #16285 from brave/brave_vpn_change_region_status

Ignore disconnected status while changing region

components/brave_vpn/brave_vpn_os_connection_api.cc

@@ -65,7 +65,7 @@ void BraveVPNOSConnectionAPI::CreateVPNConnection() {
   CreateVPNConnectionImpl(connection_info_);
 }
 
-void BraveVPNOSConnectionAPI::Connect(bool ignore_network_state) {
+void BraveVPNOSConnectionAPI::Connect() {
   if (IsInProgress()) {
     VLOG(2) << __func__ << ": Current state: " << connection_state_
             << " : prevent connecting while previous operation is in-progress";
@@ -88,12 +88,6 @@ void BraveVPNOSConnectionAPI::Connect(bool ignore_network_state) {
   VLOG(2) << __func__ << " : start connecting!";
   UpdateAndNotifyConnectionStateChange(ConnectionState::CONNECTING);
 
-  if (!ignore_network_state && !IsNetworkAvailable()) {
-    VLOG(2) << __func__ << ": Network is not available, failed to connect";
-    UpdateAndNotifyConnectionStateChange(ConnectionState::CONNECT_FAILED);
-    return;
-  }
-
   if (GetIsSimulation() || connection_info_.IsValid()) {
     VLOG(2) << __func__
             << " : Create os vpn entry with cached connection_info.";
@@ -218,16 +212,11 @@ void BraveVPNOSConnectionAPI::OnConnectFailed() {
 }
 
 void BraveVPNOSConnectionAPI::OnDisconnected() {
-  UpdateAndNotifyConnectionStateChange(IsNetworkAvailable()
-                                           ? ConnectionState::DISCONNECTED
-                                           : ConnectionState::CONNECT_FAILED);
+  UpdateAndNotifyConnectionStateChange(ConnectionState::DISCONNECTED);
 
   if (needs_connect_) {
     needs_connect_ = false;
-    // Right after disconnected, network could be unavailable shortly.
-    // In this situation, connect process should go ahead because
-    // because BraveVpnAPIRequest could handle network failure by retrying.
-    Connect(true /* ignore_network_state */);
+    Connect();
   }
 }
 
@@ -238,9 +227,9 @@ void BraveVPNOSConnectionAPI::OnIsDisconnecting() {
 
 void BraveVPNOSConnectionAPI::OnNetworkChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
-  VLOG(2) << __func__ << " : " << type;
   // It's rare but sometimes Brave doesn't get vpn status update from OS.
   // Checking here will make vpn status update properly in that situation.
+  VLOG(2) << __func__ << " : " << type;
   CheckConnection();
 }
 
@@ -250,21 +239,20 @@ void BraveVPNOSConnectionAPI::UpdateAndNotifyConnectionStateChange(
   if (connection_state_ == state)
     return;
 
-#if BUILDFLAG(IS_WIN)
-  // On Windows, we get disconnected status update twice.
-  // When user connects to different region while connected,
-  // we disconnect current connection and connect to newly selected
-  // region. To do that we monitor |DISCONNECTED| state and start
-  // connect when we get that state. But, Windows sends disconnected state
-  // noti again. So, ignore second one.
-  // On exception - we allow from connecting to disconnected in canceling
-  // scenario.
+  // Ignore disconnected state while connecting is in-progress.
+  // Network status can be changed during the vpn connection because
+  // establishing vpn connection could make system network offline temporarily.
+  // Whenever we get network status change, we check vpn connection state and
+  // it could give disconnected vpn connection during that situation.
+  // So, don't notify this disconnected state change while connecting because
+  // it's temporal state.
   if (connection_state_ == ConnectionState::CONNECTING &&
       state == ConnectionState::DISCONNECTED && !cancel_connecting_) {
     VLOG(2) << __func__ << ": Ignore disconnected state while connecting";
     return;
   }
 
+#if BUILDFLAG(IS_WIN)
   // On Windows, we could get disconnected state after connect failed.
   // To make connect failed state as a last state, ignore disconnected state.
   if (connection_state_ == ConnectionState::CONNECT_FAILED &&

components/brave_vpn/brave_vpn_os_connection_api.h

@@ -67,7 +67,7 @@ class BraveVPNOSConnectionAPI
   void SetConnectionState(mojom::ConnectionState state);
   bool IsInProgress() const;
 
-  void Connect(bool ignore_network_state = false);
+  void Connect();
   void Disconnect();
   void ToggleConnection();
   void RemoveVPNConnection();

components/brave_vpn/brave_vpn_os_connection_api_mac.mm

@@ -198,8 +198,28 @@ OSStatus StorePassword(const NSString* password) {
         OnCreateFailed();
         return;
       }
-      VLOG(2) << "Create - saveToPrefs success";
-      OnCreated();
+      // Load & save twice avoid connect failure.
+      // This load & save twice hack could eliminate connect failure
+      // when os vpn entry needs to be newly created during the connect
+      // process.
+      VLOG(2) << "Create - load & save again.";
+      [vpn_manager loadFromPreferencesWithCompletionHandler:^(
+                       NSError* load_again_error) {
+        if (load_again_error) {
+          OnCreateFailed();
+          return;
+        }
+
+        [vpn_manager saveToPreferencesWithCompletionHandler:^(
+                         NSError* save_again_error) {
+          if (save_again_error) {
+            OnCreateFailed();
+            return;
+          }
+          OnCreated();
+        }];
+      }];
+
     }];
   }];
 }

components/brave_vpn/brave_vpn_service.cc

@@ -585,14 +585,6 @@ void BraveVpnService::LoadPurchasedState(const std::string& domain) {
     return;
   }
 
-#if !BUILDFLAG(IS_ANDROID)
-  if (!IsNetworkAvailable()) {
-    VLOG(2) << __func__ << ": Network is not available, failed to connect";
-    GetBraveVPNConnectionAPI()->SetConnectionState(
-        ConnectionState::CONNECT_FAILED);
-    return;
-  }
-#endif
   if (!purchased_state_.has_value())
     SetPurchasedState(requested_env, PurchasedState::LOADING);
 

components/brave_vpn/brave_vpn_unittest.cc

@@ -870,70 +870,6 @@ TEST_F(BraveVPNServiceTest, NeedsConnectTest) {
   EXPECT_EQ(ConnectionState::CONNECTING, connection_state());
 }
 
-TEST_F(BraveVPNServiceTest, OnDisconnectedWithoutNetwork) {
-  std::string env = skus::GetDefaultEnvironment();
-  // Connection state can be changed with purchased.
-  SetPurchasedState(env, PurchasedState::PURCHASED);
-
-  SetDeviceRegion("eu-es");
-  auto network_change_notifier = net::NetworkChangeNotifier::CreateIfNeeded();
-  net::test::ScopedMockNetworkChangeNotifier mock_notifier;
-  mock_notifier.mock_network_change_notifier()->SetConnectionType(
-      net::NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_EQ(net::NetworkChangeNotifier::CONNECTION_NONE,
-            net::NetworkChangeNotifier::GetConnectionType());
-
-  // Handle connect after disconnect current connection.
-  // When we need another connect, it should make next connect call
-  // even if network is not available.
-  connection_state() = ConnectionState::CONNECTED;
-  needs_connect() = true;
-  OnDisconnected();
-  EXPECT_FALSE(needs_connect());
-  EXPECT_EQ(ConnectionState::CONNECTING, connection_state());
-
-  // Set connect failed when we don't want another connect.
-  connection_state() = ConnectionState::CONNECTED;
-  needs_connect() = false;
-  OnDisconnected();
-  EXPECT_EQ(ConnectionState::CONNECT_FAILED, connection_state());
-}
-
-TEST_F(BraveVPNServiceTest, ConnectWithoutNetwork) {
-  std::string env = skus::GetDefaultEnvironment();
-  SetPurchasedState(env, PurchasedState::PURCHASED);
-  auto network_change_notifier = net::NetworkChangeNotifier::CreateIfNeeded();
-  net::test::ScopedMockNetworkChangeNotifier mock_notifier;
-  mock_notifier.mock_network_change_notifier()->SetConnectionType(
-      net::NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_EQ(net::NetworkChangeNotifier::CONNECTION_NONE,
-            net::NetworkChangeNotifier::GetConnectionType());
-
-  // Handle connect without network.
-  connection_state() = ConnectionState::DISCONNECTED;
-  EXPECT_EQ(net::NetworkChangeNotifier::CONNECTION_NONE,
-            net::NetworkChangeNotifier::GetConnectionType());
-  TestBraveVPNServiceObserver observer;
-  AddObserver(observer.GetReceiver());
-  {
-    // State changed to Connecting.
-    base::RunLoop loop;
-    observer.WaitConnectionStateChange(loop.QuitClosure());
-    Connect();
-    loop.Run();
-    EXPECT_EQ(observer.GetConnectionState(), ConnectionState::CONNECTING);
-  }
-  {
-    // State changed to connection failed.
-    base::RunLoop loop;
-    observer.WaitConnectionStateChange(loop.QuitClosure());
-    loop.Run();
-    EXPECT_EQ(observer.GetConnectionState(), ConnectionState::CONNECT_FAILED);
-    EXPECT_FALSE(needs_connect());
-    EXPECT_EQ(ConnectionState::CONNECT_FAILED, connection_state());
-  }
-}
-
 TEST_F(BraveVPNServiceTest, LoadRegionDataFromPrefsTest) {
   std::string env = skus::GetDefaultEnvironment();
   // Initially, prefs doesn't have region data.
@@ -959,29 +895,6 @@ TEST_F(BraveVPNServiceTest, LoadRegionDataFromPrefsTest) {
   EXPECT_FALSE(regions().empty());
 }
 
-// Load purchased state without connection.
-TEST_F(BraveVPNServiceTest, PurchasedStateWithoutConnection) {
-  std::string env = skus::GetDefaultEnvironment();
-  std::string domain = skus::GetDomain("vpn", env);
-  TestBraveVPNServiceObserver observer;
-  AddObserver(observer.GetReceiver());
-  EXPECT_EQ(PurchasedState::NOT_PURCHASED, GetPurchasedStateSync());
-  SetPurchasedState(env, PurchasedState::PURCHASED);
-
-  EXPECT_EQ(PurchasedState::PURCHASED, GetPurchasedStateSync());
-  connection_state() = ConnectionState::CONNECTED;
-  auto network_change_notifier = net::NetworkChangeNotifier::CreateIfNeeded();
-  net::test::ScopedMockNetworkChangeNotifier mock_notifier;
-  mock_notifier.mock_network_change_notifier()->SetConnectionType(
-      net::NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_EQ(net::NetworkChangeNotifier::CONNECTION_NONE,
-            net::NetworkChangeNotifier::GetConnectionType());
-  LoadPurchasedState(domain);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(PurchasedState::PURCHASED, GetPurchasedStateSync());
-  EXPECT_EQ(observer.GetConnectionState(), ConnectionState::CONNECT_FAILED);
-}
-
 TEST_F(BraveVPNServiceTest, LoadPurchasedStateForAnotherEnvFailed) {
   auto development = SetupTestingStoreForEnv(skus::GetDefaultEnvironment());
   EXPECT_EQ(skus::GetEnvironmentForDomain(development),
@@ -1148,6 +1061,15 @@ TEST_F(BraveVPNServiceTest, CheckConnectionStateAfterNetworkStateChanged) {
 
   SetMockConnectionAPI(nullptr);
 }
+
+// Ignore disconnected state change while connected. See the comment at
+// BraveVPNOSConnectionAPI::UpdateAndNotifyConnectionStateChange().
+TEST_F(BraveVPNServiceTest, IgnoreDisconnectedStateWhileConnecting) {
+  connection_state() = ConnectionState::CONNECTING;
+  UpdateAndNotifyConnectionStateChange(ConnectionState::DISCONNECTED);
+  EXPECT_EQ(ConnectionState::CONNECTING, connection_state());
+}
+
 #endif
 
 TEST_F(BraveVPNServiceTest, GetPurchasedStateSync) {

components/brave_vpn/brave_vpn_utils.cc

@@ -21,7 +21,6 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
-#include "net/base/network_change_notifier.h"
 
 namespace brave_vpn {
 
@@ -116,11 +115,4 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
   RegisterVPNLocalStatePrefs(registry);
 }
 
-#if !BUILDFLAG(IS_ANDROID)
-bool IsNetworkAvailable() {
-  return net::NetworkChangeNotifier::GetConnectionType() !=
-         net::NetworkChangeNotifier::CONNECTION_NONE;
-}
-#endif  // !BUILDFLAG(IS_ANDROID)
-
 }  // namespace brave_vpn

components/brave_vpn/brave_vpn_utils.h

@@ -25,10 +25,6 @@ void RegisterLocalStatePrefs(PrefRegistrySimple* registry);
 void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 void RegisterAndroidProfilePrefs(PrefRegistrySimple* registry);
 
-#if !BUILDFLAG(IS_ANDROID)
-bool IsNetworkAvailable();
-#endif  // !BUILDFLAG(IS_ANDROID)
-
 }  // namespace brave_vpn
 
 #endif  // BRAVE_COMPONENTS_BRAVE_VPN_BRAVE_VPN_UTILS_H_