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<IBinder> 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
Loading
Please register or sign in to comment