Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash in wakelock.c regarding releasing JNI references. #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix crash in wakelock.c regarding releasing JNI references. #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/wakelock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ void belle_sip_wake_lock_init(JNIEnv *env, jobject pm) {
jfieldID fieldID;

(*env)->GetJavaVM(env, &ctx.jvm);
ctx.powerManager = (*env)->NewGlobalRef(env, pm);
pthread_key_create(&ctx.jniEnvKey, jni_key_cleanup);

powerManagerClass = (*env)->FindClass(env, "android/os/PowerManager");
Expand All @@ -45,10 +44,12 @@ void belle_sip_wake_lock_init(JNIEnv *env, jobject pm) {
} else {
belle_sip_warning("bellesip_wake_lock_init(): the wakelock system has already been initialized");
}
if (ctx.powerManager == NULL) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't set ctx.jvm to NULL anymore, we need to check outside of the if(ctx.jvm == NULL) block, to assign ctx.powerManager

ctx.powerManager = (*env)->NewGlobalRef(env, pm);
}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need ctx.jvm later in jni_key_cleanup

void belle_sip_wake_lock_uninit(JNIEnv *env) {
ctx.jvm = NULL;
if(ctx.powerManager != NULL) {
(*env)->DeleteGlobalRef(env, ctx.powerManager);
ctx.powerManager = NULL;
Expand All @@ -61,7 +62,7 @@ void belle_sip_wake_lock_uninit(JNIEnv *env) {
* @param data Unused.
*/
static void jni_key_cleanup(void *data) {
JNIEnv *env = pthread_getspecific(ctx.jniEnvKey);
JNIEnv *env = (JNIEnv*) data;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://linux.die.net/man/3/pthread_key_create

In the documentation of pthread_key_create, it says that upon thread destruction, the value is set to NULL and then the destructor (this function) is called with the value as an argument.

An optional destructor function may be associated with each key value. At thread exit, if a key value has a non-NULL destructor pointer, and the thread has a non-NULL value associated with that key, the value of the key is set to NULL, and then the function pointed to is called with the previously associated value as its sole argument. The order of destructor calls is unspecified if more than one destructor exists for a thread when it exits.

When I read this, I don't understand how this ever worked. It looks like pthread_getspecific() is supposed to return NULL when called from the destructor (this function).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Carmen,
The documentation for the pthread_key_create on Android (https://android.googlesource.com/platform/external/pthreads/+/master/pthread_key_create.c) in a bit different:

This function creates a thread-specific data key visible to all threads.
All existing and new threads have a value NULL for key until set using pthread_setspecific.
When any thread with a non-NULL value for key terminates, 'destructor' is called with key's current value for that thread.

belle_sip_message("Thread end. Cleanup wake lock jni environment");
if(env != NULL) {
if(ctx.jvm != NULL) {
Expand Down