diff --git a/components/brave_vpn/brave_vpn_os_connection_api.cc b/components/brave_vpn/brave_vpn_os_connection_api.cc index 8dde6e41a60f..27d22dd619d8 100644 --- a/components/brave_vpn/brave_vpn_os_connection_api.cc +++ b/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 && diff --git a/components/brave_vpn/brave_vpn_os_connection_api.h b/components/brave_vpn/brave_vpn_os_connection_api.h index 04f7bc4d7413..08d11c282fed 100644 --- a/components/brave_vpn/brave_vpn_os_connection_api.h +++ b/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(); diff --git a/components/brave_vpn/brave_vpn_os_connection_api_mac.mm b/components/brave_vpn/brave_vpn_os_connection_api_mac.mm index aca04a06ff6c..d4a3f534c855 100644 --- a/components/brave_vpn/brave_vpn_os_connection_api_mac.mm +++ b/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(); + }]; + }]; + }]; }]; } diff --git a/components/brave_vpn/brave_vpn_service.cc b/components/brave_vpn/brave_vpn_service.cc index ec52ca1ef5b5..dd97e467b9b3 100644 --- a/components/brave_vpn/brave_vpn_service.cc +++ b/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); diff --git a/components/brave_vpn/brave_vpn_unittest.cc b/components/brave_vpn/brave_vpn_unittest.cc index 13a087303a75..0b76b2f5011c 100644 --- a/components/brave_vpn/brave_vpn_unittest.cc +++ b/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) { diff --git a/components/brave_vpn/brave_vpn_utils.cc b/components/brave_vpn/brave_vpn_utils.cc index 40d107b57d11..49c2ca72ad8f 100644 --- a/components/brave_vpn/brave_vpn_utils.cc +++ b/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 diff --git a/components/brave_vpn/brave_vpn_utils.h b/components/brave_vpn/brave_vpn_utils.h index 4857188405de..0ff191a23be7 100644 --- a/components/brave_vpn/brave_vpn_utils.h +++ b/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_