Fixed race on cleanup of database query value and child listeners on iOS.

The assumption that FIRDatabaseQuery:removeObserverWithHandle:handle
is synchronous - it's not - could lead to C++ objects being referenced from
observer blocks after C++ Database, Query, ChildListener and ValueListener
objects have been destroyed.

This changes all Objective-C observer blocks to reference
the Objective-C FIRCPPDatabaseQueryCallbackState object instead of using direct
references to C++ objects.  Using FIRCPPDatabaseQueryCallbackState it's
possible to take advantage of ARC to handle the cleanup of the observer state
and also to remove references to C++ objects from each observer before C++
objects are destroyed.

This also, fixes all compilation warnings due to missing pointer requirement
annotations.

PiperOrigin-RevId: 250547302

Author: smiles

database/src/ios/data_snapshot_ios.h

@@ -40,6 +40,8 @@ class DatabaseReferenceInternal;
 // wrapper around the FIRDataSnapshot Obj-C class.
 OBJ_C_PTR_WRAPPER(FIRDataSnapshot);
 
+#pragma clang assume_nonnull begin
+
 // The iOS implementation of the DataSnapshot, which contains data from a
 // Firebase Database location.
 class DataSnapshotInternal {
@@ -109,7 +111,7 @@ class DataSnapshotInternal {
 
  private:
 #ifdef __OBJC__
-  FIRDataSnapshot* _Nonnull impl() const { return impl_->ptr; }
+  FIRDataSnapshot* impl() const { return impl_->ptr; }
 #endif  // __OBJC__
 
   DatabaseInternal* database_;
@@ -118,6 +120,8 @@ class DataSnapshotInternal {
   UniquePtr<FIRDataSnapshotPointer> impl_;
 };
 
+#pragma clang assume_nonnull end
+
 }  // namespace internal
 }  // namespace database
 }  // namespace firebase

database/src/ios/database_ios.h

@@ -40,6 +40,12 @@ namespace internal {
 // This defines the class FIRDatabasePointer, which is a C++-compatible wrapper
 // around the FIRDatabase Obj-C class.
 OBJ_C_PTR_WRAPPER(FIRDatabase);
+// This defines the class firebase::database::internal::NSRecursiveLockPointer,
+// which is a C++-compatible wrapper around the NSRecursiveLock Obj-C class,
+// used by observer callbacks of FIRDatabaseQuery.
+OBJ_C_PTR_WRAPPER(NSRecursiveLock);
+
+#pragma clang assume_nonnull begin
 
 // This is the iOS implementation of Database.
 class DatabaseInternal {
@@ -82,9 +88,9 @@ class DatabaseInternal {
   static void SetVerboseLogging(bool enable);
 
 #ifdef __OBJC__
-  bool RegisterValueListener(const internal::QuerySpec& spec,
-                             ValueListener* listener,
-                             const ValueListenerCleanupData& cleanup_data);
+  bool RegisterValueListener(
+      const internal::QuerySpec& spec, ValueListener* listener,
+      FIRCPPDatabaseQueryCallbackState* callback_state);
 
   bool UnregisterValueListener(const internal::QuerySpec& spec,
                                ValueListener* listener,
@@ -93,9 +99,9 @@ class DatabaseInternal {
   void UnregisterAllValueListeners(const internal::QuerySpec& spec,
                                    FIRDatabaseQuery* query_impl);
 
-  bool RegisterChildListener(const internal::QuerySpec& spec,
-                             ChildListener* listener,
-                             const ChildListenerCleanupData& cleanup_data);
+  bool RegisterChildListener(
+      const internal::QuerySpec& spec, ChildListener* listener,
+      FIRCPPDatabaseQueryCallbackState* callback_state);
 
   bool UnregisterChildListener(const internal::QuerySpec& spec,
                                ChildListener* listener,
@@ -105,38 +111,18 @@ class DatabaseInternal {
                                    FIRDatabaseQuery* query_impl);
 #endif  // __OBJC__
 
-  // Track a transient listener. If the database is deleted before the listener
-  // finishes, it should discard its pointers.
-  SingleValueListener** AddSingleValueListener(SingleValueListener* listener) {
+  // Track a transient listener.
+  void AddSingleValueListener(ValueListener* listener) {
     MutexLock lock(listener_mutex_);
-    // If the listener is already being tracked, just return the existing
-    // listener holder.
-    for (auto i = single_value_listeners_.begin();
-         i != single_value_listeners_.end(); ++i) {
-      SingleValueListener** listener_holder = *i;
-      if (*listener_holder == listener) {
-        return listener_holder;
-      }
-    }
-    // If the listener was not found, register create a new holder and return
-    // it.
-    SingleValueListener** holder = new SingleValueListener*(listener);
-    single_value_listeners_.insert(holder);
-    return holder;
+    single_value_listeners_.insert(listener);
   }
 
-  // Finish tracking a transient listener. If the database is deleted before the
-  // listener finishes, it should discard its pointers.
-  void RemoveSingleValueListener(SingleValueListener* listener) {
+  // Finish tracking a transient listener.
+  void RemoveSingleValueListener(ValueListener* listener) {
     MutexLock lock(listener_mutex_);
-    for (auto i = single_value_listeners_.begin();
-         i != single_value_listeners_.end(); ++i) {
-      SingleValueListener** listener_holder = *i;
-      if (*listener_holder == listener) {
-        single_value_listeners_.erase(i);
-        return;
-      }
-    }
+    auto it = single_value_listeners_.find(listener);
+    if (it == single_value_listeners_.end()) return;
+    single_value_listeners_.erase(it);
   }
 
   FutureManager& future_manager() { return future_manager_; }
@@ -151,9 +137,17 @@ class DatabaseInternal {
   // The url that was passed to the constructor.
   const std::string& constructor_url() const { return constructor_url_; }
 
+#ifdef __OBJC__
+  // Guard access to C++ objects referenced by
+  // FIRCPPDatabaseQueryCallbackStatePointer.
+  NSRecursiveLock* query_lock() const {
+    return query_lock_->ptr;
+  }
+#endif  // __OBJC__
+
  private:
 #ifdef __OBJC__
-  FIRDatabase* _Nonnull impl() const { return impl_->ptr; }
+  FIRDatabase* impl() const { return impl_->ptr; }
 #endif  // __OBJC__
 
   // The firebase::App that this Database was created with.
@@ -162,18 +156,22 @@ class DatabaseInternal {
   // Object lifetime managed by Objective C ARC.
   UniquePtr<FIRDatabasePointer> impl_;
 
+  // Lock used to guard access to C++ objects referenced by FIRDatabaseQuery
+  // callbacks.
+  UniquePtr<NSRecursiveLockPointer> query_lock_;
+
   // For registering listeners.
   Mutex listener_mutex_;
 
   // Listeners indexed by QuerySpec.
   ListenerCollection<ValueListener> value_listeners_by_query_;
   ListenerCollection<ChildListener> child_listeners_by_query_;
 
-  std::map<ValueListener*, ValueListenerCleanupData>
+  std::map<ValueListener*, FIRCPPDatabaseQueryCallbackStatePointer>
       cleanup_value_listener_lookup_;
-  std::map<ChildListener*, ChildListenerCleanupData>
+  std::map<ChildListener*, FIRCPPDatabaseQueryCallbackStatePointer>
       cleanup_child_listener_lookup_;
-  std::set<SingleValueListener**> single_value_listeners_;
+  std::set<ValueListener*> single_value_listeners_;
 
   FutureManager future_manager_;
 
@@ -184,6 +182,8 @@ class DatabaseInternal {
   std::string constructor_url_;
 };
 
+#pragma clang assume_nonnull end
+
 }  // namespace internal
 }  // namespace database
 }  // namespace firebase

database/src/ios/database_ios.mm

@@ -30,6 +30,7 @@
   @try {
     impl_.reset(new FIRDatabasePointer(
         [FIRDatabase databaseForApp:static_cast<FIRAppPointer*>(app->data_)->ptr]));
+    query_lock_.reset(new NSRecursiveLockPointer([[NSRecursiveLock alloc] init]));
   }
   @catch (NSException* exception) {
     LogError(
@@ -43,6 +44,7 @@
   @try {
     impl_.reset(new FIRDatabasePointer(
         [FIRDatabase databaseForApp:static_cast<FIRAppPointer*>(app->data_)->ptr URL:@(url)]));
+    query_lock_.reset(new NSRecursiveLockPointer([[NSRecursiveLock alloc] init]));
   }
   @catch (NSException* exception) {
     LogError(
@@ -57,12 +59,12 @@
   // If there are any pending listeners, delete their pointers.
   {
     MutexLock lock(listener_mutex_);
-    for (auto i = single_value_listeners_.begin(); i != single_value_listeners_.end(); ++i) {
-      SingleValueListener** listener_holder = *i;
-        delete *listener_holder;
-        *listener_holder = nullptr;
+    while (single_value_listeners_.begin() != single_value_listeners_.end()) {
+      auto it = single_value_listeners_.begin();
+      auto* listener = *it;
+      single_value_listeners_.erase(it);
+      delete listener;
     }
-    single_value_listeners_.clear();
   }
 }
 
@@ -108,12 +110,14 @@ new DatabaseReferenceInternal(const_cast<DatabaseInternal*>(this),
 
 bool DatabaseInternal::RegisterValueListener(
     const internal::QuerySpec& spec, ValueListener* listener,
-    const ValueListenerCleanupData& cleanup_data) {
+    FIRCPPDatabaseQueryCallbackState* callback_state) {
   MutexLock lock(listener_mutex_);
   if (value_listeners_by_query_.Register(spec, listener)) {
     auto found = cleanup_value_listener_lookup_.find(listener);
     if (found == cleanup_value_listener_lookup_.end()) {
-      cleanup_value_listener_lookup_.insert(std::make_pair(listener, cleanup_data));
+      cleanup_value_listener_lookup_.insert(std::make_pair(
+          listener, FIRCPPDatabaseQueryCallbackStatePointer(
+              callback_state)));
     }
     return true;
   }
@@ -127,8 +131,7 @@ new DatabaseReferenceInternal(const_cast<DatabaseInternal*>(this),
   if (value_listeners_by_query_.Unregister(spec, listener)) {
     auto found = cleanup_value_listener_lookup_.find(listener);
     if (found != cleanup_value_listener_lookup_.end()) {
-      ValueListenerCleanupData& cleanup_data = found->second;
-      [query_impl removeObserverWithHandle: cleanup_data.observer_handle];
+      [found->second.ptr removeAllObservers];
       cleanup_value_listener_lookup_.erase(found);
     }
     return true;
@@ -146,13 +149,16 @@ new DatabaseReferenceInternal(const_cast<DatabaseInternal*>(this),
   }
 }
 
-bool DatabaseInternal::RegisterChildListener(const internal::QuerySpec& spec,
-    ChildListener* listener, const ChildListenerCleanupData& cleanup_data) {
+bool DatabaseInternal::RegisterChildListener(
+    const internal::QuerySpec& spec, ChildListener* listener,
+    FIRCPPDatabaseQueryCallbackState* _Nonnull callback_state) {
   MutexLock lock(listener_mutex_);
   if (child_listeners_by_query_.Register(spec, listener)) {
     auto found = cleanup_child_listener_lookup_.find(listener);
     if (found == cleanup_child_listener_lookup_.end()) {
-      cleanup_child_listener_lookup_.insert(std::make_pair(listener, cleanup_data));
+      cleanup_child_listener_lookup_.insert(std::make_pair(
+          listener, FIRCPPDatabaseQueryCallbackStatePointer(
+              callback_state)));
     }
     return true;
   }
@@ -166,11 +172,7 @@ new DatabaseReferenceInternal(const_cast<DatabaseInternal*>(this),
   if (child_listeners_by_query_.Unregister(spec, listener)) {
     auto found = cleanup_child_listener_lookup_.find(listener);
     if (found != cleanup_child_listener_lookup_.end()) {
-      ChildListenerCleanupData& cleanup_data = found->second;
-      [query_impl removeObserverWithHandle: cleanup_data.child_added_handle];
-      [query_impl removeObserverWithHandle: cleanup_data.child_changed_handle];
-      [query_impl removeObserverWithHandle: cleanup_data.child_moved_handle];
-      [query_impl removeObserverWithHandle: cleanup_data.child_removed_handle];
+      [found->second.ptr removeAllObservers];
       cleanup_child_listener_lookup_.erase(found);
     }
     return true;

database/src/ios/database_reference_ios.h

@@ -27,6 +27,10 @@
 #include "database/src/include/firebase/database/transaction.h"
 #include "database/src/ios/query_ios.h"
 
+#ifdef __OBJC__
+#import "FIRDatabase.h"
+#endif  // __OBJC__
+
 namespace firebase {
 namespace database {
 namespace internal {
@@ -35,13 +39,16 @@ namespace internal {
 // wrapper around the FIRDatabaseReference Obj-C class.
 OBJ_C_PTR_WRAPPER(FIRDatabaseReference);
 
+#pragma clang assume_nonnull begin
+
 // The iOS implementation of the DatabaseReference class, which represents a
 // particular location in your Database and can be used for reading or writing
 // data to that Database location.
 class DatabaseReferenceInternal : public QueryInternal {
  public:
   explicit DatabaseReferenceInternal(
-      DatabaseInternal* database, UniquePtr<FIRDatabaseReferencePointer> impl);
+      DatabaseInternal* database,
+      UniquePtr<FIRDatabaseReferencePointer> impl);
 
   virtual ~DatabaseReferenceInternal();
 
@@ -103,8 +110,10 @@ class DatabaseReferenceInternal : public QueryInternal {
   // Run a user-supplied callback function, possibly multiple times, to perform
   // an atomic transaction on the database.
   Future<DataSnapshot> RunTransaction(
-      DoTransactionWithContext transaction_function, void* context,
-      void (*delete_context)(void*), bool trigger_local_events = true);
+      DoTransactionWithContext transaction_function,
+      void* _Nullable context,
+      void (* _Nullable delete_context)(void* _Nullable),
+      bool trigger_local_events = true);
 
   // Get the result of the most recent call to RunTransaction().
   Future<DataSnapshot> RunTransactionLastResult();
@@ -142,7 +151,7 @@ class DatabaseReferenceInternal : public QueryInternal {
 
   // Get the disconnect handler, which controls what actions the server will
   // perform to this location's data when this client disconnects.
-  DisconnectionHandler* OnDisconnect();
+  DisconnectionHandler* _Nullable OnDisconnect();
 
   // Manually disconnect Firebase Realtime Database from the server, and disable
   // automatic reconnection.
@@ -154,7 +163,7 @@ class DatabaseReferenceInternal : public QueryInternal {
 
  protected:
 #ifdef __OBJC__
-  FIRDatabaseReference* _Nonnull impl() const { return impl_->ptr; }
+  FIRDatabaseReference* impl() const { return impl_->ptr; }
 #endif  // __OBJC__
 
  private:
@@ -164,7 +173,7 @@ class DatabaseReferenceInternal : public QueryInternal {
   // Object lifetime managed by Objective C ARC.
   UniquePtr<FIRDatabaseReferencePointer> impl_;
 
-  DisconnectionHandler* cached_disconnection_handler_;
+  DisconnectionHandler* _Nullable cached_disconnection_handler_;
 
   // The memory location of this member variable is used to look up our
   // ReferenceCountedFutureImpl. We can't use "this" because QueryInternal and
@@ -174,6 +183,8 @@ class DatabaseReferenceInternal : public QueryInternal {
   int future_api_id_;
 };
 
+#pragma clang assume_nonnull end
+
 }  // namespace internal
 }  // namespace database
 }  // namespace firebase

database/src/ios/disconnection_ios.h

@@ -29,6 +29,8 @@ namespace firebase {
 namespace database {
 namespace internal {
 
+#pragma clang assume_nonnull begin
+
 class FIRDatabaseReferencePointer;
 class DatabaseInternal;
 class DatabaseReferenceInternal;
@@ -92,10 +94,11 @@ class DisconnectionHandlerInternal {
   friend class DatabaseReferenceInternal;
 
   explicit DisconnectionHandlerInternal(
-      DatabaseInternal* database, UniquePtr<FIRDatabaseReferencePointer> impl);
+      DatabaseInternal* database,
+      UniquePtr<FIRDatabaseReferencePointer> impl);
 
 #ifdef __OBJC__
-  FIRDatabaseReference* _Nonnull impl() const;
+  FIRDatabaseReference* impl() const;
 #endif  // __OBJC__
 
   // Get the Future for the DatabaseReferenceInternal.
@@ -106,6 +109,8 @@ class DisconnectionHandlerInternal {
   UniquePtr<FIRDatabaseReferencePointer> impl_;
 };
 
+#pragma clang assume_nonnull end
+
 }  // namespace internal
 }  // namespace database
 }  // namespace firebase

database/src/ios/mutable_data_ios.h

@@ -33,6 +33,8 @@ namespace internal {
 // wrapper around the FIRMutableData Obj-C class.
 OBJ_C_PTR_WRAPPER(FIRMutableData);
 
+#pragma clang assume_nonnull begin
+
 class DatabaseReferenceInternal;
 
 // The iOS implementation of MutableData, which encapsulates the data and
@@ -87,13 +89,15 @@ class MutableDataInternal {
                       UniquePtr<FIRMutableDataPointer> impl);
 
 #ifdef __OBJC__
-  FIRMutableData* _Nonnull impl() const { return impl_->ptr; }
+  FIRMutableData* impl() const { return impl_->ptr; }
 #endif  // __OBJC__
 
   DatabaseInternal* db_;
   UniquePtr<FIRMutableDataPointer> impl_;
 };
 
+#pragma clang assume_nonnull end
+
 }  // namespace internal
 }  // namespace database
 }  // namespace firebase