ImageReader: Fix rogue RuntimeException in #detachImage

ImageReader#detach documents that an IllegalStateException will be thrown
if there are any errors detaching Image from a Surface. As ImageReaders
are set up, it is common for this exception to be thrown. Applications
are expected to catch and recover from the exception. However,
ImageReader#nativeDetachImage, that ImageReader#detachImage calls into,
explicitly threw a RuntimeException causing applications that didn't
expect RuntimeException to crash.

This behavior has stuck around for a few years, so changing the behavior
to match the documentation will break backwards compatibility for many
apps. However, RuntimeException is an incredibly generic exception that
applications shouldn't be forced to catch as it can hide more serious
bugs.

This CL updates #nativeDetachImage to accept an additional flag.
When this flag is set to True, #nativeDetachImage only throws
IllegalStateException and preserves previous behavior when the flag is
set to False.

The flag is populated through the App Compatibility Framework which
changes the behavior only for apps that set their targetSdk to > 33.

Bug: 236825255
Bug: 204438677
Test: Manually tested that the flag is set to true for Apps targetting
      Android SDK > 33
Change-Id: I20bd986f11dbe7acf4898cf0ce794c27f42e1ee2

2 files changed, 61 insertions, 13 deletions

diff --git a/media/java/android/media/ImageReader.java b/media/java/android/media/ImageReader.java
index 472586b5e519..fde7afd21420 100644
--- a/media/java/android/media/ImageReader.java
+++ b/media/java/android/media/ImageReader.java
@@ -18,7 +18,11 @@ package android.media;
 
 import android.annotation.IntRange;
 import android.annotation.NonNull;
+import android.annotation.Nullable;
 import android.annotation.SuppressLint;
+import android.app.compat.CompatChanges;
+import android.compat.annotation.ChangeId;
+import android.compat.annotation.EnabledAfter;
 import android.graphics.GraphicBuffer;
 import android.graphics.ImageFormat;
 import android.graphics.ImageFormat.Format;
@@ -29,6 +33,7 @@ import android.hardware.HardwareBuffer;
 import android.hardware.HardwareBuffer.Usage;
 import android.hardware.SyncFence;
 import android.hardware.camera2.MultiResolutionImageReader;
+import android.os.Build;
 import android.os.Handler;
 import android.os.Looper;
 import android.os.ParcelFileDescriptor;
@@ -87,6 +92,38 @@ public class ImageReader implements AutoCloseable {
 
     /**
      * <p>
+     * Flag to gate correct exception thrown by {@code #detachImage}.
+     * </p>
+     * <p>
+     * {@code #detachImage} is documented as throwing {@link java.lang.IllegalStateException} in
+     * the event of an error; a native helper method to this threw
+     * {@link java.lang.RuntimeException} if the surface was abandoned while detaching the
+     * {@code Image}.
+     * <p>
+     * This previously undocumented exception behavior continues through Android T.
+     * </p>
+     * <p>
+     * After Android T, the native helper method only throws {@code IllegalStateExceptions} in
+     * accordance with the documentation.
+     * </p>
+     * <p>
+     * {@code #detachImage} will now throw only ISEs if it runs into errors while detaching
+     * the image. Behavior on apps targeting API levels <= T remains unchanged.
+     * </p>
+     */
+    @ChangeId
+    @EnabledAfter(targetSdkVersion = Build.VERSION_CODES.TIRAMISU)
+    private static final long DETACH_THROWS_ISE_ONLY = 236825255L;
+
+    /**
+     * Cached value of {@link #DETACH_THROWS_ISE_ONLY} flag to prevent repeated calls when
+     * detaching image.
+     */
+    private final boolean mDetachThrowsIseOnly =
+            CompatChanges.isChangeEnabled(DETACH_THROWS_ISE_ONLY);
+
+    /**
+     * <p>
      * Create a new reader for images of the desired size and format.
      * </p>
      * <p>
@@ -825,10 +862,10 @@ public class ImageReader implements AutoCloseable {
      * </p>
      * <p>
      * After this call, the ImageReader no longer owns this image, and the image
-     * ownership can be transfered to another entity like {@link ImageWriter}
+     * ownership can be transferred to another entity like {@link ImageWriter}
      * via {@link ImageWriter#queueInputImage}. It's up to the new owner to
      * release the resources held by this image. For example, if the ownership
-     * of this image is transfered to an {@link ImageWriter}, the image will be
+     * of this image is transferred to an {@link ImageWriter}, the image will be
      * freed by the ImageWriter after the image data consumption is done.
      * </p>
      * <p>
@@ -849,16 +886,22 @@ public class ImageReader implements AutoCloseable {
      * @throws IllegalStateException If the ImageReader or image have been
      *             closed, or the has been detached, or has not yet been
      *             acquired.
+     * @throws RuntimeException If there is an error detaching {@code Image} from {@code Surface}.
+     *              {@code RuntimeException} is only thrown for applications targeting SDK <=
+     *              {@link android.os.Build.VERSION_CODES#TIRAMISU}.
+     *              For applications targeting SDK >
+     *              {@link android.os.Build.VERSION_CODES#TIRAMISU},
+     *              this method only throws {@code IllegalStateException}.
      * @hide
      */
-     public void detachImage(Image image) {
-       if (image == null) {
+    public void detachImage(@Nullable Image image) {
+        if (image == null) {
            throw new IllegalArgumentException("input image must not be null");
-       }
-       if (!isImageOwnedbyMe(image)) {
+        }
+        if (!isImageOwnedbyMe(image)) {
            throw new IllegalArgumentException("Trying to detach an image that is not owned by"
                    + " this ImageReader");
-       }
+        }
 
         SurfaceImage si = (SurfaceImage) image;
         si.throwISEIfImageIsInvalid();
@@ -867,7 +910,7 @@ public class ImageReader implements AutoCloseable {
             throw new IllegalStateException("Image was already detached from this ImageReader");
         }
 
-        nativeDetachImage(image);
+        nativeDetachImage(image, mDetachThrowsIseOnly);
         si.clearSurfacePlanes();
         si.mPlanes = null;
         si.setDetached(true);
@@ -1408,7 +1451,7 @@ public class ImageReader implements AutoCloseable {
     private synchronized native void nativeClose();
     private synchronized native void nativeReleaseImage(Image i);
     private synchronized native Surface nativeGetSurface();
-    private synchronized native int nativeDetachImage(Image i);
+    private synchronized native int nativeDetachImage(Image i, boolean throwISEOnly);
     private synchronized native void nativeDiscardFreeBuffers();
 
     /**
diff --git a/media/jni/android_media_ImageReader.cpp b/media/jni/android_media_ImageReader.cpp
index 62c0d55951af..556f98cefca1 100644
--- a/media/jni/android_media_ImageReader.cpp
+++ b/media/jni/android_media_ImageReader.cpp
@@ -643,7 +643,8 @@ static jint ImageReader_imageSetup(JNIEnv* env, jobject thiz, jobject image,
     return ACQUIRE_SUCCESS;
 }
 
-static jint ImageReader_detachImage(JNIEnv* env, jobject thiz, jobject image) {
+static jint ImageReader_detachImage(JNIEnv* env, jobject thiz, jobject image,
+                                    jboolean throwISEOnly) {
     ALOGV("%s:", __FUNCTION__);
     JNIImageReaderContext* ctx = ImageReader_getContext(env, thiz);
     if (ctx == NULL) {
@@ -666,8 +667,12 @@ static jint ImageReader_detachImage(JNIEnv* env, jobject thiz, jobject image) {
     res = bufferConsumer->detachBuffer(buffer->mSlot);
     if (res != OK) {
         ALOGE("Image detach failed: %s (%d)!!!", strerror(-res), res);
-        jniThrowRuntimeException(env,
-                "nativeDetachImage failed for image!!!");
+        if ((bool) throwISEOnly) {
+            jniThrowException(env, "java/lang/IllegalStateException",
+                    "nativeDetachImage failed for image!!!");
+        } else {
+             jniThrowRuntimeException(env, "nativeDetachImage failed for image!!!");
+        }
         return res;
     }
     return OK;
@@ -965,7 +970,7 @@ static const JNINativeMethod gImageReaderMethods[] = {
     {"nativeReleaseImage",     "(Landroid/media/Image;)V",   (void*)ImageReader_imageRelease },
     {"nativeImageSetup",       "(Landroid/media/Image;Z)I",   (void*)ImageReader_imageSetup },
     {"nativeGetSurface",       "()Landroid/view/Surface;",   (void*)ImageReader_getSurface },
-    {"nativeDetachImage",      "(Landroid/media/Image;)I",   (void*)ImageReader_detachImage },
+    {"nativeDetachImage",      "(Landroid/media/Image;Z)I",   (void*)ImageReader_detachImage },
     {"nativeCreateImagePlanes",
         "(ILandroid/graphics/GraphicBuffer;IIIIII)[Landroid/media/ImageReader$ImagePlane;",
                                                              (void*)ImageReader_createImagePlanes },