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
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 7017b7c..ff8b719 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -526,21 +526,6 @@
mDesiredPresentTime = -1;
}
-void SurfaceComposerClient::doDropReferenceTransaction(const sp<IBinder>& handle) {
- sp<ISurfaceComposer> sf(ComposerService::getComposerService());
- Vector<ComposerState> composerStates;
- Vector<DisplayState> displayStates;
-
- ComposerState s;
- s.state.surface = handle;
- s.state.what |= layer_state_t::eReparent;
- s.state.parentHandleForChild = nullptr;
-
- composerStates.add(s);
- sp<IBinder> applyToken = IInterface::asBinder(TransactionCompletedListener::getIInstance());
- sf->setTransactionState(composerStates, displayStates, 0, applyToken, {}, -1, {}, false, {});
-}
-
void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) {
sp<ISurfaceComposer> sf(ComposerService::getComposerService());
@@ -1558,7 +1543,7 @@
}
ALOGE_IF(err, "SurfaceComposerClient::createWithSurfaceParent error %s", strerror(-err));
if (err == NO_ERROR) {
- return new SurfaceControl(this, handle, gbp, true /* owned */, transformHint);
+ return new SurfaceControl(this, handle, gbp, transformHint);
}
}
return nullptr;
@@ -1589,7 +1574,7 @@
}
ALOGE_IF(err, "SurfaceComposerClient::createSurface error %s", strerror(-err));
if (err == NO_ERROR) {
- *outSurface = new SurfaceControl(this, handle, gbp, true /* owned */, transformHint);
+ *outSurface = new SurfaceControl(this, handle, gbp, transformHint);
}
}
return err;
diff --git a/libs/gui/SurfaceControl.cpp b/libs/gui/SurfaceControl.cpp
index 6292388..a332a1f 100644
--- a/libs/gui/SurfaceControl.cpp
+++ b/libs/gui/SurfaceControl.cpp
@@ -46,34 +46,22 @@
// ============================================================================
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<IBinder> SurfaceControl::getHandle() const
{
- Mutex::Autolock lock(mLock);
return mHandle;
}
@@ -206,7 +193,7 @@
return new SurfaceControl(new SurfaceComposerClient(
interface_cast<ISurfaceComposerClient>(client)),
handle.get(), interface_cast<IGraphicBufferProducer>(gbp),
- false /* owned */, transformHint);
+ transformHint);
}
// ----------------------------------------------------------------------------
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index d0bb6a3..27877bb 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -184,12 +184,6 @@
static bool getProtectedContentSupport();
/**
- * Called from SurfaceControl d'tor to 'destroy' the surface (or rather, reparent it
- * to null), but without needing an sp<SurfaceControl> to avoid infinite ressurection.
- */
- static void doDropReferenceTransaction(const sp<IBinder>& handle);
-
- /**
* Uncaches a buffer in ISurfaceComposer. It must be uncached via a transaction so that it is
* in order with other transactions that use buffers.
*/
diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h
index 7bc7c68..ac2bbcc 100644
--- a/libs/gui/include/gui/SurfaceControl.h
+++ b/libs/gui/include/gui/SurfaceControl.h
@@ -58,10 +58,6 @@
static bool isSameSurface(
const sp<SurfaceControl>& lhs, const sp<SurfaceControl>& rhs);
- // Release the handles assosciated with the SurfaceControl, without reparenting
- // them off-screen. At the moment if this isn't executed before ~SurfaceControl
- // is called then the destructor will reparent the layer off-screen for you.
- void release();
// Reparent off-screen and release. This is invoked by the destructor.
void destroy();
@@ -89,7 +85,7 @@
explicit SurfaceControl(const sp<SurfaceControl>& other);
SurfaceControl(const sp<SurfaceComposerClient>& client, const sp<IBinder>& handle,
- const sp<IGraphicBufferProducer>& gbp, bool owned, uint32_t transformHint = 0);
+ const sp<IGraphicBufferProducer>& gbp, uint32_t transformHint = 0);
private:
// can't be copied
@@ -109,7 +105,6 @@
sp<IGraphicBufferProducer> mGraphicBufferProducer;
mutable Mutex mLock;
mutable sp<Surface> mSurfaceData;
- bool mOwned;
uint32_t mTransformHint;
};