refactoring FirebaseCredentialsProvider to fix the issue w.r.t. auth global dispatch queue

Author: zxu123

Firestore/core/src/firebase/firestore/auth/credentials_provider.cc

@@ -23,6 +23,9 @@ namespace auth {
 CredentialsProvider::CredentialsProvider() : user_change_listener_(nullptr) {
 }
 
+CredentialsProvider::~CredentialsProvider() {
+}
+
 }  // namespace auth
 }  // namespace firestore
 }  // namespace firebase

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

@@ -45,6 +45,8 @@ class CredentialsProvider {
  public:
   CredentialsProvider();
 
+  virtual ~CredentialsProvider();
+
   /**
    * Requests token for the current user, optionally forcing a refreshed token
    * to be fetched.

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

@@ -49,7 +49,30 @@ namespace auth {
  * from the thread backing our internal worker queue and the callbacks from
  * FIRAuth will be executed on an arbitrary different thread.
  *
- * For non-Apple desktop build, this is right now just a stub.
+ * 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_;
+ * };
  */
 class FirebaseCredentialsProvider : public CredentialsProvider {
  public:
@@ -62,20 +85,12 @@ class FirebaseCredentialsProvider : public CredentialsProvider {
    */
   explicit FirebaseCredentialsProvider(FIRApp* app);
 
+  ~FirebaseCredentialsProvider() override;
+
   void GetToken(bool force_refresh, TokenListener completion) override;
 
   void SetUserChangeListener(UserChangeListener listener) override;
 
-  friend class FirebaseCredentialsProviderTest_GetToken_Test;
-  friend class FirebaseCredentialsProviderTest_SetListener_Test;
-  friend class DISABLED_FirebaseCredentialsProviderTest_GetToken_Test;
-  friend class DISABLED_FirebaseCredentialsProviderTest_SetListener_Test;
-
-  friend class FirebaseCredentialsProviderTest_GetToken_Test;
-  friend class FirebaseCredentialsProviderTest_SetListener_Test;
-  friend class DISABLED_FirebaseCredentialsProviderTest_GetToken_Test;
-  friend class DISABLED_FirebaseCredentialsProviderTest_SetListener_Test;
-
  private:
   const FIRApp* app_;
 
@@ -96,7 +111,7 @@ class FirebaseCredentialsProvider : public CredentialsProvider {
 
   // Make it static as as it is used in some of the callbacks. Otherwise, we saw
   // mutex lock failed: Invalid argument.
-  static std::mutex mutex_;
+  std::mutex mutex_;
 };
 
 }  // namespace auth

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

@@ -27,13 +27,12 @@
 namespace firestore {
 namespace auth {
 
-std::mutex FirebaseCredentialsProvider::mutex_;
-
 FirebaseCredentialsProvider::FirebaseCredentialsProvider(FIRApp* app)
     : app_(app),
       auth_listener_handle_(nil),
       current_user_(firebase::firestore::util::MakeStringView([app getUID])),
-      user_counter_(0) {
+      user_counter_(0),
+      mutex_() {
   auth_listener_handle_ = [[NSNotificationCenter defaultCenter]
       addObserverForName:FIRAuthStateDidChangeInternalNotification
                   object:nil
@@ -64,6 +63,15 @@ User new_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.
+    [[NSNotificationCenter defaultCenter] removeObserver:auth_listener_handle_];
+  }
+}
+
 void FirebaseCredentialsProvider::GetToken(bool force_refresh,
                                            TokenListener completion) {
   FIREBASE_ASSERT_MESSAGE(auth_listener_handle_,

Firestore/core/src/firebase/firestore/auth/user.cc

@@ -22,7 +22,7 @@ namespace firebase {
 namespace firestore {
 namespace auth {
 
-User::User() : is_authenticated_(false) {
+User::User() : uid_(), is_authenticated_(false) {
 }
 
 User::User(const absl::string_view uid) : uid_(uid), is_authenticated_(true) {

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

@@ -24,6 +24,8 @@
 
 #include "gtest/gtest.h"
 
+#define UNUSED(x) (void)(x)
+
 namespace firebase {
 namespace firestore {
 namespace auth {
@@ -63,15 +65,28 @@ void SetUp() override {
     return;
   }
 
-  FirebaseCredentialsProvider credentials_provider([FIRApp defaultApp]);
-  credentials_provider.GetToken(
+  // GetToken() registers callback and thus we do not allocate it in stack.
+  FirebaseCredentialsProvider* credentials_provider =
+      new FirebaseCredentialsProvider([FIRApp defaultApp]);
+
+  credentials_provider->GetToken(
       true, [](const Token& token, const absl::string_view error) {
         EXPECT_EQ("", token.token());
         const User& user = token.user();
         EXPECT_EQ("I'm a fake uid.", user.uid());
         EXPECT_TRUE(user.is_authenticated());
         EXPECT_EQ("", error) << error;
       });
+
+  // Destruct credentials_provider via the FIRAuthGlobalWorkQueue, which is
+  // serial.
+  credentials_provider->GetToken(
+      false, [credentials_provider](const Token& token,
+                                    const absl::string_view error) {
+        UNUSED(token);
+        UNUSED(error);
+        delete credentials_provider;
+      });
 }
 
 // Set kPlist above before enable.