From e54aa1d5e194c21927f1363f690adc76dc3f53df Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Tue, 2 Jul 2024 16:28:03 -0700 Subject: [PATCH] Add ThreadTimer which ensures callback invoked from specific TaskRunner. PiperOrigin-RevId: 648878267 --- sharing/BUILD | 29 ++ sharing/fake_nearby_connection.cc | 13 +- sharing/fake_nearby_connection.h | 4 +- .../internal/test/fake_bluetooth_adapter.h | 2 +- sharing/nearby_connections_manager_impl.cc | 1 - sharing/nearby_sharing_service_impl.cc | 290 +++++++----------- sharing/nearby_sharing_service_impl.h | 26 +- sharing/nearby_sharing_service_impl_test.cc | 112 +++---- sharing/thread_timer.cc | 76 +++++ sharing/thread_timer.h | 55 ++++ sharing/thread_timer_test.cc | 105 +++++++ 11 files changed, 467 insertions(+), 246 deletions(-) create mode 100644 sharing/thread_timer.cc create mode 100644 sharing/thread_timer.h create mode 100644 sharing/thread_timer_test.cc diff --git a/sharing/BUILD b/sharing/BUILD index 842ce892..1ceeee8f 100644 --- a/sharing/BUILD +++ b/sharing/BUILD @@ -167,6 +167,19 @@ cc_library( ], ) +cc_library( + name = "thread_timer", + srcs = ["thread_timer.cc"], + hdrs = ["thread_timer.h"], + deps = [ + "//internal/platform:types", + "//sharing/internal/public:logging", + "@com_google_absl//absl/debugging:leak_check", + "@com_google_absl//absl/functional:any_invocable", + "@com_google_absl//absl/time", + ], +) + cc_library( name = "share_session", srcs = [ @@ -263,6 +276,7 @@ cc_library( ":nearby_sharing_decoder_impl", ":paired_key_verification_runner", ":share_session", + ":thread_timer", ":transfer_metadata", ":types", "//connections:core", @@ -336,6 +350,7 @@ cc_library( ":transfer_metadata", ":types", "//internal/base", + "//internal/platform:types", "//sharing/common:enum", "//sharing/internal/api:platform", "//sharing/internal/public:logging", @@ -791,3 +806,17 @@ cc_test( "@com_google_googletest//:gtest_main", ], ) + +cc_test( + name = "thread_timer_test", + srcs = ["thread_timer_test.cc"], + deps = [ + ":thread_timer", + "//internal/platform/implementation/g3", # fixdeps: keep + "//internal/test", + "@com_github_protobuf_matchers//protobuf-matchers", + "@com_google_absl//absl/synchronization", + "@com_google_absl//absl/time", + "@com_google_googletest//:gtest_main", + ], +) diff --git a/sharing/fake_nearby_connection.cc b/sharing/fake_nearby_connection.cc index 9800756a..71334df9 100644 --- a/sharing/fake_nearby_connection.cc +++ b/sharing/fake_nearby_connection.cc @@ -23,12 +23,14 @@ #include #include "absl/synchronization/mutex.h" +#include "internal/platform/task_runner.h" #include "sharing/internal/public/logging.h" #include "sharing/nearby_connection.h" namespace nearby { namespace sharing { -FakeNearbyConnection::FakeNearbyConnection() = default; +FakeNearbyConnection::FakeNearbyConnection(TaskRunner* task_runner) + : task_runner_(task_runner) {} FakeNearbyConnection::~FakeNearbyConnection() = default; void FakeNearbyConnection::Read(ReadCallback callback) { @@ -51,6 +53,15 @@ void FakeNearbyConnection::Close() { closed_ = true; { + if (task_runner_) { + task_runner_->PostTask([this]() { + absl::MutexLock lock(&disconnect_mutex_); + if (disconnect_listener_) { + std::move(disconnect_listener_)(); + } + }); + return; + } absl::MutexLock lock(&disconnect_mutex_); if (disconnect_listener_) { std::move(disconnect_listener_)(); diff --git a/sharing/fake_nearby_connection.h b/sharing/fake_nearby_connection.h index 956d2523..e6cb391d 100644 --- a/sharing/fake_nearby_connection.h +++ b/sharing/fake_nearby_connection.h @@ -23,6 +23,7 @@ #include "absl/base/thread_annotations.h" #include "absl/synchronization/mutex.h" +#include "internal/platform/task_runner.h" #include "sharing/nearby_connection.h" namespace nearby { @@ -30,7 +31,7 @@ namespace sharing { class FakeNearbyConnection : public NearbyConnection { public: - FakeNearbyConnection(); + explicit FakeNearbyConnection(TaskRunner* task_runner = nullptr); ~FakeNearbyConnection() override; // NearbyConnection: @@ -54,6 +55,7 @@ class FakeNearbyConnection : public NearbyConnection { bool closed_ = false; + TaskRunner* const task_runner_; absl::Mutex read_mutex_; bool has_read_callback_been_run_ ABSL_GUARDED_BY(read_mutex_) = false; ReadCallback callback_ ABSL_GUARDED_BY(read_mutex_); diff --git a/sharing/internal/test/fake_bluetooth_adapter.h b/sharing/internal/test/fake_bluetooth_adapter.h index da7698db..533d46ec 100644 --- a/sharing/internal/test/fake_bluetooth_adapter.h +++ b/sharing/internal/test/fake_bluetooth_adapter.h @@ -84,7 +84,7 @@ class FakeBluetoothAdapter : public sharing::api::BluetoothAdapter { std::optional> GetAddress() const override { std::array output; - if (device::ParseBluetoothAddress( + if (mac_address_.has_value() && device::ParseBluetoothAddress( mac_address_.value(), absl::MakeSpan(output.data(), output.size()))) { return output; diff --git a/sharing/nearby_connections_manager_impl.cc b/sharing/nearby_connections_manager_impl.cc index 9b7d2b12..15435030 100644 --- a/sharing/nearby_connections_manager_impl.cc +++ b/sharing/nearby_connections_manager_impl.cc @@ -20,7 +20,6 @@ #include #include #include -#include #include #include #include diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index d81508cd..10b775a5 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -17,6 +17,7 @@ #include #include +#include #include #include #include // NOLINT(build/c++17) @@ -99,6 +100,7 @@ #include "sharing/share_session.h" #include "sharing/share_target.h" #include "sharing/share_target_discovered_callback.h" +#include "sharing/thread_timer.h" #include "sharing/transfer_metadata.h" #include "sharing/transfer_metadata_builder.h" #include "sharing/transfer_update_callback.h" @@ -226,12 +228,6 @@ NearbySharingServiceImpl::NearbySharingServiceImpl( NL_DCHECK(decoder_); NL_DCHECK(nearby_connections_manager_); - certificate_download_during_discovery_timer_ = context_->CreateTimer(); - on_network_changed_delay_timer_ = context_->CreateTimer(); - mutual_acceptance_timeout_alarm_ = context_->CreateTimer(); - rotate_background_advertisement_timer_ = context_->CreateTimer(); - fast_initiation_scanner_cooldown_timer_ = context_->CreateTimer(); - is_shutting_down_ = std::make_unique(false); std::filesystem::path path = device_info_.GetAppDataPath(); @@ -308,7 +304,7 @@ void NearbySharingServiceImpl::Shutdown( context_->GetBluetoothAdapter().RemoveObserver(this); nearby_fast_initiation_->RemoveObserver(this); - on_network_changed_delay_timer_->Stop(); + on_network_changed_delay_timer_.reset(); foreground_receive_callbacks_map_.clear(); background_receive_callbacks_map_.clear(); @@ -338,6 +334,9 @@ void NearbySharingServiceImpl::Cleanup() { endpoint_discovery_events_ = {}; ClearOutgoingShareSessionMap(); + for (auto& it : incoming_share_session_map_) { + it.second.OnDisconnect(); + } incoming_share_session_map_.clear(); discovered_advertisements_to_retry_map_.clear(); discovered_advertisements_retried_set_.clear(); @@ -349,7 +348,7 @@ void NearbySharingServiceImpl::Cleanup() { last_outgoing_metadata_.reset(); locally_cancelled_share_target_ids_.clear(); - mutual_acceptance_timeout_alarm_->Stop(); + mutual_acceptance_timeout_alarm_.reset(); disconnection_timeout_alarms_.clear(); is_scanning_ = false; @@ -359,8 +358,8 @@ void NearbySharingServiceImpl::Cleanup() { is_connecting_ = false; advertising_power_level_ = PowerLevel::kUnknown; - certificate_download_during_discovery_timer_->Stop(); - rotate_background_advertisement_timer_->Stop(); + certificate_download_during_discovery_timer_.reset(); + rotate_background_advertisement_timer_.reset(); } void NearbySharingServiceImpl::SendInitialAdapterState( @@ -648,9 +647,11 @@ void NearbySharingServiceImpl::RegisterReceiveSurface( } else if (!IsBluetoothPowered()) { NL_LOG(WARNING) << __func__ << ": Bluetooth is not powered."; } else { - NL_VLOG(1) << __func__ << ": This device's MAC address is: " - << nearby::device::CanonicalizeBluetoothAddress( - *context_->GetBluetoothAdapter().GetAddress()); + NL_VLOG(1) + << __func__ << ": This device's MAC address is: " + << nearby::device::CanonicalizeBluetoothAddress( + context_->GetBluetoothAdapter().GetAddress().value_or( + std::array{})); } } @@ -1862,13 +1863,9 @@ void NearbySharingServiceImpl::ScheduleCertificateDownloadDuringDiscovery( return; } - if (certificate_download_during_discovery_timer_->IsRunning()) { - certificate_download_during_discovery_timer_->Stop(); - } - - certificate_download_during_discovery_timer_->Start( - absl::ToInt64Milliseconds(kCertificateDownloadDuringDiscoveryPeriod), 0, - [this, attempt_count]() { + certificate_download_during_discovery_timer_ = std::make_unique( + *service_thread_, "certificate_download_during_discovery_timer", + kCertificateDownloadDuringDiscoveryPeriod, [this, attempt_count]() { OnCertificateDownloadDuringDiscoveryTimerFired(attempt_count); }); } @@ -2260,7 +2257,7 @@ NearbySharingService::StatusCodes NearbySharingServiceImpl::StopScanning() { nearby_connections_manager_->StopDiscovery(); is_scanning_ = false; - certificate_download_during_discovery_timer_->Stop(); + certificate_download_during_discovery_timer_.reset(); discovered_advertisements_to_retry_map_.clear(); discovered_advertisements_retried_set_.clear(); @@ -2292,7 +2289,8 @@ void NearbySharingServiceImpl::InvalidateFastInitiationScanning() { settings_->SetIsFastInitiationHardwareSupported( is_hardware_offloading_supported); - if (fast_initiation_scanner_cooldown_timer_->IsRunning()) { + if (fast_initiation_scanner_cooldown_timer_ && + fast_initiation_scanner_cooldown_timer_->IsRunning()) { NL_VLOG(1) << __func__ << ": Stopping background scanning due to post-transfer " "cooldown period"; @@ -2407,39 +2405,29 @@ void NearbySharingServiceImpl::StopFastInitiationScanning() { void NearbySharingServiceImpl::ScheduleRotateBackgroundAdvertisementTimer() { absl::BitGen bitgen; - uint64_t delayRangeMilliseconds = - absl::ToInt64Milliseconds(kBackgroundAdvertisementRotationDelayMax - - kBackgroundAdvertisementRotationDelayMin); - uint64_t bias = absl::Uniform(bitgen, 0u, delayRangeMilliseconds); - uint64_t delayMilliseconds = - bias + - absl::ToInt64Milliseconds(kBackgroundAdvertisementRotationDelayMin); - if (rotate_background_advertisement_timer_->IsRunning()) { - rotate_background_advertisement_timer_->Stop(); - } - rotate_background_advertisement_timer_->Start(delayMilliseconds, 0, [this]() { - OnRotateBackgroundAdvertisementTimerFired(); - }); + uint64_t delayMilliseconds = absl::Uniform( + bitgen, + absl::ToInt64Milliseconds(kBackgroundAdvertisementRotationDelayMin), + absl::ToInt64Milliseconds(kBackgroundAdvertisementRotationDelayMax)); + rotate_background_advertisement_timer_ = std::make_unique( + *service_thread_, "rotate_background_advertisement_timer", + absl::Milliseconds(delayMilliseconds), + [this]() { OnRotateBackgroundAdvertisementTimerFired(); }); } void NearbySharingServiceImpl::OnRotateBackgroundAdvertisementTimerFired() { NL_LOG(INFO) << __func__ << ": Rotate background advertisement timer fired."; - RunOnNearbySharingServiceThread( - "on-rotate-background-advertisement-timer-fired", [this]() { - if (!foreground_receive_callbacks_map_.empty()) { - rotate_background_advertisement_timer_->Stop(); - ScheduleRotateBackgroundAdvertisementTimer(); - } else { - StopAdvertising(); - InvalidateSurfaceState(); - } - }); + if (!foreground_receive_callbacks_map_.empty()) { + ScheduleRotateBackgroundAdvertisementTimer(); + } else { + StopAdvertising(); + InvalidateSurfaceState(); + } } void NearbySharingServiceImpl::RemoveOutgoingShareTargetWithEndpointId( absl::string_view endpoint_id) { - disconnection_timeout_alarms_.erase(endpoint_id); auto it = outgoing_share_target_map_.find(endpoint_id); if (it == outgoing_share_target_map_.end()) { return; @@ -2451,9 +2439,10 @@ void NearbySharingServiceImpl::RemoveOutgoingShareTargetWithEndpointId( ShareTarget share_target = std::move(it->second); outgoing_share_target_map_.erase(it); - auto info_it = outgoing_share_session_map_.find(share_target.id); - if (info_it != outgoing_share_session_map_.end()) { - outgoing_share_session_map_.erase(info_it); + auto session_it = outgoing_share_session_map_.find(share_target.id); + if (session_it != outgoing_share_session_map_.end()) { + session_it->second.OnDisconnect(); + outgoing_share_session_map_.erase(session_it); } else { NL_LOG(WARNING) << __func__ << ": share_target.id=" << it->second.id << " not found in outgoing share session map."; @@ -2500,7 +2489,7 @@ void NearbySharingServiceImpl::OnTransferStarted(bool is_incoming) { void NearbySharingServiceImpl::ReceivePayloads( IncomingShareSession& session, std::function status_codes_callback) { - mutual_acceptance_timeout_alarm_->Stop(); + mutual_acceptance_timeout_alarm_.reset(); // Log analytics event of starting to receive payloads. analytics_recorder_->NewReceiveAttachmentsStart( @@ -2563,8 +2552,7 @@ void NearbySharingServiceImpl::OnOutgoingConnection( OutgoingShareSession& session) { int64_t share_target_id = session.share_target().id; if (!session.OnConnected(connect_start_time, connection)) { - AbortAndCloseConnectionIfNecessary(session.disconnect_status(), - share_target_id); + AbortAndCloseConnectionIfNecessary(session, session.disconnect_status()); return; } @@ -2641,7 +2629,7 @@ void NearbySharingServiceImpl::SendIntroduction( NL_LOG(WARNING) << __func__ << ": No payloads tied to transfer, disconnecting."; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kMissingPayloads, session.share_target().id); + session, TransferMetadata::Status::kMissingPayloads); return; } @@ -2649,11 +2637,21 @@ void NearbySharingServiceImpl::SendIntroduction( // remote side to accept. NL_VLOG(1) << __func__ << ": Successfully wrote the introduction frame"; - mutual_acceptance_timeout_alarm_->Stop(); - mutual_acceptance_timeout_alarm_->Start( - absl::ToInt64Milliseconds(kReadResponseFrameTimeout), 0, + mutual_acceptance_timeout_alarm_ = std::make_unique( + *service_thread_, "mutual_acceptance_timeout_alarm", + kReadResponseFrameTimeout, [this, share_target_id = session.share_target().id]() { - OnOutgoingMutualAcceptanceTimeout(share_target_id); + NL_VLOG(1) + << __func__ + << ": Outgoing mutual acceptance timed out, closing connection for " + << share_target_id; + OutgoingShareSession* session = + GetOutgoingShareSession(share_target_id); + if (session == nullptr) { + return; + } + AbortAndCloseConnectionIfNecessary(*session, + TransferMetadata::Status::kTimedOut); }); session.UpdateTransferMetadata( @@ -2800,7 +2798,7 @@ void NearbySharingServiceImpl::Fail(int64_t share_target_id, } void NearbySharingServiceImpl::OnIncomingAdvertisementDecoded( - absl::string_view endpoint_id, const IncomingShareSession& session, + absl::string_view endpoint_id, IncomingShareSession& session, std::unique_ptr advertisement) { int64_t placeholder_share_target_id = session.share_target().id; if (!session.IsConnected()) { @@ -2814,8 +2812,7 @@ void NearbySharingServiceImpl::OnIncomingAdvertisementDecoded( << ": Failed to parse incoming connection from endpoint - " << endpoint_id << ", disconnecting."; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kDecodeAdvertisementFailed, - placeholder_share_target_id); + session, TransferMetadata::Status::kDecodeAdvertisementFailed); return; } @@ -2993,17 +2990,13 @@ void NearbySharingServiceImpl::OnIncomingDecryptedCertificate( return; } if (!it->second.IsConnected()) { - NL_VLOG(1) << __func__ << ": Connection has been closedfor endpoint id - " + NL_VLOG(1) << __func__ << ": Connection has been closed for endpoint id - " << endpoint_id; incoming_share_session_map_.erase(it); return; } NearbyConnection* connection = it->second.connection(); - // Remove placeholder share target since we are creating the actual share - // target below. - incoming_share_session_map_.erase(it); - std::optional share_target = CreateShareTarget(endpoint_id, advertisement, certificate, /*is_incoming=*/true); @@ -3012,10 +3005,13 @@ void NearbySharingServiceImpl::OnIncomingDecryptedCertificate( << ": Failed to convert advertisement to share target for " "incoming connection, disconnecting"; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kMissingShareTarget, - placeholder_share_target_id); + it->second, TransferMetadata::Status::kMissingShareTarget); return; } + // Remove placeholder share target since we are creating the actual share + // target below. + incoming_share_session_map_.erase(it); + int64_t share_target_id = share_target->id; NL_VLOG(1) << __func__ << ": Received incoming connection from " << share_target_id; @@ -3067,8 +3063,7 @@ void NearbySharingServiceImpl::OnIncomingConnectionKeyVerificationDone( NL_VLOG(1) << __func__ << ": Paired key handshake failed for target " << share_target_id << ". Disconnecting."; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kPairedKeyVerificationFailed, - share_target_id); + *session, TransferMetadata::Status::kPairedKeyVerificationFailed); return; case PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess: @@ -3092,8 +3087,7 @@ void NearbySharingServiceImpl::OnIncomingConnectionKeyVerificationDone( << ": Unknown PairedKeyVerificationResult for target " << share_target_id << ". Disconnecting."; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kPairedKeyVerificationFailed, - share_target_id); + *session, TransferMetadata::Status::kPairedKeyVerificationFailed); break; } } @@ -3114,8 +3108,7 @@ void NearbySharingServiceImpl::OnOutgoingConnectionKeyVerificationDone( NL_VLOG(1) << __func__ << ": Paired key handshake failed for target " << share_target_id << ". Disconnecting."; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kPairedKeyVerificationFailed, - share_target_id); + *session, TransferMetadata::Status::kPairedKeyVerificationFailed); return; case PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess: @@ -3154,8 +3147,7 @@ void NearbySharingServiceImpl::OnOutgoingConnectionKeyVerificationDone( << ": Unknown PairedKeyVerificationResult for target " << share_target_id << ". Disconnecting."; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kPairedKeyVerificationFailed, - share_target_id); + *session, TransferMetadata::Status::kPairedKeyVerificationFailed); break; } } @@ -3191,7 +3183,7 @@ void NearbySharingServiceImpl::OnReceivedIntroduction( if (!frame.has_value()) { AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kInvalidIntroductionFrame, share_target_id); + *session, TransferMetadata::Status::kInvalidIntroductionFrame); NL_LOG(WARNING) << __func__ << ": Invalid introduction frame"; return; } @@ -3269,12 +3261,12 @@ void NearbySharingServiceImpl::OnReceiveConnectionResponse( << __func__ << ": Failed to read a response from the remote device. Disconnecting."; AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kFailedToReadOutgoingConnectionResponse, - share_target_id); + *session, + TransferMetadata::Status::kFailedToReadOutgoingConnectionResponse); return; } - mutual_acceptance_timeout_alarm_->Stop(); + mutual_acceptance_timeout_alarm_.reset(); NL_VLOG(1) << __func__ << ": Successfully read the connection response frame."; @@ -3318,8 +3310,8 @@ void NearbySharingServiceImpl::OnReceiveConnectionResponse( break; } case nearby::sharing::service::proto::ConnectionResponseFrame::REJECT: - AbortAndCloseConnectionIfNecessary(TransferMetadata::Status::kRejected, - share_target_id); + AbortAndCloseConnectionIfNecessary(*session, + TransferMetadata::Status::kRejected); NL_VLOG(1) << __func__ << ": The connection was rejected. The connection has been closed."; @@ -3327,7 +3319,7 @@ void NearbySharingServiceImpl::OnReceiveConnectionResponse( case nearby::sharing::service::proto::ConnectionResponseFrame:: NOT_ENOUGH_SPACE: AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kNotEnoughSpace, share_target_id); + *session, TransferMetadata::Status::kNotEnoughSpace); NL_VLOG(1) << __func__ << ": The connection was rejected because the remote device " "does not have enough space for our attachments. The " @@ -3336,23 +3328,22 @@ void NearbySharingServiceImpl::OnReceiveConnectionResponse( case nearby::sharing::service::proto::ConnectionResponseFrame:: UNSUPPORTED_ATTACHMENT_TYPE: AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kUnsupportedAttachmentType, - share_target_id); + *session, TransferMetadata::Status::kUnsupportedAttachmentType); NL_VLOG(1) << __func__ << ": The connection was rejected because the remote device " "does not support the attachments we were sending. The " "connection has been closed."; break; case nearby::sharing::service::proto::ConnectionResponseFrame::TIMED_OUT: - AbortAndCloseConnectionIfNecessary(TransferMetadata::Status::kTimedOut, - share_target_id); + AbortAndCloseConnectionIfNecessary(*session, + TransferMetadata::Status::kTimedOut); NL_VLOG(1) << __func__ << ": The connection was rejected because the remote device " "timed out. The connection has been closed."; break; default: - AbortAndCloseConnectionIfNecessary(TransferMetadata::Status::kFailed, - share_target_id); + AbortAndCloseConnectionIfNecessary(*session, + TransferMetadata::Status::kFailed); NL_VLOG(1) << __func__ << ": The connection failed. The connection has been closed."; break; @@ -3369,18 +3360,22 @@ void NearbySharingServiceImpl::OnStorageCheckCompleted( << share_target_id; return; } - ShareSession* session = GetShareSession(share_target_id); + IncomingShareSession* session = GetIncomingShareSession(share_target_id); if (!session || !session->IsConnected()) { NL_LOG(WARNING) << __func__ << ": Invalid connection for share target - " << share_target_id; return; } - mutual_acceptance_timeout_alarm_->Stop(); - mutual_acceptance_timeout_alarm_->Start( - absl::ToInt64Milliseconds(kReadResponseFrameTimeout), 0, - [this, share_target_id]() { - OnIncomingMutualAcceptanceTimeout(share_target_id); + mutual_acceptance_timeout_alarm_ = std::make_unique( + *service_thread_, "mutual_acceptance_timeout_alarm", + kReadResponseFrameTimeout, [this, share_target_id]() { + NL_VLOG(1) + << __func__ + << ": Incoming mutual acceptance timed out, closing connection for " + << share_target_id; + + Fail(share_target_id, TransferMetadata::Status::kTimedOut); }); bool is_self_share = !four_digit_token.has_value() && session->self_share(); @@ -3400,14 +3395,6 @@ void NearbySharingServiceImpl::OnStorageCheckCompleted( OnTransferStarted(/*is_incoming=*/true); } - if (!incoming_share_session_map_.count(share_target_id)) { - NL_VLOG(1) << __func__ << ": IncomingShareTarget not found, disconnecting " - << share_target_id; - AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status::kMissingShareTarget, share_target_id); - return; - } - session->set_disconnect_status( TransferMetadata::Status::kUnexpectedDisconnection); @@ -3521,27 +3508,6 @@ void NearbySharingServiceImpl::OnConnectionDisconnected( UnregisterShareTarget(share_target_id); } -void NearbySharingServiceImpl::OnIncomingMutualAcceptanceTimeout( - int64_t share_target_id) { - NL_VLOG(1) - << __func__ - << ": Incoming mutual acceptance timed out, closing connection for " - << share_target_id; - - Fail(share_target_id, TransferMetadata::Status::kTimedOut); -} - -void NearbySharingServiceImpl::OnOutgoingMutualAcceptanceTimeout( - int64_t share_target_id) { - NL_VLOG(1) - << __func__ - << ": Outgoing mutual acceptance timed out, closing connection for " - << share_target_id; - - AbortAndCloseConnectionIfNecessary(TransferMetadata::Status::kTimedOut, - share_target_id); -} - std::optional NearbySharingServiceImpl::CreateShareTarget( absl::string_view endpoint_id, const Advertisement& advertisement, const std::optional& certificate, @@ -3634,11 +3600,10 @@ void NearbySharingServiceImpl::OnPayloadTransferUpdate( payload_incomplete = true; } - fast_initiation_scanner_cooldown_timer_->Stop(); - fast_initiation_scanner_cooldown_timer_->Start( - absl::ToInt64Milliseconds(kFastInitiationScannerCooldown), 0, - [this]() { - fast_initiation_scanner_cooldown_timer_->Stop(); + fast_initiation_scanner_cooldown_timer_ = std::make_unique( + *service_thread_, "fast_initiation_scanner_cooldown_timer", + kFastInitiationScannerCooldown, [this]() { + fast_initiation_scanner_cooldown_timer_.reset(); InvalidateFastInitiationScanning(); }); } else if (metadata.status() == TransferMetadata::Status::kCancelled) { @@ -3730,26 +3695,19 @@ void NearbySharingServiceImpl::Disconnect(int64_t share_target_id, } // Disconnect after a timeout to make sure any pending payloads are sent. - auto timer = context_->CreateTimer(); - timer->Start( - absl::ToInt64Milliseconds(kOutgoingDisconnectionDelay), 0, - [this, endpoint_id]() { OnDisconnectingConnectionTimeout(endpoint_id); }); + auto timer = std::make_unique( + *service_thread_, "disconnection_timeout_alarm", + kOutgoingDisconnectionDelay, + [this, endpoint_id]() { + disconnection_timeout_alarms_.erase(endpoint_id); + nearby_connections_manager_->Disconnect(endpoint_id); + }); disconnection_timeout_alarms_[endpoint_id] = std::move(timer); session->set_disconnect_status(TransferMetadata::Status::kUnknown); } -void NearbySharingServiceImpl::OnDisconnectingConnectionTimeout( - absl::string_view endpoint_id) { - RunOnNearbySharingServiceThread( - "on_disconnecting_connection_timeout", - [this, endpoint_id = std::string(endpoint_id)]() { - disconnection_timeout_alarms_.erase(endpoint_id); - }); - nearby_connections_manager_->Disconnect(endpoint_id); -} - IncomingShareSession& NearbySharingServiceImpl::CreateIncomingShareSession( const ShareTarget& share_target, absl::string_view endpoint_id, std::optional certificate) { @@ -3886,7 +3844,7 @@ void NearbySharingServiceImpl::UnregisterShareTarget(int64_t share_target_id) { NL_VLOG(1) << __func__ << ": Unregister share target: " << share_target_id; } - mutual_acceptance_timeout_alarm_->Stop(); + mutual_acceptance_timeout_alarm_.reset(); } void NearbySharingServiceImpl::OnStartAdvertisingResult(bool used_device_name, @@ -3961,41 +3919,29 @@ void NearbySharingServiceImpl::SetInHighVisibility( } void NearbySharingServiceImpl::AbortAndCloseConnectionIfNecessary( - TransferMetadata::Status status, int64_t share_target_id) { - RunOnNearbySharingServiceThread( - "abort_and_close_connection_if_necessary", - [this, status, share_target_id]() { - TransferMetadata metadata = - TransferMetadataBuilder().set_status(status).build(); - ShareSession* session = GetShareSession(share_target_id); + ShareSession& session, + TransferMetadata::Status status) { + TransferMetadata metadata = + TransferMetadataBuilder().set_status(status).build(); - if (session == nullptr) { - NL_LOG(WARNING) << ": Share target " << share_target_id << " lost"; - return; - } + // First invoke the appropriate transfer callback with the final + // |status|. + session.UpdateTransferMetadata(metadata); - // First invoke the appropriate transfer callback with the final - // |status|. - session->UpdateTransferMetadata(metadata); - - // Close connection if necessary. - if (session->IsConnected()) { - // Final status already sent above. No need to send it again. - session->set_disconnect_status(TransferMetadata::Status::kUnknown); - session->connection()->Close(); - } - }); + // Close connection if necessary. + if (session.IsConnected()) { + // Final status already sent above. No need to send it again. + session.set_disconnect_status(TransferMetadata::Status::kUnknown); + session.connection()->Close(); + } } void NearbySharingServiceImpl::OnNetworkChanged( nearby::ConnectivityManager::ConnectionType type) { - on_network_changed_delay_timer_->Stop(); - on_network_changed_delay_timer_->Start( - absl::ToInt64Milliseconds(kProcessNetworkChangeTimerDelay), 0, [this]() { - RunOnNearbySharingServiceThread("on-network-changed", [this]() { - StopAdvertisingAndInvalidateSurfaceState(); - }); - }); + on_network_changed_delay_timer_ = std::make_unique( + *service_thread_, "on_network_changed_delay_timer", + kProcessNetworkChangeTimerDelay, + [this]() { StopAdvertisingAndInvalidateSurfaceState(); }); } void NearbySharingServiceImpl::OnLanConnectedChanged(bool connected) { diff --git a/sharing/nearby_sharing_service_impl.h b/sharing/nearby_sharing_service_impl.h index 84c0db2e..3abfa698 100644 --- a/sharing/nearby_sharing_service_impl.h +++ b/sharing/nearby_sharing_service_impl.h @@ -40,7 +40,6 @@ #include "internal/platform/device_info.h" #include "internal/platform/implementation/account_manager.h" #include "internal/platform/task_runner.h" -#include "internal/platform/timer.h" #include "proto/sharing_enums.pb.h" #include "sharing/advertisement.h" #include "sharing/analytics/analytics_recorder.h" @@ -77,6 +76,7 @@ #include "sharing/share_session.h" #include "sharing/share_target.h" #include "sharing/share_target_discovered_callback.h" +#include "sharing/thread_timer.h" #include "sharing/transfer_metadata.h" #include "sharing/transfer_update_callback.h" #include "sharing/wrapped_share_target_discovered_callback.h" @@ -333,7 +333,7 @@ class NearbySharingServiceImpl void Fail(int64_t share_target_id, TransferMetadata::Status status); void OnIncomingAdvertisementDecoded( - absl::string_view endpoint_id, const IncomingShareSession& session, + absl::string_view endpoint_id, IncomingShareSession& session, std::unique_ptr advertisement); void OnIncomingTransferUpdate(const IncomingShareSession& session, const TransferMetadata& metadata); @@ -374,9 +374,6 @@ class NearbySharingServiceImpl void OnConnectionDisconnected(int64_t share_target_id); - void OnIncomingMutualAcceptanceTimeout(int64_t share_target_id); - void OnOutgoingMutualAcceptanceTimeout(int64_t share_target_id); - void Cleanup(); std::optional CreateShareTarget( @@ -388,7 +385,6 @@ class NearbySharingServiceImpl TransferMetadata metadata); void RemoveIncomingPayloads(const IncomingShareSession& session); void Disconnect(int64_t share_target_id, TransferMetadata metadata); - void OnDisconnectingConnectionTimeout(absl::string_view endpoint_id); IncomingShareSession& CreateIncomingShareSession( const ShareTarget& share_target, absl::string_view endpoint_id, @@ -420,8 +416,8 @@ class NearbySharingServiceImpl std::function status_codes_callback, bool is_initiator_of_cancellation); - void AbortAndCloseConnectionIfNecessary(TransferMetadata::Status status, - int64_t share_target_id); + void AbortAndCloseConnectionIfNecessary(ShareSession& session, + TransferMetadata::Status status); // Monitor connectivity changes. void OnNetworkChanged(nearby::ConnectivityManager::ConnectionType type); @@ -497,9 +493,8 @@ class NearbySharingServiceImpl std::unique_ptr service_extension_; NearbyFileHandler file_handler_; bool is_screen_locked_ = false; - std::unique_ptr rotate_background_advertisement_timer_; - std::unique_ptr certificate_download_during_discovery_timer_; - std::unique_ptr process_shutdown_pending_timer_; + std::unique_ptr rotate_background_advertisement_timer_; + std::unique_ptr certificate_download_during_discovery_timer_; // A list of service observers. ObserverList observers_; @@ -558,11 +553,11 @@ class NearbySharingServiceImpl // This alarm is used to disconnect the sharing connection if both sides do // not press accept within the timeout. - std::unique_ptr mutual_acceptance_timeout_alarm_; + std::unique_ptr mutual_acceptance_timeout_alarm_; // A map of ShareTarget id to disconnection timeout callback. Used to only // disconnect after a timeout to keep sending any pending payloads. - absl::flat_hash_map> + absl::flat_hash_map> disconnection_timeout_alarms_; // The current advertising power level. PowerLevel::kUnknown while not @@ -589,14 +584,13 @@ class NearbySharingServiceImpl // the time between an incoming share being accepted and the first payload // byte being processed. absl::Time incoming_share_accepted_timestamp_; - std::unique_ptr clear_recent_nearby_process_shutdown_count_timer_; // Used to debounce OnNetworkChanged processing. - std::unique_ptr on_network_changed_delay_timer_; + std::unique_ptr on_network_changed_delay_timer_; // Used to prevent the "Device nearby is sharing" notification from appearing // immediately after a completed share. - std::unique_ptr fast_initiation_scanner_cooldown_timer_; + std::unique_ptr fast_initiation_scanner_cooldown_timer_; // A queue of endpoint-discovered and endpoint-lost events that ensures the // events are processed sequentially, in the order received from Nearby diff --git a/sharing/nearby_sharing_service_impl_test.cc b/sharing/nearby_sharing_service_impl_test.cc index d6d58f0d..197476ba 100644 --- a/sharing/nearby_sharing_service_impl_test.cc +++ b/sharing/nearby_sharing_service_impl_test.cc @@ -402,6 +402,8 @@ class NearbySharingServiceImplTest : public testing::Test { auto fake_task_runner = std::make_unique(fake_context_.fake_clock(), 1); sharing_service_task_runner_ = fake_task_runner.get(); + connection_ = + std::make_unique(fake_task_runner.get()); SetBluetoothIsPresent(true); SetBluetoothIsPowered(true); SetScreenLocked(false); @@ -682,7 +684,7 @@ class NearbySharingServiceImplTest : public testing::Test { return std::unique_ptr(frame); })); - connection_.AppendReadableData(encryption_bytes); + connection_->AppendReadableData(encryption_bytes); FlushTesting(); std::string encryption_result = "test_encryption_result"; @@ -704,7 +706,7 @@ class NearbySharingServiceImplTest : public testing::Test { return std::unique_ptr(frame); })); - connection_.AppendReadableData(result_bytes); + connection_->AppendReadableData(result_bytes); FlushTesting(); } @@ -714,9 +716,11 @@ class NearbySharingServiceImplTest : public testing::Test { size_t expected_number_of_calls) { EXPECT_CALL(fake_decoder_, DecodeAdvertisement(testing::Eq(endpoint_info))) .Times(expected_number_of_calls) - .WillRepeatedly(testing::Invoke([=](absl::Span data) { + .WillRepeatedly(testing::Invoke([this, return_empty_advertisement, + return_empty_device_name]( + absl::Span data) { if (return_empty_advertisement) { - connection_.AppendReadableData({}); + connection_->AppendReadableData({}); FlushTesting(); return std::unique_ptr(nullptr); } @@ -744,7 +748,7 @@ class NearbySharingServiceImplTest : public testing::Test { return GetValidIntroductionFrame(); })); - connection_.AppendReadableData(bytes); + connection_->AppendReadableData(bytes); FlushTesting(); } @@ -755,7 +759,7 @@ class NearbySharingServiceImplTest : public testing::Test { .WillOnce(testing::Invoke([=](absl::Span data) { return GetConnectionResponseFrame(status); })); - connection_.AppendReadableData(bytes); + connection_->AppendReadableData(bytes); FlushTesting(); } @@ -765,7 +769,7 @@ class NearbySharingServiceImplTest : public testing::Test { EXPECT_CALL(fake_decoder_, DecodeFrame(testing::Eq(bytes))) .WillOnce(testing::Invoke( [=](absl::Span data) { return GetCancelFrame(); })); - connection_.AppendReadableData(bytes); + connection_->AppendReadableData(bytes); FlushTesting(); } @@ -814,7 +818,7 @@ class NearbySharingServiceImplTest : public testing::Test { } EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true, for_self_share); EXPECT_TRUE( @@ -832,7 +836,7 @@ class NearbySharingServiceImplTest : public testing::Test { fake_nearby_connections_manager_->SetRawAuthenticationToken(kEndpointId, GetToken()); - fake_nearby_connections_manager_->set_nearby_connection(&connection_); + fake_nearby_connections_manager_->set_nearby_connection(connection_.get()); return DiscoverShareTarget(transfer_callback, discovery_callback); } @@ -874,7 +878,7 @@ class NearbySharingServiceImplTest : public testing::Test { Frame GetWrittenFrame() { EXPECT_TRUE( sharing_service_task_runner_->SyncWithTimeout(absl::Seconds(2))); - std::vector data = connection_.GetWrittenData(); + std::vector data = connection_->GetWrittenData(); Frame frame; frame.ParseFromArray(data.data(), data.size()); return frame; @@ -1283,7 +1287,7 @@ class NearbySharingServiceImplTest : public testing::Test { FakeNearbyShareCertificateManager::Factory certificate_manager_factory_; std::unique_ptr nearby_fast_initiation_factory_; - FakeNearbyConnection connection_; + std::unique_ptr connection_; MockNearbySharingDecoder fake_decoder_; StrictMock* mock_app_info_ = nullptr; std::unique_ptr service_; @@ -1621,7 +1625,7 @@ TEST_F(NearbySharingServiceImplTest, RegisterSendSurfaceAlreadyReceivingNotDiscovering) { NiceMock callback; SetUpIncomingConnection(callback); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); MockTransferUpdateCallback send_callback; MockShareTargetDiscoveredCallback discovery_callback; @@ -2446,7 +2450,7 @@ TEST_F(NearbySharingServiceImplTest, IncomingConnectionClosedAfterShutdown) { Shutdown(); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); sharing_service_task_runner_->SyncWithTimeout(kTaskWaitTimeout); service_.reset(); @@ -2476,8 +2480,8 @@ TEST_F(NearbySharingServiceImplTest, SetUpForegroundReceiveSurface(callback); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); - sharing_service_task_runner_->PostTask([this]() { connection_.Close(); }); + connection_.get()); + sharing_service_task_runner_->PostTask([this]() { connection_->Close(); }); sharing_service_task_runner_->SyncWithTimeout(kTaskWaitTimeout); // To avoid UAF in OnIncomingTransferUpdate(). @@ -2503,7 +2507,7 @@ TEST_F(NearbySharingServiceImplTest, SetUpForegroundReceiveSurface(callback); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); @@ -2544,7 +2548,7 @@ TEST_F(NearbySharingServiceImplTest, IncomingConnectionEmptyIntroductionFrame) { SetUpForegroundReceiveSurface(callback); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); // Check data written to connection_. @@ -2594,10 +2598,10 @@ TEST_F(NearbySharingServiceImplTest, SetUpForegroundReceiveSurface(callback); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/false); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -2606,7 +2610,7 @@ TEST_F(NearbySharingServiceImplTest, TEST_F(NearbySharingServiceImplTest, IncomingConnectionTimedOut) { NiceMock callback; SetUpIncomingConnection(callback); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); EXPECT_CALL(callback, OnTransferUpdate(testing::_, testing::_, testing::_)) .WillOnce(testing::Invoke([](const ShareTarget& share_target, @@ -2620,7 +2624,7 @@ TEST_F(NearbySharingServiceImplTest, IncomingConnectionTimedOut) { // Waits for delay to close connection. FastForward(kIncomingRejectionDelay); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); } TEST_F(NearbySharingServiceImplTest, @@ -2637,7 +2641,7 @@ TEST_F(NearbySharingServiceImplTest, TransferMetadata::Status::kUnexpectedDisconnection); })); - sharing_service_task_runner_->PostTask([this]() { connection_.Close(); }); + sharing_service_task_runner_->PostTask([this]() { connection_->Close(); }); sharing_service_task_runner_->SyncWithTimeout(kTaskWaitTimeout); // To avoid UAF in OnIncomingTransferUpdate(). @@ -2685,7 +2689,7 @@ TEST_F(NearbySharingServiceImplTest, IncomingConnectionOutOfStorage) { frame->set_allocated_v1(v1_frame); return std::unique_ptr(frame); })); - connection_.AppendReadableData(std::move(bytes)); + connection_->AppendReadableData(std::move(bytes)); FlushTesting(); SetConnectionType(ConnectionType::kWifi); @@ -2712,7 +2716,7 @@ TEST_F(NearbySharingServiceImplTest, IncomingConnectionOutOfStorage) { SetUpForegroundReceiveSurface(callback); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); // To avoid UAF in OnIncomingTransferUpdate(). @@ -2768,7 +2772,7 @@ TEST_F(NearbySharingServiceImplTest, IncomingConnectionFileSizeOverflow) { frame->set_allocated_v1(v1_frame); return std::unique_ptr(frame); })); - connection_.AppendReadableData(std::move(bytes)); + connection_->AppendReadableData(std::move(bytes)); FlushTesting(); SetConnectionType(ConnectionType::kWifi); @@ -2793,7 +2797,7 @@ TEST_F(NearbySharingServiceImplTest, IncomingConnectionFileSizeOverflow) { SetUpForegroundReceiveSurface(callback); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); // To avoid UAF in OnIncomingTransferUpdate(). @@ -2840,12 +2844,12 @@ TEST_F(NearbySharingServiceImplTest, SetUpForegroundReceiveSurface(callback); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -2894,7 +2898,7 @@ TEST_F(NearbySharingServiceImplTest, AcceptValidShareTarget) { EXPECT_TRUE(ExpectConnectionResponseFrame( service::proto::ConnectionResponseFrame::ACCEPT)); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -3183,7 +3187,7 @@ TEST_F(NearbySharingServiceImplTest, RejectValidShareTarget) { EXPECT_TRUE(ExpectConnectionResponseFrame(ConnectionResponseFrame::REJECT)); FastForward(kIncomingRejectionDelay + kDelta); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -3228,7 +3232,7 @@ TEST_F(NearbySharingServiceImplTest, EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); @@ -3236,7 +3240,7 @@ TEST_F(NearbySharingServiceImplTest, EXPECT_TRUE( fake_nearby_connections_manager_->DidUpgradeBandwidth(kEndpointId)); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -3286,7 +3290,7 @@ TEST_F(NearbySharingServiceImplTest, EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); EXPECT_TRUE(notification.WaitForNotificationWithTimeout(kWaitTimeout)); @@ -3294,7 +3298,7 @@ TEST_F(NearbySharingServiceImplTest, EXPECT_TRUE( fake_nearby_connections_manager_->DidUpgradeBandwidth(kEndpointId)); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -3319,12 +3323,12 @@ TEST_F(NearbySharingServiceImplTest, std::string intro = "introduction_frame"; std::vector bytes(intro.begin(), intro.end()); EXPECT_CALL(fake_decoder_, DecodeFrame(testing::Eq(bytes))).Times(0); - connection_.AppendReadableData(bytes); + connection_->AppendReadableData(bytes); FlushTesting(); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); @@ -3332,7 +3336,7 @@ TEST_F(NearbySharingServiceImplTest, // are processed prior to checking if connection is closed. EXPECT_TRUE( sharing_service_task_runner_->SyncWithTimeout(absl::Milliseconds(200))); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -3355,16 +3359,16 @@ TEST_F(NearbySharingServiceImplTest, std::string intro = "introduction_frame"; std::vector bytes(intro.begin(), intro.end()); EXPECT_CALL(fake_decoder_, DecodeFrame(testing::Eq(bytes))).Times(0); - connection_.AppendReadableData(bytes); + connection_->AppendReadableData(bytes); FlushTesting(); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); service_->OnIncomingConnection(kEndpointId, GetValidV1EndpointInfo(), - &connection_); + connection_.get()); ProcessLatestPublicCertificateDecryption(/*expected_num_calls=*/1, /*success=*/true); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -3373,7 +3377,7 @@ TEST_F(NearbySharingServiceImplTest, TEST_F(NearbySharingServiceImplTest, RegisterReceiveSurfaceAlreadyReceiving) { NiceMock callback; SetUpIncomingConnection(callback); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); EXPECT_EQ( RegisterReceiveSurface( @@ -3530,7 +3534,7 @@ TEST_F(NearbySharingServiceImplTest, SendTextFailedKeyVerification) { SetUpKeyVerification(/*is_incoming=*/false, PairedKeyResultFrame::FAIL); fake_nearby_connections_manager_->SetRawAuthenticationToken(kEndpointId, GetToken()); - fake_nearby_connections_manager_->set_nearby_connection(&connection_); + fake_nearby_connections_manager_->set_nearby_connection(connection_.get()); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); EXPECT_EQ(SendAttachments(target_id, CreateTextAttachments({kTextPayload})), @@ -3556,7 +3560,7 @@ TEST_F(NearbySharingServiceImplTest, SendTextUnableToVerifyKey) { SetUpKeyVerification(/*is_incoming=*/false, PairedKeyResultFrame::UNABLE); fake_nearby_connections_manager_->SetRawAuthenticationToken(kEndpointId, GetToken()); - fake_nearby_connections_manager_->set_nearby_connection(&connection_); + fake_nearby_connections_manager_->set_nearby_connection(connection_.get()); EXPECT_CALL(*mock_app_info_, SetActiveFlag()); EXPECT_EQ(SendAttachments(target_id, CreateTextAttachments({kTextPayload})), @@ -3603,7 +3607,7 @@ TEST_P(NearbySharingServiceImplSendFailureTest, SendTextRemoteFailure) { SendConnectionResponse(GetParam().response_status); EXPECT_TRUE(reject_notification.WaitForNotificationWithTimeout(kWaitTimeout)); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); UnregisterSendSurface(&transfer_callback); } @@ -3656,7 +3660,7 @@ TEST_P(NearbySharingServiceImplSendFailureTest, SendFilesRemoteFailure) { SendConnectionResponse(GetParam().response_status); EXPECT_TRUE(reject_notification.WaitForNotificationWithTimeout(kWaitTimeout)); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); UnregisterSendSurface(&transfer_callback); } @@ -3743,7 +3747,7 @@ TEST_F(NearbySharingServiceImplTest, SendTextSuccessClosedConnection) { fake_nearby_connections_manager_->connection_endpoint_info(kEndpointId)); // Call disconnect on the connection early before the timeout has passed. - sharing_service_task_runner_->PostTask([this]() { connection_.Close(); }); + sharing_service_task_runner_->PostTask([this]() { connection_->Close(); }); // Expect that we haven't called disconnect again as the endpoint is already // disconnected. @@ -3938,9 +3942,9 @@ TEST_F(NearbySharingServiceImplTest, CancelSenderInitiator) { // other device. EXPECT_TRUE(ExpectProgressUpdateFrame()); EXPECT_TRUE(ExpectCancelFrame()); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); FastForward(kInitiatorCancelDelay); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); } TEST_F(NearbySharingServiceImplTest, CancelSenderNoninitiator) { @@ -3971,7 +3975,7 @@ TEST_F(NearbySharingServiceImplTest, CancelSenderNoninitiator) { fake_nearby_connections_manager_->WasPayloadCanceled(info.payload_id)); // The non-initiator should close the connection immediately - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); } TEST_F(NearbySharingServiceImplTest, CancelReceiverInitiator) { @@ -4009,9 +4013,9 @@ TEST_F(NearbySharingServiceImplTest, CancelReceiverInitiator) { // then wait a few seconds before disconnecting to allow for processing on the // other device. ASSERT_TRUE(ExpectCancelFrame()); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); FastForward(kInitiatorCancelDelay); - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); } TEST_F(NearbySharingServiceImplTest, CancelReceiverNoninitiator) { @@ -4040,7 +4044,7 @@ TEST_F(NearbySharingServiceImplTest, CancelReceiverNoninitiator) { fake_nearby_connections_manager_->WasPayloadCanceled(kFilePayloadId)); // The non-initiator should close the connection immediately - EXPECT_TRUE(connection_.IsClosed()); + EXPECT_TRUE(connection_->IsClosed()); } TEST_F(NearbySharingServiceImplTest, @@ -4726,7 +4730,7 @@ TEST_F(NearbySharingServiceImplTest, SelfShareAutoAccept) { ExpectPairedKeyResultFrame(); ExpectConnectionResponseFrame(ConnectionResponseFrame::ACCEPT); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); @@ -4754,7 +4758,7 @@ TEST_F(NearbySharingServiceImplTest, SelfShareNoAutoAcceptInForeground) { ExpectPairedKeyResultFrame(); ExpectConnectionResponseFrame(ConnectionResponseFrame::ACCEPT); - EXPECT_FALSE(connection_.IsClosed()); + EXPECT_FALSE(connection_->IsClosed()); // To avoid UAF in OnIncomingTransferUpdate(). UnregisterReceiveSurface(&callback); diff --git a/sharing/thread_timer.cc b/sharing/thread_timer.cc new file mode 100644 index 00000000..fa943f71 --- /dev/null +++ b/sharing/thread_timer.cc @@ -0,0 +1,76 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "sharing/thread_timer.h" + +#include +#include +#include +#include +#include + +#include "absl/debugging/leak_check.h" +#include "absl/functional/any_invocable.h" +#include "absl/time/time.h" +#include "internal/platform/task_runner.h" +#include "sharing/internal/public/logging.h" + +namespace nearby::sharing { + +ThreadTimer::ThreadTimer(TaskRunner& task_runner, std::string name, + absl::Duration delay, absl::AnyInvocable task) + : name_(std::move(name)) { + auto run_cnt = std::make_unique>(0); + // Do not capture any member variables in the lambda. as the object may be + // deleted before the task is run. + if (task_runner.PostDelayedTask( + delay, [run_cnt = run_cnt.get(), task = std::move(task), + name = name_]() mutable { + if (run_cnt->fetch_add(1) == 0) { + // Timer has not been cancelled, run the task. + NL_LOG(INFO) << "Timer " << name << " fired."; + std::move(task)(); + } else { + // Timer has been cancelled, need to delete the run_cnt. + NL_VLOG(1) << "Timer " << name << " expired but was cancelled."; + delete run_cnt; + } + })) { + // During tests, long running timers may not expire leaving run_cnt_ + // undeleted. Ignore leaks here to reduce noise. + run_cnt_ = absl::IgnoreLeak(run_cnt.release()); + } +} + +ThreadTimer::~ThreadTimer() { Cancel(); } + +void ThreadTimer::Cancel() { + if (run_cnt_ != nullptr) { + NL_LOG(INFO) << "Timer " << name_ << " cancelled."; + if (run_cnt_->fetch_add(1) > 0) { + // Timer has already fired, delete the run_cnt. + delete run_cnt_; + run_cnt_ = nullptr; + } + } +} + +bool ThreadTimer::IsRunning() { + if (run_cnt_ == nullptr) { + return false; + } + return run_cnt_->load() == 0; +} + +} // namespace nearby::sharing diff --git a/sharing/thread_timer.h b/sharing/thread_timer.h new file mode 100644 index 00000000..17c9d23f --- /dev/null +++ b/sharing/thread_timer.h @@ -0,0 +1,55 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef THIRD_PARTY_NEARBY_SHARING_THREAD_TIMER_H_ +#define THIRD_PARTY_NEARBY_SHARING_THREAD_TIMER_H_ + +#include +#include +#include + +#include "absl/functional/any_invocable.h" +#include "absl/time/time.h" +#include "internal/platform/task_runner.h" + +namespace nearby::sharing { + +// A one shot timer that runs a task on the |task_runner| thread on expiration. +// +// The timer is started when the object is created. The timer can be cancelled +// by calling |Cancel()|. If |Cancel()| is called from the |task_runner| +// thread before expiration, the task will not be run. If |Cancel()| is called +// from a different thread, an inflight task may continue until completion. +// +// This class is thread-safe. +class ThreadTimer { + public: + explicit ThreadTimer(TaskRunner& task_runner, std::string name, + absl::Duration delay, absl::AnyInvocable task); + ~ThreadTimer(); + + void Cancel(); + bool IsRunning(); + + private: + const std::string name_; + // The timer state is dependent on the order of expiration and cancellation. + // The |run_cnt_| is used to track what order these tasks occured. + // The task that runs last will be responsible for deleting the |run_cnt_|. + std::atomic* run_cnt_ = nullptr; +}; + +} // namespace nearby::sharing + +#endif // THIRD_PARTY_NEARBY_SHARING_THREAD_TIMER_H_ diff --git a/sharing/thread_timer_test.cc b/sharing/thread_timer_test.cc new file mode 100644 index 00000000..0a770988 --- /dev/null +++ b/sharing/thread_timer_test.cc @@ -0,0 +1,105 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "sharing/thread_timer.h" + +#include +#include + +#include "gtest/gtest.h" +#include "absl/synchronization/notification.h" +#include "absl/time/time.h" +#include "internal/test/fake_clock.h" +#include "internal/test/fake_task_runner.h" + +namespace nearby::sharing { +namespace { + +class ThreadTimerTest : public ::testing::Test { + public: + ThreadTimerTest() : task_runner_(&fake_clock_, 1) {} + + void Sync(absl::Duration timeout) { + absl::Notification notification; + task_runner_.PostTask([¬ification]() { notification.Notify(); }); + notification.WaitForNotificationWithTimeout(timeout); + } + + protected: + FakeClock fake_clock_; + FakeTaskRunner task_runner_; +}; + +TEST_F(ThreadTimerTest, TimerFires) { + bool fired = false; + ThreadTimer timer(task_runner_, "test", absl::Milliseconds(100), + [&fired]() { fired = true; }); + EXPECT_TRUE(timer.IsRunning()); + fake_clock_.FastForward(absl::Milliseconds(100)); + Sync(absl::Milliseconds(100)); + EXPECT_FALSE(timer.IsRunning()); + EXPECT_TRUE(fired); +} + +TEST_F(ThreadTimerTest, CancelBeforeFires) { + bool fired = false; + ThreadTimer timer(task_runner_, "test", absl::Milliseconds(100), + [&fired]() { fired = true; }); + EXPECT_TRUE(timer.IsRunning()); + timer.Cancel(); + EXPECT_FALSE(timer.IsRunning()); + fake_clock_.FastForward(absl::Milliseconds(100)); + Sync(absl::Milliseconds(100)); + EXPECT_FALSE(fired); +} + +TEST_F(ThreadTimerTest, DeleteBeforeFires) { + bool fired = false; + auto timer = std::make_unique(task_runner_, "test", + absl::Milliseconds(100), + [&fired]() { fired = true; }); + timer.reset(); + fake_clock_.FastForward(absl::Milliseconds(100)); + Sync(absl::Milliseconds(100)); + EXPECT_FALSE(fired); +} + +TEST_F(ThreadTimerTest, CancelInCallback) { + bool fired = false; + ThreadTimer timer(task_runner_, "test", absl::Milliseconds(100), + [&fired, &timer]() { + fired = true; + timer.Cancel(); + }); + fake_clock_.FastForward(absl::Milliseconds(100)); + Sync(absl::Milliseconds(100)); + EXPECT_TRUE(fired); +} + +TEST_F(ThreadTimerTest, DeleteInCallback) { + bool fired = false; + std::unique_ptr timer; + timer = std::make_unique( + task_runner_, "test", absl::Milliseconds(100), + [&fired, timer = std::move(timer)]() mutable { + fired = true; + timer.reset(); + }); + fake_clock_.FastForward(absl::Milliseconds(100)); + Sync(absl::Milliseconds(100)); + EXPECT_TRUE(fired); +} + +} // namespace +} // namespace nearby::sharing