From 09d9db838c1695df45a2a7e8dbb773e58da4301a Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Thu, 8 Feb 2018 11:11:26 -0800 Subject: [PATCH 1/2] Revert "refactoring FirebaseCredentialsProvider to fix the issue w.r.t. auth global dispatch queue" This reverts commit 87175a4146267d403a774f138b85f8d3b532fa4b. --- .../firebase_credentials_provider_apple.h | 25 +------------------ .../firebase_credentials_provider_apple.mm | 8 +++--- .../firebase_credentials_provider_test.mm | 20 ++------------- 3 files changed, 7 insertions(+), 46 deletions(-) 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 3fe1270c818..a6bc22c1e0c 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,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: 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 980180b47ed..9c383102ba6 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 @@ -64,12 +64,12 @@ 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 +// if (auth_listener_handle_) { + // 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_]; - } +// [[NSNotificationCenter defaultCenter] removeObserver:auth_listener_handle_]; +// } } void FirebaseCredentialsProvider::GetToken(bool force_refresh, 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 c9ed030ac29..95dd886888c 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,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. From 1a497b520f7daf55a43def187ba5809b50bd2e8b Mon Sep 17 00:00:00 2001 From: Marek Gilbert Date: Thu, 8 Feb 2018 11:28:12 -0800 Subject: [PATCH 2/2] Use a shared_ptr/weak_ptr handoff on FirebaseCredentialsProvider This avoids any problems with callsbacks retaining pointers to objects destroyed by a C++ destructor --- .../firebase_credentials_provider_apple.h | 40 +++++--- .../firebase_credentials_provider_apple.mm | 92 ++++++++++--------- .../firebase_credentials_provider_test.mm | 3 + 3 files changed, 80 insertions(+), 55 deletions(-) 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 a6bc22c1e0c..65c4c655ae3 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 @@ -69,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 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_; + /** + * 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; + + 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 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_; }; } // 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 9c383102ba6..f463958a6f2 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 @@ -28,48 +28,51 @@ 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(app, util::MakeStringView([app getUID]))) { + std::weak_ptr weak_contents = contents_; + auth_listener_handle_ = [[NSNotificationCenter defaultCenter] addObserverForName:FIRAuthStateDidChangeInternalNotification object:nil queue:nil usingBlock:^(NSNotification* notification) { - std::unique_lock lock(mutex_); + std::shared_ptr contents = weak_contents.lock(); + if (!contents) { + return; + } + + std::unique_lock lock(contents->mutex); NSDictionary* 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_) { - // 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_]; -// } + if (auth_listener_handle_) { + // 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_]; + } } void FirebaseCredentialsProvider::GetToken(bool force_refresh, @@ -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 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 weak_contents = contents_; + void (^get_token_callback)(NSString*, NSError*) = ^( + NSString* _Nullable token, NSError* _Nullable error) { + std::shared_ptr contents = weak_contents.lock(); + if (!contents) { + return; + } + + std::unique_lock 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 lock(mutex_); + std::unique_lock 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!"); 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 95dd886888c..8d2b361b965 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 @@ -87,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); }