Photo Picker dialog: Convert to AIDL for easier security review.

chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java

Index: chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java b/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java
index e5cf11c0f56eff54e0fa97d7b8a32f18e8bf22ff..73ec205754e42b02530b09f740d6e6cbf7786021 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java
@@ -10,16 +10,14 @@ import android.content.Intent;
 import android.content.ServiceConnection;
 import android.graphics.Bitmap;
 import android.os.Bundle;
-import android.os.Handler;
 import android.os.IBinder;
-import android.os.Message;
-import android.os.Messenger;
 import android.os.ParcelFileDescriptor;
 import android.os.RemoteException;
 import android.os.StrictMode;
 import android.os.SystemClock;
 
 import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.util.ConversionUtils;
 
@@ -27,17 +25,31 @@ import java.io.File;
 import java.io.FileDescriptor;
 import java.io.FileInputStream;
 import java.io.IOException;
-import java.lang.ref.WeakReference;
 import java.util.LinkedHashMap;
 import java.util.concurrent.TimeUnit;
 
 /**
  * A class to communicate with the {@link DecoderService}.
  */
-public class DecoderServiceHost {
+public class DecoderServiceHost extends IDecoderServiceCallback.Stub {
     // A tag for logging error messages.
     private static final String TAG = "ImageDecoderHost";
 
+    IDecoderService mIRemoteService;
+    private ServiceConnection mConnection = new ServiceConnection() {
+        public void onServiceConnected(ComponentName className, IBinder service) {
+            mIRemoteService = IDecoderService.Stub.asInterface(service);
+            mBound = true;
+            mCallback.serviceReady();
+        }
+
+        public void onServiceDisconnected(ComponentName className) {
+            Log.e(TAG, "Service has unexpectedly disconnected");
+            mIRemoteService = null;
+            mBound = false;
+        }
+    };
+
     /**
      * Interface for notifying clients of the service being ready.
      */
@@ -60,30 +72,6 @@ public class DecoderServiceHost {
         void imageDecodedCallback(String filePath, Bitmap bitmap);
     }
 
-    /**
-     * Class for interacting with the main interface of the service.
-     */
-    private class DecoderServiceConnection implements ServiceConnection {
-        // The callback to use to notify the service being ready.
-        private ServiceReadyCallback mCallback;
-
-        public DecoderServiceConnection(ServiceReadyCallback callback) {
-            mCallback = callback;
-        }
-
-        // Called when a connection to the service has been established.
-        public void onServiceConnected(ComponentName name, IBinder service) {
-            mService = new Messenger(service);
-            mBound = true;
-            mCallback.serviceReady();
-        }
-
-        // Called when a connection to the service has been lost.
-        public void onServiceDisconnected(ComponentName name) {
-            mBound = false;
-        }
-    }
-
     /**
      * Class for keeping track of the data involved with each request.
      */
@@ -116,24 +104,18 @@ public class DecoderServiceHost {
     // The callback used to notify the client when the service is ready.
     private ServiceReadyCallback mCallback;
 
-    // Messenger for communicating with the remote service.
-    Messenger mService = null;
-
-    // Our service connection to the {@link DecoderService}.
-    private DecoderServiceConnection mConnection;
-
     // Flag indicating whether we are bound to the service.
     boolean mBound;
 
-    // The inbound messenger used by the remote service to communicate with us.
-    final Messenger mMessenger = new Messenger(new IncomingHandler(this));
+    private Context mContext;

[Robert Sesek, 2017/06/20 20:49:43]

final?

 
     /**
      * The DecoderServiceHost constructor.
      * @param callback The callback to use when communicating back to the client.
      */
-    public DecoderServiceHost(ServiceReadyCallback callback) {
+    public DecoderServiceHost(ServiceReadyCallback callback, Context context) {
         mCallback = callback;
+        mContext = context;
     }
 
     /**
@@ -141,9 +123,9 @@ public class DecoderServiceHost {
      * @param context The context to use.
      */
     public void bind(Context context) {
-        mConnection = new DecoderServiceConnection(mCallback);
-        Intent intent = new Intent(context, DecoderService.class);
-        context.bindService(intent, mConnection, Context.BIND_AUTO_CREATE);
+        Intent intent = new Intent(mContext, DecoderService.class);
+        intent.setAction(IDecoderService.class.getName());
+        mContext.bindService(intent, mConnection, Context.BIND_AUTO_CREATE);
     }
 
     /**
@@ -181,6 +163,23 @@ public class DecoderServiceHost {
         }
     }
 
+    @Override
+    public void onDecodeImageDone(final Bundle payload) {
+        ThreadUtils.runOnUiThread(new Runnable() {
+            @Override
+            public void run() {
+                // Read the reply back from the service.
+                String filePath = payload.getString(DecoderService.KEY_FILE_PATH);
+                Boolean success = payload.getBoolean(DecoderService.KEY_SUCCESS);
+                Bitmap bitmap = success
+                        ? (Bitmap) payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP)
+                        : null;
+                long decodeTime = payload.getLong(DecoderService.KEY_DECODE_TIME);
+                closeRequest(filePath, bitmap, decodeTime);
+            }
+        });
+    }
+
     /**
      * Ties up all the loose ends from the decoding request (communicates the results of the
      * decoding process back to the client, and takes care of house-keeping chores regarding
@@ -248,13 +247,10 @@ public class DecoderServiceHost {
         if (pfd == null) return;
 
         // Prepare and send the data over.
-        Message payload = Message.obtain(null, DecoderService.MSG_DECODE_IMAGE);
-        payload.replyTo = mMessenger;
         bundle.putString(DecoderService.KEY_FILE_PATH, filePath);
         bundle.putInt(DecoderService.KEY_SIZE, size);
-        payload.setData(bundle);
         try {
-            mService.send(payload);
+            mIRemoteService.decodeImage(bundle, this);

[Theresa, 2017/06/20 16:53:12]

Does the service automatically run this on a different thread than the UI
thread?

Also, would it make sense for the DecoderService to be responsible for calling
onDecodeImageDone on the correct (UI) thread? The asymmetry of making the
request on the UI thread but receiving it not on the UI thread is a bit
confusing.

[Finnur, 2017/06/20 20:55:05]

No, no problem. This is exactly the kind of thing that one would expect and hope
are brought up during code review. The later we catch this kind of issues, the
more expensive it is to fix them.
That is what I experienced. The callback is on some arbitrary thread and jumping
to the UI thread on the callback was necessary to be able to touch UI objects
without exceptions.

This doc touches upon threading issues:
https://developer.android.com/guide/components/aidl.html

[Theresa, 2017/06/21 14:58:36]

Thanks for the link, that is very helpful.

So this would fall under "Calls from a remote process are dispatched from a
thread pool the platform maintains inside of your own process. You must be
prepared for incoming calls from unknown threads, with multiple calls happening
at the same time. In other words, an implementation of an AIDL interface must be
completely thread-safe." Correct?

Will you please add a short comment to #onDecodeImageDone() explaining that the
AIDL callback may happen on any thread?

[Finnur, 2017/06/21 15:20:34]

Yes. Done.

             pfd.close();
         } catch (RemoteException e) {
             Log.e(TAG, "Communications failed (Remote): " + e);
@@ -272,46 +268,4 @@ public class DecoderServiceHost {
     public void cancelDecodeImage(String filePath) {
         mRequests.remove(filePath);
     }
-
-    /**
-     * A class for handling communications from the service to us.
-     */
-    static class IncomingHandler extends Handler {
-        // The DecoderServiceHost object to communicate with.
-        private final WeakReference<DecoderServiceHost> mHost;
-
-        /**
-         * Constructor for IncomingHandler.
-         * @param host The DecoderServiceHost object to communicate with.
-         */
-        IncomingHandler(DecoderServiceHost host) {
-            mHost = new WeakReference<DecoderServiceHost>(host);
-        }
-
-        @Override
-        public void handleMessage(Message msg) {
-            DecoderServiceHost host = mHost.get();
-            if (host == null) {
-                super.handleMessage(msg);
-                return;
-            }
-
-            switch (msg.what) {
-                case DecoderService.MSG_IMAGE_DECODED_REPLY:
-                    Bundle payload = msg.getData();
-
-                    // Read the reply back from the service.
-                    String filePath = payload.getString(DecoderService.KEY_FILE_PATH);
-                    Boolean success = payload.getBoolean(DecoderService.KEY_SUCCESS);
-                    Bitmap bitmap = success
-                            ? (Bitmap) payload.getParcelable(DecoderService.KEY_IMAGE_BITMAP)
-                            : null;
-                    long decodeTime = payload.getLong(DecoderService.KEY_DECODE_TIME);
-                    host.closeRequest(filePath, bitmap, decodeTime);
-                    break;
-                default:
-                    super.handleMessage(msg);
-            }
-        }
-    }
 }