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

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

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chrome.browser.photo_picker;
 
 import android.content.ComponentName;
 import android.content.Context;
 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;
 
 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.
      */
     public interface ServiceReadyCallback {
         /**
          * A function to define to receive a notification once the service is up and running.
          */
         void serviceReady();
     }
 
     /**
      * An interface notifying clients when an image has finished decoding.
      */
     public interface ImageDecodedCallback {
         /**
          * A function to define to receive a notification that an image has been decoded.
          * @param filePath The file path for the newly decoded image.
          * @param bitmap The results of the decoding (or placeholder image, if failed).
          */
         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.
      */
     private static class DecoderServiceParams {
         // The path to the file containing the bitmap to decode.
         public String mFilePath;
 
         // The requested size (width and height) of the bitmap, once decoded.
         public int mSize;
 
         // The callback to use to communicate the results of the decoding.
@@ skipping 11 matching lines @@
 
     // Map of file paths to decoder parameters in order of request.
     private LinkedHashMap<String, DecoderServiceParams> mRequests = new LinkedHashMap<>();
     LinkedHashMap<String, DecoderServiceParams> getRequests() {
         return mRequests;
     }
 
     // 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;
     }
 
     /**
      * Initiate binding with the {@link DecoderService}.
      * @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);
     }
 
     /**
      * Unbind from the {@link DecoderService}.
      * @param context The context to use.
      */
     public void unbind(Context context) {
         if (mBound) {
             context.unbindService(mConnection);
             mBound = false;
@@ skipping 17 matching lines @@
      * Dispatches the next image for decoding (from the queue).
      */
     private void dispatchNextDecodeImageRequest() {
         if (mRequests.entrySet().iterator().hasNext()) {
             DecoderServiceParams params = mRequests.entrySet().iterator().next().getValue();
             params.mTimestamp = SystemClock.elapsedRealtime();
             dispatchDecodeImageRequest(params.mFilePath, params.mSize);
         }
     }
 
+    @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
      * the request queue).
      * @param filePath The path to the image that was just decoded.
      * @param bitmap The resulting decoded bitmap.
      * @param decodeTime The length of time it took to decode the bitmap.
      */
     public void closeRequest(String filePath, Bitmap bitmap, long decodeTime) {
         DecoderServiceParams params = getRequests().get(filePath);
@@ skipping 47 matching lines @@
                 if (inputFile != null) inputFile.close();
             } catch (IOException e) {
                 Log.e(TAG, "Unable to close inputFile: " + e);
             }
             StrictMode.setThreadPolicy(oldPolicy);
         }
 
         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);
             closeRequest(filePath, null, -1);
         } catch (IOException e) {
             Log.e(TAG, "Communications failed (IO): " + e);
             closeRequest(filePath, null, -1);
         }
     }
 
     /**
      * Cancels a request to decode an image (if it hasn't already been dispatched).
      * @param filePath The path to the image to cancel decoding.
      */
     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);
-            }
-        }
-    }
 }