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 files changed, 2 insertions, 15 deletions

diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp
index 6292388ac3..a332a1f2a8 100644
--- a/libs/gui/SurfaceControl.cpp
+++ b/libs/gui/SurfaceControl.cpp
@@ -46,34 +46,22 @@ namespace android {
 // ============================================================================
 
 SurfaceControl::SurfaceControl(const sp<SurfaceComposerClient>& client, const sp<IBinder>& handle,
-                               const sp<IGraphicBufferProducer>& gbp, bool owned,
+                               const sp<IGraphicBufferProducer>& gbp,
                                uint32_t transform)
       : mClient(client),
         mHandle(handle),
         mGraphicBufferProducer(gbp),
-        mOwned(owned),
         mTransformHint(transform) {}
 
 SurfaceControl::SurfaceControl(const sp<SurfaceControl>& other) {
     mClient = other->mClient;
     mHandle = other->mHandle;
     mGraphicBufferProducer = other->mGraphicBufferProducer;
-    mOwned = false;
     mTransformHint = other->mTransformHint;
 }
 
 SurfaceControl::~SurfaceControl()
 {
-    // Avoid reparenting the server-side surface to null if we are not the owner of it,
-    // meaning that we retrieved it from another process.
-    if (mHandle != nullptr && mOwned) {
-        SurfaceComposerClient::doDropReferenceTransaction(mHandle);
-    }
-    release();
-}
-
-void SurfaceControl::release()
-{
     // Trigger an IPC now, to make sure things
     // happen without delay, since these resources are quite heavy.
     mClient.clear();
@@ -157,7 +145,6 @@ sp<Surface> SurfaceControl::createSurface() const
 
 sp<IBinder> SurfaceControl::getHandle() const
 {
-    Mutex::Autolock lock(mLock);
     return mHandle;
 }
 
@@ -206,7 +193,7 @@ sp<SurfaceControl> SurfaceControl::readFromParcel(const Parcel* parcel) {
     return new SurfaceControl(new SurfaceComposerClient(
                                       interface_cast<ISurfaceComposerClient>(client)),
                               handle.get(), interface_cast<IGraphicBufferProducer>(gbp),
-                              false /* owned */, transformHint);
+                               transformHint);
 }
 
 // ----------------------------------------------------------------------------