Preserved v1's selected region name for backward compatibility

brave · Sep 17, 2024 · 7414d1c · 7414d1c
1 parent 90d5261
commit 7414d1c
Show file tree

Hide file tree

Showing 7 changed files with 25 additions and 13 deletions.
diff --git a/components/brave_vpn/browser/brave_vpn_service.cc b/components/brave_vpn/browser/brave_vpn_service.cc
@@ -389,7 +389,7 @@ void BraveVpnService::UpdatePurchasedStateForSessionExpired(
 bool BraveVpnService::IsCurrentRegionSelectedAutomatically(
     const brave_vpn::mojom::RegionPtr& region) {
   const auto selected_region_name =
-      local_prefs_->GetString(prefs::kBraveVPNSelectedRegion);
+      local_prefs_->GetString(prefs::kBraveVPNSelectedRegionV2);
 
   if (region->region_precision == brave_vpn::mojom::kRegionPrecisionCountry &&
       selected_region_name.empty()) {

diff --git a/components/brave_vpn/browser/brave_vpn_service_unittest.cc b/components/brave_vpn/browser/brave_vpn_service_unittest.cc
@@ -865,7 +865,7 @@ TEST_F(BraveVPNServiceTest, GetProperRegionFromTimeZoneTest) {
   // timezone.
   // Clear selected region to set with device region when timezone
   // data is fetched.
-  local_pref_service_.ClearPref(prefs::kBraveVPNSelectedRegion);
+  local_pref_service_.ClearPref(prefs::kBraveVPNSelectedRegionV2);
   local_pref_service_.SetInteger(prefs::kBraveVPNRegionListVersion, 2);
   OnFetchRegionList(GetRegionsDataV2(), true);
   OnFetchTimezones(GetTimeZonesData(), true);

diff --git a/components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc b/components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc
@@ -40,7 +40,7 @@ bool BraveVPNRegionDataManager::IsRegionDataReady() const {
 }
 
 void BraveVPNRegionDataManager::SetSelectedRegion(std::string_view name) {
-  local_prefs_->SetString(prefs::kBraveVPNSelectedRegion, name);
+  local_prefs_->SetString(prefs::kBraveVPNSelectedRegionV2, name);
 
   if (selected_region_changed_callback_) {
     selected_region_changed_callback_.Run(GetSelectedRegion());
@@ -52,7 +52,7 @@ std::string BraveVPNRegionDataManager::GetSelectedRegion() const {
     CHECK_IS_TEST();
   }
 
-  auto region_name = local_prefs_->GetString(prefs::kBraveVPNSelectedRegion);
+  auto region_name = local_prefs_->GetString(prefs::kBraveVPNSelectedRegionV2);
   if (region_name.empty()) {
     // Gives device region if there is no cached selected region.
     VLOG(2) << __func__ << " : give device region instead.";
@@ -128,7 +128,7 @@ void BraveVPNRegionDataManager::SetDeviceRegionWithTimezone(
             GetMigratedNameIfNeeded(local_prefs_, *region_name);
         SetDeviceRegion(new_name);
         // Use device region as a default selected region.
-        if (local_prefs_->GetString(prefs::kBraveVPNSelectedRegion).empty()) {
+        if (local_prefs_->GetString(prefs::kBraveVPNSelectedRegionV2).empty()) {
           SetSelectedRegion(new_name);
         }
         return;

diff --git a/components/brave_vpn/browser/connection/ikev2/system_vpn_connection_api_unittest.cc b/components/brave_vpn/browser/connection/ikev2/system_vpn_connection_api_unittest.cc
@@ -473,7 +473,7 @@ TEST_F(SystemVPNConnectionAPIUnitTest, CreateOSVPNEntryWithInvalidInfoTest) {
 
   auto* test_api = GetConnectionAPI();
   test_api->CheckConnection();
-  local_state()->SetString(prefs::kBraveVPNSelectedRegion, "eu-be");
+  local_state()->SetString(prefs::kBraveVPNSelectedRegionV2, "eu-be");
   // Prepare valid connection info.
   test_api->OnFetchHostnames("eu-be", kHostNamesTestData, true);
   test_api->SetPreventCreationForTesting(true);
@@ -496,7 +496,7 @@ TEST_F(SystemVPNConnectionAPIUnitTest, NeedsConnectTest) {
 
   // Check ignore Connect() request while connecting or disconnecting is
   // in-progress.
-  local_state()->SetString(prefs::kBraveVPNSelectedRegion, "eu-be");
+  local_state()->SetString(prefs::kBraveVPNSelectedRegionV2, "eu-be");
   test_api->connection_state_ = mojom::ConnectionState::CONNECTING;
   test_api->Connect();
   EXPECT_EQ(mojom::ConnectionState::CONNECTING, test_api->GetConnectionState());

diff --git a/components/brave_vpn/common/brave_vpn_utils.cc b/components/brave_vpn/common/brave_vpn_utils.cc
@@ -36,6 +36,7 @@ void RegisterVPNLocalStatePrefs(PrefRegistrySimple* registry) {
   registry->RegisterTimePref(prefs::kBraveVPNRegionListFetchedDate, {});
   registry->RegisterStringPref(prefs::kBraveVPNDeviceRegion, "");
   registry->RegisterStringPref(prefs::kBraveVPNSelectedRegion, "");
+  registry->RegisterStringPref(prefs::kBraveVPNSelectedRegionV2, "");
 #endif
   registry->RegisterStringPref(prefs::kBraveVPNEnvironment,
                                skus::GetDefaultEnvironment());
@@ -87,12 +88,10 @@ void MigrateFromV1ToV2(PrefService* local_prefs) {
   // In this migration, selected region name is updated to matched v2's country
   // name.
   if (kV1ToV2Map.contains(selected_region_v1)) {
-    local_prefs->SetString(prefs::kBraveVPNSelectedRegion,
+    local_prefs->SetString(prefs::kBraveVPNSelectedRegionV2,
                            kV1ToV2Map.at(selected_region_v1));
-  } else {
-    // This will not be happened but added for safe during the startup.
-    local_prefs->ClearPref(prefs::kBraveVPNSelectedRegion);
   }
+
   local_prefs->SetInteger(prefs::kBraveVPNRegionListVersion, 2);
 }
 #endif

diff --git a/components/brave_vpn/common/brave_vpn_utils_unittest.cc b/components/brave_vpn/common/brave_vpn_utils_unittest.cc
@@ -203,9 +203,13 @@ TEST(BraveVPNUtilsUnitTest, SelectedRegionNameMigration) {
                                      "au-au");
   brave_vpn::MigrateLocalStatePrefs(&local_state_pref_service);
   EXPECT_EQ("ocn-aus", local_state_pref_service.GetString(
-                           brave_vpn::prefs::kBraveVPNSelectedRegion));
+                           brave_vpn::prefs::kBraveVPNSelectedRegionV2));
   EXPECT_EQ(2, local_state_pref_service.GetInteger(
                    brave_vpn::prefs::kBraveVPNRegionListVersion));
+
+  // Check v1's selected region is preserved.
+  EXPECT_EQ("au-au", local_state_pref_service.GetString(
+                         brave_vpn::prefs::kBraveVPNSelectedRegion));
 }
 
 TEST(BraveVPNUtilsUnitTest, InvalidSelectedRegionNameMigration) {
@@ -218,7 +222,7 @@ TEST(BraveVPNUtilsUnitTest, InvalidSelectedRegionNameMigration) {
                                      "invalid");
   brave_vpn::MigrateLocalStatePrefs(&local_state_pref_service);
   EXPECT_EQ("", local_state_pref_service.GetString(
-                    brave_vpn::prefs::kBraveVPNSelectedRegion));
+                    brave_vpn::prefs::kBraveVPNSelectedRegionV2));
   EXPECT_EQ(2, local_state_pref_service.GetInteger(
                    brave_vpn::prefs::kBraveVPNRegionListVersion));
 }

diff --git a/components/brave_vpn/common/pref_names.h b/components/brave_vpn/common/pref_names.h
@@ -23,8 +23,17 @@ inline constexpr char kBraveVPNRegionListFetchedDate[] =
     "brave.brave_vpn.region_list_fetched_date";
 inline constexpr char kBraveVPNDeviceRegion[] =
     "brave.brave_vpn.device_region_name";
+
+// For backward-compatibility, v1's selected region name is preserved.
+// If user runs older brave with migrated profile, it could make crash as
+// region data list v1 doesn't have entry for v2's selected region name.
+// Retaining only this data only is sufficient for backward compatibility
+// because region list and timezone data are fetched again as v2 data becomes
+// invalid in old browser.
 inline constexpr char kBraveVPNSelectedRegion[] =
     "brave.brave_vpn.selected_region_name";
+inline constexpr char kBraveVPNSelectedRegionV2[] =
+    "brave.brave_vpn.selected_region_name_v2";
 inline constexpr char kBraveVPNRegionListVersion[] =
     "brave.brave_vpn.region_list_version";
 #if BUILDFLAG(IS_WIN)