From 176412a830076ed42da935fd206b2a72e5ae563c Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Thu, 6 Feb 2020 16:41:07 -0800 Subject: [PATCH] SurfaceControl: C++ Binding Lifetime refactoring First we eliminate the "dropReferenceTransaction" semantic. This semantic reparents the surface to null if the C++ object dies before release() is called. This is a legacy semantic from before SurfaceControls were reference counted. I point that it's unused by noting that all Java code paths will lead to calling release() in the JNI code before dropping the last reference. With dropReferenceTransaction gone we can remove mOwned it has no further uses. With these gone we now remove release() all together on the native side. This means that mClient and mHandle will only be written from the constructor and destructor making access to them thread-safe as long as you hold an sp<> to the SurfaceControl. This should prevent bugs like we've had in the past about who calls release when, no one calls it! The final question is: is removing the call to release on the Java side safe? We still need an explicit Java binding release call so we can drop the native reference in a timely fashion. This then breaks down in to two scenarios: 1. We are the last reference 2. Someone else holds a reference If we are in the first scenario, then calling release or not is equivalent to just dropping the reference. If we are in the second scenario, calling release() will be unsafe. Because we could at any time overwrite mClient/mHandle after the other ref holder had verified it was null. The main path I know of for how native code could acquire a second reference to the JNI owned SurfaceControl is via Transaction::registerSurfaceControlForCallback then if we release while Transaction::writeToParcel is running, it will inevitably segfault. This change could lead to the extension of life-time for SurfaceControl.cpp objects while the Transaction containing them is alive (but previously the SurfaceControl.cpp proxy would have been released). I also argue this is safe since the sp itself was reffed in another place in the Transaction so the lifetime of the actual server side resource isn't extended at all. Only the lightweight proxy object. Bug: 149055469 Bug: 149315421 Test: Existing tests pass. Change-Id: Ibd4d1804ef18a9c389c7f9112d15872cfe44b22e --- core/jni/android_view_SurfaceControl.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/core/jni/android_view_SurfaceControl.cpp b/core/jni/android_view_SurfaceControl.cpp index f564d75588328..14204cc962359 100644 --- a/core/jni/android_view_SurfaceControl.cpp +++ b/core/jni/android_view_SurfaceControl.cpp @@ -235,7 +235,6 @@ static jlong nativeCreate(JNIEnv* env, jclass clazz, jobject sessionObj, static void nativeRelease(JNIEnv* env, jclass clazz, jlong nativeObject) { sp ctrl(reinterpret_cast(nativeObject)); - ctrl->release(); ctrl->decStrong((void *)nativeCreate); }