Skip to content

Commit ae09bf0

Browse files
smilesa-maurice
authored andcommitted
Fixed race on shutdown in messaging on Android.
g_app was being access outside of the lock which could lead to a null pointer exception. In addition, the global mutex could be used after it has been destroyed so moved from a POSIX mutex to our internal Mutex class that is statically allocated instead. PiperOrigin-RevId: 246226419
1 parent b43aafa commit ae09bf0

File tree

1 file changed

+31
-19
lines changed

1 file changed

+31
-19
lines changed

messaging/src/android/cpp/messaging.cc

Lines changed: 31 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include "app/src/include/firebase/future.h"
3434
#include "app/src/include/firebase/version.h"
3535
#include "app/src/log.h"
36+
#include "app/src/mutex.h"
3637
#include "app/src/reference_counted_future_impl.h"
3738
#include "app/src/util.h"
3839
#include "app/src/util_android.h"
@@ -70,7 +71,7 @@ static const ::firebase::App* g_app = nullptr;
7071

7172
// Mutex used to arbitrate access to g_app between the thread calling this
7273
// module and MessageProcessingThread().
73-
static pthread_mutex_t g_app_mutex;
74+
static Mutex g_app_mutex;
7475

7576
// Global reference to the Firebase Cloud Messaging instance.
7677
// This is initialized in messaging::Initialize() and never released
@@ -297,10 +298,9 @@ static void ConsumeEvents() {
297298
// thread knows that it is time to quit.
298299
static bool TerminateRequested() {
299300
bool result;
300-
pthread_mutex_lock(&g_app_mutex);
301+
MutexLock lock(g_app_mutex);
301302
// If the app is removed, Terminate() has been called.
302303
result = !g_app;
303-
pthread_mutex_unlock(&g_app_mutex);
304304
return result;
305305
}
306306

@@ -336,9 +336,11 @@ Future<void> RequestPermissionLastResult() {
336336

337337
// Process queued messages & tokens.
338338
void ProcessMessages() {
339-
pthread_mutex_lock(&g_app_mutex);
340-
JNIEnv* env = g_app ? g_app->GetJNIEnv() : nullptr;
341-
pthread_mutex_unlock(&g_app_mutex);
339+
JNIEnv* env;
340+
{
341+
MutexLock lock(g_app_mutex);
342+
env = g_app ? g_app->GetJNIEnv() : nullptr;
343+
}
342344
if (HasListener() && env) {
343345
// Check to see if there was a message in the intent that started this
344346
// activity.
@@ -352,10 +354,12 @@ void ProcessMessages() {
352354
// changes to that file and when messages are written it relays them to the
353355
// OnMessage callback.
354356
static void* MessageProcessingThread(void*) {
355-
pthread_mutex_lock(&g_app_mutex);
356-
JavaVM* jvm = g_app ? g_app->java_vm() : nullptr;
357-
pthread_mutex_unlock(&g_app_mutex);
358-
if (jvm == nullptr) return nullptr;
357+
JavaVM* jvm;
358+
{
359+
MutexLock lock(g_app_mutex);
360+
jvm = g_app ? g_app->java_vm() : nullptr;
361+
if (jvm == nullptr) return nullptr;
362+
}
359363

360364
// Set up inotify.
361365
int file_descriptor = inotify_init();
@@ -408,7 +412,6 @@ static void TerminateMessageProcessingThread() {
408412
pthread_join(g_poll_thread, nullptr);
409413
pthread_mutex_destroy(&g_thread_wait_mutex);
410414
pthread_cond_destroy(&g_thread_wait_cond);
411-
pthread_mutex_destroy(&g_app_mutex);
412415
}
413416

414417
static bool StringStartsWith(const char* s, const char* prefix) {
@@ -494,11 +497,19 @@ static void FireIntentMessage(JNIEnv* env) {
494497
// when retrieving a notification via an Intent. http://b/32316101
495498
if (g_intent_message_fired || !HasListener()) return;
496499
g_intent_message_fired = true;
500+
jobject activity;
501+
{
502+
MutexLock lock(g_app_mutex);
503+
if (!g_app) return;
504+
activity = env->NewLocalRef(g_app->activity());
505+
assert(env->ExceptionCheck() == false);
506+
}
497507
// Intent intent = app.getIntent();
498508
jobject intent = env->CallObjectMethod(
499-
g_app->activity(),
509+
activity,
500510
util::activity::GetMethodId(util::activity::kGetIntent));
501511
assert(env->ExceptionCheck() == false);
512+
env->DeleteLocalRef(activity);
502513

503514
if (intent != nullptr) {
504515
// Bundle bundle = intent.getExtras();
@@ -578,10 +589,10 @@ InitResult Initialize(const ::firebase::App& app, Listener* listener,
578589
return kInitResultFailedMissingDependency;
579590
}
580591

581-
g_app_mutex = PTHREAD_MUTEX_INITIALIZER;
582-
pthread_mutex_lock(&g_app_mutex);
583-
g_app = &app;
584-
pthread_mutex_unlock(&g_app_mutex);
592+
{
593+
MutexLock lock(g_app_mutex);
594+
g_app = &app;
595+
}
585596
g_registration_token_mutex = new Mutex();
586597
g_file_locker_mutex = new Mutex();
587598
g_pending_subscriptions = new std::vector<PendingTopic>();
@@ -666,9 +677,10 @@ void Terminate() {
666677
internal::UnregisterTerminateOnDefaultAppDestroy();
667678
JNIEnv* env = g_app->GetJNIEnv();
668679
// Dereference the app.
669-
pthread_mutex_lock(&g_app_mutex);
670-
g_app = nullptr;
671-
pthread_mutex_unlock(&g_app_mutex);
680+
{
681+
MutexLock lock(g_app_mutex);
682+
g_app = nullptr;
683+
}
672684
TerminateMessageProcessingThread();
673685

674686
delete g_registration_token_mutex;

0 commit comments

Comments
 (0)