From d5ae8356b4b95b1af46dc8e6fff2a087b08323d3 Mon Sep 17 00:00:00 2001 From: Eric House Date: Sun, 3 Mar 2019 14:40:07 -0800 Subject: [PATCH] fixing refcounting and adding logging Seeing the occasional crash and trying to plug leaks that might be contributing. I think I fixed the Gameptr.finalize() assertions I've been seeing forever but the thread match assertion remains. Logs will help catch that. --- .../eehouse/android/xw4/BoardDelegate.java | 8 ++-- .../eehouse/android/xw4/jni/JNIThread.java | 1 + .../org/eehouse/android/xw4/jni/XwJNI.java | 21 +++++---- xwords4/android/jni/xwjni.c | 46 +++++++++++-------- xwords4/linux/gtkboard.c | 2 + 5 files changed, 44 insertions(+), 34 deletions(-) diff --git a/xwords4/android/app/src/main/java/org/eehouse/android/xw4/BoardDelegate.java b/xwords4/android/app/src/main/java/org/eehouse/android/xw4/BoardDelegate.java index 0ad45e233..9d8572e32 100644 --- a/xwords4/android/app/src/main/java/org/eehouse/android/xw4/BoardDelegate.java +++ b/xwords4/android/app/src/main/java/org/eehouse/android/xw4/BoardDelegate.java @@ -124,7 +124,7 @@ public class BoardDelegate extends DelegateBase private boolean m_haveInvited = false; private boolean m_showedReInvite; private boolean m_overNotShown; - private boolean m_dropOnDismiss; + private boolean m_dropRelayOnDismiss; private DBAlert m_inviteAlert; private boolean m_haveStartedShowing; @@ -1146,7 +1146,7 @@ public class BoardDelegate extends DelegateBase boolean handled = true; switch ( action ) { case ENABLE_RELAY_DO_OR: - m_dropOnDismiss = true; + m_dropRelayOnDismiss = true; break; case DROP_SMS_ACTION: dropConViaAndRestart(CommsConnType.COMMS_CONN_SMS); @@ -1169,7 +1169,7 @@ public class BoardDelegate extends DelegateBase boolean handled = true; switch ( action ) { case ENABLE_RELAY_DO_OR: - if ( m_dropOnDismiss ) { + if ( m_dropRelayOnDismiss ) { postDelayed( new Runnable() { @Override public void run() { @@ -2466,7 +2466,7 @@ public class BoardDelegate extends DelegateBase } if ( m_connTypes.contains( CommsConnType.COMMS_CONN_RELAY ) ) { if ( !RelayService.relayEnabled( m_activity ) ) { - m_dropOnDismiss = false; + m_dropRelayOnDismiss = false; String msg = getString( R.string.warn_relay_disabled ) + "\n\n" + getString( R.string.warn_relay_remove ); makeConfirmThenBuilder( msg, Action.ENABLE_RELAY_DO_OR ) diff --git a/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/JNIThread.java b/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/JNIThread.java index cbf5cad61..8a895950b 100644 --- a/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/JNIThread.java +++ b/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/JNIThread.java @@ -216,6 +216,7 @@ public class JNIThread extends Thread implements AutoCloseable { // Assert.assertNull( m_jniGamePtr ); // fired!! if ( null != m_jniGamePtr ) { Log.d( TAG, "configure(): m_jniGamePtr not null; that ok?" ); + m_jniGamePtr.release(); } synchronized ( this ) { diff --git a/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/XwJNI.java b/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/XwJNI.java index 725e30c85..19b6806e6 100644 --- a/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/XwJNI.java +++ b/xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/XwJNI.java @@ -25,6 +25,7 @@ import android.graphics.Rect; import java.util.Arrays; import org.eehouse.android.xw4.Assert; +import org.eehouse.android.xw4.BuildConfig; import org.eehouse.android.xw4.Log; import org.eehouse.android.xw4.NetLaunchInfo; import org.eehouse.android.xw4.Utils; @@ -78,6 +79,8 @@ public class XwJNI { game_dispose( this ); // will crash if haveEnv fails m_ptr = 0; } + } else { + Assert.assertTrue( m_refCount > 0 || !BuildConfig.DEBUG ); } } @@ -90,11 +93,10 @@ public class XwJNI { // @Override public void finalize() throws java.lang.Throwable { - if ( 0 != m_ptr ) { - Log.e( TAG, "ptr not cleared; creator: %s", mStack ); + if ( BuildConfig.DEBUG && (0 != m_refCount || 0 != m_ptr) ) { + Log.e( TAG, "finalize(): called prematurely: refCount: %d" + + "; ptr: %d; creator: %s", m_refCount, m_ptr, mStack ); } - Assert.assertTrue( 0 == m_refCount ); - Assert.assertTrue( 0 == m_ptr ); super.finalize(); } } @@ -199,12 +201,11 @@ public class XwJNI { CommonPrefs cp, TransportProcs procs ) { - GamePtr gamePtr = initGameJNI( rowid ); - if ( game_makeFromStream( gamePtr, stream, gi, dictNames, dictBytes, - dictPaths, langName, util, draw, - cp, procs ) ) { - gamePtr.retain(); - } else { + GamePtr gamePtr = initGameJNI( rowid ).retain(); + if ( ! game_makeFromStream( gamePtr, stream, gi, dictNames, dictBytes, + dictPaths, langName, util, draw, + cp, procs ) ) { + gamePtr.release(); gamePtr = null; } diff --git a/xwords4/android/jni/xwjni.c b/xwords4/android/jni/xwjni.c index 5f087c92a..9d4e9808e 100644 --- a/xwords4/android/jni/xwjni.c +++ b/xwords4/android/jni/xwjni.c @@ -46,9 +46,15 @@ #include "jniutlswrapper.h" #include "paths.h" +#define LOG_MAPPING +// #define LOG_MAPPING_ALL + typedef struct _EnvThreadEntry { JNIEnv* env; pthread_t owner; +#ifdef LOG_MAPPING + const char* ownerFunc; +#endif } EnvThreadEntry; struct _EnvThreadInfo { @@ -89,8 +95,6 @@ releaseMPool( JNIGlobalState* globalState ) # define releaseMPool(s) #endif -#define LOG_MAPPING -// #define LOG_MAPPING_ALL #define GAMEPTR_IS_OBJECT #ifdef GAMEPTR_IS_OBJECT @@ -119,8 +123,10 @@ countUsed(const EnvThreadInfo* ti) # endif #endif +#define MAP_THREAD( ti, env ) map_thread_prv( (ti), (env), __func__ ) + static void -map_thread( EnvThreadInfo* ti, JNIEnv* env ) +map_thread_prv( EnvThreadInfo* ti, JNIEnv* env, const char* caller ) { pthread_t self = pthread_self(); @@ -170,6 +176,7 @@ map_thread( EnvThreadInfo* ti, JNIEnv* env ) firstEmpty->owner = self; firstEmpty->env = env; #ifdef LOG_MAPPING + firstEmpty->ownerFunc = caller; XP_LOGF( "%s: entry %d: mapped env %p to thread %x", __func__, firstEmpty - ti->entries, env, (int)self ); XP_LOGF( "%s: num entries USED now %d", __func__, countUsed(ti) ); @@ -177,19 +184,19 @@ map_thread( EnvThreadInfo* ti, JNIEnv* env ) } pthread_mutex_unlock( &ti->mtxThreads ); -} /* map_thread */ +} /* map_thread_prv */ static void map_init( MPFORMAL EnvThreadInfo* ti, JNIEnv* env ) { pthread_mutex_init( &ti->mtxThreads, NULL ); MPASSIGN( ti->mpool, mpool ); - map_thread( ti, env ); + MAP_THREAD( ti, env ); } -#define MAP_REMOVE( ti, env ) map_remove((ti), (env), __func__) +#define MAP_REMOVE( ti, env ) map_remove_prv((ti), (env), __func__) static void -map_remove( EnvThreadInfo* ti, JNIEnv* env, const char* func ) +map_remove_prv( EnvThreadInfo* ti, JNIEnv* env, const char* func ) { XP_Bool found = false; @@ -197,12 +204,13 @@ map_remove( EnvThreadInfo* ti, JNIEnv* env, const char* func ) for ( int ii = 0; !found && ii < ti->nEntries; ++ii ) { found = env == ti->entries[ii].env; if ( found ) { - XP_ASSERT( pthread_self() == ti->entries[ii].owner ); #ifdef LOG_MAPPING - XP_LOGF( "%s: UNMAPPED env %p to thread %x (from %s)", __func__, + XP_LOGF( "%s: UNMAPPED env %p to thread %x (from %s; mapped by %s)", __func__, ti->entries[ii].env, - (int)ti->entries[ii].owner, func ); + (int)ti->entries[ii].owner, func, + ti->entries[ii].ownerFunc ); #endif + XP_ASSERT( pthread_self() == ti->entries[ii].owner ); ti->entries[ii].env = NULL; ti->entries[ii].owner = 0; #ifdef LOG_MAPPING @@ -724,7 +732,7 @@ Java_org_eehouse_android_xw4_jni_XwJNI_dict_1getInfo { jboolean result = false; JNIGlobalState* globalState = (JNIGlobalState*)jniGlobalPtr; - map_thread( &globalState->ti, env ); + MAP_THREAD( &globalState->ti, env ); #ifdef MEM_DEBUG MemPoolCtx* mpool = getMPool( globalState ); @@ -828,7 +836,7 @@ Java_org_eehouse_android_xw4_jni_XwJNI_smsproto_1prepOutbound { jobjectArray result = NULL; JNIGlobalState* globalState = (JNIGlobalState*)jniGlobalPtr; - map_thread( &globalState->ti, env ); + MAP_THREAD( &globalState->ti, env ); SMS_CMD cmd = jEnumToInt( env, jCmd ); jbyte* data = NULL; @@ -867,7 +875,7 @@ Java_org_eehouse_android_xw4_jni_XwJNI_smsproto_1prepInbound if ( !!jData ) { JNIGlobalState* globalState = (JNIGlobalState*)jniGlobalPtr; - map_thread( &globalState->ti, env ); + MAP_THREAD( &globalState->ti, env ); int len = (*env)->GetArrayLength( env, jData ); jbyte* data = (*env)->GetByteArrayElements( env, jData, NULL ); @@ -906,7 +914,7 @@ struct _JNIState { MPSLOT; \ MPASSIGN( mpool, state->mpool); \ XP_ASSERT( !!state->globalJNI ); \ - map_thread( &state->globalJNI->ti, env ); \ + MAP_THREAD( &state->globalJNI->ti, env ); \ #define XWJNI_START_GLOBALS() \ XWJNI_START() \ @@ -924,7 +932,7 @@ Java_org_eehouse_android_xw4_jni_XwJNI_initGameJNI #endif JNIState* state = (JNIState*)XP_CALLOC( mpool, sizeof(*state) ); state->globalJNI = (JNIGlobalState*)jniGlobalPtr; - map_thread( &state->globalJNI->ti, env ); + MAP_THREAD( &state->globalJNI->ti, env ); AndGameGlobals* globals = &state->globals; globals->dutil = state->globalJNI->dutil; globals->state = (JNIState*)state; @@ -1052,16 +1060,14 @@ Java_org_eehouse_android_xw4_jni_XwJNI_game_1makeFromStream dict_unref( dict ); /* game owns it now */ dict_unref_all( &dicts ); + /* If game_makeFromStream() fails, the platform-side caller still needs to + call game_dispose. That requirement's better than having cleanup code + in two places. */ if ( result ) { XP_ASSERT( 0 != globals->gi->gameID ); if ( !!jgi ) { setJGI( env, jgi, globals->gi ); } - } else { - destroyDraw( &globals->dctx ); - destroyXportProcs( &globals->xportProcs ); - destroyUtil( &globals->util ); - destroyGI( MPPARM(mpool) &globals->gi ); } XWJNI_END(); diff --git a/xwords4/linux/gtkboard.c b/xwords4/linux/gtkboard.c index 82a37944d..adc6cc8c0 100644 --- a/xwords4/linux/gtkboard.c +++ b/xwords4/linux/gtkboard.c @@ -2892,6 +2892,8 @@ loadGameNoDraw( GtkGameGlobals* globals, LaunchParams* params, XP_FALSE ); } #endif + } else { + game_dispose( &cGlobals->game ); } } stream_destroy( stream );