Merge branch '696-npe-key-agreement-task' into 'master'

Fix NPE when stopping KeyAgreementTask, improve thread safety

This branch fixes #696 and improves the thread safety of the camera code, mostly by adding @UiThread annotations and occasionally by moving stuff onto the UI thread that might have happened on other threads before.

Closes #696

See merge request !345

briar-android/src/org/briarproject/android/keyagreement/ShowQrCodeFragment.java

@@ -89,10 +89,8 @@ public class ShowQrCodeFragment extends BaseEventFragment
 
 	private BluetoothStateReceiver receiver;
 	private QrCodeDecoder decoder;
-	private boolean gotRemotePayload;
-
-	private volatile KeyAgreementTask task;
-	private volatile boolean waitingForBluetooth;
+	private boolean gotRemotePayload, waitingForBluetooth;
+	private KeyAgreementTask task;
 
 	public static ShowQrCodeFragment newInstance() {
 
@@ -190,26 +188,33 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		if (receiver != null) getActivity().unregisterReceiver(receiver);
 	}
 
+	@UiThread
 	private void startListening() {
-		task = keyAgreementTaskFactory.getTask();
+		final KeyAgreementTask oldTask = task;
+		final KeyAgreementTask newTask = keyAgreementTaskFactory.getTask();
+		task = newTask;
 		ioExecutor.execute(new Runnable() {
 			@Override
 			public void run() {
-				task.listen();
+				if (oldTask != null) oldTask.stopListening();
+				newTask.listen();
 			}
 		});
 	}
 
+	@UiThread
 	private void stopListening() {
+		final KeyAgreementTask oldTask = task;
 		ioExecutor.execute(new Runnable() {
 			@Override
 			public void run() {
-				task.stopListening();
+				if (oldTask != null) oldTask.stopListening();
 			}
 		});
 	}
 
 	@SuppressWarnings("deprecation")
+	@UiThread
 	private void openCamera() {
 		LOG.info("Opening camera");
 		Camera camera;
@@ -229,6 +234,7 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		cameraView.start(camera, decoder, 0);
 	}
 
+	@UiThread
 	private void releaseCamera() {
 		LOG.info("Releasing camera");
 		try {
@@ -240,6 +246,7 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		}
 	}
 
+	@UiThread
 	private void reset() {
 		statusView.setVisibility(INVISIBLE);
 		cameraView.setVisibility(VISIBLE);
@@ -248,6 +255,7 @@ public class ShowQrCodeFragment extends BaseEventFragment
 		startListening();
 	}
 
+	@UiThread
 	private void qrCodeScanned(String content) {
 		try {
 			Payload remotePayload = payloadParser.parse(
@@ -320,7 +328,6 @@ public class ShowQrCodeFragment extends BaseEventFragment
 	}
 
 	private void setQrCode(final Payload localPayload) {
-
 		listener.runOnUiThread(new Runnable() {
 			@Override
 			public void run() {
@@ -397,6 +404,7 @@ public class ShowQrCodeFragment extends BaseEventFragment
 	}
 
 	private class BluetoothStateReceiver extends BroadcastReceiver {
+		@UiThread
 		@Override
 		public void onReceive(Context ctx, Intent intent) {
 			int state = intent.getIntExtra(EXTRA_STATE, 0);

briar-android/src/org/briarproject/android/util/PreviewConsumer.java

@@ -1,11 +1,14 @@
 package org.briarproject.android.util;
 
 import android.hardware.Camera;
+import android.support.annotation.UiThread;
 
 @SuppressWarnings("deprecation")
 public interface PreviewConsumer {
 
+	@UiThread
 	void start(Camera camera);
 
+	@UiThread
 	void stop();
 }

briar-android/src/org/briarproject/android/util/QrCodeDecoder.java

@@ -4,6 +4,7 @@ import android.hardware.Camera;
 import android.hardware.Camera.PreviewCallback;
 import android.hardware.Camera.Size;
 import android.os.AsyncTask;
+import android.support.annotation.UiThread;
 
 import com.google.zxing.BinaryBitmap;
 import com.google.zxing.LuminanceSource;
@@ -33,19 +34,24 @@ public class QrCodeDecoder implements PreviewConsumer, PreviewCallback {
 		this.callback = callback;
 	}
 
+	@Override
 	public void start(Camera camera) {
 		stopped = false;
 		askForPreviewFrame(camera);
 	}
 
+	@Override
 	public void stop() {
 		stopped = true;
 	}
 
+	@UiThread
 	private void askForPreviewFrame(Camera camera) {
 		if (!stopped) camera.setOneShotPreviewCallback(this);
 	}
 
+	@UiThread
+	@Override
 	public void onPreviewFrame(byte[] data, Camera camera) {
 		if (!stopped) {
 			Size size = camera.getParameters().getPreviewSize();
@@ -55,9 +61,9 @@ public class QrCodeDecoder implements PreviewConsumer, PreviewCallback {
 
 	private class DecoderTask extends AsyncTask<Void, Void, Void> {
 
-		final Camera camera;
-		final byte[] data;
-		final int width, height;
+		private final Camera camera;
+		private final byte[] data;
+		private final int width, height;
 
 		DecoderTask(Camera camera, byte[] data, int width, int height) {
 			this.camera = camera;

briar-android/src/org/briarproject/android/view/CameraView.java

@@ -7,6 +7,7 @@ import android.hardware.Camera.CameraInfo;
 import android.hardware.Camera.Parameters;
 import android.hardware.Camera.Size;
 import android.os.Build;
+import android.support.annotation.UiThread;
 import android.util.AttributeSet;
 import android.view.Surface;
 import android.view.SurfaceHolder;
@@ -72,6 +73,7 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		if (surface != null) surface.release();
 	}
 
+	@UiThread
 	public void start(Camera camera, PreviewConsumer previewConsumer,
 			int rotationDegrees) {
 		this.camera = camera;
@@ -97,17 +99,18 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		if (surface != null) startPreview(getHolder());
 	}
 
+	@UiThread
 	public void stop() {
 		stopPreview();
 		try {
-			if (camera != null)
-				camera.release();
+			if (camera != null) camera.release();
 		} catch (RuntimeException e) {
 			LOG.log(WARNING, "Error releasing camera", e);
 		}
 		camera = null;
 	}
 
+	@UiThread
 	private void startPreview(SurfaceHolder holder) {
 		try {
 			camera.setPreviewDisplay(holder);
@@ -118,28 +121,29 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		}
 	}
 
+	@UiThread
 	private void stopPreview() {
 		try {
 			stopConsumer();
-			if (camera != null)
-				camera.stopPreview();
+			if (camera != null) camera.stopPreview();
 		} catch (RuntimeException e) {
 			LOG.log(WARNING, "Error stopping camera preview", e);
 		}
 	}
 
+	@UiThread
 	public void startConsumer() {
 		if (autoFocus) camera.autoFocus(this);
 		previewConsumer.start(camera);
 	}
 
+	@UiThread
 	public void stopConsumer() {
-		if (previewConsumer != null) {
-			previewConsumer.stop();
-		}
+		if (previewConsumer != null) previewConsumer.stop();
 		if (autoFocus) camera.cancelAutoFocus();
 	}
 
+	@UiThread
 	private void setDisplayOrientation(int rotationDegrees) {
 		int orientation;
 		CameraInfo info = new CameraInfo();
@@ -160,6 +164,7 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		displayOrientation = orientation;
 	}
 
+	@UiThread
 	private Parameters setSceneMode(Camera camera, Parameters params) {
 		List<String> sceneModes = params.getSupportedSceneModes();
 		if (sceneModes == null) return params;
@@ -172,18 +177,21 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		return params;
 	}
 
+	@UiThread
 	private Parameters disableFlash(Camera camera, Parameters params) {
 		params.setFlashMode(FLASH_MODE_OFF);
 		camera.setParameters(params);
 		return camera.getParameters();
 	}
 
+	@UiThread
 	private Parameters disableSceneMode(Camera camera, Parameters params) {
 		params.setSceneMode(SCENE_MODE_AUTO);
 		camera.setParameters(params);
 		return camera.getParameters();
 	}
 
+	@UiThread
 	private Parameters setBestParameters(Camera camera, Parameters params) {
 		setVideoStabilisation(params);
 		setFocusMode(params);
@@ -193,6 +201,7 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		return camera.getParameters();
 	}
 
+	@UiThread
 	private void setVideoStabilisation(Parameters params) {
 		if (Build.VERSION.SDK_INT >= 15 &&
 				params.isVideoStabilizationSupported()) {
@@ -200,6 +209,7 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		}
 	}
 
+	@UiThread
 	private void setFocusMode(Parameters params) {
 		List<String> focusModes = params.getSupportedFocusModes();
 		if (LOG.isLoggable(INFO)) LOG.info("Focus modes: " + focusModes);
@@ -218,6 +228,7 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		}
 	}
 
+	@UiThread
 	private void setPreviewSize(Parameters params) {
 		if (surfaceWidth == 0 || surfaceHeight == 0) return;
 		float idealRatio = (float) surfaceWidth / surfaceHeight;
@@ -249,11 +260,13 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		}
 	}
 
+	@UiThread
 	private void enableAutoFocus(String focusMode) {
 		autoFocus = FOCUS_MODE_AUTO.equals(focusMode) ||
 				FOCUS_MODE_MACRO.equals(focusMode);
 	}
 
+	@UiThread
 	private void logCameraParameters() {
 		if (LOG.isLoggable(INFO)) {
 			Parameters params = camera.getParameters();
@@ -270,7 +283,17 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 	}
 
 	@Override
-	public void surfaceCreated(SurfaceHolder holder) {
+	public void surfaceCreated(final SurfaceHolder holder) {
+		post(new Runnable() {
+			@Override
+			public void run() {
+				surfaceCreatedUi(holder);
+			}
+		});
+	}
+
+	@UiThread
+	private void surfaceCreatedUi(SurfaceHolder holder) {
 		LOG.info("Surface created");
 		if (surface != null) throw new IllegalStateException();
 		surface = holder.getSurface();
@@ -278,7 +301,18 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 	}
 
 	@Override
-	public void surfaceChanged(SurfaceHolder holder, int format, int w, int h) {
+	public void surfaceChanged(final SurfaceHolder holder, int format,
+			final int w, final int h) {
+		post(new Runnable() {
+			@Override
+			public void run() {
+				surfaceChangedUi(holder, w, h);
+			}
+		});
+	}
+
+	@UiThread
+	private void surfaceChangedUi(SurfaceHolder holder, int w, int h) {
 		if (LOG.isLoggable(INFO)) LOG.info("Surface changed: " + w + "x" + h);
 		// Release the previous surface if necessary
 		if (surface != null && surface != holder.getSurface())
@@ -300,7 +334,17 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 	}
 
 	@Override
-	public void surfaceDestroyed(SurfaceHolder holder) {
+	public void surfaceDestroyed(final SurfaceHolder holder) {
+		post(new Runnable() {
+			@Override
+			public void run() {
+				surfaceDestroyedUi(holder);
+			}
+		});
+	}
+
+	@UiThread
+	private void surfaceDestroyedUi(SurfaceHolder holder) {
 		LOG.info("Surface destroyed");
 		if (holder.getSurface() != surface) throw new IllegalStateException();
 		if (surface != null) surface.release();
@@ -317,6 +361,7 @@ public class CameraView extends SurfaceView implements SurfaceHolder.Callback,
 		}, AUTO_FOCUS_RETRY_DELAY);
 	}
 
+	@UiThread
 	private void retryAutoFocus() {
 		try {
 			if (camera != null) camera.autoFocus(this);

briar-core/src/org/briarproject/keyagreement/KeyAgreementTaskImpl.java

@@ -39,7 +39,7 @@ class KeyAgreementTaskImpl extends Thread implements
 	private Payload localPayload;
 	private Payload remotePayload;
 
-	public KeyAgreementTaskImpl(Clock clock, CryptoComponent crypto,
+	KeyAgreementTaskImpl(Clock clock, CryptoComponent crypto,
 			EventBus eventBus, PayloadEncoder payloadEncoder,
 			PluginManager pluginManager, Executor ioExecutor) {
 		this.crypto = crypto;