Skip to content

Commit 8fecdb9

Browse files
a-mauricea-maurice
authored
Clean up JNI task listeners per object (#1358)
* Clean up Installations JNI task listeners * Formatting * Update installations_android.cc * Update installations_android.cc * Add logic for other products * Update common_android.cc * Add string includes * Add a test * Add a unique id to the created string * Update util.cc * Update util.cc
1 parent 0859b17 commit 8fecdb9

22 files changed

+143
-70
lines changed

‎app/src/util.cc

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,5 +342,21 @@ std::vector<std::string> SplitString(const std::string& s,
342342
return split_parts;
343343
}
344344

345+
// Id used as part of the logic to make unique api identifiers.
346+
static int g_api_identifier_count = 0;
347+
348+
std::string CreateApiIdentifier(const char* api_id, void* object) {
349+
std::string created;
350+
const char* format = "%s0x%016llx_%d";
351+
unsigned long long object_ptr = // NOLINT
352+
static_cast<unsigned long long>( // NOLINT
353+
reinterpret_cast<intptr_t>(object));
354+
int extra_id = g_api_identifier_count++;
355+
int size = snprintf(nullptr, 0, format, api_id, object_ptr, extra_id) + 1;
356+
created.reserve(size);
357+
snprintf(&(created[0]), size, format, api_id, object_ptr, extra_id);
358+
return created;
359+
}
360+
345361
// NOLINTNEXTLINE - allow namespace overridden
346362
} // namespace firebase

‎app/src/util.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,11 @@ class StaticFutureData {
238238
// delimiter. Returns a vector of constituent parts.
239239
std::vector<std::string> SplitString(const std::string& s, char delimiter);
240240

241-
// NOLINTNEXTLINE - allow namespace overridden
241+
// Helper function to combine a string and other object into a unique
242+
// identifier. Primarily used to manage listening to JNI Tasks on Android.
243+
// Note that repeated calls are expected to return different values.
244+
std::string CreateApiIdentifier(const char* api_id, void* object);
245+
242246
} // namespace firebase
243247

244248
#endif // FIREBASE_APP_SRC_UTIL_H_

‎app/tests/util_test.cc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,5 +54,17 @@ TEST(UtilTest, SplitStringDelimetersOnly) {
5454
EXPECT_THAT(SplitString("///", '/'), IsEmpty());
5555
}
5656

57+
TEST(UtilTest, CreateApiIdentifierUnique) {
58+
int v1, v2;
59+
EXPECT_STRNE(CreateApiIdentifier("Test", &v1).c_str(),
60+
CreateApiIdentifier("Test", &v2).c_str());
61+
}
62+
63+
TEST(UtilTest, CreateApiIdentifierReallyUnique) {
64+
int v1;
65+
EXPECT_STRNE(CreateApiIdentifier("Test", &v1).c_str(),
66+
CreateApiIdentifier("Test", &v1).c_str());
67+
}
68+
5769
} // namespace
5870
} // namespace firebase

‎app_check/src/android/app_check_android.cc

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include "app/src/include/firebase/future.h"
2222
#include "app/src/include/firebase/internal/mutex.h"
2323
#include "app/src/reference_counted_future_impl.h"
24+
#include "app/src/util.h"
2425
#include "app/src/util_android.h"
2526
#include "app_check/app_check_resources.h"
2627
#include "app_check/src/android/common_android.h"
@@ -135,8 +136,6 @@ static const JNINativeMethod kNativeJniAppCheckListenerMethods[] = {
135136
reinterpret_cast<void*>(JniAppCheckListener_nativeOnAppCheckTokenChanged)},
136137
};
137138

138-
static const char* kApiIdentifier = "AppCheck";
139-
140139
static AppCheckProviderFactory* g_provider_factory = nullptr;
141140
static int g_initialized_count = 0;
142141

@@ -306,6 +305,9 @@ AppCheckInternal::AppCheckInternal(App* app) : app_(app) {
306305
g_initialized_count++;
307306
}
308307

308+
static const char* kApiIdentifier = "AppCheck";
309+
jni_task_id_ = CreateApiIdentifier(kApiIdentifier, this);
310+
309311
// Create the FirebaseAppCheck class in Java.
310312
jobject platform_app = app->GetPlatformApp();
311313
jobject j_app_check_local = env->CallStaticObjectMethod(
@@ -364,6 +366,7 @@ AppCheckInternal::~AppCheckInternal() {
364366
JNIEnv* env = app_->GetJNIEnv();
365367
app_ = nullptr;
366368
listeners_.clear();
369+
util::CancelCallbacks(env, jni_task_id_.c_str());
367370

368371
if (j_app_check_listener_ != nullptr) {
369372
env->CallVoidMethod(
@@ -393,7 +396,6 @@ AppCheckInternal::~AppCheckInternal() {
393396
FIREBASE_ASSERT(g_initialized_count);
394397
g_initialized_count--;
395398
if (g_initialized_count == 0) {
396-
util::CancelCallbacks(env, kApiIdentifier);
397399
ReleaseClasses(env);
398400
util::Terminate(env);
399401
}
@@ -435,7 +437,7 @@ Future<AppCheckToken> AppCheckInternal::GetAppCheckToken(bool force_refresh) {
435437
auto data_handle = new FutureDataHandle(future(), handle);
436438
util::RegisterCallbackOnTask(env, j_task, TokenResultCallback,
437439
reinterpret_cast<void*>(data_handle),
438-
kApiIdentifier);
440+
jni_task_id_.c_str());
439441
} else {
440442
AppCheckToken empty_token;
441443
future()->CompleteWithResult(handle, kAppCheckErrorUnknown, error.c_str(),

‎app_check/src/android/app_check_android.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#ifndef FIREBASE_APP_CHECK_SRC_ANDROID_APP_CHECK_ANDROID_H_
1616
#define FIREBASE_APP_CHECK_SRC_ANDROID_APP_CHECK_ANDROID_H_
1717

18+
#include <string>
1819
#include <vector>
1920

2021
#include "app/src/future_manager.h"
@@ -70,6 +71,9 @@ class AppCheckInternal {
7071
Mutex listeners_mutex_;
7172

7273
FutureManager future_manager_;
74+
75+
// String to be used when registering for JNI task callbacks.
76+
std::string jni_task_id_;
7377
};
7478

7579
} // namespace internal

‎app_check/src/android/common_android.cc

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include <string>
1818

1919
#include "app/src/app_common.h"
20+
#include "app/src/util.h"
2021
#include "app/src/util_android.h"
2122
#include "app_check/src/include/firebase/app_check.h"
2223

@@ -45,8 +46,6 @@ METHOD_LOOKUP_DEFINITION(app_check_token,
4546
"com/google/firebase/appcheck/AppCheckToken",
4647
APP_CHECK_TOKEN_METHODS)
4748

48-
static const char* kApiIdentifier = "AppCheckProvider";
49-
5049
bool CacheCommonAndroidMethodIds(JNIEnv* env, jobject activity) {
5150
// Cache the token and provider classes.
5251
if (!(app_check_token::CacheMethodIds(env, activity) &&
@@ -124,12 +123,15 @@ void TokenResultCallback(JNIEnv* env, jobject result,
124123

125124
AndroidAppCheckProvider::AndroidAppCheckProvider(jobject local_provider)
126125
: android_provider_(nullptr) {
126+
static const char* kApiIdentifier = "AppCheckProvider";
127+
jni_task_id_ = CreateApiIdentifier(kApiIdentifier, this);
127128
JNIEnv* env = GetJniEnv();
128129
android_provider_ = env->NewGlobalRef(local_provider);
129130
}
130131

131132
AndroidAppCheckProvider::~AndroidAppCheckProvider() {
132133
JNIEnv* env = GetJniEnv();
134+
util::CancelCallbacks(env, jni_task_id_.c_str());
133135
if (env != nullptr && android_provider_ != nullptr) {
134136
env->DeleteGlobalRef(android_provider_);
135137
}
@@ -150,7 +152,8 @@ void AndroidAppCheckProvider::GetToken(
150152
new TokenResultCallbackData(completion_callback);
151153
util::RegisterCallbackOnTask(
152154
env, j_task, TokenResultCallback,
153-
reinterpret_cast<void*>(completion_callback_data), kApiIdentifier);
155+
reinterpret_cast<void*>(completion_callback_data),
156+
jni_task_id_.c_str());
154157
} else {
155158
AppCheckToken empty_token;
156159
completion_callback(empty_token, kAppCheckErrorUnknown, error.c_str());

‎app_check/src/android/common_android.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ class AndroidAppCheckProvider : public AppCheckProvider {
6161

6262
private:
6363
jobject android_provider_;
64+
65+
// String to be used when registering for JNI task callbacks.
66+
std::string jni_task_id_;
6467
};
6568

6669
} // namespace internal

‎auth/src/auth.cc

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -111,15 +111,8 @@ Auth::Auth(App* app, void* auth_impl) : auth_data_(new AuthData) {
111111
auth_data_->user_impl = nullptr;
112112
InitPlatformAuth(auth_data_);
113113

114-
std::string* future_id = &auth_data_->future_api_id;
115114
static const char* kApiIdentifier = "Auth";
116-
future_id->reserve(strlen(kApiIdentifier) +
117-
16 /* hex characters in the pointer */ +
118-
1 /* null terminator */);
119-
snprintf(&((*future_id)[0]), future_id->capacity(), "%s0x%016llx",
120-
kApiIdentifier,
121-
static_cast<unsigned long long>( // NOLINT
122-
reinterpret_cast<intptr_t>(this)));
115+
auth_data_->future_api_id = CreateApiIdentifier(kApiIdentifier, this);
123116

124117
// Cleanup this object if app is destroyed.
125118
CleanupNotifier* notifier = CleanupNotifier::FindByOwner(app);

‎database/src/android/database_android.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "app/src/include/firebase/internal/common.h"
2525
#include "app/src/include/firebase/log.h"
2626
#include "app/src/reference_counted_future_impl.h"
27+
#include "app/src/util.h"
2728
#include "app/src/util_android.h"
2829
#include "database/database_resources.h"
2930
#include "database/src/android/data_snapshot_android.h"
@@ -172,6 +173,7 @@ DatabaseInternal::DatabaseInternal(App* app)
172173
app_ = nullptr;
173174
if (!Initialize(app)) return;
174175
app_ = app;
176+
jni_task_id_ = CreateApiIdentifier(kApiIdentifier, this);
175177

176178
JNIEnv* env = app_->GetJNIEnv();
177179
jobject platform_app = app->GetPlatformApp();
@@ -198,6 +200,7 @@ DatabaseInternal::DatabaseInternal(App* app, const char* url)
198200
app_ = nullptr;
199201
if (!Initialize(app)) return;
200202
app_ = app;
203+
jni_task_id_ = CreateApiIdentifier(kApiIdentifier, this);
201204

202205
JNIEnv* env = app_->GetJNIEnv();
203206
jobject url_string = env->NewStringUTF(url);
@@ -399,6 +402,7 @@ DatabaseInternal::~DatabaseInternal() {
399402
cleanup_.CleanupAll();
400403

401404
JNIEnv* env = app_->GetJNIEnv();
405+
util::CancelCallbacks(env, jni_task_id_.c_str());
402406
{
403407
// If there are any pending listeners, delete their pointers.
404408
MutexLock lock(listener_mutex_);

‎database/src/android/database_android.h

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
#include <map>
2121
#include <set>
22+
#include <string>
2223
#include <vector>
2324

2425
#include "app/src/cleanup_notifier.h"
@@ -42,10 +43,6 @@ namespace internal {
4243
// For constructing, copying or moving DatabaseReferences atomically.
4344
extern Mutex g_database_reference_constructor_mutex;
4445

45-
// Used for registering global callbacks. See
46-
// firebase::util::RegisterCallbackOnTask for context.
47-
extern const char kApiIdentifier[];
48-
4946
// This is the Android implementation of Database.
5047
class DatabaseInternal {
5148
public:
@@ -183,6 +180,10 @@ class DatabaseInternal {
183180

184181
Logger* logger() { return &logger_; }
185182

183+
// Used for registering global callbacks. See
184+
// firebase::util::RegisterCallbackOnTask for context.
185+
const char* jni_task_id() { return jni_task_id_.c_str(); }
186+
186187
private:
187188
static bool Initialize(App* app);
188189
static void ReleaseClasses(App* app);
@@ -222,6 +223,9 @@ class DatabaseInternal {
222223
std::string constructor_url_;
223224

224225
Logger logger_;
226+
227+
// String to be used when registering for JNI task callbacks.
228+
std::string jni_task_id_;
225229
};
226230

227231
} // namespace internal

‎database/src/android/database_reference_android.cc

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,7 @@ Future<void> DatabaseReferenceInternal::RemoveValue() {
329329
// FutureCallback will delete the newed FutureCallbackData.
330330
reinterpret_cast<void*>(
331331
new FutureCallbackData(handle, ref_future(), db_)),
332-
kApiIdentifier);
332+
db_->jni_task_id());
333333
util::CheckAndClearJniExceptions(env);
334334
env->DeleteLocalRef(task);
335335
return MakeFuture(ref_future(), handle);
@@ -358,7 +358,7 @@ Future<void> DatabaseReferenceInternal::SetValue(Variant value) {
358358
// FutureCallback will delete the newed FutureCallbackData.
359359
reinterpret_cast<void*>(
360360
new FutureCallbackData(handle, ref_future(), db_)),
361-
kApiIdentifier);
361+
db_->jni_task_id());
362362
env->DeleteLocalRef(task);
363363
if (value_obj) env->DeleteLocalRef(value_obj);
364364
}
@@ -391,7 +391,7 @@ Future<void> DatabaseReferenceInternal::SetPriority(Variant priority) {
391391
// FutureCallback will delete the newed FutureCallbackData.
392392
reinterpret_cast<void*>(
393393
new FutureCallbackData(handle, ref_future(), db_)),
394-
kApiIdentifier);
394+
db_->jni_task_id());
395395
util::CheckAndClearJniExceptions(env);
396396
env->DeleteLocalRef(task);
397397
if (priority_obj) env->DeleteLocalRef(priority_obj);
@@ -432,7 +432,7 @@ Future<void> DatabaseReferenceInternal::SetValueAndPriority(Variant value,
432432
// FutureCallback will delete the newed FutureCallbackData.
433433
reinterpret_cast<void*>(
434434
new FutureCallbackData(handle, ref_future(), db_)),
435-
kApiIdentifier);
435+
db_->jni_task_id());
436436
env->DeleteLocalRef(task);
437437
if (value_obj) env->DeleteLocalRef(value_obj);
438438
if (priority_obj) env->DeleteLocalRef(priority_obj);
@@ -464,7 +464,7 @@ Future<void> DatabaseReferenceInternal::UpdateChildren(Variant values) {
464464
// FutureCallback will delete the newed FutureCallbackData.
465465
reinterpret_cast<void*>(
466466
new FutureCallbackData(handle, ref_future(), db_)),
467-
kApiIdentifier);
467+
db_->jni_task_id());
468468
env->DeleteLocalRef(task);
469469
if (values_obj) env->DeleteLocalRef(values_obj);
470470
}

‎database/src/android/disconnection_android.cc

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ Future<void> DisconnectionHandlerInternal::Cancel() {
120120
env, task, FutureCallback,
121121
// FutureCallback will delete the newed FutureCallbackData.
122122
reinterpret_cast<void*>(new FutureCallbackData(handle, future(), db_)),
123-
kApiIdentifier);
123+
db_->jni_task_id());
124124
util::CheckAndClearJniExceptions(env);
125125
env->DeleteLocalRef(task);
126126
return MakeFuture(future(), handle);
@@ -141,7 +141,7 @@ Future<void> DisconnectionHandlerInternal::RemoveValue() {
141141
env, task, FutureCallback,
142142
// FutureCallback will delete the newed FutureCallbackData.
143143
reinterpret_cast<void*>(new FutureCallbackData(handle, future(), db_)),
144-
kApiIdentifier);
144+
db_->jni_task_id());
145145
util::CheckAndClearJniExceptions(env);
146146
return MakeFuture(future(), handle);
147147
}
@@ -166,7 +166,7 @@ Future<void> DisconnectionHandlerInternal::SetValue(Variant value) {
166166
env, task, FutureCallback,
167167
// FutureCallback will delete the newed FutureCallbackData.
168168
reinterpret_cast<void*>(new FutureCallbackData(handle, future(), db_)),
169-
kApiIdentifier);
169+
db_->jni_task_id());
170170
util::CheckAndClearJniExceptions(env);
171171
env->DeleteLocalRef(task);
172172
if (value_obj) env->DeleteLocalRef(value_obj);
@@ -211,7 +211,7 @@ Future<void> DisconnectionHandlerInternal::SetValueAndPriority(
211211
env, task, FutureCallback,
212212
// FutureCallback will delete the newed FutureCallbackData.
213213
reinterpret_cast<void*>(new FutureCallbackData(handle, future(), db_)),
214-
kApiIdentifier);
214+
db_->jni_task_id());
215215
env->DeleteLocalRef(task);
216216
if (value_obj) env->DeleteLocalRef(value_obj);
217217
}
@@ -240,7 +240,7 @@ Future<void> DisconnectionHandlerInternal::UpdateChildren(Variant values) {
240240
env, task, FutureCallback,
241241
// FutureCallback will delete the newed FutureCallbackData.
242242
reinterpret_cast<void*>(new FutureCallbackData(handle, future(), db_)),
243-
kApiIdentifier);
243+
db_->jni_task_id());
244244
env->DeleteLocalRef(task);
245245
if (values_obj) env->DeleteLocalRef(values_obj);
246246
}

‎functions/src/android/callable_reference_android.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ Future<HttpsCallableResult> HttpsCallableReferenceInternal::Call() {
188188
// FutureCallback will delete the newed FutureCallbackData.
189189
reinterpret_cast<void*>(new FutureCallbackData(
190190
handle, future(), functions_, kCallableReferenceFnCall)),
191-
kApiIdentifier);
191+
functions_->jni_task_id());
192192

193193
util::CheckAndClearJniExceptions(env);
194194
env->DeleteLocalRef(task);
@@ -219,7 +219,7 @@ Future<HttpsCallableResult> HttpsCallableReferenceInternal::Call(
219219
// FutureCallback will delete the newed FutureCallbackData.
220220
reinterpret_cast<void*>(new FutureCallbackData(
221221
handle, future(), functions_, kCallableReferenceFnCall)),
222-
kApiIdentifier);
222+
functions_->jni_task_id());
223223

224224
util::CheckAndClearJniExceptions(env);
225225
env->DeleteLocalRef(task);

0 commit comments

Comments
 (0)