OSDN Git Service

Really make Surface thread-safe.
authorJeff Brown <jeffbrown@google.com>
Tue, 30 Apr 2013 23:33:00 +0000 (16:33 -0700)
committerJeff Brown <jeffbrown@google.com>
Wed, 1 May 2013 22:28:01 +0000 (15:28 -0700)
There were many places where the native object was being
accessed improperly.  Also some places where CloseGuard might
not be acquired or released correctly or where the generation
count might not be updated.

Fixed them all.

That said, Surface isn't intended to be used concurrently
so please don't do it.  This is only intended to make
hard to find crashes less likely.

Bug: 8328715
Change-Id: I981ef33425823e0fd7ad6b64443f2ec9b0c8335e

core/java/android/view/Surface.java
core/java/android/view/SurfaceControl.java
core/jni/android_view_Surface.cpp

index 4989c3a..e0786f7 100644 (file)
@@ -34,19 +34,21 @@ public class Surface implements Parcelable {
 
     private static native int nativeCreateFromSurfaceTexture(SurfaceTexture surfaceTexture)
             throws OutOfResourcesException;
+    private static native int nativeCreateFromSurfaceControl(int surfaceControlNativeObject);
 
-    private native Canvas nativeLockCanvas(int nativeObject, Rect dirty);
-    private native void nativeUnlockCanvasAndPost(int nativeObject, Canvas canvas);
+    private static native void nativeLockCanvas(int nativeObject, Canvas canvas, Rect dirty)
+            throws OutOfResourcesException;
+    private static native void nativeUnlockCanvasAndPost(int nativeObject, Canvas canvas);
 
     private static native void nativeRelease(int nativeObject);
     private static native boolean nativeIsValid(int nativeObject);
     private static native boolean nativeIsConsumerRunningBehind(int nativeObject);
-    private static native int nativeCopyFrom(int nativeObject, int surfaceControlNativeObject);
     private static native int nativeReadFromParcel(int nativeObject, Parcel source);
     private static native void nativeWriteToParcel(int nativeObject, Parcel dest);
 
     public static final Parcelable.Creator<Surface> CREATOR =
             new Parcelable.Creator<Surface>() {
+        @Override
         public Surface createFromParcel(Parcel source) {
             try {
                 Surface s = new Surface();
@@ -57,26 +59,20 @@ public class Surface implements Parcelable {
                 return null;
             }
         }
+
+        @Override
         public Surface[] newArray(int size) {
             return new Surface[size];
         }
     };
 
     private final CloseGuard mCloseGuard = CloseGuard.get();
-    private String mName;
 
-    // Note: These fields are accessed by native code.
-    // The mSurfaceControl will only be present for Surfaces used by the window
-    // server or system processes. When this class is parceled we defer to the
-    // mSurfaceControl to do the parceling. Otherwise we parcel the
-    // mNativeSurface.
+    // Guarded state.
+    final Object mLock = new Object(); // protects the native state
+    private String mName;
     int mNativeObject; // package scope only for SurfaceControl access
-
-    // protects the native state
-    private final Object mNativeObjectLock = new Object();
-
-    private int mGenerationId; // incremented each time mNativeSurface changes
-    @SuppressWarnings("UnusedDeclaration")
+    private int mGenerationId; // incremented each time mNativeObject changes
     private final Canvas mCanvas = new CompatibleCanvas();
 
     // A matrix to scale the matrix set by application. This is set to null for
@@ -125,21 +121,22 @@ public class Surface implements Parcelable {
             throw new IllegalArgumentException("surfaceTexture must not be null");
         }
 
-        mName = surfaceTexture.toString();
-        try {
-            mNativeObject = nativeCreateFromSurfaceTexture(surfaceTexture);
-        } catch (OutOfResourcesException ex) {
-            // We can't throw OutOfResourcesException because it would be an API change.
-            throw new RuntimeException(ex);
+        synchronized (mLock) {
+            mName = surfaceTexture.toString();
+            try {
+                setNativeObjectLocked(nativeCreateFromSurfaceTexture(surfaceTexture));
+            } catch (OutOfResourcesException ex) {
+                // We can't throw OutOfResourcesException because it would be an API change.
+                throw new RuntimeException(ex);
+            }
         }
-
-        mCloseGuard.open("release");
     }
 
     /* called from android_view_Surface_createFromIGraphicBufferProducer() */
     private Surface(int nativeObject) {
-        mNativeObject = nativeObject;
-        mCloseGuard.open("release");
+        synchronized (mLock) {
+            setNativeObjectLocked(nativeObject);
+        }
     }
 
     @Override
@@ -160,13 +157,11 @@ public class Surface implements Parcelable {
      * This will make the surface invalid.
      */
     public void release() {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             if (mNativeObject != 0) {
                 nativeRelease(mNativeObject);
-                mNativeObject = 0;
-                mGenerationId++;
+                setNativeObjectLocked(0);
             }
-            mCloseGuard.close();
         }
     }
 
@@ -187,7 +182,7 @@ public class Surface implements Parcelable {
      * Otherwise returns false.
      */
     public boolean isValid() {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             if (mNativeObject == 0) return false;
             return nativeIsValid(mNativeObject);
         }
@@ -201,7 +196,9 @@ public class Surface implements Parcelable {
      * @hide
      */
     public int getGenerationId() {
-        return mGenerationId;
+        synchronized (mLock) {
+            return mGenerationId;
+        }
     }
 
     /**
@@ -211,7 +208,7 @@ public class Surface implements Parcelable {
      * @hide
      */
     public boolean isConsumerRunningBehind() {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             checkNotReleasedLocked();
             return nativeIsConsumerRunningBehind(mNativeObject);
         }
@@ -234,9 +231,10 @@ public class Surface implements Parcelable {
      */
     public Canvas lockCanvas(Rect inOutDirty)
             throws OutOfResourcesException, IllegalArgumentException {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             checkNotReleasedLocked();
-            return nativeLockCanvas(mNativeObject, inOutDirty);
+            nativeLockCanvas(mNativeObject, mCanvas, inOutDirty);
+            return mCanvas;
         }
     }
 
@@ -247,7 +245,12 @@ public class Surface implements Parcelable {
      * @param canvas The canvas previously obtained from {@link #lockCanvas}.
      */
     public void unlockCanvasAndPost(Canvas canvas) {
-        synchronized (mNativeObjectLock) {
+        if (canvas != mCanvas) {
+            throw new IllegalArgumentException("canvas object must be the same instance that "
+                    + "was previously returned by lockCanvas");
+        }
+
+        synchronized (mLock) {
             checkNotReleasedLocked();
             nativeUnlockCanvasAndPost(mNativeObject, canvas);
         }
@@ -273,7 +276,6 @@ public class Surface implements Parcelable {
         }
     }
 
-
     /**
      * Copy another surface to this one.  This surface now holds a reference
      * to the same data as the original surface, and is -not- the owner.
@@ -287,22 +289,24 @@ public class Surface implements Parcelable {
         if (other == null) {
             throw new IllegalArgumentException("other must not be null");
         }
-        if (other.mNativeObject == 0) {
+
+        int surfaceControlPtr = other.mNativeObject;
+        if (surfaceControlPtr == 0) {
             throw new NullPointerException(
                     "SurfaceControl native object is null. Are you using a released SurfaceControl?");
         }
-        synchronized (mNativeObjectLock) {
-            mNativeObject = nativeCopyFrom(mNativeObject, other.mNativeObject);
-            if (mNativeObject == 0) {
-                // nativeCopyFrom released our reference
-                mCloseGuard.close();
+        int newNativeObject = nativeCreateFromSurfaceControl(surfaceControlPtr);
+
+        synchronized (mLock) {
+            if (mNativeObject != 0) {
+                nativeRelease(mNativeObject);
             }
-            mGenerationId++;
+            setNativeObjectLocked(newNativeObject);
         }
     }
 
     /**
-     * This is intended to be used by {@link SurfaceView.updateWindow} only.
+     * This is intended to be used by {@link SurfaceView#updateWindow} only.
      * @param other access is not thread safe
      * @hide
      * @deprecated
@@ -313,21 +317,18 @@ public class Surface implements Parcelable {
             throw new IllegalArgumentException("other must not be null");
         }
         if (other != this) {
-            synchronized (mNativeObjectLock) {
+            final int newPtr;
+            synchronized (other.mLock) {
+                newPtr = other.mNativeObject;
+                other.setNativeObjectLocked(0);
+            }
+
+            synchronized (mLock) {
                 if (mNativeObject != 0) {
-                    // release our reference to our native object
                     nativeRelease(mNativeObject);
                 }
-                // transfer the reference from other to us
-                if (other.mNativeObject != 0 && mNativeObject == 0) {
-                    mCloseGuard.open("release");
-                }
-                mNativeObject = other.mNativeObject;
-                mGenerationId++;
+                setNativeObjectLocked(newPtr);
             }
-            other.mNativeObject = 0;
-            other.mGenerationId++;
-            other.mCloseGuard.close();
         }
     }
 
@@ -340,14 +341,10 @@ public class Surface implements Parcelable {
         if (source == null) {
             throw new IllegalArgumentException("source must not be null");
         }
-        synchronized (mNativeObjectLock) {
+
+        synchronized (mLock) {
             mName = source.readString();
-            int nativeObject = nativeReadFromParcel(mNativeObject, source);
-            if (nativeObject !=0 && mNativeObject == 0) {
-                mCloseGuard.open("release");
-            }
-            mNativeObject = nativeObject;
-            mGenerationId++;
+            setNativeObjectLocked(nativeReadFromParcel(mNativeObject, source));
         }
     }
 
@@ -356,7 +353,7 @@ public class Surface implements Parcelable {
         if (dest == null) {
             throw new IllegalArgumentException("dest must not be null");
         }
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             dest.writeString(mName);
             nativeWriteToParcel(mNativeObject, dest);
         }
@@ -367,7 +364,27 @@ public class Surface implements Parcelable {
 
     @Override
     public String toString() {
-        return "Surface(name=" + mName + ")";
+        synchronized (mLock) {
+            return "Surface(name=" + mName + ")";
+        }
+    }
+
+    private void setNativeObjectLocked(int ptr) {
+        if (mNativeObject != ptr) {
+            if (mNativeObject == 0 && ptr != 0) {
+                mCloseGuard.open("release");
+            } else if (mNativeObject != 0 && ptr == 0) {
+                mCloseGuard.close();
+            }
+            mNativeObject = ptr;
+            mGenerationId += 1;
+        }
+    }
+
+    private void checkNotReleasedLocked() {
+        if (mNativeObject == 0) {
+            throw new IllegalStateException("Surface has already been released.");
+        }
     }
 
     /**
@@ -451,9 +468,4 @@ public class Surface implements Parcelable {
             mOrigMatrix.set(m);
         }
     }
-
-    private void checkNotReleasedLocked() {
-        if (mNativeObject == 0) throw new NullPointerException(
-                "mNativeObject is null. Have you called release() already?");
-    }
 }
index e869d09..6b530ef 100644 (file)
@@ -496,8 +496,14 @@ public class SurfaceControl {
         if (displayToken == null) {
             throw new IllegalArgumentException("displayToken must not be null");
         }
-        int nativeSurface = surface != null ? surface.mNativeObject : 0;
-        nativeSetDisplaySurface(displayToken, nativeSurface);
+
+        if (surface != null) {
+            synchronized (surface.mLock) {
+                nativeSetDisplaySurface(displayToken, surface.mNativeObject);
+            }
+        } else {
+            nativeSetDisplaySurface(displayToken, 0);
+        }
     }
 
     public static IBinder createDisplay(String name, boolean secure) {
index a41a389..9a19ce5 100644 (file)
@@ -55,8 +55,7 @@ static const char* const OutOfResourcesException =
 static struct {
     jclass clazz;
     jfieldID mNativeObject;
-    jfieldID mNativeObjectLock;
-    jfieldID mCanvas;
+    jfieldID mLock;
     jmethodID ctor;
 } gSurfaceClassInfo;
 
@@ -93,7 +92,7 @@ sp<ANativeWindow> android_view_Surface_getNativeWindow(JNIEnv* env, jobject surf
 sp<Surface> android_view_Surface_getSurface(JNIEnv* env, jobject surfaceObj) {
     sp<Surface> sur;
     jobject lock = env->GetObjectField(surfaceObj,
-            gSurfaceClassInfo.mNativeObjectLock);
+            gSurfaceClassInfo.mLock);
     if (env->MonitorEnter(lock) == JNI_OK) {
         sur = reinterpret_cast<Surface *>(
                 env->GetIntField(surfaceObj, gSurfaceClassInfo.mNativeObject));
@@ -200,12 +199,13 @@ static inline void swapCanvasPtr(JNIEnv* env, jobject canvasObj, SkCanvas* newCa
   SkSafeUnref(previousCanvas);
 }
 
-static jobject nativeLockCanvas(JNIEnv* env, jobject surfaceObj, jint nativeObject, jobject dirtyRectObj) {
+static void nativeLockCanvas(JNIEnv* env, jclass clazz,
+        jint nativeObject, jobject canvasObj, jobject dirtyRectObj) {
     sp<Surface> surface(reinterpret_cast<Surface *>(nativeObject));
 
     if (!isSurfaceValid(surface)) {
         doThrowIAE(env);
-        return NULL;
+        return;
     }
 
     // get dirty region
@@ -232,11 +232,10 @@ static jobject nativeLockCanvas(JNIEnv* env, jobject surfaceObj, jint nativeObje
                 OutOfResourcesException :
                 "java/lang/IllegalArgumentException";
         jniThrowException(env, exception, NULL);
-        return NULL;
+        return;
     }
 
     // Associate a SkCanvas object to this surface
-    jobject canvasObj = env->GetObjectField(surfaceObj, gSurfaceClassInfo.mCanvas);
     env->SetIntField(canvasObj, gCanvasClassInfo.mSurfaceFormat, outBuffer.format);
 
     SkBitmap bitmap;
@@ -277,17 +276,10 @@ static jobject nativeLockCanvas(JNIEnv* env, jobject surfaceObj, jint nativeObje
         env->SetIntField(dirtyRectObj, gRectClassInfo.right, bounds.right);
         env->SetIntField(dirtyRectObj, gRectClassInfo.bottom, bounds.bottom);
     }
-
-    return canvasObj;
 }
 
-static void nativeUnlockCanvasAndPost(JNIEnv* env, jobject surfaceObj, jint nativeObject, jobject canvasObj) {
-    jobject ownCanvasObj = env->GetObjectField(surfaceObj, gSurfaceClassInfo.mCanvas);
-    if (!env->IsSameObject(ownCanvasObj, canvasObj)) {
-        doThrowIAE(env);
-        return;
-    }
-
+static void nativeUnlockCanvasAndPost(JNIEnv* env, jclass clazz,
+        jint nativeObject, jobject canvasObj) {
     sp<Surface> surface(reinterpret_cast<Surface *>(nativeObject));
     if (!isSurfaceValid(surface)) {
         return;
@@ -306,8 +298,8 @@ static void nativeUnlockCanvasAndPost(JNIEnv* env, jobject surfaceObj, jint nati
 
 // ----------------------------------------------------------------------------
 
-static jint nativeCopyFrom(JNIEnv* env, jclass clazz,
-        jint nativeObject, jint surfaceControlNativeObj) {
+static jint nativeCreateFromSurfaceControl(JNIEnv* env, jclass clazz,
+        jint surfaceControlNativeObj) {
     /*
      * This is used by the WindowManagerService just after constructing
      * a Surface and is necessary for returning the Surface reference to
@@ -315,17 +307,11 @@ static jint nativeCopyFrom(JNIEnv* env, jclass clazz,
      */
 
     sp<SurfaceControl> ctrl(reinterpret_cast<SurfaceControl *>(surfaceControlNativeObj));
-    sp<Surface> other(ctrl->getSurface());
-    if (other != NULL) {
-        other->incStrong(&sRefBaseOwner);
-    }
-
-    sp<Surface> sur(reinterpret_cast<Surface *>(nativeObject));
-    if (sur != NULL) {
-        sur->decStrong(&sRefBaseOwner);
+    sp<Surface> surface(ctrl->getSurface());
+    if (surface != NULL) {
+        surface->incStrong(&sRefBaseOwner);
     }
-
-    return int(other.get());
+    return reinterpret_cast<jint>(surface.get());
 }
 
 static jint nativeReadFromParcel(JNIEnv* env, jclass clazz,
@@ -386,12 +372,12 @@ static JNINativeMethod gSurfaceMethods[] = {
             (void*)nativeIsValid },
     {"nativeIsConsumerRunningBehind", "(I)Z",
             (void*)nativeIsConsumerRunningBehind },
-    {"nativeLockCanvas", "(ILandroid/graphics/Rect;)Landroid/graphics/Canvas;",
+    {"nativeLockCanvas", "(ILandroid/graphics/Canvas;Landroid/graphics/Rect;)V",
             (void*)nativeLockCanvas },
     {"nativeUnlockCanvasAndPost", "(ILandroid/graphics/Canvas;)V",
             (void*)nativeUnlockCanvasAndPost },
-    {"nativeCopyFrom", "(II)I",
-            (void*)nativeCopyFrom },
+    {"nativeCreateFromSurfaceControl", "(I)I",
+            (void*)nativeCreateFromSurfaceControl },
     {"nativeReadFromParcel", "(ILandroid/os/Parcel;)I",
             (void*)nativeReadFromParcel },
     {"nativeWriteToParcel", "(ILandroid/os/Parcel;)V",
@@ -407,10 +393,8 @@ int register_android_view_Surface(JNIEnv* env)
     gSurfaceClassInfo.clazz = jclass(env->NewGlobalRef(clazz));
     gSurfaceClassInfo.mNativeObject =
             env->GetFieldID(gSurfaceClassInfo.clazz, "mNativeObject", "I");
-    gSurfaceClassInfo.mNativeObjectLock =
-            env->GetFieldID(gSurfaceClassInfo.clazz, "mNativeObjectLock", "Ljava/lang/Object;");
-    gSurfaceClassInfo.mCanvas =
-            env->GetFieldID(gSurfaceClassInfo.clazz, "mCanvas", "Landroid/graphics/Canvas;");
+    gSurfaceClassInfo.mLock =
+            env->GetFieldID(gSurfaceClassInfo.clazz, "mLock", "Ljava/lang/Object;");
     gSurfaceClassInfo.ctor = env->GetMethodID(gSurfaceClassInfo.clazz, "<init>", "(I)V");
 
     clazz = env->FindClass("android/graphics/Canvas");