Photo Picker Dialog: Use sandboxed utility process for decoding images.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java b/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java
new file mode 100644
index 0000000000000000000000000000000000000000..acc2ef03901dc8628a6513f2d446c79c67da6ab2
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java
@@ -0,0 +1,237 @@
+// 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.app.Service;
+import android.content.Intent;
+import android.graphics.Bitmap;
+import android.os.Bundle;
+import android.os.Handler;
+import android.os.IBinder;
+import android.os.MemoryFile;
+import android.os.Message;
+import android.os.Messenger;
+import android.os.ParcelFileDescriptor;
+import android.os.RemoteException;
+
+import java.io.FileDescriptor;
+import java.io.IOException;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.nio.ByteBuffer;
+
+/**
+ * A service to accept requests to take image file contents and decode them.
+ */
+public class DecoderService extends Service {
+    // Message ids for communicating with the client.
+
+    // A message sent by the client to register as a consumer of this service.
+    static final int MSG_REGISTER_CLIENT = 1;
+    // A message sent by the client to decode an image.
+    static final int MSG_DECODE_IMAGE = 2;
+    // A message sent by the server to notify the client of the results of the decoding.
+    static final int MSG_IMAGE_DECODED_REPLY = 3;
+
+    // The keys for the bundle when passing data to and from this service.
+    static final String KEY_FILE_DESCRIPTOR = "file_descriptor";
+    static final String KEY_FILE_PATH = "file_path";
+    static final String KEY_IMAGE_BITMAP = "image_bitmap";
+    static final String KEY_IMAGE_BYTE_COUNT = "image_byte_count";
+    static final String KEY_IMAGE_DESCRIPTOR = "image_descriptor";
+    static final String KEY_WIDTH = "width";

[Theresa, 2017/04/13 02:25:27]

Since this represents width and height, could it be called "size" to cover both?

[Finnur, 2017/04/18 17:21:14]

Done.

+    static final String KEY_SUCCESS = "success";
+
+    // A method for getFileDescriptor, obtained via Reflection. Can be null if not supported by

[Theresa, 2017/04/13 02:25:27]

We shouldn't be accessing methods that aren't publicly accessible. Is there a
way to do this without reflection?

[Finnur, 2017/04/18 17:21:14]

If by 'this' you mean 'obtain a FileDescriptor for a MemoryFile', then no -- I
don't know of another way. I'd love to be proven wrong, but I've researched this
a fair amount and not found a better answer. 

If by 'this' you mean 'transfer the data', then I'd say the answer is a bit
trickier. As I see it, there are two ways of getting the data across sandboxed
process boundaries. You can either pass a handle to a pre-existing copy of the
bits (the FileDescriptor) or pass the bits themselves.

I initially tried the latter, but found it wasteful and inefficient but the
worst part is that that the IPC mechanism wasn't designed to transfer large
amounts, such as bitmaps, as the data cap is very low. Bitmaps can easily brush
up against the limit causing failures in decoding. Yes, it's possible to divide
the image into smaller packets to avoid the transfer limit, but it is so much
simpler, easier and efficient to just send over FileDescriptors and eliminate
these extra copies as well as the errors caused by the transfer limits. 

Also, I should mention that getFileDescriptor is used on a code path to support
pre-M-devices, so eventually it can be phased out.

[Theresa, 2017/04/18 19:06:20]

By "this" I mean a solution that doesn't use reflection to access hidden
methods. Our policy is not to use reflection unless we have a clear path to
remove it at some point (e.g. after a support library roll or after a new
Android SDK becomes public).

We need a solution, or at least a fallback, that doesn't rely on
getFileDescriptor or createAshmemBitmap, since there's no guarantee that these
methods exist. I haven't dug into how these methods are being used, but I can
look more closely and try to recommend some alternatives if needed.

[Finnur, 2017/04/19 16:01:35]

I would appreciate it if you could recommend some alternatives. I'd like to
avoid copying the bits and would actually prefer to lobby the Android team to
make this function public rather than resorting to a method that introduces a
bitmap copy to get around it.

[Theresa, 2017/04/19 17:31:14]

This is my understanding of the current process, am I missing anything?

1) After receiving a decode request from the client, we're using a
ParcelFileDescriptor to decode a bitmap using a BitmapUtils method
2) On M+, creating an AshmemBitmap from the decoded bitmap and attaching it to
the bundle. On pre-M, we copy the bitmap bytes to a buffer, writing the buffer
bytes to a MemoryFile, creating a FileDescriptor then attaching the
FileDescriptor to the bundle.
3) Sending the bundle to the client

It's probably worthwhile to reach out to android chatty (ping me for the email
if you don't have it) to ask for best practices in sharing large bitmaps between
processes.

Stepping away from shared memory, when you were passing the bits themselves,
what process were you using? There are ways to compress the bytes
(Bitmap.compress()) which may help with the size.

+    // the Android OS.
+    private static final Method sMethodGetFileDescriptor;
+    static {
+        sMethodGetFileDescriptor = getMethod("getFileDescriptor");
+    }
+
+    // A method for createAshmemBitmap, obtained via Reflection. Can be null if not supported by
+    // the Android OS.
+    private static final Method sMethodCreateAshmemBitmap;
+    static {
+        sMethodCreateAshmemBitmap = getMethod("createAshmemBitmap");
+    }
+
+    /**
+     * A helper function to look up methods by name.
+     * @param name The name of the method to look up.
+     * @return The method, if found. Otherwise null.
+     */
+    private static Method getMethod(String name) {
+        try {
+            if (name.equals("getFileDescriptor")) {
+                return MemoryFile.class.getDeclaredMethod(name);
+            }
+            if (name.equals("createAshmemBitmap")) {
+                return Bitmap.class.getDeclaredMethod(name);
+            }
+            return null;
+        } catch (NoSuchMethodException e) {
+            if (name.equals("createAshmemBitmap")) {
+                return null; // Expected error on pre-M devices.
+            }
+
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * A helper function to obtain a FileDescriptor from a MemoryFile.
+     * @param file The MemoryFile to get a FileDescriptor for.
+     * @return The resulting FileDescriptor.
+     */
+    public static FileDescriptor getFileDescriptor(MemoryFile file) {
+        try {
+            return (FileDescriptor) sMethodGetFileDescriptor.invoke(file);
+        } catch (IllegalAccessException e) {
+            throw new RuntimeException(e);
+        } catch (InvocationTargetException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * A helper function to obtain an ashmemBitmap version of a regular |bitmap|.
+     * @param bitmap The bitmap to use.
+     * @return an ashmemBitmap.
+     */
+    public static Bitmap createAshmemBitmap(Bitmap bitmap) {
+        try {
+            return (Bitmap) sMethodCreateAshmemBitmap.invoke(bitmap);
+        } catch (IllegalAccessException e) {
+            throw new RuntimeException(e);
+        } catch (InvocationTargetException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+     * Handler for incoming messages from clients.
+     */
+    static class IncomingHandler extends Handler {
+        // The client we are communicating with.
+        private Messenger mClient;
+
+        @Override
+        public void handleMessage(Message msg) {
+            switch (msg.what) {
+                case MSG_REGISTER_CLIENT:

[Yusuf, 2017/04/13 19:05:25]

why do we need the two separate messages here? ie, why not do mClient =
msg.reply inside MSG_DECODE_IMAGE?

Note that they can still bind and not send anything, if we worry about
initialization taking a long time.

[Finnur, 2017/04/21 14:47:55]

Yeah, that's much better. Done.

+                    mClient = msg.replyTo;
+                    break;
+                case MSG_DECODE_IMAGE:
+                    Bundle bundle = new Bundle();
+                    try {
+                        Bundle payload = msg.getData();
+
+                        String filePath = payload.getString(KEY_FILE_PATH);
+                        ParcelFileDescriptor pfd = payload.getParcelable(KEY_FILE_DESCRIPTOR);
+                        int width = payload.getInt(KEY_WIDTH);
+
+                        // Setup a minimum viable response to parent process. Will be fleshed out
+                        // further below.
+                        bundle.putString(KEY_FILE_PATH, filePath);
+                        bundle.putInt(KEY_WIDTH, width);
+                        bundle.putBoolean(KEY_SUCCESS, false);
+
+                        FileDescriptor fd = pfd.getFileDescriptor();
+                        Bitmap bitmap = BitmapUtils.decodeBitmapFromFileDescriptor(fd, width);
+                        try {
+                            pfd.close();
+                        } catch (IOException e) {
+                            e.printStackTrace();

[Yusuf, 2017/04/13 19:05:25]

Log an error message with details if applicable, rather than a stack trace.

[Finnur, 2017/04/21 14:47:55]

Done.

+                        }
+
+                        MemoryFile imageFile = null;
+                        ParcelFileDescriptor imagePfd = null;
+
+                        // createAshmemBitmap was not available until Marshmallow.
+                        if (sMethodCreateAshmemBitmap != null) {
+                            Bitmap ashmemBitmap = createAshmemBitmap(bitmap);
+                            bitmap.recycle();
+                            bundle.putParcelable(KEY_IMAGE_BITMAP, ashmemBitmap);
+                            bundle.putBoolean(KEY_SUCCESS, true);
+                        } else {
+                            int byteCount = bitmap.getByteCount();
+
+                            ByteBuffer buffer = ByteBuffer.allocate(byteCount);
+                            bitmap.copyPixelsToBuffer(buffer);
+                            bitmap.recycle();
+                            buffer.rewind();
+
+                            try {
+                                imageFile = new MemoryFile(filePath, byteCount);
+                                imageFile.writeBytes(buffer.array(), 0, 0, byteCount);
+
+                                fd = getFileDescriptor(imageFile);
+                                imagePfd = ParcelFileDescriptor.dup(fd);
+                            } catch (IOException e) {
+                                e.printStackTrace();

[Yusuf, 2017/04/13 19:05:24]

see above.

[Finnur, 2017/04/21 14:47:55]

Done.

+                            }
+
+                            bundle.putParcelable(KEY_IMAGE_DESCRIPTOR, imagePfd);
+                            bundle.putInt(KEY_IMAGE_BYTE_COUNT, byteCount);
+                            bundle.putBoolean(KEY_SUCCESS, true);

[Theresa, 2017/04/13 02:25:27]

If there's an IOException, should we still be returning true here?

[Finnur, 2017/04/18 17:21:14]

Hmm... no, we should probably not do so...

+                        }
+
+                        try {
+                            Message reply = Message.obtain(null, MSG_IMAGE_DECODED_REPLY);
+                            reply.setData(bundle);
+                            mClient.send(reply);
+                            bundle = null;
+                        } catch (RemoteException e) {

[Yusuf, 2017/04/13 19:05:25]

RemoteException can be a DeadObject or a TransactionTooLarge so maybe not make
Jim worry for no good reason here.

How big are the bitmaps we are sending here? For your own sake, you might want
to have a separate TransactionTooLargeException catch here and record it just in
case you will be curious whether these bitmaps are actually arriving to the
other end.

so two catches, one sets the client to null the other just records, maybe?

[Finnur, 2017/04/21 14:47:55]

So... at the moment, we don't send _any_ bitmap data over the IPC channel. We
write to a memory file and send its FileDescriptor over for reading on the other
side.  I don't expect any TransactionTooLargeExceptions. DeadObject... maybe.
But I think logging and attempting to send a Sucess=false over is probably
sufficient.

+                            mClient = null; // He's dead, Jim.
+                        }
+
+                        if (imageFile != null) {

[Yusuf, 2017/04/13 19:05:25]

one line

[Finnur, 2017/04/21 14:47:55]

Done.

+                            imageFile.close();
+                        }
+                        try {
+                            if (imagePfd != null) {

[Yusuf, 2017/04/13 19:05:25]

one line

[Finnur, 2017/04/21 14:47:55]

Done.

+                                imagePfd.close();
+                            }
+                        } catch (IOException e) {

[Yusuf, 2017/04/13 19:05:24]

see above

[Finnur, 2017/04/21 14:47:55]

Done.

+                            e.printStackTrace();
+                        }
+                    } catch (Exception e) {

[Yusuf, 2017/04/13 19:05:25]

but why would it crash? (You have all these other try/catch blocks inside that
should capture related exceptions) StrictMode?

[Finnur, 2017/04/21 14:47:55]

StrictMode? I don't know... :s

My thinking was that this is a sandboxed utility process decoding untrusted
content, so who knows what's going to happen? 

I'm willing to follow your guidance on how an unexpected error should be handled
if/when it occurs. The code is not catching null pointer errors, for example.
Not sure what else. I'd rather gracefully fail to decode the image if there's an
unexpected error because it has two benefits:

1) I can notify the host so that it can delete the request and free up memory. 
2) It is a much more pleasant UX than letting the process crash (user getting
multiple "this process crashed" dialogs)

But I'm open to suggestions.

+                        // This service has no UI and maintains no state so if it crashes on
+                        // decoding a photo, it is better UX to eat the exception instead of showing
+                        // a crash dialog and discarding other requests that have already been sent.
+                        e.printStackTrace();
+
+                        if (bundle != null && mClient != null) {
+                            Message reply = Message.obtain(null, MSG_IMAGE_DECODED_REPLY);

[Yusuf, 2017/04/13 19:05:25]

add a private send message below and avoid duplication of the whole try/catch
here.

[Finnur, 2017/04/21 14:47:55]

Done.

+                            reply.setData(bundle);
+                            try {
+                                mClient.send(reply);
+                            } catch (RemoteException remoteException) {

[Yusuf, 2017/04/13 19:05:25]

see above

[Finnur, 2017/04/21 14:47:55]

Done.

+                                mClient = null; // He's dead, Jim.
+                            }
+                        }
+                    }
+                    break;
+                default:
+                    super.handleMessage(msg);
+            }
+        }
+    }
+
+    /**
+     * The target we publish for clients to send messages to IncomingHandler.
+     */
+    final Messenger mMessenger = new Messenger(new IncomingHandler());
+
+    /**
+     * When binding to the service, we return an interface to our messenger
+     * for sending messages to the service.
+     */
+    @Override
+    public IBinder onBind(Intent intent) {
+        return mMessenger.getBinder();
+    }
+}