Make JNIThread autoclosable and use everywhere

JNIThread is somehow sticking around sometimes and holding the lock for
a game so that that game can never be opened again. On the theory that
there's some place retain() was called but not release(), use the thing
in try-with-resources wherever possible. Which is pretty much
everywhere.

Also added age to the lock-holder report being uploaded.

xwords4/android/app/src/main/java/org/eehouse/android/xw4/BoardDelegate.java

@@ -89,7 +89,6 @@ public class BoardDelegate extends DelegateBase
     private Activity m_activity;
     private BoardView m_view;
     private GamePtr m_jniGamePtr;
-    private GameLock m_gameLock;
     private CurGameInfo m_gi;
     private GameSummary m_summary;
     private boolean m_relayMissing;
@@ -539,16 +538,16 @@ public class BoardDelegate extends DelegateBase
         m_haveInvited = args.getBoolean( GameUtils.INVITED, false );
         m_overNotShown = true;
 
-        GameLock.callWithLock( m_rowid, 100L, new Handler(),
+        GameLock.getLockThen( m_rowid, 100L, new Handler(),
-                               new GameLock.LockProc() {
+                              new GameLock.GotLockProc() {
                 @Override
                 public void gotLock( GameLock lock ) {
                     if ( null == lock ) {
                         finish();
                         if ( BuildConfig.REPORT_LOCKS && ++s_noLockCount == 3 ) {
                             String msg = "BoardDelegate unable to get lock; holder stack: "
-                                + GameLock.getHolderStack( m_rowid );
+                                + GameLock.getHolderDump( m_rowid );
-                            CrashTrack.logAndSend( msg );
+                            CrashTrack.logAndSend( TAG, msg );
                         }
                     } else {
                         s_noLockCount = 0;
@@ -1501,7 +1500,7 @@ public class BoardDelegate extends DelegateBase
 
     private void deleteAndClose()
     {
-        GameUtils.deleteGame( m_activity, m_gameLock, false );
+        GameUtils.deleteGame( m_activity, m_jniThread.getLock(), false );
         waitCloseGame( false );
         finish();
     }
@@ -2158,7 +2157,6 @@ public class BoardDelegate extends DelegateBase
             m_jniThread = m_jniThreadRef.retain();
             m_gi = m_jniThread.getGI();
             m_summary = m_jniThread.getSummary();
-            m_gameLock = m_jniThread.getLock();
 
             m_view.startHandling( m_activity, m_jniThread, m_connTypes );
 
@@ -2427,8 +2425,6 @@ public class BoardDelegate extends DelegateBase
             m_jniThread = null;
 
             m_view.stopHandling();
-
-            m_gameLock = null;
         }
     }
 
@@ -2440,7 +2436,6 @@ public class BoardDelegate extends DelegateBase
             // m_jniGamePtr = null;
 
             // m_gameLock.unlock(); // likely the problem
-            m_gameLock = null;
         }
     }
 
@@ -2793,7 +2788,8 @@ public class BoardDelegate extends DelegateBase
         GamePtr gamePtr = null;
         GameSummary summary = null;
         CurGameInfo gi = null;
-        JNIThread thread = JNIThread.getRetained( rowID );
+
+        try ( JNIThread thread = JNIThread.getRetained( rowID ) ) {
             if ( null != thread ) {
                 gamePtr = thread.getGamePtr().retain();
                 summary = thread.getSummary();
@@ -2818,9 +2814,6 @@ public class BoardDelegate extends DelegateBase
             } else {
                 Log.w( TAG, "setupRematchFor(): unable to lock game" );
             }
-
-        if ( null != thread ) {
-            thread.release();
         }
     }
 

xwords4/android/app/src/main/java/org/eehouse/android/xw4/ChatDelegate.java

@@ -59,7 +59,6 @@ public class ChatDelegate extends DelegateBase {
     private EditText m_edit;
     private TableLayout m_layout;
     private ScrollView m_scroll;
-    private JNIThread m_jniThreadRef;
 
     public ChatDelegate( Delegator delegator, Bundle savedInstanceState )
     {
@@ -130,11 +129,7 @@ public class ChatDelegate extends DelegateBase {
     protected void onResume()
     {
         super.onResume();
-        m_jniThreadRef = JNIThread.getRetained( m_rowid );
+
-        if ( null == m_jniThreadRef ) {
-            Log.w( TAG, "onResume(): m_jniThreadRef null; exiting" );
-            finish();
-        } else {
         s_visibleThis = this;
         int[] startAndEnd = new int[2];
         String curMsg = DBUtils.getCurChat( m_activity, m_rowid,
@@ -144,14 +139,10 @@ public class ChatDelegate extends DelegateBase {
             m_edit.setSelection( startAndEnd[0], startAndEnd[1] );
         }
     }
-    }
 
     @Override
     protected void onPause()
     {
-        if ( null != m_jniThreadRef ) {
-            m_jniThreadRef.release();
-        }
         s_visibleThis = null;
 
         String curText = m_edit.getText().toString();
@@ -205,7 +196,11 @@ public class ChatDelegate extends DelegateBase {
         addRow( text, m_curPlayer, (int)ts );
         m_edit.setText( null );
 
-        m_jniThreadRef.sendChat( text );
+        try ( JNIThread thread = JNIThread.getRetained( m_rowid ) ) {
+            if ( null != thread ) {
+                thread.sendChat( text );
+            }
+        }
     }
 
     @Override

xwords4/android/app/src/main/java/org/eehouse/android/xw4/DbgUtils.java

@@ -66,7 +66,7 @@ public class DbgUtils {
         }
         Log.w( tag, format, args );
         Log.w( tag, "stack for lock owner for %d", rowid );
-        Log.w( tag, GameLock.getHolderStack( rowid ) );
+        Log.w( tag, GameLock.getHolderDump( rowid ) );
     }
 
     public static void assertOnUIThread()

xwords4/android/app/src/main/java/org/eehouse/android/xw4/GameConfigDelegate.java

@@ -536,17 +536,16 @@ public class GameConfigDelegate extends DelegateBase
         if ( null == m_giOrig ) {
             m_giOrig = new CurGameInfo( m_activity );
 
-            GameLock gameLock = null;
             XwJNI.GamePtr gamePtr = null;
-            if ( null == m_jniThread ) {
+            if ( null != m_jniThread ) {
-                gameLock = GameLock.tryLockRO( m_rowid );
+                gamePtr = m_jniThread.getGamePtr().retain();
-                if ( null != gameLock ) {
-                    gamePtr = GameUtils.loadMakeGame( m_activity, m_giOrig,
-                                                      gameLock );
-                }
             } else {
-                gameLock = m_jniThread.getLock();
+                try ( GameLock lock = GameLock.tryLockRO( m_rowid ) ) {
-                gamePtr = m_jniThread.getGamePtr();
+                    if ( null != lock ) {
+                        gamePtr = GameUtils.loadMakeGame( m_activity, m_giOrig,
+                                                          lock );
+                    }
+                }
             }
 
             if ( null == gamePtr ) {
@@ -588,10 +587,7 @@ public class GameConfigDelegate extends DelegateBase
                 buildDisabledsMap( gamePtr );
                 setDisableds();
 
-                if ( null == m_jniThread ) {
                 gamePtr.release();
-                    gameLock.unlock();
-                }
 
                 m_car = new CommsAddrRec( m_carOrig );
 
@@ -1230,18 +1226,33 @@ public class GameConfigDelegate extends DelegateBase
         m_car.conTypes = m_conTypes;
     } // saveChanges
 
+    private void applyChanges( GameLock lock, boolean forceNew )
+    {
+        GameUtils.applyChanges( m_activity, m_gi, m_car, m_disabMap,
+                                lock, forceNew );
+        DBUtils.saveThumbnail( m_activity, lock, null ); // clear it
+    }
+
     private void applyChanges( boolean forceNew )
     {
         if ( !isFinishing() ) {
-            GameLock gameLock = m_jniThread == null
+            if ( null != m_jniThread ) {
-                ? GameLock.tryLock( m_rowid ) : m_jniThread.getLock();
+                applyChanges( m_jniThread.getLock(), forceNew );
-            GameUtils.applyChanges( m_activity, m_gi, m_car, m_disabMap,
+            } else {
-                                    gameLock, forceNew );
+                try ( GameLock lock = GameLock.tryLock( m_rowid ) ) {
-            DBUtils.saveThumbnail( m_activity, gameLock, null ); // clear it
+                    applyChanges( lock, forceNew );
-            if ( null == m_jniThread ) {
-                gameLock.unlock();
                 }
             }
+        // }
+            // GameLock gameLock = m_jniThread == null
+            //     ? GameLock.tryLock( m_rowid ) : m_jniThread.getLock();
+            // GameUtils.applyChanges( m_activity, m_gi, m_car, m_disabMap,
+            //                         gameLock, forceNew );
+            // DBUtils.saveThumbnail( m_activity, gameLock, null ); // clear it
+            // if ( null == m_jniThread ) {
+            //     gameLock.unlock();
+            // }
+        }
     }
 
     private void launchGame( boolean forceNew )

xwords4/android/app/src/main/java/org/eehouse/android/xw4/GameLock.java

@@ -60,18 +60,24 @@ public class GameLock implements AutoCloseable, Serializable {
     private static class Owner {
         Thread mThread;
         String mTrace;
+        long mStamp;
 
         Owner()
         {
             mThread = Thread.currentThread();
             mTrace = android.util.Log.getStackTraceString(new Exception());
+            setStamp();
         }
 
         @Override
         public String toString()
         {
-            return String.format( "Owner: {%s/%s}", mThread, mTrace );
+            long ageMS = System.currentTimeMillis() - mStamp;
+            return String.format( "Owner: {age: %dms; thread: {%s}; stack: {%s}}",
+                                  ageMS, mThread, mTrace );
         }
+
+        void setStamp() { mStamp = System.currentTimeMillis(); }
     }
 
     private static class GameLockState {
@@ -233,6 +239,7 @@ public class GameLock implements AutoCloseable, Serializable {
         {
             synchronized ( mOwners ) {
                 mOwners.pop();
+                owner.setStamp();
                 mOwners.push( owner );
             }
         }
@@ -296,7 +303,7 @@ public class GameLock implements AutoCloseable, Serializable {
         return getFor( rowid ).lock( maxMillis );
     }
 
-    public static GameLock lockRO( long rowid, long maxMillis )
+    public static GameLock lockRO( long rowid, long maxMillis ) throws GameLockedException
     {
         return getFor( rowid ).lockRO( maxMillis );
     }
@@ -317,17 +324,17 @@ public class GameLock implements AutoCloseable, Serializable {
         return m_rowid;
     }
 
-    public interface LockProc {
+    public interface GotLockProc {
         public void gotLock( GameLock lock );
     }
 
     // Meant to be called from UI thread, returning immediately, but when it
     // gets the lock, or time runs out, calls the callback (using the Handler
     // passed in) with the lock or null.
-    public static void callWithLock( final long rowid,
+    public static void getLockThen( final long rowid,
                                     final long maxMillis,
                                     final Handler handler,
-                                     final LockProc proc )
+                                    final GotLockProc proc )
     {
         // capture caller thread and stack
         final Owner owner = new Owner();
@@ -360,11 +367,11 @@ public class GameLock implements AutoCloseable, Serializable {
             } ).start();
     }
 
-    public static String getHolderStack( long rowid )
+    public static String getHolderDump( long rowid )
     {
         GameLockState state = getFor( rowid );
         Owner owner = state.mOwners.peek();
-        return owner.mTrace;
+        return owner.toString();
     }
 
     // used only for asserts

xwords4/android/app/src/main/java/org/eehouse/android/xw4/GameUtils.java

@@ -225,24 +225,16 @@ public class GameUtils {
                                           long maxMillis )
     {
         GameSummary result = null;
-        JNIThread thread = JNIThread.getRetained( rowid );
+        try ( JNIThread thread = JNIThread.getRetained( rowid ) ) {
-        GameLock lock = null;
             if ( null != thread ) {
-            lock = thread.getLock();
+                result = DBUtils.getSummary( context, thread.getLock() );
             } else {
-            try {
+                try ( GameLock lock = GameLock.lockRO( rowid, maxMillis ) ) {
-                lock = GameLock.lockRO( rowid, maxMillis );
-            } catch ( GameLock.GameLockedException gle ) {
-                Log.ex( TAG, gle );
-            }
-        }
-
                     if ( null != lock ) {
                         result = DBUtils.getSummary( context, lock );
-            if ( null == thread ) {
+                    }
-                lock.unlock();
+                } catch ( GameLock.GameLockedException gle ) {
-            } else {
+                }
-                thread.release();
             }
         }
         return result;
@@ -260,35 +252,41 @@ public class GameUtils {
 
     public static long dupeGame( Context context, long rowidIn, long groupID )
     {
-        long rowid = DBUtils.ROWID_NOTFOUND;
+        long result = DBUtils.ROWID_NOTFOUND;
-        GameLock lockSrc = null;
 
-        JNIThread thread = JNIThread.getRetained( rowidIn );
+        try ( JNIThread thread = JNIThread.getRetained( rowidIn ) ) {
             if ( null != thread ) {
-            lockSrc = thread.getLock();
+                result = dupeGame( context, thread.getLock(), groupID );
             } else {
-            lockSrc = GameLock.lockRO( rowidIn, 300 );
+                try ( GameLock lockSrc = GameLock.lockRO( rowidIn, 300 ) ) {
-        }
-
                     if ( null != lockSrc ) {
-            boolean juggle = CommonPrefs.getAutoJuggle( context );
+                        result = dupeGame( context, lockSrc, groupID );
-            GameLock lockDest = resetGame( context, lockSrc, null, groupID,
-                                           juggle );
-            rowid = lockDest.getRowid();
-            lockDest.unlock();
-
-            if ( null != thread ) {
-                thread.release();
-            } else {
-                lockSrc.unlock();
                     }
-        } else {
+                } catch ( GameLock.GameLockedException gle ) {
+                }
+            }
+        }
+
+        if ( DBUtils.ROWID_NOTFOUND == result ) {
             Log.d( TAG, "dupeGame: unable to open rowid %d", rowidIn );
         }
-        return rowid;
+        return result;
     }
 
-    public static void deleteGame( Context context, GameLock lock, boolean informNow )
+    private static long dupeGame( Context context, GameLock lock, long groupID )
+    {
+        long result;
+        boolean juggle = CommonPrefs.getAutoJuggle( context );
+        try ( GameLock lockDest = resetGame( context, lock,
+                                             null, groupID,
+                                             juggle ) ) {
+            result = lockDest.getRowid();
+        }
+        return result;
+    }
+
+    public static void deleteGame( Context context, GameLock lock,
+                                   boolean informNow )
     {
         if ( null != lock ) {
             tellDied( context, lock, informNow );
@@ -1290,11 +1288,10 @@ public class GameUtils {
                                    + " failed for rowid %d", rowid );
                         }
                     } else {
-                        JNIThread jniThread = JNIThread.getRetained( rowid );
+                        try ( JNIThread thread = JNIThread.getRetained( rowid ) ) {
-                        if ( null != jniThread ) {
+                            if ( null != thread ) {
-                            jniThread.handle( JNIThread.JNICmd.CMD_RESEND, false,
+                                thread.handle( JNIThread.JNICmd.CMD_RESEND, false,
                                                false, false );
-                            jniThread.release();
                             } else {
                                 Log.w( TAG, "Resender.doInBackground: unable to unlock %d",
                                        rowid );
@@ -1302,6 +1299,7 @@ public class GameUtils {
                         }
                     }
                 }
+            }
 
             if ( null != m_doneProc ) {
                 final int fSentTotal = nSentTotal;

xwords4/android/app/src/main/java/org/eehouse/android/xw4/XWServiceHelper.java

@@ -76,11 +76,12 @@ abstract class XWServiceHelper {
     {
         boolean allConsumed = true;
         boolean[] isLocalP = new boolean[1];
-        JNIThread jniThread = JNIThread.getRetained( rowid );
         boolean consumed = false;
+
+        try ( JNIThread jniThread = JNIThread.getRetained( rowid ) ) {
             if ( null != jniThread ) {
+                jniThread.receive( msg, addr );
                 consumed = true;
-            jniThread.receive( msg, addr ).release();
             } else {
                 GameUtils.BackMoveResult bmr = new GameUtils.BackMoveResult();
                 if ( null == sink ) {
@@ -88,9 +89,10 @@ abstract class XWServiceHelper {
                 }
                 if ( GameUtils.feedMessage( context, rowid, msg, addr,
                                             sink, bmr, isLocalP ) ) {
-                consumed = true;
                     GameUtils.postMoveNotification( context, rowid, bmr,
                                                     isLocalP[0] );
+                    consumed = true;
+                }
             }
         }
         if ( allConsumed && !consumed ) {
@@ -149,10 +151,10 @@ abstract class XWServiceHelper {
 
                     if ( null == gi ) {
                         // locked. Maybe it's open?
-                        JNIThread thread = JNIThread.getRetained( rowid );
+                        try ( JNIThread thrd = JNIThread.getRetained( rowid ) ) {
-                        if ( null != thread ) {
+                            if ( null != thrd ) {
-                            gi = thread.getGI();
+                                gi = thrd.getGI();
-                            thread.release( false );
+                            }
                         }
                     }
                     success = null != gi && gi.forceChannel != nli.forceChannel;

xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/JNIThread.java

@@ -49,7 +49,7 @@ import java.util.Iterator;
 import java.util.Map;
 import java.util.concurrent.LinkedBlockingQueue;
 
-public class JNIThread extends Thread {
+public class JNIThread extends Thread implements AutoCloseable {
     private static final String TAG = JNIThread.class.getSimpleName();
 
     public enum JNICmd { CMD_NONE,
@@ -265,7 +265,16 @@ public class JNIThread extends Thread {
         } catch ( java.lang.InterruptedException ie ) {
             Log.ex( TAG, ie );
         }
+
+        unlockOnce();
+    }
+
+    private synchronized void unlockOnce()
+    {
+        if ( null != m_lock ) {
             m_lock.unlock();
+            m_lock = null;
+        }
     }
 
     public boolean busy()
@@ -746,9 +755,18 @@ public class JNIThread extends Thread {
             m_jniGamePtr.release();
             m_jniGamePtr = null;
         }
+
+        unlockOnce();
         Log.d( TAG, "run() finished" );
     } // run
 
+    @Override
+    public void finalize() throws java.lang.Throwable
+    {
+        Assert.assertTrue( null == m_lock || !BuildConfig.DEBUG );
+        super.finalize();
+    }
+
     public void handleBkgrnd( JNICmd cmd, Object... args )
     {
         // DbgUtils.logf( "adding: %s", cmd.toString() );
@@ -823,6 +841,12 @@ public class JNIThread extends Thread {
         }
     }
 
+    @Override
+    public void close()
+    {
+        release();
+    }
+
     public static JNIThread getRetained( long rowid )
     {
         return getRetained( rowid, null );

xwords4/android/app/src/main/java/org/eehouse/android/xw4/jni/XwJNI.java

@@ -467,7 +467,6 @@ public class XwJNI {
             release();
             super.finalize();
         }
-
     }
 
     public static native boolean dict_tilesAreSame( int dict1, int dict2 );

xwords4/android/app/src/xw4/java/org/eehouse/android/xw4/CrashTrack.java

@@ -24,5 +24,5 @@ import android.content.Context;
 
 public class CrashTrack {
     public static void init( Context context ) {} // does nothing here
-    public static void logAndSend( String msg ) {}
+    public static void logAndSend( String tag, String msg ) {}
 }

xwords4/android/app/src/xw4d/java/org/eehouse/android/xw4/CrashTrack.java

@@ -61,9 +61,12 @@ public class CrashTrack {
         }
     }
 
-    public static void logAndSend( String msg )
+    public static void logAndSend( String tag, String msg )
     {
         Crashlytics.log( msg );
+        Log.e( tag, msg );
+
+        // Now crash so Crashlytics will upload the log
         new Thread( new Runnable() {
                 @Override
                 public void run() {