Skip to content

Commit

Permalink
Fix threading issues in Java JNI code for Android Chromoting client.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
lambroslambrou@chromium.org committed Dec 7, 2013
1 parent fd77aee commit ad0be0c
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 66 deletions.
42 changes: 33 additions & 9 deletions remoting/android/java/src/org/chromium/chromoting/DesktopView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
* 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;
Expand All @@ -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. */
Expand Down Expand Up @@ -149,8 +154,7 @@ public void onScreenConfigurationChanged() {
* 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()) {
Expand Down Expand Up @@ -181,10 +185,21 @@ public void run() {
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;
Expand Down Expand Up @@ -249,15 +264,22 @@ public void surfaceChanged(SurfaceHolder holder, int format, int width, int heig
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;
}
}

/**
Expand All @@ -266,10 +288,12 @@ public void surfaceCreated(SurfaceHolder holder) {
*/
@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. */
Expand Down
112 changes: 55 additions & 57 deletions remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand All @@ -102,16 +108,10 @@ public static void loadLibrary(Activity context) {
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);
Expand All @@ -124,11 +124,11 @@ public static void connectToHost(String username, String authToken,
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;
Expand All @@ -148,12 +148,12 @@ public static void disconnectFromHost() {
/** 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().
Expand All @@ -165,16 +165,14 @@ public void onCancel(DialogInterface dialog) {
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;
Expand All @@ -196,7 +194,7 @@ public void onCancel(DialogInterface dialog) {
}
}

/** 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);
Expand Down Expand Up @@ -265,18 +263,19 @@ public void onCancel(DialogInterface dialog) {
/** 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;
Expand All @@ -288,7 +287,7 @@ public static void mouseAction(int x, int y, int whichButton, boolean buttonDown
/** 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;
Expand All @@ -300,7 +299,7 @@ public static void mouseWheelDeltaAction(int deltaX, int deltaY) {
/** 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;
Expand All @@ -314,17 +313,16 @@ public static void keyboardAction(int keyCode, boolean keyDown) {

/**
* 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;
Expand All @@ -333,11 +331,15 @@ public static boolean redrawGraphics() {
/** 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();
}
}

Expand All @@ -350,10 +352,6 @@ public static Bitmap getVideoFrame() {
Log.w("jniiface", "Canvas being redrawn on UI thread");
}

if (!sConnected) {
return null;
}

synchronized (sFrameLock) {
return sFrameBitmap;
}
Expand Down Expand Up @@ -397,9 +395,9 @@ public static void updateCursorShape(int width, int height, int hotspotX, int ho
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; }
}

0 comments on commit ad0be0c

Please sign in to comment.