Avoid crashing in unlockCanvasAndPost

It's possible to update the native surface pointer while the
surface is locked (via lockCanvas).  This leads to a surprise when
the surface is unlocked.  Avoid the surprise by tracking the
locked surface separately.

Bug 10289713

Change-Id: I84346c952be859bbd91ceae7df07b91dabe0948e

2 files changed, 40 insertions, 10 deletions

diff --git a/core/java/android/view/Surface.java b/core/java/android/view/Surface.java
index e0786f7..409db84 100644
--- a/core/java/android/view/Surface.java
+++ b/core/java/android/view/Surface.java
@@ -36,7 +36,7 @@ public class Surface implements Parcelable {
             throws OutOfResourcesException;
     private static native int nativeCreateFromSurfaceControl(int surfaceControlNativeObject);
 
-    private static native void nativeLockCanvas(int nativeObject, Canvas canvas, Rect dirty)
+    private static native int nativeLockCanvas(int nativeObject, Canvas canvas, Rect dirty)
             throws OutOfResourcesException;
     private static native void nativeUnlockCanvasAndPost(int nativeObject, Canvas canvas);
 
@@ -72,6 +72,7 @@ public class Surface implements Parcelable {
     final Object mLock = new Object(); // protects the native state
     private String mName;
     int mNativeObject; // package scope only for SurfaceControl access
+    private int mLockedObject;
     private int mGenerationId; // incremented each time mNativeObject changes
     private final Canvas mCanvas = new CompatibleCanvas();
 
@@ -233,7 +234,14 @@ public class Surface implements Parcelable {
             throws OutOfResourcesException, IllegalArgumentException {
         synchronized (mLock) {
             checkNotReleasedLocked();
-            nativeLockCanvas(mNativeObject, mCanvas, inOutDirty);
+            if (mLockedObject != 0) {
+                // Ideally, nativeLockCanvas() would throw in this situation and prevent the
+                // double-lock, but that won't happen if mNativeObject was updated.  We can't
+                // abandon the old mLockedObject because it might still be in use, so instead
+                // we just refuse to re-lock the Surface.
+                throw new RuntimeException("Surface was already locked");
+            }
+            mLockedObject = nativeLockCanvas(mNativeObject, mCanvas, inOutDirty);
             return mCanvas;
         }
     }
@@ -252,11 +260,21 @@ public class Surface implements Parcelable {
 
         synchronized (mLock) {
             checkNotReleasedLocked();
-            nativeUnlockCanvasAndPost(mNativeObject, canvas);
+            if (mNativeObject != mLockedObject) {
+                Log.w(TAG, "WARNING: Surface's mNativeObject (0x" +
+                        Integer.toHexString(mNativeObject) + ") != mLockedObject (0x" +
+                        Integer.toHexString(mLockedObject) +")");
+            }
+            if (mLockedObject == 0) {
+                throw new RuntimeException("Surface was not locked");
+            }
+            nativeUnlockCanvasAndPost(mLockedObject, canvas);
+            nativeRelease(mLockedObject);
+            mLockedObject = 0;
         }
     }
 
-    /** 
+    /**
      * @deprecated This API has been removed and is not supported.  Do not use.
      */
     @Deprecated
@@ -343,6 +361,10 @@ public class Surface implements Parcelable {
         }
 
         synchronized (mLock) {
+            // nativeReadFromParcel() will either return mNativeObject, or
+            // create a new native Surface and return it after reducing
+            // the reference count on mNativeObject.  Either way, it is
+            // not necessary to call nativeRelease() here.
             mName = source.readString();
             setNativeObjectLocked(nativeReadFromParcel(mNativeObject, source));
         }
@@ -365,7 +387,8 @@ public class Surface implements Parcelable {
     @Override
     public String toString() {
         synchronized (mLock) {
-            return "Surface(name=" + mName + ")";
+            return "Surface(name=" + mName + ")/@0x" +
+                    Integer.toHexString(System.identityHashCode(this));
         }
     }
 
@@ -463,7 +486,7 @@ public class Surface implements Parcelable {
         public void getMatrix(Matrix m) {
             super.getMatrix(m);
             if (mOrigMatrix == null) {
-                mOrigMatrix = new Matrix(); 
+                mOrigMatrix = new Matrix();
             }
             mOrigMatrix.set(m);
         }
diff --git a/core/jni/android_view_Surface.cpp b/core/jni/android_view_Surface.cpp
index 304514b..3f54fd7 100644
--- a/core/jni/android_view_Surface.cpp
+++ b/core/jni/android_view_Surface.cpp
@@ -196,13 +196,13 @@ static inline void swapCanvasPtr(JNIEnv* env, jobject canvasObj, SkCanvas* newCa
   SkSafeUnref(previousCanvas);
 }
 
-static void nativeLockCanvas(JNIEnv* env, jclass clazz,
+static jint nativeLockCanvas(JNIEnv* env, jclass clazz,
         jint nativeObject, jobject canvasObj, jobject dirtyRectObj) {
     sp<Surface> surface(reinterpret_cast<Surface *>(nativeObject));
 
     if (!isSurfaceValid(surface)) {
         doThrowIAE(env);
-        return;
+        return 0;
     }
 
     Rect dirtyRect;
@@ -223,7 +223,7 @@ static void nativeLockCanvas(JNIEnv* env, jclass clazz,
                 OutOfResourcesException :
                 "java/lang/IllegalArgumentException";
         jniThrowException(env, exception, NULL);
-        return;
+        return 0;
     }
 
     // Associate a SkCanvas object to this surface
@@ -255,6 +255,13 @@ static void nativeLockCanvas(JNIEnv* env, jclass clazz,
         env->SetIntField(dirtyRectObj, gRectClassInfo.right,  dirtyRect.right);
         env->SetIntField(dirtyRectObj, gRectClassInfo.bottom, dirtyRect.bottom);
     }
+
+    // Create another reference to the surface and return it.  This reference
+    // should be passed to nativeUnlockCanvasAndPost in place of mNativeObject,
+    // because the latter could be replaced while the surface is locked.
+    sp<Surface> lockedSurface(surface);
+    lockedSurface->incStrong(&sRefBaseOwner);
+    return (int) lockedSurface.get();
 }
 
 static void nativeUnlockCanvasAndPost(JNIEnv* env, jclass clazz,
@@ -351,7 +358,7 @@ static JNINativeMethod gSurfaceMethods[] = {
             (void*)nativeIsValid },
     {"nativeIsConsumerRunningBehind", "(I)Z",
             (void*)nativeIsConsumerRunningBehind },
-    {"nativeLockCanvas", "(ILandroid/graphics/Canvas;Landroid/graphics/Rect;)V",
+    {"nativeLockCanvas", "(ILandroid/graphics/Canvas;Landroid/graphics/Rect;)I",
             (void*)nativeLockCanvas },
     {"nativeUnlockCanvasAndPost", "(ILandroid/graphics/Canvas;)V",
             (void*)nativeUnlockCanvasAndPost },