make Surface.java internal state thread-safe
it's still incorrect to use Surface from different
threads, however this shouldn't result to native crashes
anymore.
Bug: 8328715
Change-Id: I89ac5cc1218dc5aa0e35f8e6d4737879a442f0c6
diff --git a/core/java/android/view/Surface.java b/core/java/android/view/Surface.java
index 0492d29..edfef56 100644
--- a/core/java/android/view/Surface.java
+++ b/core/java/android/view/Surface.java
@@ -72,6 +72,9 @@
// mNativeSurface.
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 final Canvas mCanvas = new CompatibleCanvas();
@@ -157,12 +160,14 @@
* This will make the surface invalid.
*/
public void release() {
- if (mNativeObject != 0) {
- nativeRelease(mNativeObject);
- mNativeObject = 0;
- mGenerationId++;
+ synchronized (mNativeObjectLock) {
+ if (mNativeObject != 0) {
+ nativeRelease(mNativeObject);
+ mNativeObject = 0;
+ mGenerationId++;
+ }
+ mCloseGuard.close();
}
- mCloseGuard.close();
}
/**
@@ -182,8 +187,10 @@
* Otherwise returns false.
*/
public boolean isValid() {
- if (mNativeObject == 0) return false;
- return nativeIsValid(mNativeObject);
+ synchronized (mNativeObjectLock) {
+ if (mNativeObject == 0) return false;
+ return nativeIsValid(mNativeObject);
+ }
}
/**
@@ -204,8 +211,10 @@
* @hide
*/
public boolean isConsumerRunningBehind() {
- checkNotReleased();
- return nativeIsConsumerRunningBehind(mNativeObject);
+ synchronized (mNativeObjectLock) {
+ checkNotReleasedLocked();
+ return nativeIsConsumerRunningBehind(mNativeObject);
+ }
}
/**
@@ -225,8 +234,10 @@
*/
public Canvas lockCanvas(Rect inOutDirty)
throws OutOfResourcesException, IllegalArgumentException {
- checkNotReleased();
- return nativeLockCanvas(mNativeObject, inOutDirty);
+ synchronized (mNativeObjectLock) {
+ checkNotReleasedLocked();
+ return nativeLockCanvas(mNativeObject, inOutDirty);
+ }
}
/**
@@ -236,8 +247,10 @@
* @param canvas The canvas previously obtained from {@link #lockCanvas}.
*/
public void unlockCanvasAndPost(Canvas canvas) {
- checkNotReleased();
- nativeUnlockCanvasAndPost(mNativeObject, canvas);
+ synchronized (mNativeObjectLock) {
+ checkNotReleasedLocked();
+ nativeUnlockCanvasAndPost(mNativeObject, canvas);
+ }
}
/**
@@ -278,38 +291,40 @@
throw new NullPointerException(
"SurfaceControl native object is null. Are you using a released SurfaceControl?");
}
- mNativeObject = nativeCopyFrom(mNativeObject, other.mNativeObject);
- if (mNativeObject == 0) {
- // nativeCopyFrom released our reference
- mCloseGuard.close();
+ synchronized (mNativeObjectLock) {
+ mNativeObject = nativeCopyFrom(mNativeObject, other.mNativeObject);
+ if (mNativeObject == 0) {
+ // nativeCopyFrom released our reference
+ mCloseGuard.close();
+ }
+ mGenerationId++;
}
- mGenerationId++;
}
/**
- * Transfer the native state from 'other' to this surface, releasing it
- * from 'other'. This is for use in the client side for drawing into a
- * surface; not guaranteed to work on the window manager side.
- * This is for use by the client to move the underlying surface from
- * one Surface object to another, in particular in SurfaceFlinger.
- * @hide.
+ * This is intended to be used by {@link SurfaceView.updateWindow} only.
+ * @param other access is not thread safe
+ * @hide
+ * @deprecated
*/
+ @Deprecated
public void transferFrom(Surface other) {
if (other == null) {
throw new IllegalArgumentException("other must not be null");
}
if (other != this) {
- if (mNativeObject != 0) {
- // release our reference to our native object
- nativeRelease(mNativeObject);
+ synchronized (mNativeObjectLock) {
+ 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++;
}
- // transfer the reference from other to us
- if (other.mNativeObject != 0 && mNativeObject == 0) {
- mCloseGuard.open("release");
- }
- mNativeObject = other.mNativeObject;
- mGenerationId++;
-
other.mNativeObject = 0;
other.mGenerationId++;
other.mCloseGuard.close();
@@ -325,13 +340,15 @@
if (source == null) {
throw new IllegalArgumentException("source must not be null");
}
- mName = source.readString();
- int nativeObject = nativeReadFromParcel(mNativeObject, source);
- if (nativeObject !=0 && mNativeObject == 0) {
- mCloseGuard.open("release");
+ synchronized (mNativeObjectLock) {
+ mName = source.readString();
+ int nativeObject = nativeReadFromParcel(mNativeObject, source);
+ if (nativeObject !=0 && mNativeObject == 0) {
+ mCloseGuard.open("release");
+ }
+ mNativeObject = nativeObject;
+ mGenerationId++;
}
- mNativeObject = nativeObject;
- mGenerationId++;
}
@Override
@@ -339,8 +356,10 @@
if (dest == null) {
throw new IllegalArgumentException("dest must not be null");
}
- dest.writeString(mName);
- nativeWriteToParcel(mNativeObject, dest);
+ synchronized (mNativeObjectLock) {
+ dest.writeString(mName);
+ nativeWriteToParcel(mNativeObject, dest);
+ }
if ((flags & Parcelable.PARCELABLE_WRITE_RETURN_VALUE) != 0) {
release();
}
@@ -433,7 +452,7 @@
}
}
- private void checkNotReleased() {
+ private void checkNotReleasedLocked() {
if (mNativeObject == 0) throw new NullPointerException(
"mNativeObject is null. Have you called release() already?");
}
diff --git a/core/jni/android_view_Surface.cpp b/core/jni/android_view_Surface.cpp
index 4671282..a41a389 100644
--- a/core/jni/android_view_Surface.cpp
+++ b/core/jni/android_view_Surface.cpp
@@ -55,6 +55,7 @@
static struct {
jclass clazz;
jfieldID mNativeObject;
+ jfieldID mNativeObjectLock;
jfieldID mCanvas;
jmethodID ctor;
} gSurfaceClassInfo;
@@ -90,8 +91,15 @@
}
sp<Surface> android_view_Surface_getSurface(JNIEnv* env, jobject surfaceObj) {
- return reinterpret_cast<Surface *>(
- env->GetIntField(surfaceObj, gSurfaceClassInfo.mNativeObject));
+ sp<Surface> sur;
+ jobject lock = env->GetObjectField(surfaceObj,
+ gSurfaceClassInfo.mNativeObjectLock);
+ if (env->MonitorEnter(lock) == JNI_OK) {
+ sur = reinterpret_cast<Surface *>(
+ env->GetIntField(surfaceObj, gSurfaceClassInfo.mNativeObject));
+ env->MonitorExit(lock);
+ }
+ return sur;
}
jobject android_view_Surface_createFromIGraphicBufferProducer(JNIEnv* env,
@@ -399,6 +407,8 @@
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.ctor = env->GetMethodID(gSurfaceClassInfo.clazz, "<init>", "(I)V");