From 87175a4146267d403a774f138b85f8d3b532fa4b Mon Sep 17 00:00:00 2001 From: zxu123 Date: Wed, 7 Feb 2018 10:22:14 -0500 Subject: [PATCH] refactoring FirebaseCredentialsProvider to fix the issue w.r.t. auth global dispatch queue --- .../firestore/auth/credentials_provider.cc | 3 ++ .../firestore/auth/credentials_provider.h | 2 + .../firebase_credentials_provider_apple.h | 39 +++++++++++++------ .../firebase_credentials_provider_apple.mm | 14 +++++-- .../core/src/firebase/firestore/auth/user.cc | 2 +- .../firebase_credentials_provider_test.mm | 19 ++++++++- 6 files changed, 61 insertions(+), 18 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/auth/credentials_provider.cc b/Firestore/core/src/firebase/firestore/auth/credentials_provider.cc index 786f6d26d24..03019441341 100644 --- a/Firestore/core/src/firebase/firestore/auth/credentials_provider.cc +++ b/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 diff --git a/Firestore/core/src/firebase/firestore/auth/credentials_provider.h b/Firestore/core/src/firebase/firestore/auth/credentials_provider.h index c9419e24539..2a52c99fcf1 100644 --- a/Firestore/core/src/firebase/firestore/auth/credentials_provider.h +++ b/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. diff --git a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h index d082d6cef68..3fe1270c818 100644 --- a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h +++ b/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 diff --git a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm index eb617a7123d..980180b47ed 100644 --- a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm +++ b/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_, diff --git a/Firestore/core/src/firebase/firestore/auth/user.cc b/Firestore/core/src/firebase/firestore/auth/user.cc index 4793fed100d..f442d7b3d3d 100644 --- a/Firestore/core/src/firebase/firestore/auth/user.cc +++ b/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) { diff --git a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm index 8222811837d..85a9cd7acb0 100644 --- a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm +++ b/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,8 +65,11 @@ 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(); @@ -72,6 +77,16 @@ void SetUp() override { 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.