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
1 file changed