Skip to content

Commit 38f9f74

Browse files
var-consta-maurice
authored andcommitted
Unstub Unity desktop builds.
Summary of changes: 1. Remove `firestore_google3` target so that all builds use `firestore`. To achieve this, Firestore on Android now unconditionally uses `timestamp_portable.cc` and `geo_point_portable.cc`. While it's unfortunate not to use the canonical versions from GitHub, doing so significantly simplifies build configuration. 2. In `core/BUILD` under `third_party/`, merge most finer-grained library targets that previously were dependencies of `util` into `util`. This simplifies the build. 3. Fix deadlock while initializing credentials provider. The problem was that credentials provider was registering an Auth listener and then calling `Auth::current_user` for the first time. The first invocation of `Auth::current_user` may trigger Auth listeners and block until they are done; the credentials provider' Auth listener tries to call `Auth::current_user` again, leading to a deadlock. 4. Clear C#-to-C++ callbacks when the C# Firestore instance is destroyed. This prevents assertions from triggering. 5. Register the C# Firestore instance to listen to the event when Firebase App is disposed and clean up accordingly. PiperOrigin-RevId: 292995447
1 parent b79661c commit 38f9f74

File tree

6 files changed

+30
-5
lines changed

6 files changed

+30
-5
lines changed

firestore/src/android/firestore_android_test_main.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,12 @@
3535

3636
// non-google3 code should include "firebase/app.h" instead.
3737
#include "app/src/include/firebase/app.h"
38+
#include "app/src/assert.h"
3839
#include "firestore/src/include/firebase/firestore.h"
3940
#include "firestore/src/android/firestore_android.h"
4041
// non-google3 code should include "gtest/gtest.h" instead.
4142
#include "testing/base/public/gunit.h"
4243
// For GTEST_FLAG(filter).
43-
#include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
4444
#include "third_party/googletest/googletest/src/gtest-internal-inl.h"
4545

4646
#ifndef NATIVE_FUNCTION_NAME
@@ -87,7 +87,7 @@ App* GetApp(const char* name) {
8787
return App::Create(g_env, g_activity);
8888
} else {
8989
App* default_app = App::GetInstance();
90-
HARD_ASSERT(default_app,
90+
FIREBASE_ASSERT_MESSAGE(default_app,
9191
"Cannot create a named app before the default app");
9292
return App::Create(default_app->options(), name, g_env, g_activity);
9393
}

firestore/src/include/firebase/csharp/document_event_listener.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ void DocumentEventListener::OnEvent(const DocumentSnapshot& value,
2525
void DocumentEventListener::SetCallback(
2626
DocumentEventListenerCallback callback) {
2727
MutexLock lock(g_mutex);
28+
if (!callback) {
29+
g_document_snapshot_event_listener_callback = nullptr;
30+
return;
31+
}
32+
2833
if (g_document_snapshot_event_listener_callback) {
2934
FIREBASE_ASSERT(g_document_snapshot_event_listener_callback == callback);
3035
} else {

firestore/src/include/firebase/csharp/query_event_listener.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@ void QueryEventListener::OnEvent(const QuerySnapshot& value, Error error) {
2222
/* static */
2323
void QueryEventListener::SetCallback(QueryEventListenerCallback callback) {
2424
MutexLock lock(g_mutex);
25+
if (!callback) {
26+
g_query_snapshot_event_listener_callback = nullptr;
27+
return;
28+
}
29+
2530
if (g_query_snapshot_event_listener_callback) {
2631
FIREBASE_ASSERT(g_query_snapshot_event_listener_callback == callback);
2732
} else {

firestore/src/include/firebase/csharp/unity_transaction_function.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ Error UnityTransactionFunction::Apply(Transaction* transaction,
2626
void UnityTransactionFunction::SetCallback(
2727
UnityTransactionFunctionCallback callback) {
2828
MutexLock lock(*mutex_);
29+
if (!callback) {
30+
transaction_function_callback_ = nullptr;
31+
return;
32+
}
33+
2934
if (transaction_function_callback_) {
3035
FIREBASE_ASSERT(transaction_function_callback_ == callback);
3136
} else {

firestore/src/ios/credentials_provider_ios.cc

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,6 @@ void OnToken(const Future<std::string>& future_token, Auth* firebase_auth,
6363
FirebaseCppCredentialsProvider::FirebaseCppCredentialsProvider(
6464
Auth* firebase_auth)
6565
: contents_(std::make_shared<Contents>(NOT_NULL(firebase_auth))) {
66-
// Always listen to Auth events -- if there is no `change_listener_` set, the
67-
// events will simply be ignored.
68-
contents_->firebase_auth->AddAuthStateListener(this);
6966
}
7067

7168
void FirebaseCppCredentialsProvider::SetCredentialChangeListener(
@@ -76,12 +73,22 @@ void FirebaseCppCredentialsProvider::SetCredentialChangeListener(
7673
HARD_ASSERT_IOS(change_listener_,
7774
"Change listener removed without being set!");
7875
change_listener_ = {};
76+
// Note: not removing the Auth listener here because the Auth might already
77+
// be destroyed. Note that Auth listeners unregister themselves upon
78+
// destruction anyway.
7979
return;
8080
}
8181

8282
HARD_ASSERT_IOS(!change_listener_, "Set change listener twice!");
8383
change_listener_ = std::move(listener);
8484
change_listener_(GetCurrentUser(contents_->firebase_auth));
85+
86+
// Note: make sure to only register the Auth listener _after_ calling
87+
// `Auth::current_user` for the first time. The reason is that upon the only
88+
// first call only, `Auth::current_user` might block as it would
89+
// asynchronously notify Auth listeners; getting the Firestore listener
90+
// notified while `Auth::current_user` is pending can lead to a deadlock.
91+
contents_->firebase_auth->AddAuthStateListener(this);
8592
}
8693

8794
void FirebaseCppCredentialsProvider::GetToken(TokenListener listener) {

firestore/src/ios/credentials_provider_ios.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ class FirebaseCppCredentialsProvider
6767
// mutex is always already locked. The mutex is recursive to avoid one
6868
// potential case of deadlock (attaching a continuation to a `Future` which
6969
// may be invoked immediately or asynchronously).
70+
//
71+
// TODO(b/148688333): make sure not to hold the mutex while calling methods
72+
// on `firebase_auth`.
7073
std::recursive_mutex mutex;
7174

7275
::firebase::auth::Auth* firebase_auth = nullptr;

0 commit comments

Comments
 (0)