A couple of fixes in Future, which modify the way that each Future's reference count is kept. This:

- Fixes a race condition between creating a FutureHandle and creating a Future from that handle.
- Fixes Future OnCompletion handlers so that they run even if there is no Future instance left.
This CL also includes new and modified tests to exercise these bugs, including a test that allocates a couple million futures and makes sure that memory isn't leaking.

This is accomplished by refactoring FutureHandle so that it is included in the reference count in each FutureBackingData. Previously only fully-realized Future instances would count towards the reference count.

PiperOrigin-RevId: 307743575

Author: jonsimantov

app/src/include/firebase/future.h

@@ -19,6 +19,7 @@
 
 #include <stddef.h>
 #include <stdint.h>
+
 #include <utility>
 
 #include "firebase/internal/common.h"
@@ -56,7 +57,55 @@ enum FutureStatus {
 
 /// Handle that the API uses to identify an asynchronous call.
 /// The exact interpretation of the handle is up to the API.
-typedef uintptr_t FutureHandle;
+typedef uintptr_t FutureHandleId;
+
+/// Class that provides more context to FutureHandleId, which allows the
+/// underlying API to track handles, perform reference counting, etc.
+class FutureHandle {
+ public:
+  /// @cond FIREBASE_APP_INTERNAL
+  FutureHandle();
+  explicit FutureHandle(FutureHandleId id) : FutureHandle(id, nullptr) {}
+  FutureHandle(FutureHandleId id, detail::FutureApiInterface* api);
+  ~FutureHandle();
+
+  // Copy constructor and assignment operator.
+  FutureHandle(const FutureHandle& rhs);
+  FutureHandle& operator=(const FutureHandle& rhs);
+
+#if defined(FIREBASE_USE_MOVE_OPERATORS)
+  // Move constructor and assignment operator.
+  FutureHandle(FutureHandle&& rhs) noexcept;
+  FutureHandle& operator=(FutureHandle&& rhs) noexcept;
+#endif  // defined(FIREBASE_USE_MOVE_OPERATORS)
+
+  // Comparison operators.
+  bool operator!=(const FutureHandle& rhs) const { return !(*this == rhs); }
+  bool operator==(const FutureHandle& rhs) const {
+    // Only compare IDs, since the API is irrelevant (comparison will only occur
+    // within the context of a single API anyway).
+    return id() == rhs.id();
+  }
+
+  FutureHandleId id() const { return id_; }
+  detail::FutureApiInterface* api() const { return api_; }
+
+  // Detach from the FutureApi. This handle will no longer increment the
+  // Future's reference count. This is mainly used for testing, so that you can
+  // still look up the Future based on its handle's ID without affecting the
+  // reference count yourself.
+  void Detach();
+
+  // Called by CleanupNotifier when the API is being deleted. At this point we
+  // can ignore all of the reference counts since all Future data is about to be
+  // deleted anyway.
+  void Cleanup() { api_ = nullptr; }
+
+ private:
+  FutureHandleId id_;
+  detail::FutureApiInterface* api_;
+  /// @endcond
+};
 
 /// @brief Type-independent return type of asynchronous calls.
 ///
@@ -100,7 +149,7 @@ class FutureBase {
   ///
   /// @param api API class used to provide the future implementation.
   /// @param handle Handle to the future.
-  FutureBase(detail::FutureApiInterface* api, FutureHandle handle);
+  FutureBase(detail::FutureApiInterface* api, const FutureHandle& handle);
 
   /// @endcond
 
@@ -229,8 +278,8 @@ class FutureBase {
   /// @param[in] user_data Optional user data. We will pass this back to your
   /// callback.
   /// @return A handle that can be passed to RemoveOnCompletion.
-  CompletionCallbackHandle
-  AddOnCompletion(CompletionCallback callback, void* user_data) const;
+  CompletionCallbackHandle AddOnCompletion(CompletionCallback callback,
+                                           void* user_data) const;
 
 #if defined(FIREBASE_USE_STD_FUNCTION) || defined(DOXYGEN)
   /// Like OnCompletion, but allows adding multiple callbacks.
@@ -272,7 +321,7 @@ class FutureBase {
 
 #if defined(INTERNAL_EXPERIMENTAL)
   /// Returns the API-specific handle. Should only be called by the API.
-  FutureHandle GetHandle() const { return handle_; }
+  const FutureHandle& GetHandle() const { return handle_; }
 #endif  // defined(INTERNAL_EXPERIMENTAL)
 
  protected:
@@ -365,7 +414,7 @@ class Future : public FutureBase {
   ///
   /// @param api API class used to provide the future implementation.
   /// @param handle Handle to the future.
-  Future(detail::FutureApiInterface* api, FutureHandle handle)
+  Future(detail::FutureApiInterface* api, const FutureHandle& handle)
       : FutureBase(api, handle) {}
 
   /// @endcond
@@ -409,8 +458,8 @@ class Future : public FutureBase {
   /// @note This is the same callback as FutureBase::OnCompletion(), so you
   /// can't expect to set both and have both run; again, only the most recently
   /// registered one will run.
-  inline void
-  OnCompletion(TypedCompletionCallback callback, void* user_data) const;
+  inline void OnCompletion(TypedCompletionCallback callback,
+                           void* user_data) const;
 
 #if defined(FIREBASE_USE_STD_FUNCTION) || defined(DOXYGEN)
   /// Register a single callback that will be called at most once, when the
@@ -427,8 +476,8 @@ class Future : public FutureBase {
   /// @note This is the same callback as FutureBase::OnCompletion(), so you
   /// can't expect to set both and have both run; again, only the most recently
   /// registered one will run.
-  inline void
-  OnCompletion(std::function<void(const Future<ResultType>&)> callback) const;
+  inline void OnCompletion(
+      std::function<void(const Future<ResultType>&)> callback) const;
 #endif  // defined(FIREBASE_USE_STD_FUNCTION) || defined(DOXYGEN)
 
 #if defined(INTERNAL_EXPERIMENTAL)
@@ -447,8 +496,8 @@ class Future : public FutureBase {
   /// @param[in] user_data Optional user data. We will pass this back to your
   /// callback.
   /// @return A handle that can be passed to RemoveOnCompletion.
-  inline CompletionCallbackHandle
-  AddOnCompletion(TypedCompletionCallback callback, void* user_data) const;
+  inline CompletionCallbackHandle AddOnCompletion(
+      TypedCompletionCallback callback, void* user_data) const;
 
 #if defined(FIREBASE_USE_STD_FUNCTION) || defined(DOXYGEN)
   /// Like OnCompletion, but allows adding multiple callbacks.
@@ -467,9 +516,8 @@ class Future : public FutureBase {
   ///
   /// @note This method is not available when using STLPort on Android, as
   /// `std::function` is not supported on STLPort.
-  inline CompletionCallbackHandle
-  AddOnCompletion(std::function<void(const Future<ResultType>&)> callback)
-      const;
+  inline CompletionCallbackHandle AddOnCompletion(
+      std::function<void(const Future<ResultType>&)> callback) const;
 #endif  // defined(FIREBASE_USE_STD_FUNCTION) || defined(DOXYGEN)
 #endif  // defined(INTERNAL_EXPERIMENTAL)
 };

app/src/include/firebase/internal/future_impl.h

@@ -52,34 +52,35 @@ class FutureApiInterface {
 
   /// Increment the reference count on handle's asynchronous call.
   /// Called when the Future is copied.
-  virtual void ReferenceFuture(FutureHandle handle) = 0;
+  virtual void ReferenceFuture(const FutureHandle& handle) = 0;
 
   /// Decrement the reference count on handle's asynchronous call.
   /// Called when the Future is destroyed or moved.
   /// If the reference count drops to zero, the asynchronous call can be
   /// forgotten.
-  virtual void ReleaseFuture(FutureHandle handle) = 0;
+  virtual void ReleaseFuture(const FutureHandle& handle) = 0;
 
   /// Return the status of the asynchronous call.
-  virtual FutureStatus GetFutureStatus(FutureHandle handle) const = 0;
+  virtual FutureStatus GetFutureStatus(const FutureHandle& handle) const = 0;
 
   /// Return the API-specific error.
   /// Valid when GetFutureStatus() is kFutureStatusComplete, and undefined
   /// otherwise.
-  virtual int GetFutureError(FutureHandle handle) const = 0;
+  virtual int GetFutureError(const FutureHandle& handle) const = 0;
 
   /// Return the API-specific error, in human-readable form, or "" if no message
   /// has been provided.
   /// Valid when GetFutureStatus() is kFutureStatusComplete, and undefined
   /// otherwise.
-  virtual const char* GetFutureErrorMessage(FutureHandle handle) const = 0;
+  virtual const char* GetFutureErrorMessage(
+      const FutureHandle& handle) const = 0;
 
   /// Return a pointer to the completed asynchronous result, or NULL if
   /// result is still pending.
   /// After an asynchronous call is marked complete, the API should not
   /// modify the result (especially on a callback thread), since the threads
   /// owning the Future can reference the result memory via this function.
-  virtual const void* GetFutureResult(FutureHandle handle) const = 0;
+  virtual const void* GetFutureResult(const FutureHandle& handle) const = 0;
 
   /// Register a callback that will be called when this future's status is set
   /// to Complete. If clear_existing_callbacks is true, then the new callback
@@ -92,16 +93,14 @@ class FutureApiInterface {
   /// After the callback has been called, if `user_data_delete_fn_ptr` is
   /// non-null, then `(*user_data_delete_fn_ptr)(user_data)` will be called.
   virtual CompletionCallbackHandle AddCompletionCallback(
-      FutureHandle handle, FutureBase::CompletionCallback callback,
-      void* user_data,
-      void (* user_data_delete_fn)(void *),
+      const FutureHandle& handle, FutureBase::CompletionCallback callback,
+      void* user_data, void (*user_data_delete_fn)(void*),
       bool clear_existing_callbacks) = 0;
 
   /// Unregister a callback that was previously registered with
   /// `AddCompletionCallback`.
   virtual void RemoveCompletionCallback(
-      FutureHandle handle,
-      CompletionCallbackHandle callback_handle) = 0;
+      const FutureHandle& handle, CompletionCallbackHandle callback_handle) = 0;
 
 #if defined(FIREBASE_USE_STD_FUNCTION)
   /// Register a callback that will be called when this future's status is set
@@ -116,7 +115,8 @@ class FutureApiInterface {
   ///
   /// @return A handle that can be passed to `FutureBase::RemoveCompletion`.
   virtual CompletionCallbackHandle AddCompletionCallbackLambda(
-      FutureHandle handle, std::function<void(const FutureBase&)> callback,
+      const FutureHandle& handle,
+      std::function<void(const FutureBase&)> callback,
       bool clear_existing_callbacks) = 0;
 #endif  // defined(FIREBASE_USE_STD_FUNCTION)
 
@@ -143,37 +143,36 @@ class CompletionCallbackHandle {
  public:
   // Construct a null CompletionCallbackHandle.
   CompletionCallbackHandle()
-      : callback_(nullptr), user_data_(nullptr),
-      user_data_delete_fn_(nullptr) {}
+      : callback_(nullptr),
+        user_data_(nullptr),
+        user_data_delete_fn_(nullptr) {}
+
  private:
   friend class ::FIREBASE_NAMESPACE::FutureBase;
   friend class ::FIREBASE_NAMESPACE::ReferenceCountedFutureImpl;
-  CompletionCallbackHandle(
-      FutureBase::CompletionCallback callback,
-      void* user_data,
-      void (* user_data_delete_fn)(void *))
-      : callback_(callback), user_data_(user_data),
-      user_data_delete_fn_(user_data_delete_fn) {}
+  CompletionCallbackHandle(FutureBase::CompletionCallback callback,
+                           void* user_data, void (*user_data_delete_fn)(void*))
+      : callback_(callback),
+        user_data_(user_data),
+        user_data_delete_fn_(user_data_delete_fn) {}
 
   FutureBase::CompletionCallback callback_;
   void* user_data_;
-  void (* user_data_delete_fn_)(void *);
+  void (*user_data_delete_fn_)(void*);
 };
 
 }  // namespace detail
 
 template <class T>
-void
-Future<T>::OnCompletion(
-    TypedCompletionCallback callback, void* user_data) const {
-  FutureBase::OnCompletion(
-      reinterpret_cast<CompletionCallback>(callback), user_data);
+void Future<T>::OnCompletion(TypedCompletionCallback callback,
+                             void* user_data) const {
+  FutureBase::OnCompletion(reinterpret_cast<CompletionCallback>(callback),
+                           user_data);
 }
 
 #if defined(FIREBASE_USE_STD_FUNCTION)
 template <class ResultType>
-inline void
-Future<ResultType>::OnCompletion(
+inline void Future<ResultType>::OnCompletion(
     std::function<void(const Future<ResultType>&)> callback) const {
   FutureBase::OnCompletion(
       *reinterpret_cast<std::function<void(const FutureBase&)>*>(&callback));
@@ -182,17 +181,15 @@ Future<ResultType>::OnCompletion(
 
 #if defined(INTERNAL_EXPERIMENTAL)
 template <class T>
-FutureBase::CompletionCallbackHandle
-Future<T>::AddOnCompletion(
+FutureBase::CompletionCallbackHandle Future<T>::AddOnCompletion(
     TypedCompletionCallback callback, void* user_data) const {
   return FutureBase::AddOnCompletion(
       reinterpret_cast<CompletionCallback>(callback), user_data);
 }
 
 #if defined(FIREBASE_USE_STD_FUNCTION)
 template <class ResultType>
-inline FutureBase::CompletionCallbackHandle
-Future<ResultType>::AddOnCompletion(
+inline FutureBase::CompletionCallbackHandle Future<ResultType>::AddOnCompletion(
     std::function<void(const Future<ResultType>&)> callback) const {
   return FutureBase::AddOnCompletion(
       *reinterpret_cast<std::function<void(const FutureBase&)>*>(&callback));
@@ -204,7 +201,7 @@ Future<ResultType>::AddOnCompletion(
 inline FutureBase::FutureBase() : api_(NULL), handle_(0) {}  // NOLINT
 
 inline FutureBase::FutureBase(detail::FutureApiInterface* api,
-                              FutureHandle handle)
+                              const FutureHandle& handle)
     : api_(api), handle_(handle) {
   api_->ReferenceFuture(handle_);
   detail::RegisterForCleanup(api_, this);
@@ -280,17 +277,16 @@ inline void FutureBase::OnCompletion(CompletionCallback callback,
                                      void* user_data) const {
   if (api_ != NULL) {  // NOLINT
     api_->AddCompletionCallback(handle_, callback, user_data, nullptr,
-                                /*clear_existing_callbacks=*/ true);
+                                /*clear_existing_callbacks=*/true);
   }
 }
 
 #if defined(INTERNAL_EXPERIMENTAL)
-inline FutureBase::CompletionCallbackHandle
-FutureBase::AddOnCompletion(CompletionCallback callback,
-                            void* user_data) const {
+inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion(
+    CompletionCallback callback, void* user_data) const {
   if (api_ != NULL) {  // NOLINT
     return api_->AddCompletionCallback(handle_, callback, user_data, nullptr,
-                                       /*clear_existing_callbacks=*/ false);
+                                       /*clear_existing_callbacks=*/false);
   }
   return CompletionCallbackHandle();
 }
@@ -308,16 +304,17 @@ inline void FutureBase::OnCompletion(
     std::function<void(const FutureBase&)> callback) const {
   if (api_ != NULL) {  // NOLINT
     api_->AddCompletionCallbackLambda(handle_, callback,
-                                      /*clear_existing_callbacks=*/ true);
+                                      /*clear_existing_callbacks=*/true);
   }
 }
 
 #if defined(INTERNAL_EXPERIMENTAL)
 inline FutureBase::CompletionCallbackHandle FutureBase::AddOnCompletion(
     std::function<void(const FutureBase&)> callback) const {
   if (api_ != NULL) {  // NOLINT
-    return api_->AddCompletionCallbackLambda(handle_, callback,
-                                      /*clear_existing_callbacks=*/ false);
+    return api_->AddCompletionCallbackLambda(
+        handle_, callback,
+        /*clear_existing_callbacks=*/false);
   }
   return CompletionCallbackHandle();
 }