Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ignore disconnected status while changing region #16285

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Ignore disconnected status while changing region #16285

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59e4589
Ignore disconnected status while changing region
simonhong Dec 8, 2022
75920b0
Removed network state checking via IsNetworkAvailable()
simonhong Dec 9, 2022
edaedb9
Do load & save prefs twice when creating vpn entry on macOS
simonhong Dec 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 12 additions & 24 deletions components/brave_vpn/brave_vpn_os_connection_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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.";
Expand Down Expand Up @@ -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();
}
}

Expand All @@ -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();
}

Expand All @@ -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.
Copy link
Member

@bsclifton bsclifton Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These updates to comments lose the context for WHEN this can happen- which is when person is changing regions.

Can we add that back in?

// When user connects to different region while connected,
// we disconnect current connection and connect to newly selected
// region.

Copy link
Member

@bsclifton bsclifton Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait- I missed the actual change to the logic 😅 You're correct to delete this, sorry!

// 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 &&
Expand Down
2 changes: 1 addition & 1 deletion components/brave_vpn/brave_vpn_os_connection_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
8 changes: 0 additions & 8 deletions components/brave_vpn/brave_vpn_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,14 +585,6 @@ void BraveVpnService::LoadPurchasedState(const std::string& domain) {
return;
}

#if !BUILDFLAG(IS_ANDROID)
if (!IsNetworkAvailable()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing this code is great 👍 LoadPurchasedState is called when person is clicking on VPN button... and if they didn't have network connectivity (briefly, for example), this code would be letting the connection go into FAILED (when no connect ever was made by user).

Nice! 😄 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we cache expiration date, browser will enter to purchased state even network is disabled.
Then, user can get connect failure when user try to connect.
Or it's already expired, browser will ask to refresh skus credential and skus service will make use not purchased state.
So we could get proper state w/o this early checking.

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);

Expand Down
96 changes: 9 additions & 87 deletions components/brave_vpn/brave_vpn_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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),
Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 0 additions & 8 deletions components/brave_vpn/brave_vpn_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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
4 changes: 0 additions & 4 deletions components/brave_vpn/brave_vpn_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_