Use a shared_ptr/weak_ptr handoff on FirebaseCredentialsProvider

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h

@@ -49,30 +49,7 @@ namespace auth {
  * from the thread backing our internal worker queue and the callbacks from
  * FIRAuth will be executed on an arbitrary different thread.
  *
- * Any instance that has GetToken() calls has to be destructed in
- * FIRAuthGlobalWorkQueue i.e through another call to GetToken. This prevents
- * the object being destructed before the callback. For example, use the
- * following pattern:
- *
- * class Bar {
- *   Bar(): provider_(new FirebaseCredentialsProvider([FIRApp defaultApp])) {}
- *
- *   ~Bar() {
- *     credentials_provider->GetToken(
- *         false, [provider_](const Token& token, const absl::string_view error)
- * { delete provider_;
- *     });
- *   }
- *
- *   Foo() {
- *      credentials_provider->GetToken(
- *          true, [](const Token& token, const absl::string_view error) {
- *              ... ...
- *      });
- *   }
- *
- *   FirebaseCredentialsProvider* provider_;
- * };
+ * For non-Apple desktop build, this is right now just a stub.
  */
 class FirebaseCredentialsProvider : public CredentialsProvider {
  public:
@@ -92,26 +69,40 @@ class FirebaseCredentialsProvider : public CredentialsProvider {
   void SetUserChangeListener(UserChangeListener listener) override;
 
  private:
-  const FIRApp* app_;
-
   /**
-   * Handle used to stop receiving auth changes once userChangeListener is
-   * removed.
+   * Most contents of the FirebaseCredentialProvider are kept in this
+   * Contents object and pointed to with a shared pointer. Callbacks
+   * registered with FirebaseAuth use weak pointers to the Contents to
+   * avoid races between notifications arriving and C++ object destruction.
    */
-  id<NSObject> auth_listener_handle_;
+  struct Contents {
+    Contents(FIRApp* app, const absl::string_view uid)
+        : app(app), current_user(uid), mutex() {
+    }
+
+    const FIRApp* app;
+
+    /**
+     * The current user as reported to us via our AuthStateDidChangeListener.
+     */
+    User current_user;
+
+    /**
+     * Counter used to detect if the user changed while a
+     * -getTokenForcingRefresh: request was outstanding.
+     */
+    int user_counter = 0;
 
-  /** The current user as reported to us via our AuthStateDidChangeListener. */
-  User current_user_;
+    std::mutex mutex;
+  };
 
   /**
-   * Counter used to detect if the user changed while a -getTokenForcingRefresh:
-   * request was outstanding.
+   * Handle used to stop receiving auth changes once userChangeListener is
+   * removed.
    */
-  int user_counter_;
+  id<NSObject> auth_listener_handle_;
 
-  // Make it static as as it is used in some of the callbacks. Otherwise, we saw
-  // mutex lock failed: Invalid argument.
-  std::mutex mutex_;
+  std::shared_ptr<Contents> contents_;
 };
 
 }  // namespace auth

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm

@@ -28,46 +28,49 @@
 namespace auth {
 
 FirebaseCredentialsProvider::FirebaseCredentialsProvider(FIRApp* app)
-    : app_(app),
-      auth_listener_handle_(nil),
-      current_user_(firebase::firestore::util::MakeStringView([app getUID])),
-      user_counter_(0),
-      mutex_() {
+    : contents_(
+          std::make_shared<Contents>(app, util::MakeStringView([app getUID]))) {
+  std::weak_ptr<Contents> weak_contents = contents_;
+
   auth_listener_handle_ = [[NSNotificationCenter defaultCenter]
       addObserverForName:FIRAuthStateDidChangeInternalNotification
                   object:nil
                    queue:nil
               usingBlock:^(NSNotification* notification) {
-                std::unique_lock<std::mutex> lock(mutex_);
+                std::shared_ptr<Contents> contents = weak_contents.lock();
+                if (!contents) {
+                  return;
+                }
+
+                std::unique_lock<std::mutex> lock(contents->mutex);
                 NSDictionary<NSString*, id>* user_info = notification.userInfo;
 
                 // ensure we're only notifiying for the current app.
                 FIRApp* notified_app =
                     user_info[FIRAuthStateDidChangeInternalNotificationAppKey];
-                if (![app_ isEqual:notified_app]) {
+                if (![contents->app isEqual:notified_app]) {
                   return;
                 }
 
                 NSString* user_id =
                     user_info[FIRAuthStateDidChangeInternalNotificationUIDKey];
-                User new_user(
-                    firebase::firestore::util::MakeStringView(user_id));
-                if (new_user != current_user_) {
-                  current_user_ = new_user;
-                  user_counter_++;
+                User new_user(util::MakeStringView(user_id));
+                if (new_user != contents->current_user) {
+                  contents->current_user = new_user;
+                  contents->user_counter++;
                   UserChangeListener listener = user_change_listener_;
                   if (listener) {
-                    listener(current_user_);
+                    listener(contents->current_user);
                   }
                 }
               }];
 }
 
 FirebaseCredentialsProvider::~FirebaseCredentialsProvider() {
   if (auth_listener_handle_) {
-    // For iOS 9.0 and later or macOS 10.11 and later, it is not required to
-    // unregister an observer in dealloc. Nothing is said for C++ destruction
-    // and thus we do it here just to be sure.
+    // Even though iOS 9 (and later) and macOS 10.11 (and later) keep a weak
+    // reference to the observer so we could avoid this removeObserver call, we
+    // still support iOS 8 which requires it.
     [[NSNotificationCenter defaultCenter] removeObserver:auth_listener_handle_];
   }
 }
@@ -79,37 +82,42 @@ User new_user(
 
   // Take note of the current value of the userCounter so that this method can
   // fail if there is a user change while the request is outstanding.
-  int initial_user_counter = user_counter_;
-
-  void (^get_token_callback)(NSString*, NSError*) =
-      ^(NSString* _Nullable token, NSError* _Nullable error) {
-        std::unique_lock<std::mutex> lock(mutex_);
-        if (initial_user_counter != user_counter_) {
-          // Cancel the request since the user changed while the request was
-          // outstanding so the response is likely for a previous user (which
-          // user, we can't be sure).
-          completion({"", User::Unauthenticated()},
-                     "getToken aborted due to user change.");
-        } else {
-          completion(
-              {firebase::firestore::util::MakeStringView(token), current_user_},
-              error == nil ? ""
-                           : firebase::firestore::util::MakeStringView(
-                                 error.localizedDescription));
-        }
-      };
-
-  [app_ getTokenForcingRefresh:force_refresh withCallback:get_token_callback];
+  int initial_user_counter = contents_->user_counter;
+
+  std::weak_ptr<Contents> weak_contents = contents_;
+  void (^get_token_callback)(NSString*, NSError*) = ^(
+      NSString* _Nullable token, NSError* _Nullable error) {
+    std::shared_ptr<Contents> contents = weak_contents.lock();
+    if (!contents) {
+      return;
+    }
+
+    std::unique_lock<std::mutex> lock(contents->mutex);
+    if (initial_user_counter != contents->user_counter) {
+      // Cancel the request since the user changed while the request was
+      // outstanding so the response is likely for a previous user (which
+      // user, we can't be sure).
+      completion({"", User::Unauthenticated()},
+                 "getToken aborted due to user change.");
+    } else {
+      completion(
+          {util::MakeStringView(token), contents->current_user},
+          error == nil ? "" : util::MakeStringView(error.localizedDescription));
+    }
+  };
+
+  [contents_->app getTokenForcingRefresh:force_refresh
+                            withCallback:get_token_callback];
 }
 
 void FirebaseCredentialsProvider::SetUserChangeListener(
     UserChangeListener listener) {
-  std::unique_lock<std::mutex> lock(mutex_);
+  std::unique_lock<std::mutex> lock(contents_->mutex);
   if (listener) {
     FIREBASE_ASSERT_MESSAGE(!user_change_listener_,
                             "set user_change_listener twice!");
     // Fire initial event.
-    listener(current_user_);
+    listener(contents_->current_user);
   } else {
     FIREBASE_ASSERT_MESSAGE(auth_listener_handle_,
                             "removed user_change_listener twice!");

Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm

@@ -24,8 +24,6 @@
 
 #include "gtest/gtest.h"
 
-#define UNUSED(x) (void)(x)
-
 namespace firebase {
 namespace firestore {
 namespace auth {
@@ -65,11 +63,8 @@ void SetUp() override {
     return;
   }
 
-  // GetToken() registers callback and thus we do not allocate it in stack.
-  FirebaseCredentialsProvider* credentials_provider =
-      new FirebaseCredentialsProvider([FIRApp defaultApp]);
-
-  credentials_provider->GetToken(
+  FirebaseCredentialsProvider credentials_provider([FIRApp defaultApp]);
+  credentials_provider.GetToken(
       /*force_refresh=*/true,
       [](const Token& token, const absl::string_view error) {
         EXPECT_EQ("", token.token());
@@ -78,17 +73,6 @@ void SetUp() override {
         EXPECT_TRUE(user.is_authenticated());
         EXPECT_EQ("", error) << error;
       });
-
-  // Destruct credentials_provider via the FIRAuthGlobalWorkQueue, which is
-  // serial.
-  credentials_provider->GetToken(
-      /*force_refresh=*/false,
-      [credentials_provider](const Token& token,
-                             const absl::string_view error) {
-        UNUSED(token);
-        UNUSED(error);
-        delete credentials_provider;
-      });
 }
 
 // Set kPlist above before enable.
@@ -103,6 +87,9 @@ void SetUp() override {
     EXPECT_TRUE(user.is_authenticated());
   });
 
+  // TODO(wilhuff): We should wait for the above expectations to actually happen
+  // before continuing.
+
   credentials_provider.SetUserChangeListener(nullptr);
 }