diff --git a/sharing/flags/generated/nearby_sharing_feature_flags.h b/sharing/flags/generated/nearby_sharing_feature_flags.h index 800299ae..426056d0 100755 --- a/sharing/flags/generated/nearby_sharing_feature_flags.h +++ b/sharing/flags/generated/nearby_sharing_feature_flags.h @@ -73,9 +73,6 @@ constexpr auto kShowAutoUpdateSetting = // When true, use gRpc client to access backend. constexpr auto kUseGrpcClient = flags::Flag(kConfigPackage, "45630055", false); -// When true, dedup discovered endpoints. -constexpr auto kApplyEndpointsDedup = - flags::Flag(kConfigPackage, "45656298", false); // When true, call the 3P Nearby Identity API instead of the 1P private API constexpr auto kCallNearbyIdentityApi = flags::Flag(kConfigPackage, "45667328", false); @@ -128,7 +125,6 @@ inline absl::btree_map&> GetBoolFlags() { {45411353, kSenderSkipsConfirmation}, {45409033, kShowAutoUpdateSetting}, {45630055, kUseGrpcClient}, - {45656298, kApplyEndpointsDedup}, {45667328, kCallNearbyIdentityApi}, {45664277, kDedupInUnregisterShareTarget}, {45657036, kDeleteUnexpectedReceivedFileFix}, diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index c334b821..c1736dea 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -1667,19 +1667,6 @@ void NearbySharingServiceImpl::HandleEndpointDiscovered( // Check outgoingShareSessionMap first and pass the same shareTarget if we // found one. - // Looking for the ShareTarget based on endpoint id. - if (!NearbyFlags::GetInstance().GetBoolFlag( - config_package_nearby::nearby_sharing_feature:: - kApplyEndpointsDedup)) { - if (outgoing_share_target_map_.find(endpoint_id) != - outgoing_share_target_map_.end()) { - LOG(INFO) << __func__ << ": Ignoring endpoint_id: " << endpoint_id - << " because it is already in the " - << "outgoingShareTargetMap."; - FinishEndpointDiscoveryEvent(); - return; - } - } // Once we get the advertisement, the first thing to do is decrypt the // certificate. NearbyShareEncryptedMetadataKey encrypted_metadata_key( @@ -1723,16 +1710,10 @@ void NearbySharingServiceImpl::HandleEndpointLost( discovered_advertisements_to_retry_map_.erase(endpoint_id); discovered_advertisements_retried_set_.erase(endpoint_id); - if (NearbyFlags::GetInstance().GetBoolFlag( - config_package_nearby::nearby_sharing_feature:: - kApplyEndpointsDedup)) { - MoveToDiscoveryCache(endpoint_id, - NearbyFlags::GetInstance().GetInt64Flag( - config_package_nearby::nearby_sharing_feature:: - kDiscoveryCacheLostExpiryMs)); - } else { - RemoveOutgoingShareTargetAndReportLost(endpoint_id); - } + MoveToDiscoveryCache(endpoint_id, + NearbyFlags::GetInstance().GetInt64Flag( + config_package_nearby::nearby_sharing_feature:: + kDiscoveryCacheLostExpiryMs)); FinishEndpointDiscoveryEvent(); } @@ -1753,19 +1734,6 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( absl::string_view endpoint_id, absl::Span endpoint_info, const Advertisement& advertisement, std::optional certificate) { - bool apply_endpoints_dedup = NearbyFlags::GetInstance().GetBoolFlag( - config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup); - if (!apply_endpoints_dedup) { - // Check again for this endpoint id, to avoid race conditions. - if (outgoing_share_target_map_.find(endpoint_id) != - outgoing_share_target_map_.end()) { - LOG(INFO) << __func__ << ": Ignoring endpoint_id: " << endpoint_id - << " because it is already in the " - << "outgoingShareTargetMap."; - FinishEndpointDiscoveryEvent(); - return; - } - } // The certificate provides the device name, in order to create a ShareTarget // to represent this remote device. @@ -1793,19 +1761,17 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( FinishEndpointDiscoveryEvent(); return; } - if (apply_endpoints_dedup) { - if (FindDuplicateInOutgoingShareTargets(endpoint_id, *share_target)) { - DeduplicateInOutgoingShareTarget(*share_target, endpoint_id, - std::move(certificate)); - FinishEndpointDiscoveryEvent(); - return; - } - if (FindDuplicateInDiscoveryCache(endpoint_id, *share_target)) { - DeDuplicateInDiscoveryCache(*share_target, endpoint_id, - std::move(certificate)); - FinishEndpointDiscoveryEvent(); - return; - } + if (FindDuplicateInOutgoingShareTargets(endpoint_id, *share_target)) { + DeduplicateInOutgoingShareTarget(*share_target, endpoint_id, + std::move(certificate)); + FinishEndpointDiscoveryEvent(); + return; + } + if (FindDuplicateInDiscoveryCache(endpoint_id, *share_target)) { + DeDuplicateInDiscoveryCache(*share_target, endpoint_id, + std::move(certificate)); + FinishEndpointDiscoveryEvent(); + return; } VLOG(1) << __func__ << ": Adding (endpoint_id=" << endpoint_id @@ -3518,9 +3484,6 @@ void NearbySharingServiceImpl::UnregisterShareTarget(int64_t share_target_id) { auto it = outgoing_share_session_map_.find(share_target_id); if (it != outgoing_share_session_map_.end()) { if (NearbyFlags::GetInstance().GetBoolFlag( - config_package_nearby::nearby_sharing_feature:: - kApplyEndpointsDedup) && - NearbyFlags::GetInstance().GetBoolFlag( config_package_nearby::nearby_sharing_feature:: kDedupInUnregisterShareTarget)) { LOG(INFO) << __func__ << ": [Dedupped] Move the endpoint " diff --git a/sharing/nearby_sharing_service_impl_test.cc b/sharing/nearby_sharing_service_impl_test.cc index b0baaa44..6cf8a8d9 100644 --- a/sharing/nearby_sharing_service_impl_test.cc +++ b/sharing/nearby_sharing_service_impl_test.cc @@ -3972,19 +3972,19 @@ TEST_F(NearbySharingServiceImplTest, OrderedEndpointDiscoveryEvents) { // Order of events: // - Nearby Connections discovers endpoint 1 // - Nearby Connections loses endpoint 1 - // - Nearby Share processes these two events in order. + // - Nearby Share processes discovered event. + // - endpoint 1 moved to discovery cache. // - Nearby Connections discovers endpoint 2 // - Nearby Connections discovers endpoint 3 // - Nearby Connections loses endpoint 3 // - Nearby Connections loses endpoint 2 - // - Nearby Share processes these four events in order. + // - Nearby Share processes these 2 discovered events in order. + // - endpoints 2 and 3 moved to discovery cache. { absl::Notification notification; FindEndpoint(/*endpoint_id=*/"1"); LoseEndpoint(/*endpoint_id=*/"1"); - InSequence s; - EXPECT_CALL(discovery_callback, OnShareTargetDiscovered); - EXPECT_CALL(discovery_callback, OnShareTargetLost) + EXPECT_CALL(discovery_callback, OnShareTargetDiscovered) .WillOnce([&](ShareTarget share_target) { notification.Notify(); }); // Needed for discovery processing. @@ -4004,16 +4004,8 @@ TEST_F(NearbySharingServiceImplTest, OrderedEndpointDiscoveryEvents) { EXPECT_EQ(share_target.device_id, "2"); }); EXPECT_CALL(discovery_callback, OnShareTargetDiscovered) - .WillOnce([](ShareTarget share_target) { - EXPECT_EQ(share_target.device_id, "3"); - }); - EXPECT_CALL(discovery_callback, OnShareTargetLost) - .WillOnce([](ShareTarget share_target) { - EXPECT_EQ(share_target.device_id, "3"); - }); - EXPECT_CALL(discovery_callback, OnShareTargetLost) .WillOnce([&](ShareTarget share_target) { - EXPECT_EQ(share_target.device_id, "2"); + EXPECT_EQ(share_target.device_id, "3"); notification.Notify(); }); @@ -4064,9 +4056,6 @@ TEST_F(NearbySharingServiceImplTest, } TEST_F(NearbySharingServiceImplTest, DedupSameEndpointId) { - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, - true); // Start discovery. SetConnectionType(ConnectionType::kWifi); MockTransferUpdateCallback transfer_callback; @@ -4113,9 +4102,6 @@ TEST_F(NearbySharingServiceImplTest, DedupSameEndpointId) { TEST_F(NearbySharingServiceImplTest, OnLostDedupSameEndpointIdBeforeExpiryNoOnShareTargetLost) { - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, - true); // Start discovery. SetConnectionType(ConnectionType::kWifi); MockTransferUpdateCallback transfer_callback; @@ -4172,9 +4158,6 @@ TEST_F(NearbySharingServiceImplTest, } TEST_F(NearbySharingServiceImplTest, OnLostDedupSameEndpointIdAfterExpiry) { - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, - true); // Start discovery. SetConnectionType(ConnectionType::kWifi); MockTransferUpdateCallback transfer_callback; @@ -4236,77 +4219,9 @@ TEST_F(NearbySharingServiceImplTest, OnLostDedupSameEndpointIdAfterExpiry) { service_.reset(); } -TEST_F(NearbySharingServiceImplTest, - RetryDiscoveredEndpointsDownloadCertsAndRetryDecryption) { - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, - false); - // Start discovery. - SetConnectionType(ConnectionType::kWifi); - MockTransferUpdateCallback transfer_callback; - MockShareTargetDiscoveredCallback discovery_callback; - RegisterSendSurface(&transfer_callback, &discovery_callback, - SendSurfaceState::kForeground); - EXPECT_EQ(certificate_manager()->num_download_public_certificates_calls(), - 1u); - EXPECT_TRUE(fake_nearby_connections_manager_->IsDiscovering()); - // Order of events: - // - Discover endpoint 1 --> decrypts public certificate - // - Discover endpoint 2 --> cannot decrypt public certificate - // - Discover endpoint 3 --> decrypts public certificate - // - Discover endpoint 4 --> cannot decrypt public certificate - // - Lose endpoint 3 - // - Fire certificate download timer --> certificates downloaded - // - (Re)discover endpoints 2 and 4 - { - absl::Notification notification; - FindInvalidEndpoint(/*endpoint_id=*/"1"); - FindInvalidEndpoint(/*endpoint_id=*/"2"); - FindInvalidEndpoint(/*endpoint_id=*/"3"); - FindInvalidEndpoint(/*endpoint_id=*/"4"); - LoseEndpoint(/*endpoint_id=*/"3"); - ::testing::InSequence s; - EXPECT_CALL(discovery_callback, OnShareTargetDiscovered).Times(2); - EXPECT_CALL(discovery_callback, OnShareTargetLost) - .WillOnce([&](ShareTarget share_target) { notification.Notify(); }); - ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, - /*success=*/true); - ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/2, - /*success=*/false); - ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/3, - /*success=*/true); - ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/4, - /*success=*/false); - EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); - } - FastForward(kCertificateDownloadDuringDiscoveryPeriod); - EXPECT_EQ(certificate_manager()->num_download_public_certificates_calls(), - 2u); - certificate_manager()->NotifyPublicCertificatesDownloaded(); - FlushTesting(); - { - absl::Notification notification; - ::testing::InSequence s; - EXPECT_CALL(discovery_callback, OnShareTargetDiscovered); - EXPECT_CALL(discovery_callback, OnShareTargetDiscovered) - .WillOnce([&](ShareTarget share_target) { notification.Notify(); }); - ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/5, - /*success=*/true); - ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/6, - /*success=*/true); - EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); - } - EXPECT_CALL(discovery_callback, OnShareTargetLost).Times(3); - Shutdown(); - service_.reset(); -} - // This test verifies the de-dup logic. Since certificates are the same, all the // share targets are duplicates. TEST_F(NearbySharingServiceImplTest, EndpointDedupBasedOnDeviceId) { - NearbyFlags::GetInstance().OverrideBoolFlagValue( - config_package_nearby::nearby_sharing_feature::kApplyEndpointsDedup, - true); // Make kDiscoveryCacheLostExpiryMs larger than // kCertificateDownloadDuringDiscoveryPeriod (10s). NearbyFlags::GetInstance().OverrideInt64FlagValue(