Fix lifetime management of NearbySharingService::Observer.

PiperOrigin-RevId: 792782838

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",
     ],

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,

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,

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);

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);
       });
 }
 

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<ThreadTimer> certificate_download_during_discovery_timer_;
 
   // A list of service observers.
-  ObserverList<NearbySharingService::Observer> observers_;
+  ServiceObservers service_observers_;
   // A map of foreground receiver callbacks -> vendor ID.
   absl::flat_hash_map<TransferUpdateCallback*, Advertisement::BlockedVendorId>
       foreground_receive_callbacks_map_;

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);

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

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<NearbySharingService::Observer> 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_