From b6829008f2a9729e7caddaec2a2dd33a873c8623 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Mon, 5 Aug 2024 11:04:43 -0700 Subject: [PATCH] Deprecate flag kProcessBwuFrameAfterPcpConnected PiperOrigin-RevId: 659611911 --- connections/implementation/bwu_manager.cc | 5 +- .../implementation/bwu_manager_test.cc | 122 +++++++----------- .../flags/nearby_connections_feature_flags.h | 4 - connections/implementation/payload_manager.cc | 5 +- .../implementation/payload_manager_test.cc | 4 - 5 files changed, 48 insertions(+), 92 deletions(-) diff --git a/connections/implementation/bwu_manager.cc b/connections/implementation/bwu_manager.cc index 6facce46..0a763182 100644 --- a/connections/implementation/bwu_manager.cc +++ b/connections/implementation/bwu_manager.cc @@ -474,10 +474,7 @@ void BwuManager::OnBwuNegotiationFrame(ClientProxy* client, << BwuNegotiationFrame::EventType_Name(frame.event_type()) << " frame for endpoint " << endpoint_id; - if (NearbyFlags::GetInstance().GetBoolFlag( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected) && - !client->IsConnectedToEndpoint(endpoint_id)) { + if (!client->IsConnectedToEndpoint(endpoint_id)) { NEARBY_LOGS(WARNING) << "BwuManager skips the process BANDWIDTH_UPGRADE_NEGOTIATION before " "PCP connected, " diff --git a/connections/implementation/bwu_manager_test.cc b/connections/implementation/bwu_manager_test.cc index 347ef579..56d07afd 100644 --- a/connections/implementation/bwu_manager_test.cc +++ b/connections/implementation/bwu_manager_test.cc @@ -33,6 +33,7 @@ #include "connections/implementation/service_id_constants.h" #include "connections/listeners.h" #include "internal/flags/nearby_flags.h" +#include "internal/platform/byte_array.h" #include "internal/platform/exception.h" #include "internal/platform/feature_flags.h" #include "internal/proto/analytics/connections_log.pb.h" @@ -94,9 +95,15 @@ class BwuManagerTest : public ::testing::Test { // Create the initial device-to-device connection, before bandwidth upgrade. // Typically |medium| will be Bluetooth. - FakeEndpointChannel* CreateInitialEndpoint(absl::string_view service_id, + FakeEndpointChannel* CreateInitialEndpoint(ClientProxy* client, + absl::string_view service_id, absl::string_view endpoint_id, Medium medium) { + client->OnConnectionInitiated( + std::string(endpoint_id), + {.remote_endpoint_info = ByteArray("remote endpoint")}, + {.auto_upgrade_bandwidth = false}, {}, ""); + client->OnConnectionAccepted(std::string(endpoint_id)); auto channel = std::make_unique(medium, std::string(service_id)); FakeEndpointChannel* channel_raw = channel.get(); @@ -239,16 +246,9 @@ class BwuManagerTestParam : public BwuManagerTest, }; TEST_P(BwuManagerTestParam, InitiateBwu_Success) { - auto flag = NearbyFlags::GetInstance().GetBoolFlag( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected); - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected, - false); // Create the initial device-to-device Bluetooth connection. - FakeEndpointChannel* initial_channel = - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + FakeEndpointChannel* initial_channel = CreateInitialEndpoint( + &client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); // Initiate BWU, and send BANDWIDTH_UPGRADE_NEGOTIATION.UPGRADE_PATH_AVAILABLE // to the Responder over the initial Bluetooth channel. @@ -306,15 +306,11 @@ TEST_P(BwuManagerTestParam, InitiateBwu_Success) { EXPECT_EQ(location::nearby::proto::connections::DisconnectionReason::UPGRADED, old_channel->disconnection_reason()); UnRegisterChannelForEndpoint(kEndpointId1); - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected, - flag); } TEST_P(BwuManagerTestParam, InitiateBwu_Error_DontUpgradeIfAlreadyConenctedOverTheRequestedMedium) { - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); EXPECT_EQ(1u, fake_web_rtc_bwu_handler_->handle_initialize_calls().size()); @@ -328,7 +324,7 @@ TEST_P(BwuManagerTestParam, TEST_P(BwuManagerTestParam, InitiateBwu_Error_DontUpgradeFromWIFI_LANToWIFI_HOTSPOT) { - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::WIFI_LAN); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::WIFI_LAN); // Ignore request to upgrade to WebRTC if we're already connected. bwu_manager_->InitiateBwuForEndpoint(&client_, std::string(kEndpointId1), @@ -352,7 +348,7 @@ TEST_P(BwuManagerTestParam, InitiateBwu_Error_NoInitialMedium) { } TEST_P(BwuManagerTestParam, InitiateBwu_Error_UpgradeAlreadyInProgress) { - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); bwu_manager_->InitiateBwuForEndpoint(&client_, std::string(kEndpointId1), Medium::WEB_RTC); @@ -373,8 +369,8 @@ TEST_P(BwuManagerTestParam, InitiateBwu_Error_UpgradeAlreadyInProgress) { TEST_P(BwuManagerTestParam, InitiateBwu_Error_FailedToWriteUpgradePathAvailableFrame) { // Create the initial device-to-device Bluetooth connection. - FakeEndpointChannel* initial_channel = - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + FakeEndpointChannel* initial_channel = CreateInitialEndpoint( + &client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); // Make the initial endpoint channel fail when writing the // UPGRADE_PATH_AVAILABLE frame. @@ -407,8 +403,8 @@ TEST_F(BwuManagerTest, FeatureFlags::GetMutableFlagsForTesting().support_multiple_bwu_mediums = true; // Say we have two already upgraded WebRTC connections for the same service. - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdA, kEndpointId2, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId2, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); FullyUpgradeEndpoint(kEndpointId2, /*initial_medium=*/Medium::BLUETOOTH, @@ -457,8 +453,8 @@ TEST_F(BwuManagerTest, false; // Say we have two already upgraded WebRTC connections for the same service. - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdA, kEndpointId2, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId2, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); FullyUpgradeEndpoint(kEndpointId2, /*initial_medium=*/Medium::BLUETOOTH, @@ -513,8 +509,8 @@ TEST_F(BwuManagerTest, FeatureFlags::GetMutableFlagsForTesting().support_multiple_bwu_mediums = true; // Say we have two already upgraded WLAN connections for different services. - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdB, kEndpointId2, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdB, kEndpointId2, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WIFI_LAN); FullyUpgradeEndpoint(kEndpointId2, /*initial_medium=*/Medium::BLUETOOTH, @@ -568,8 +564,8 @@ TEST_F(BwuManagerTest, false; // Say we have two already upgraded WLAN connections for different services. - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdB, kEndpointId2, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdB, kEndpointId2, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WIFI_LAN); FullyUpgradeEndpoint(kEndpointId2, /*initial_medium=*/Medium::BLUETOOTH, @@ -627,11 +623,11 @@ TEST_F( // Say we have three upgraded connections for two different services and two // different mediums. - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdA, kEndpointId2, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdB, kEndpointId3, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdB, kEndpointId4, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdB, kEndpointId5, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId2, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdB, kEndpointId3, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdB, kEndpointId4, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdB, kEndpointId5, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); FullyUpgradeEndpoint(kEndpointId4, /*initial_medium=*/Medium::BLUETOOTH, @@ -775,15 +771,15 @@ TEST_F(BwuManagerTest, InitiateBwu_Revert_OnUpgradeFailure_FlagEnabled) { FeatureFlags::GetMutableFlagsForTesting().support_multiple_bwu_mediums = true; // Say we have two already upgraded WebRTC connections for service A. - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdA, kEndpointId2, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId2, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); FullyUpgradeEndpoint(kEndpointId2, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); // Service B has an initial Bluetooth connection that it tries to upgrade. - CreateInitialEndpoint(kServiceIdB, kEndpointId3, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdB, kEndpointId3, Medium::BLUETOOTH); bwu_manager_->InitiateBwuForEndpoint(&client_, std::string(kEndpointId3), Medium::WEB_RTC); fake_web_rtc_bwu_handler_->NotifyBwuManagerOfIncomingConnection( @@ -813,15 +809,15 @@ TEST_F(BwuManagerTest, InitiateBwu_Revert_OnUpgradeFailure_FlagDisabled) { false; // Say we have two already upgraded WebRTC connections for service A. - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - CreateInitialEndpoint(kServiceIdA, kEndpointId2, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId2, Medium::BLUETOOTH); FullyUpgradeEndpoint(kEndpointId1, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); FullyUpgradeEndpoint(kEndpointId2, /*initial_medium=*/Medium::BLUETOOTH, /*upgrade_medium=*/Medium::WEB_RTC); // Service B has an initial Bluetooth connection that it tries to upgrade. - CreateInitialEndpoint(kServiceIdB, kEndpointId3, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdB, kEndpointId3, Medium::BLUETOOTH); bwu_manager_->InitiateBwuForEndpoint(&client_, std::string(kEndpointId3), Medium::WEB_RTC); fake_web_rtc_bwu_handler_->NotifyBwuManagerOfIncomingConnection( @@ -848,7 +844,7 @@ TEST_F(BwuManagerTest, InitiateBwu_Revert_OnUpgradeFailure_FlagDisabled) { TEST_F(BwuManagerTest, InitiateBwu_Revert_OnDisconnect_WifiDirect) { FeatureFlags::GetMutableFlagsForTesting().support_multiple_bwu_mediums = true; OfflineFrame frame; - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); ByteArray bytes = parser::ForBwuWifiDirectPathAvailable( /*ssid=*/"Direct-12345678", /*password=*/"87654321", /*port=*/2143, @@ -881,7 +877,7 @@ TEST_F(BwuManagerTest, InitiateBwu_Revert_OnDisconnect_WifiDirect) { TEST_F(BwuManagerTest, InitiateBwu_Revert_OnDisconnect_Hotspot) { FeatureFlags::GetMutableFlagsForTesting().support_multiple_bwu_mediums = true; - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); ExceptionOr hotspot_path_available_frame = parser::FromBytes(parser::ForBwuWifiHotspotPathAvailable( @@ -911,7 +907,7 @@ TEST_F(BwuManagerTest, InitiateBwu_Revert_OnDisconnect_Hotspot) { TEST_F(BwuManagerTest, InitiateBwu_Revert_OnDisconnect_Wlan) { FeatureFlags::GetMutableFlagsForTesting().support_multiple_bwu_mediums = true; - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); + CreateInitialEndpoint(&client_, kServiceIdA, kEndpointId1, Medium::BLUETOOTH); ExceptionOr wlan_path_available_frame = parser::FromBytes( parser::ForBwuWifiLanPathAvailable(/*ip_address=*/"ABCD", @@ -945,39 +941,10 @@ TEST_F(BwuManagerTest, OnProcessBwuEvent) { } TEST_F(BwuManagerTest, BlockBwuFrameBeforeAccept) { - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected, - false); - CreateInitialEndpoint(kServiceIdA, kEndpointId1, Medium::BLUETOOTH); - - ExceptionOr hotspot_path_available_frame = - parser::FromBytes(parser::ForBwuWifiHotspotPathAvailable( - /*ssid=*/"Direct-357a2d8c", /*password=*/"b592f7d3", - /*port=*/1234, /*frequency=*/2412, /*gateway=*/"123.234.23.1", true)); - OfflineFrame frame = hotspot_path_available_frame.result(); - frame.set_version(OfflineFrame::V1); - auto* v1_frame = frame.mutable_v1(); - auto* sub_frame = v1_frame->mutable_bandwidth_upgrade_negotiation(); - sub_frame->set_event_type( - BandwidthUpgradeNegotiationFrame::UPGRADE_PATH_AVAILABLE); - auto* upgrade_path_info = sub_frame->mutable_upgrade_path_info(); - - upgrade_path_info->set_supports_client_introduction_ack(false); - upgrade_path_info->set_supports_disabling_encryption(true); - bwu_manager_->OnIncomingFrame(frame, std::string(kEndpointId1), &client_, - Medium::BLUETOOTH, packet_meta_data_); - CountDownLatch latch(1); - // The BWU frame should not be drop, so the inProgressUpgrades should not be - // empty. - ASSERT_EQ(bwu_manager_->IsUpgradeOngoing(std::string(kEndpointId1)), true); - UnRegisterChannelForEndpoint(kEndpointId1); - - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected, - true); - CreateInitialEndpoint(kServiceIdA, kEndpointId2, Medium::BLUETOOTH); + auto channel = std::make_unique( + Medium::BLUETOOTH, std::string(kServiceIdA)); + ecm_.RegisterChannelForEndpoint(&client_, std::string(kEndpointId2), + std::move(channel)); ExceptionOr hotspot_path_available_frame2 = parser::FromBytes(parser::ForBwuWifiHotspotPathAvailable( @@ -987,7 +954,7 @@ TEST_F(BwuManagerTest, BlockBwuFrameBeforeAccept) { frame2.set_version(OfflineFrame::V1); auto* v1_frame2 = frame2.mutable_v1(); auto* sub_frame2 = v1_frame2->mutable_bandwidth_upgrade_negotiation(); - sub_frame->set_event_type( + sub_frame2->set_event_type( BandwidthUpgradeNegotiationFrame::UPGRADE_PATH_AVAILABLE); auto* upgrade_path_info2 = sub_frame2->mutable_upgrade_path_info(); @@ -1024,7 +991,10 @@ TEST_F(BwuManagerTest, BlockBwuFrameFromAdvertiser) { }; ConnectionOptions connection_options; - CreateInitialEndpoint(kServiceIdA, kEndpointId2, Medium::BLUETOOTH); + auto channel = std::make_unique( + Medium::BLUETOOTH, std::string(kServiceIdA)); + ecm_.RegisterChannelForEndpoint(&client_, std::string(kEndpointId2), + std::move(channel)); client_.OnConnectionInitiated(std::string(kEndpointId2), response_info, connection_options, {}, "token"); diff --git a/connections/implementation/flags/nearby_connections_feature_flags.h b/connections/implementation/flags/nearby_connections_feature_flags.h index 55f35217..bf1d944d 100644 --- a/connections/implementation/flags/nearby_connections_feature_flags.h +++ b/connections/implementation/flags/nearby_connections_feature_flags.h @@ -62,10 +62,6 @@ constexpr auto kEnablePayloadReceivedAck = constexpr auto kSafeToDisconnectVersion = flags::Flag(kConfigPackage, "45425841", 0); -// When true, skip the BWU frame if the pcp is not connected. -constexpr auto kProcessBwuFrameAfterPcpConnected = - flags::Flag(kConfigPackage, "45627835", false); - // When true, check illegal characters in the file name and parent folder. constexpr auto kCheckIllegalCharacters = flags::Flag(kConfigPackage, "45632028", false); diff --git a/connections/implementation/payload_manager.cc b/connections/implementation/payload_manager.cc index 30dbbc51..9c6d2c52 100644 --- a/connections/implementation/payload_manager.cc +++ b/connections/implementation/payload_manager.cc @@ -524,10 +524,7 @@ void PayloadManager::OnIncomingFrame( // Block any payload before the connection been accepted by both sides // to prevent unauthorized transfer. - if (NearbyFlags::GetInstance().GetBoolFlag( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected) && - !to_client->IsConnectedToEndpoint(from_endpoint_id)) { + if (!to_client->IsConnectedToEndpoint(from_endpoint_id)) { if (frame.packet_type() == PayloadTransferFrame::DATA) { PendingPayloadHandle pending_payload = pending_payloads_.GetPayload(frame.payload_header().id()); diff --git a/connections/implementation/payload_manager_test.cc b/connections/implementation/payload_manager_test.cc index 508fd9d4..2aa86412 100644 --- a/connections/implementation/payload_manager_test.cc +++ b/connections/implementation/payload_manager_test.cc @@ -416,10 +416,6 @@ TEST_P(PayloadManagerTest, SendPayloadWithSkip_StreamPayload) { } TEST_P(PayloadManagerTest, OfflineFrame_BeforeConnected_ShouldDrop) { - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_connections_feature:: - kProcessBwuFrameAfterPcpConnected, - false); env_.Start(); PayloadSimulationUser user(kDeviceB, GetParam()); auto [input, tx] = CreatePipe();