Revert "SurfaceFlinger: Share ownership of layers between State and Handle."
This reverts commit f478f9f70b544d9f93680d6734a1af45bbd7f509.
Reason for revert: Causes presubmit issues.
Change-Id: Ibe872baed863a8ee9b9da5722e1671d087ec3a9f
Bug: 118257858
Bug: 62536731
Bug: 111373437
Bug: 111297488
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 9f1c662..8afd3b3 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -118,20 +118,13 @@
c->detachLayer(this);
}
- mFrameTracker.logAndResetStats(mName);
-
- // The remote sync points are cleared out when we are
- // removed from current state.
- Mutex::Autolock lock(mLocalSyncPointMutex);
+ for (auto& point : mRemoteSyncPoints) {
+ point->setTransactionApplied();
+ }
for (auto& point : mLocalSyncPoints) {
point->setFrameAvailable();
}
-
- abandon();
-
- destroyAllHwcLayers();
-
- mFlinger->onLayerDestroyed();
+ mFrameTracker.logAndResetStats(mName);
}
// ---------------------------------------------------------------------------
@@ -146,9 +139,10 @@
void Layer::onLayerDisplayed(const sp<Fence>& /*releaseFence*/) {}
void Layer::onRemovedFromCurrentState() {
- mRemovedFromCurrentState = true;
-
// the layer is removed from SF mCurrentState to mLayersPendingRemoval
+
+ mPendingRemoval = true;
+
if (mCurrentState.zOrderRelativeOf != nullptr) {
sp<Layer> strongRelative = mCurrentState.zOrderRelativeOf.promote();
if (strongRelative != nullptr) {
@@ -158,21 +152,22 @@
mCurrentState.zOrderRelativeOf = nullptr;
}
- // Since we are no longer reachable from CurrentState SurfaceFlinger
- // will no longer invoke doTransaction for us, and so we will
- // never finish applying transactions. We signal the sync point
- // now so that another layer will not become indefinitely
- // blocked.
- for (auto& point: mRemoteSyncPoints) {
- point->setTransactionApplied();
- }
- mRemoteSyncPoints.clear();
-
for (const auto& child : mCurrentChildren) {
child->onRemovedFromCurrentState();
}
}
+void Layer::onRemoved() {
+ // the layer is removed from SF mLayersPendingRemoval
+ abandon();
+
+ destroyAllHwcLayers();
+
+ for (const auto& child : mCurrentChildren) {
+ child->onRemoved();
+ }
+}
+
// ---------------------------------------------------------------------------
// set-up
// ---------------------------------------------------------------------------
@@ -232,10 +227,6 @@
}
LOG_ALWAYS_FATAL_IF(!getBE().mHwcLayers.empty(),
"All hardware composer layers should have been destroyed");
-
- for (const sp<Layer>& child : mDrawingChildren) {
- child->destroyAllHwcLayers();
- }
}
Rect Layer::getContentCrop() const {
@@ -792,9 +783,7 @@
// If this transaction is waiting on the receipt of a frame, generate a sync
// point and send it to the remote layer.
- // We don't allow installing sync points after we are removed from the current state
- // as we won't be able to signal our end.
- if (mCurrentState.barrierLayer_legacy != nullptr && !mRemovedFromCurrentState) {
+ if (mCurrentState.barrierLayer_legacy != nullptr) {
sp<Layer> barrierLayer = mCurrentState.barrierLayer_legacy.promote();
if (barrierLayer == nullptr) {
ALOGE("[%s] Unable to promote barrier Layer.", mName.string());
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index fdec0f7..e2d1178 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -345,7 +345,7 @@
virtual bool isCreatedFromMainThread() const { return false; }
- bool isRemovedFromCurrentState() const { return mRemovedFromCurrentState; }
+ bool isPendingRemoval() const { return mPendingRemoval; }
void writeToProto(LayerProto* layerInfo,
LayerVector::StateSet stateSet = LayerVector::StateSet::Drawing);
@@ -474,6 +474,12 @@
*/
void onRemovedFromCurrentState();
+ /*
+ * called with the state lock from the main thread when the layer is
+ * removed from the pending removal list
+ */
+ void onRemoved();
+
// Updates the transform hint in our SurfaceFlingerConsumer to match
// the current orientation of the display device.
void updateTransformHint(const sp<const DisplayDevice>& display) const;
@@ -588,12 +594,12 @@
*/
class LayerCleaner {
sp<SurfaceFlinger> mFlinger;
- sp<Layer> mLayer;
+ wp<Layer> mLayer;
protected:
~LayerCleaner() {
// destroy client resources
- mFlinger->onHandleDestroyed(mLayer);
+ mFlinger->onLayerDestroyed(mLayer);
}
public:
@@ -695,8 +701,6 @@
virtual PixelFormat getPixelFormat() const { return PIXEL_FORMAT_NONE; }
bool getPremultipledAlpha() const;
- bool mPendingHWCDestroy{false};
-
protected:
// -----------------------------------------------------------------------
bool usingRelativeZ(LayerVector::StateSet stateSet);
@@ -740,7 +744,7 @@
// Whether filtering is needed b/c of the drawingstate
bool mNeedsFiltering{false};
- bool mRemovedFromCurrentState{false};
+ bool mPendingRemoval{false};
// page-flip thread (currently main thread)
bool mProtectedByApp{false}; // application requires protected path to external sink
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index badfc6b..f0723e8 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2878,14 +2878,7 @@
for (const auto& l : mLayersPendingRemoval) {
recordBufferingStats(l->getName().string(),
l->getOccupancyHistory(true));
-
- // We need to release the HWC layers when the Layer is removed
- // from the current state otherwise the HWC layer just continues
- // showing at it's last configured state until we eventually
- // abandon the buffer queue.
- if (l->isRemovedFromCurrentState()) {
- l->destroyAllHwcLayers();
- }
+ l->onRemoved();
}
mLayersPendingRemoval.clear();
}
@@ -3318,6 +3311,10 @@
if (parent == nullptr) {
mCurrentState.layersSortedByZ.add(lbc);
} else {
+ if (parent->isPendingRemoval()) {
+ ALOGE("addClientLayer called with a removed parent");
+ return NAME_NOT_FOUND;
+ }
parent->addChild(lbc);
}
@@ -3344,22 +3341,52 @@
return removeLayerLocked(mStateLock, layer, topLevelOnly);
}
-status_t SurfaceFlinger::removeLayerLocked(const Mutex& lock, const sp<Layer>& layer,
+status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp<Layer>& layer,
bool topLevelOnly) {
+ if (layer->isPendingRemoval()) {
+ return NO_ERROR;
+ }
+
const auto& p = layer->getParent();
ssize_t index;
if (p != nullptr) {
if (topLevelOnly) {
return NO_ERROR;
}
+
+ sp<Layer> ancestor = p;
+ while (ancestor->getParent() != nullptr) {
+ ancestor = ancestor->getParent();
+ }
+ if (mCurrentState.layersSortedByZ.indexOf(ancestor) < 0) {
+ ALOGE("removeLayer called with a layer whose parent has been removed");
+ return NAME_NOT_FOUND;
+ }
+
index = p->removeChild(layer);
} else {
index = mCurrentState.layersSortedByZ.remove(layer);
}
- layer->onRemovedFromCurrentState();
+ // As a matter of normal operation, the LayerCleaner will produce a second
+ // attempt to remove the surface. The Layer will be kept alive in mDrawingState
+ // so we will succeed in promoting it, but it's already been removed
+ // from mCurrentState. As long as we can find it in mDrawingState we have no problem
+ // otherwise something has gone wrong and we are leaking the layer.
+ if (index < 0 && mDrawingState.layersSortedByZ.indexOf(layer) < 0) {
+ ALOGE("Failed to find layer (%s) in layer parent (%s).",
+ layer->getName().string(),
+ (p != nullptr) ? p->getName().string() : "no-parent");
+ return BAD_VALUE;
+ } else if (index < 0) {
+ return NO_ERROR;
+ }
- markLayerPendingRemovalLocked(lock, layer);
+ layer->onRemovedFromCurrentState();
+ mLayersPendingRemoval.add(layer);
+ mLayersRemoved = true;
+ mNumLayers -= 1 + layer->getChildrenCount();
+ setTransactionFlags(eTransactionNeeded);
return NO_ERROR;
}
@@ -3560,6 +3587,11 @@
return 0;
}
+ if (layer->isPendingRemoval()) {
+ ALOGW("Attempting to set client state on removed layer: %s", layer->getName().string());
+ return 0;
+ }
+
uint32_t flags = 0;
const uint32_t what = s.what;
@@ -3763,6 +3795,11 @@
return;
}
+ if (layer->isPendingRemoval()) {
+ ALOGW("Attempting to destroy on removed layer: %s", layer->getName().string());
+ return;
+ }
+
if (state.what & layer_state_t::eDestroySurface) {
removeLayerLocked(mStateLock, layer);
}
@@ -3935,16 +3972,17 @@
return err;
}
-void SurfaceFlinger::markLayerPendingRemovalLocked(const Mutex&, const sp<Layer>& layer) {
- mLayersPendingRemoval.add(layer);
- mLayersRemoved = true;
- setTransactionFlags(eTransactionNeeded);
-}
-
-void SurfaceFlinger::onHandleDestroyed(const sp<Layer>& layer)
+status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer)
{
- Mutex::Autolock lock(mStateLock);
- markLayerPendingRemovalLocked(mStateLock, layer);
+ // called by ~LayerCleaner() when all references to the IBinder (handle)
+ // are gone
+ sp<Layer> l = layer.promote();
+ if (l == nullptr) {
+ // The layer has already been removed, carry on
+ return NO_ERROR;
+ }
+ // If we have a parent, then we can continue to live as long as it does.
+ return removeLayer(l, true);
}
// ---------------------------------------------------------------------------
@@ -5337,7 +5375,7 @@
auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
auto parent = layerHandle->owner.promote();
- if (parent == nullptr || parent->isRemovedFromCurrentState()) {
+ if (parent == nullptr || parent->isPendingRemoval()) {
ALOGE("captureLayers called with a removed parent");
return NAME_NOT_FOUND;
}
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index f72bb65..f535c4e 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -356,8 +356,6 @@
bool authenticateSurfaceTextureLocked(
const sp<IGraphicBufferProducer>& bufferProducer) const;
- inline void onLayerDestroyed() { mNumLayers--; }
-
private:
friend class Client;
friend class DisplayEventConnection;
@@ -563,12 +561,10 @@
// ISurfaceComposerClient::destroySurface()
status_t onLayerRemoved(const sp<Client>& client, const sp<IBinder>& handle);
- void markLayerPendingRemovalLocked(const Mutex& /* mStateLock */, const sp<Layer>& layer);
-
// called when all clients have released all their references to
// this layer meaning it is entirely safe to destroy all
// resources associated to this layer.
- void onHandleDestroyed(const sp<Layer>& layer);
+ status_t onLayerDestroyed(const wp<Layer>& layer);
// remove a layer from SurfaceFlinger immediately
status_t removeLayer(const sp<Layer>& layer, bool topLevelOnly = false);
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index df1fee2..3166a8c 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -2550,37 +2550,6 @@
}
}
-TEST_F(ChildLayerTest, ChildrenSurviveParentDestruction) {
- sp<SurfaceControl> mGrandChild =
- mClient->createSurface(String8("Grand Child"), 10, 10,
- PIXEL_FORMAT_RGBA_8888, 0, mChild.get());
- fillSurfaceRGBA8(mGrandChild, 111, 111, 111);
-
- {
- SCOPED_TRACE("Grandchild visible");
- ScreenCapture::captureScreen(&mCapture);
- mCapture->checkPixel(64, 64, 111, 111, 111);
- }
-
- mChild->clear();
-
- {
- SCOPED_TRACE("After destroying child");
- ScreenCapture::captureScreen(&mCapture);
- mCapture->expectFGColor(64, 64);
- }
-
- asTransaction([&](Transaction& t) {
- t.reparent(mGrandChild, mFGSurfaceControl->getHandle());
- });
-
- {
- SCOPED_TRACE("After reparenting grandchild");
- ScreenCapture::captureScreen(&mCapture);
- mCapture->checkPixel(64, 64, 111, 111, 111);
- }
-}
-
TEST_F(ChildLayerTest, DetachChildrenSameClient) {
asTransaction([&](Transaction& t) {
t.show(mChild);