Fix threading issues in Java JNI code for Android Chromoting client.

Updated comments in JniInterface.java to document which threads are
used to access fields and methods. Also added synchronization where it's
needed and removed it where it isn't.

A private lock is used instead of synchronizing on JniInterface.class.

BUG=305770

Review URL: https://codereview.chromium.org/90443002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@239293 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 88 insertions, 66 deletions

diff --git a/remoting/android/java/src/org/chromium/chromoting/DesktopView.java b/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
index fad53eb..70940ce 100644
--- a/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
+++ b/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
@@ -33,7 +33,7 @@ import org.chromium.chromoting.jni.JniInterface;
  * multitouch pan and zoom gestures, and collects and forwards input events.
  */
 /** GUI element that holds the drawing canvas. */
-public class DesktopView extends SurfaceView implements DesktopViewInterface, Runnable,
+public class DesktopView extends SurfaceView implements DesktopViewInterface,
         SurfaceHolder.Callback {
     private RenderData mRenderData;
     private TouchInputHandler mInputHandler;
@@ -44,6 +44,11 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface, Ru
     // thread, so the access should be synchronized on |mRenderData|.
     private boolean mRepaintPending;
 
+    // Flag used to ensure that the SurfaceView is only painted between calls to surfaceCreated()
+    // and surfaceDestroyed(). Accessed on main thread and display thread, so this should be
+    // synchronized on |mRenderData|.
+    private boolean mSurfaceCreated = false;
+
     /** Helper class for displaying the long-press feedback animation. This class is thread-safe. */
     private static class FeedbackAnimator {
         /** Total duration of the animation, in milliseconds. */
@@ -149,8 +154,7 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface, Ru
      * cause the UI to lag. Specifically, it is currently invoked on the native
      * graphics thread using a JNI.
      */
-    @Override
-    public void run() {
+    public void paint() {
         long startTimeMs = SystemClock.uptimeMillis();
 
         if (Looper.myLooper() == Looper.getMainLooper()) {
@@ -181,10 +185,21 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface, Ru
             mInputHandler.onHostSizeChanged(width, height);
         }
 
-        Canvas canvas = getHolder().lockCanvas();
+        Canvas canvas;
         int x, y;
         synchronized (mRenderData) {
             mRepaintPending = false;
+            // Don't try to lock the canvas before it is ready, as the implementation of
+            // lockCanvas() may throttle these calls to a slow rate in order to avoid consuming CPU.
+            // Note that a successful call to lockCanvas() will prevent the framework from
+            // destroying the Surface until it is unlocked.
+            if (!mSurfaceCreated) {
+                return;
+            }
+            canvas = getHolder().lockCanvas();
+            if (canvas == null) {
+                return;
+            }
             canvas.setMatrix(mRenderData.transform);
             x = mRenderData.cursorPosition.x;
             y = mRenderData.cursorPosition.y;
@@ -249,15 +264,22 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface, Ru
             mRenderData.screenHeight = height;
         }
 
+        JniInterface.provideRedrawCallback(new Runnable() {
+            @Override
+            public void run() {
+                paint();
+            }
+        });
         mInputHandler.onClientSizeChanged(width, height);
-
-        JniInterface.provideRedrawCallback(this);
+        requestRepaint();
     }
 
     /** Called when the canvas is first created. */
     @Override
     public void surfaceCreated(SurfaceHolder holder) {
-        Log.i("deskview", "DesktopView.surfaceCreated(...)");
+        synchronized (mRenderData) {
+            mSurfaceCreated = true;
+        }
     }
 
     /**
@@ -266,10 +288,12 @@ public class DesktopView extends SurfaceView implements DesktopViewInterface, Ru
      */
     @Override
     public void surfaceDestroyed(SurfaceHolder holder) {
-        Log.i("deskview", "DesktopView.surfaceDestroyed(...)");
-
         // Stop this canvas from being redrawn.
         JniInterface.provideRedrawCallback(null);
+
+        synchronized (mRenderData) {
+            mSurfaceCreated = false;
+        }
     }
 
     /** Called when a software keyboard is requested, and specifies its options. */
diff --git a/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java b/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
index 8074ad2..bb84a85 100644
--- a/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
+++ b/remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
@@ -41,50 +41,56 @@ public class JniInterface {
     /*
      * Library-loading state machine.
      */
-    /** Whether we've already loaded the library. */
+    /** Whether the library has been loaded. Accessed on the UI thread. */
     private static boolean sLoaded = false;
 
-    /** The application context. */
+    /** The application context. Accessed on the UI thread. */
     private static Activity sContext = null;
 
     /*
      * Connection-initiating state machine.
      */
-    /** Whether the native code is attempting a connection. */
+    /** Whether the native code is attempting a connection. Accessed on the UI thread. */
     private static boolean sConnected = false;
 
-    /** Callback to signal upon successful connection. */
+    /** Callback to signal upon successful connection. Accessed on the UI thread. */
     private static Runnable sSuccessCallback = null;
 
-    /** Dialog for reporting connection progress. */
+    /** Dialog for reporting connection progress. Accessed on the UI thread. */
     private static ProgressDialog sProgressIndicator = null;
 
-    /** Callback to signal whenever we need to redraw. */
+    // Protects access to |sProgressIndicator|. Used only to silence FindBugs warnings - the
+    // variable it protects is only accessed on a single thread.
+    // TODO(lambroslambrou): Refactor the ProgressIndicator into a separate class.
+    private static Object sProgressIndicatorLock = new Object();
+
+    /**
+     * Callback invoked on the graphics thread to repaint the desktop. Accessed on the UI and
+     * graphics threads.
+     */
     private static Runnable sRedrawCallback = null;
 
-    /** Bitmap holding a copy of the latest video frame. */
+    /** Bitmap holding a copy of the latest video frame. Accessed on the UI and graphics threads. */
     private static Bitmap sFrameBitmap = null;
 
-    /** Lock to protect the frame bitmap reference. */
+    /** Protects access to sFrameBitmap. */
     private static final Object sFrameLock = new Object();
 
-    /** Position of cursor hot-spot. */
+    /** Position of cursor hot-spot. Accessed on the graphics thread. */
     private static Point sCursorHotspot = new Point();
 
-    /** Bitmap holding the cursor shape. */
+    /** Bitmap holding the cursor shape. Accessed on the graphics thread. */
     private static Bitmap sCursorBitmap = null;
 
     /**
      * To be called once from the main Activity. Any subsequent calls will update the application
      * context, but not reload the library. This is useful e.g. when the activity is closed and the
-     * user later wants to return to the application.
+     * user later wants to return to the application. Called on the UI thread.
      */
     public static void loadLibrary(Activity context) {
         sContext = context;
 
-        synchronized(JniInterface.class) {
-            if (sLoaded) return;
-        }
+        if (sLoaded) return;
 
         System.loadLibrary("remoting_client_jni");
 
@@ -102,16 +108,10 @@ public class JniInterface {
     public static native String nativeGetClientId();
     public static native String nativeGetClientSecret();
 
-    /** Attempts to form a connection to the user-selected host. */
+    /** Attempts to form a connection to the user-selected host. Called on the UI thread. */
     public static void connectToHost(String username, String authToken,
             String hostJid, String hostId, String hostPubkey, Runnable successCallback) {
-        synchronized(JniInterface.class) {
-            if (!sLoaded) return;
-
-            if (sConnected) {
-                disconnectFromHost();
-            }
-        }
+        disconnectFromHost();
 
         sSuccessCallback = successCallback;
         SharedPreferences prefs = sContext.getPreferences(Activity.MODE_PRIVATE);
@@ -124,11 +124,11 @@ public class JniInterface {
     private static native void nativeConnect(String username, String authToken, String hostJid,
             String hostId, String hostPubkey, String pairId, String pairSecret);
 
-    /** Severs the connection and cleans up. */
+    /** Severs the connection and cleans up. Called on the UI thread. */
     public static void disconnectFromHost() {
-        synchronized(JniInterface.class) {
-            if (!sLoaded || !sConnected) return;
+        if (!sConnected) return;
 
+        synchronized (sProgressIndicatorLock) {
             if (sProgressIndicator != null) {
                 sProgressIndicator.dismiss();
                 sProgressIndicator = null;
@@ -148,12 +148,12 @@ public class JniInterface {
     /** Performs the native portion of the cleanup. */
     private static native void nativeDisconnect();
 
-    /** Reports whenever the connection status changes. */
+    /** Reports whenever the connection status changes. Called on the UI thread. */
     @CalledByNative
     private static void reportConnectionStatus(int state, int error) {
         if (state < SUCCESSFUL_CONNECTION && error == 0) {
             // The connection is still being established, so we'll report the current progress.
-            synchronized (JniInterface.class) {
+            synchronized (sProgressIndicatorLock) {
                 if (sProgressIndicator == null) {
                     sProgressIndicator = ProgressDialog.show(sContext, sContext.
                             getString(R.string.progress_title), sContext.getResources().
@@ -165,16 +165,14 @@ public class JniInterface {
                                     disconnectFromHost();
                                 }
                             });
-                }
-                else {
+                } else {
                     sProgressIndicator.setMessage(
                             sContext.getResources().getStringArray(R.array.protoc_states)[state]);
                 }
             }
-        }
-        else {
+        } else {
             // The connection is complete or has failed, so we can lose the progress indicator.
-            synchronized (JniInterface.class) {
+            synchronized (sProgressIndicatorLock) {
                 if (sProgressIndicator != null) {
                     sProgressIndicator.dismiss();
                     sProgressIndicator = null;
@@ -196,7 +194,7 @@ public class JniInterface {
         }
     }
 
-    /** Prompts the user to enter a PIN. */
+    /** Prompts the user to enter a PIN. Called on the UI thread. */
     @CalledByNative
     private static void displayAuthenticationPrompt(boolean pairingSupported) {
         AlertDialog.Builder pinPrompt = new AlertDialog.Builder(sContext);
@@ -265,18 +263,19 @@ public class JniInterface {
     /** Performs the native response to the user's PIN. */
     private static native void nativeAuthenticationResponse(String pin, boolean createPair);
 
-    /** Saves newly-received pairing credentials to permanent storage. */
+    /** Saves newly-received pairing credentials to permanent storage. Called on the UI thread. */
     @CalledByNative
     private static void commitPairingCredentials(String host, byte[] id, byte[] secret) {
-        synchronized (sContext) {
-            sContext.getPreferences(Activity.MODE_PRIVATE).edit().
-                    putString(host + "_id", new String(id)).
-                    putString(host + "_secret", new String(secret)).
-                    apply();
-        }
+        sContext.getPreferences(Activity.MODE_PRIVATE).edit().
+                putString(host + "_id", new String(id)).
+                putString(host + "_secret", new String(secret)).
+                apply();
     }
 
-    /** Moves the mouse cursor, possibly while clicking the specified (nonnegative) button. */
+    /**
+     * Moves the mouse cursor, possibly while clicking the specified (nonnegative) button. Called
+     * on the UI thread.
+     */
     public static void mouseAction(int x, int y, int whichButton, boolean buttonDown) {
         if (!sConnected) {
             return;
@@ -288,7 +287,7 @@ public class JniInterface {
     /** Passes mouse information to the native handling code. */
     private static native void nativeMouseAction(int x, int y, int whichButton, boolean buttonDown);
 
-    /** Injects a mouse-wheel event with delta values. */
+    /** Injects a mouse-wheel event with delta values. Called on the UI thread. */
     public static void mouseWheelDeltaAction(int deltaX, int deltaY) {
         if (!sConnected) {
             return;
@@ -300,7 +299,7 @@ public class JniInterface {
     /** Passes mouse-wheel information to the native handling code. */
     private static native void nativeMouseWheelDeltaAction(int deltaX, int deltaY);
 
-    /** Presses and releases the specified (nonnegative) key. */
+    /** Presses and releases the specified (nonnegative) key. Called on the UI thread. */
     public static void keyboardAction(int keyCode, boolean keyDown) {
         if (!sConnected) {
             return;
@@ -314,17 +313,16 @@ public class JniInterface {
 
     /**
      * Sets the redraw callback to the provided functor. Provide a value of null whenever the
-     * window is no longer visible so that we don't continue to draw onto it.
+     * window is no longer visible so that we don't continue to draw onto it. Called on the UI
+     * thread.
      */
     public static void provideRedrawCallback(Runnable redrawCallback) {
         sRedrawCallback = redrawCallback;
     }
 
-    /** Forces the native graphics thread to redraw to the canvas. */
+    /** Forces the native graphics thread to redraw to the canvas. Called on the UI thread. */
     public static boolean redrawGraphics() {
-        synchronized(JniInterface.class) {
-            if (!sConnected || sRedrawCallback == null) return false;
-        }
+        if (!sConnected || sRedrawCallback == null) return false;
 
         nativeScheduleRedraw();
         return true;
@@ -333,11 +331,15 @@ public class JniInterface {
     /** Schedules a redraw on the native graphics thread. */
     private static native void nativeScheduleRedraw();
 
-    /** Performs the redrawing callback. This is a no-op if the window isn't visible. */
+    /**
+     * Performs the redrawing callback. This is a no-op if the window isn't visible. Called on the
+     * graphics thread.
+     */
     @CalledByNative
     private static void redrawGraphicsInternal() {
-        if (sRedrawCallback != null) {
-            sRedrawCallback.run();
+        Runnable callback = sRedrawCallback;
+        if (callback != null) {
+            callback.run();
         }
     }
 
@@ -350,10 +352,6 @@ public class JniInterface {
             Log.w("jniiface", "Canvas being redrawn on UI thread");
         }
 
-        if (!sConnected) {
-            return null;
-        }
-
         synchronized (sFrameLock) {
             return sFrameBitmap;
         }
@@ -397,9 +395,9 @@ public class JniInterface {
         sCursorBitmap = Bitmap.createBitmap(data, width, height, Bitmap.Config.ARGB_8888);
     }
 
-    /** Position of cursor hotspot within cursor image. */
+    /** Position of cursor hotspot within cursor image. Called on the graphics thread. */
     public static Point getCursorHotspot() { return sCursorHotspot; }
 
-    /** Returns the current cursor shape. */
+    /** Returns the current cursor shape. Called on the graphics thread. */
     public static Bitmap getCursorBitmap() { return sCursorBitmap; }
 }