From 1a930d93166275148f6f5019d455c187a82ddb09 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Fri, 8 Aug 2025 15:26:13 -0700 Subject: [PATCH] Fix lifetime management of NearbySharingService::Observer. PiperOrigin-RevId: 792782838 --- sharing/BUILD | 2 + sharing/fake_nearby_sharing_service.cc | 6 - sharing/fake_nearby_sharing_service.h | 1 - sharing/nearby_sharing_service.h | 4 - sharing/nearby_sharing_service_impl.cc | 57 +++----- sharing/nearby_sharing_service_impl.h | 4 +- sharing/nearby_sharing_service_impl_test.cc | 14 -- sharing/service_observers.cc | 140 ++++++++++++++++++++ sharing/service_observers.h | 71 ++++++++++ 9 files changed, 230 insertions(+), 69 deletions(-) create mode 100644 sharing/service_observers.cc create mode 100644 sharing/service_observers.h diff --git a/sharing/BUILD b/sharing/BUILD index 91d89d0e..fa80ce65 100644 --- a/sharing/BUILD +++ b/sharing/BUILD @@ -271,6 +271,8 @@ cc_library( "nearby_sharing_service_impl.cc", "nearby_sharing_settings.cc", "nearby_sharing_util.cc", + "service_observers.cc", + "service_observers.h", "transfer_manager.cc", "wrapped_share_target_discovered_callback.cc", ], diff --git a/sharing/fake_nearby_sharing_service.cc b/sharing/fake_nearby_sharing_service.cc index 15868a2c..2fcb7932 100644 --- a/sharing/fake_nearby_sharing_service.cc +++ b/sharing/fake_nearby_sharing_service.cc @@ -191,12 +191,6 @@ void FakeNearbySharingService::FireStartDiscoveryResult(bool success) { } } -void FakeNearbySharingService::FireShutdown() { - for (auto& observer : observers_.GetObservers()) { - observer->OnShutdown(); - } -} - void FakeNearbySharingService::FireSendTransferUpdate( SendSurfaceState state, const ShareTarget& share_target, const AttachmentContainer& attachment_container, diff --git a/sharing/fake_nearby_sharing_service.h b/sharing/fake_nearby_sharing_service.h index fd8449a0..9aaf0376 100644 --- a/sharing/fake_nearby_sharing_service.h +++ b/sharing/fake_nearby_sharing_service.h @@ -118,7 +118,6 @@ class FakeNearbySharingService : public NearbySharingService { void FireHighVisibilityChanged(bool in_high_visibility); void FireStartAdvertisingFailure(); void FireStartDiscoveryResult(bool success); - void FireShutdown(); // Fire transfer update events. void FireSendTransferUpdate(SendSurfaceState state, diff --git a/sharing/nearby_sharing_service.h b/sharing/nearby_sharing_service.h index dce269c3..fb05d5e1 100644 --- a/sharing/nearby_sharing_service.h +++ b/sharing/nearby_sharing_service.h @@ -115,10 +115,6 @@ class NearbySharingService { virtual void OnIrrecoverableHardwareErrorReported() {} virtual void OnCredentialError() {} - - // Called during the |KeyedService| shutdown, but before everything has been - // cleaned up. It is safe to remove any observers on this event. - virtual void OnShutdown() = 0; }; static std::string StatusCodeToString(StatusCodes status_code); diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index 26e9d740..047b71ff 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -44,7 +44,6 @@ #include "absl/types/span.h" #include "internal/base/bluetooth_address.h" #include "internal/base/file_path.h" -#include "internal/base/observer_list.h" #include "internal/flags/nearby_flags.h" #include "internal/network/url.h" #include "internal/platform/device_info.h" @@ -291,11 +290,7 @@ void NearbySharingServiceImpl::Shutdown( "api_shutdown", [this, status_codes_callback = std::move(status_codes_callback)]() { *is_shutting_down_ = true; - for (auto* observer : observers_.GetObservers()) { - observer->OnShutdown(); - } - - observers_.Clear(); + service_observers_.Clear(); StopAdvertising(); StopFastInitiationScanning(); @@ -377,7 +372,7 @@ void NearbySharingServiceImpl::SendInitialAdapterState( // |observer| may have been removed before the task is run. This is not // sufficient to catch all cases, but without taking some form of // ownership of the observer, this is the best we can do. - if (!observers_.HasObserver(observer)) { + if (!service_observers_.HasObserver(observer)) { return; } observer->OnBluetoothStatusChanged( @@ -393,12 +388,12 @@ void NearbySharingServiceImpl::SendInitialAdapterState( void NearbySharingServiceImpl::AddObserver( NearbySharingService::Observer* observer) { SendInitialAdapterState(observer); - observers_.AddObserver(observer); + service_observers_.AddObserver(observer); } void NearbySharingServiceImpl::RemoveObserver( NearbySharingService::Observer* observer) { - observers_.RemoveObserver(observer); + service_observers_.RemoveObserver(observer); } void NearbySharingServiceImpl::RegisterSendSurface( @@ -1339,9 +1334,7 @@ void NearbySharingServiceImpl::OnLogoutSucceeded(absl::string_view account_id, // Reset all settings. ResetAllSettings(/*logout=*/true); if (credential_error) { - for (auto& observer : observers_.GetObservers()) { - observer->OnCredentialError(); - } + service_observers_.NotifyCredentialError(); } }); } @@ -1390,9 +1383,7 @@ void NearbySharingServiceImpl::AdapterPresentChanged( << present << ")"; NearbySharingService::Observer::AdapterState state = MapAdapterState(present, adapter->IsPowered()); - for (auto& observer : observers_.GetObservers()) { - observer->OnBluetoothStatusChanged(state); - } + service_observers_.NotifyBluetoothStatusChanged(state); InvalidateSurfaceState(); }); } @@ -1405,9 +1396,7 @@ void NearbySharingServiceImpl::AdapterPoweredChanged( << powered << ")"; NearbySharingService::Observer::AdapterState state = MapAdapterState(adapter->IsPresent(), powered); - for (auto& observer : observers_.GetObservers()) { - observer->OnBluetoothStatusChanged(state); - } + service_observers_.NotifyBluetoothStatusChanged(state); InvalidateSurfaceState(); }); } @@ -1420,9 +1409,7 @@ void NearbySharingServiceImpl::AdapterPresentChanged( << present << ")"; NearbySharingService::Observer::AdapterState state = MapAdapterState(present, adapter->IsPowered()); - for (auto& observer : observers_.GetObservers()) { - observer->OnWifiStatusChanged(state); - } + service_observers_.NotifyWifiStatusChanged(state); InvalidateSurfaceState(); }); } @@ -1435,9 +1422,7 @@ void NearbySharingServiceImpl::AdapterPoweredChanged( << powered << ")"; NearbySharingService::Observer::AdapterState state = MapAdapterState(adapter->IsPresent(), powered); - for (auto& observer : observers_.GetObservers()) { - observer->OnWifiStatusChanged(state); - } + service_observers_.NotifyWifiStatusChanged(state); InvalidateSurfaceState(); }); } @@ -1446,9 +1431,7 @@ void NearbySharingServiceImpl::HardwareErrorReported( NearbyFastInitiation* fast_init) { RunOnNearbySharingServiceThread("hardware_error_reported", [this]() { VLOG(1) << __func__ << ": Hardware error reported, need to restart PC."; - for (auto& observer : observers_.GetObservers()) { - observer->OnIrrecoverableHardwareErrorReported(); - } + service_observers_.NotifyIrrecoverableHardwareErrorReported(); InvalidateSurfaceState(); }); } @@ -2083,9 +2066,7 @@ void NearbySharingServiceImpl::InvalidateAdvertisingState() { return; } if (device_name.has_value()) { - for (auto& observer : observers_.GetObservers()) { - observer->OnHighVisibilityChangeRequested(); - } + service_observers_.NotifyHighVisibilityChangeRequested(); } advertising_session_id_ = analytics_recorder_.GenerateNextId(); @@ -3432,9 +3413,7 @@ void NearbySharingServiceImpl::OnStartAdvertisingResult(bool used_device_name, << ": StartAdvertising over Nearby Connections failed: " << NearbyConnectionsManager::ConnectionsStatusToString(status); SetInHighVisibility(false); - for (auto& observer : observers_.GetObservers()) { - observer->OnStartAdvertisingFailure(); - } + service_observers_.NotifyStartAdvertisingFailure(); } } @@ -3473,9 +3452,7 @@ void NearbySharingServiceImpl::OnStartDiscoveryResult(Status status) { << ": StartDiscovery over Nearby Connections failed: " << NearbyConnectionsManager::ConnectionsStatusToString(status); } - for (auto& observer : observers_.GetObservers()) { - observer->OnStartDiscoveryResult(success); - } + service_observers_.NotifyStartDiscoveryResult(success); } void NearbySharingServiceImpl::SetInHighVisibility( @@ -3485,9 +3462,7 @@ void NearbySharingServiceImpl::SetInHighVisibility( } in_high_visibility_ = new_in_high_visibility; - for (auto& observer : observers_.GetObservers()) { - observer->OnHighVisibilityChanged(in_high_visibility_); - } + service_observers_.NotifyHighVisibilityChanged(in_high_visibility_); } void NearbySharingServiceImpl::OnNetworkChanged( @@ -3507,9 +3482,7 @@ void NearbySharingServiceImpl::OnLanConnectedChanged(bool connected) { NearbySharingService::Observer::AdapterState state = connected ? NearbySharingService::Observer::AdapterState::ENABLED : NearbySharingService::Observer::AdapterState::DISABLED; - for (auto& observer : observers_.GetObservers()) { - observer->OnLanStatusChanged(state); - } + service_observers_.NotifyLanStatusChanged(state); }); } diff --git a/sharing/nearby_sharing_service_impl.h b/sharing/nearby_sharing_service_impl.h index a4ada80b..adbf1db1 100644 --- a/sharing/nearby_sharing_service_impl.h +++ b/sharing/nearby_sharing_service_impl.h @@ -34,7 +34,6 @@ #include "absl/strings/string_view.h" #include "absl/time/time.h" #include "absl/types/span.h" -#include "internal/base/observer_list.h" #include "internal/platform/clock.h" #include "internal/platform/device_info.h" #include "internal/platform/implementation/account_manager.h" @@ -69,6 +68,7 @@ #include "sharing/paired_key_verification_runner.h" #include "sharing/proto/enums.pb.h" #include "sharing/proto/wire_format.pb.h" +#include "sharing/service_observers.h" #include "sharing/share_session.h" #include "sharing/share_target.h" #include "sharing/share_target_discovered_callback.h" @@ -492,7 +492,7 @@ class NearbySharingServiceImpl std::unique_ptr certificate_download_during_discovery_timer_; // A list of service observers. - ObserverList observers_; + ServiceObservers service_observers_; // A map of foreground receiver callbacks -> vendor ID. absl::flat_hash_map foreground_receive_callbacks_map_; diff --git a/sharing/nearby_sharing_service_impl_test.cc b/sharing/nearby_sharing_service_impl_test.cc index d2151fd5..f8a573aa 100644 --- a/sharing/nearby_sharing_service_impl_test.cc +++ b/sharing/nearby_sharing_service_impl_test.cc @@ -1375,14 +1375,7 @@ class TestObserver : public NearbySharingService::Observer { void OnCredentialError() override { credential_error_called_ = true; } - void OnShutdown() override { - shutdown_called_ = true; - service_->RemoveObserver(this); - service_ = nullptr; - } - bool in_high_visibility_ = false; - bool shutdown_called_ = false; bool on_start_advertising_failure_called_ = false; bool credential_error_called_ = false; NearbySharingService* service_; @@ -3969,13 +3962,6 @@ TEST_F(NearbySharingServiceImplTest, AddObserverLanAdapterUpdate) { service_->RemoveObserver(&observer); } -TEST_F(NearbySharingServiceImplTest, ShutdownCallsObservers) { - TestObserver observer(service_.get()); - EXPECT_FALSE(observer.shutdown_called_); - Shutdown(); - EXPECT_TRUE(observer.shutdown_called_); -} - TEST_F(NearbySharingServiceImplTest, RotateBackgroundAdvertisementPeriodic) { certificate_manager()->set_next_salt({0x00, 0x01}); SetVisibility(DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS); diff --git a/sharing/service_observers.cc b/sharing/service_observers.cc new file mode 100644 index 00000000..119d141d --- /dev/null +++ b/sharing/service_observers.cc @@ -0,0 +1,140 @@ +// Copyright 2025 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/service_observers.h" + +#include "absl/synchronization/mutex.h" +#include "sharing/nearby_sharing_service.h" + +namespace nearby::sharing { + +using ::absl::Seconds; + +// An RAII class that tracks the number of inflight observer notifications. +// This is used to ensure that observers are not destroyed until all inflight +// notifications have completed. +class ObserverNotifyTracker { + public: + explicit ObserverNotifyTracker(ServiceObservers& service_observers) + : service_observers_(service_observers) { + service_observers_.BeginNotify(); + } + ~ObserverNotifyTracker() { service_observers_.EndNotify(); } + + private: + ServiceObservers& service_observers_; +}; + +void ServiceObservers::AddObserver(NearbySharingService::Observer* observer) { + observers_.AddObserver(observer); +} + +void ServiceObservers::RemoveObserver( + NearbySharingService::Observer* observer) { + if (!observers_.HasObserver(observer)) { + return; + } + observers_.RemoveObserver(observer); + // Wait an arbitrary amount of time (1s) for inflight callbacks to complete. + absl::MutexLock lock(mutex_); + mutex_.AwaitWithTimeout(absl::Condition( + +[](int* in_flight_notify_count) { return *in_flight_notify_count == 0; }, + &in_flight_notify_count_), Seconds(1)); +} + +void ServiceObservers::Clear() { + observers_.Clear(); +} + +bool ServiceObservers::HasObserver(NearbySharingService::Observer* observer) { + return observers_.HasObserver(observer); +} + +void ServiceObservers::BeginNotify() { + absl::MutexLock lock(mutex_); + ++in_flight_notify_count_; +} + +void ServiceObservers::EndNotify() { + absl::MutexLock lock(mutex_); + --in_flight_notify_count_; +} + +void ServiceObservers::NotifyHighVisibilityChangeRequested() { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnHighVisibilityChangeRequested(); + } +} + +void ServiceObservers::NotifyHighVisibilityChanged(bool in_high_visibility) { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnHighVisibilityChanged(in_high_visibility); + } +} + +void ServiceObservers::NotifyStartAdvertisingFailure() { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnStartAdvertisingFailure(); + } +} + +void ServiceObservers::NotifyStartDiscoveryResult(bool success) { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnStartDiscoveryResult(success); + } +} + +void ServiceObservers::NotifyBluetoothStatusChanged( + NearbySharingService::Observer::AdapterState state) { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnBluetoothStatusChanged(state); + } +} + +void ServiceObservers::NotifyWifiStatusChanged( + NearbySharingService::Observer::AdapterState state) { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnWifiStatusChanged(state); + } +} + +void ServiceObservers::NotifyLanStatusChanged( + NearbySharingService::Observer::AdapterState state) { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnLanStatusChanged(state); + } +} + +void ServiceObservers::NotifyIrrecoverableHardwareErrorReported() { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnIrrecoverableHardwareErrorReported(); + } +} + +void ServiceObservers::NotifyCredentialError() { + ObserverNotifyTracker notify_tracker(*this); + for (auto& observer : observers_.GetObservers()) { + observer->OnCredentialError(); + } +} + +} // namespace nearby::sharing diff --git a/sharing/service_observers.h b/sharing/service_observers.h new file mode 100644 index 00000000..63ea33db --- /dev/null +++ b/sharing/service_observers.h @@ -0,0 +1,71 @@ +// Copyright 2025 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_SERVICE_OBSERVERS_H_ +#define THIRD_PARTY_NEARBY_SHARING_SERVICE_OBSERVERS_H_ + +#include "absl/base/thread_annotations.h" +#include "absl/synchronization/mutex.h" +#include "internal/base/observer_list.h" +#include "sharing/nearby_sharing_service.h" + +namespace nearby::sharing { + +// A class that manages a list of NearbySharingService::Observers. +// This class is thread-safe. +// +// This class is used to ensure that observers are not destroyed until all +// inflight notifications have completed. +// +// Do not call RemoveObserver() from an Observer callback, otherwise a deadlock +// will occur. +// TODO(ftsui): Consider using a callback in RemoveObserver() to avoid deadlock. +class ServiceObservers { + public: + void AddObserver(NearbySharingService::Observer* observer); + // Remove an observer from the list. + // This method will block until all inflight callbacks to observers have + // completed. + void RemoveObserver(NearbySharingService::Observer* observer); + + void Clear(); + bool HasObserver(NearbySharingService::Observer* observer); + + void NotifyHighVisibilityChangeRequested(); + void NotifyHighVisibilityChanged(bool in_high_visibility); + void NotifyStartAdvertisingFailure(); + void NotifyStartDiscoveryResult(bool success); + void NotifyBluetoothStatusChanged( + NearbySharingService::Observer::AdapterState state); + void NotifyWifiStatusChanged( + NearbySharingService::Observer::AdapterState state); + void NotifyLanStatusChanged( + NearbySharingService::Observer::AdapterState state); + void NotifyIrrecoverableHardwareErrorReported(); + void NotifyCredentialError(); + + private: + friend class ObserverNotifyTracker; + + void BeginNotify(); + void EndNotify(); + + ObserverList observers_; + absl::Mutex mutex_; + int in_flight_notify_count_ ABSL_GUARDED_BY(mutex_) = 0; +}; + +} // namespace nearby::sharing + +#endif // THIRD_PARTY_NEARBY_SHARING_SERVICE_OBSERVERS_H_