Revert "SurfaceFlinger: Allows Surfaces to outlive their parents."
This reverts commit 4340607f5f5c277eb031fdd1424fc8a1a69924e2.
Reason for revert: Causes bug 117401269
Change-Id: I10973c9dd734499e09c18f48c06f1b1a6a1f8da0
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 63a010d..f9bc1e7 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -118,6 +118,12 @@
c->detachLayer(this);
}
+ for (auto& point : mRemoteSyncPoints) {
+ point->setTransactionApplied();
+ }
+ for (auto& point : mLocalSyncPoints) {
+ point->setFrameAvailable();
+ }
mFrameTracker.logAndResetStats(mName);
}
@@ -135,6 +141,8 @@
void Layer::onRemovedFromCurrentState() {
// the layer is removed from SF mCurrentState to mLayersPendingRemoval
+ mPendingRemoval = true;
+
if (mCurrentState.zOrderRelativeOf != nullptr) {
sp<Layer> strongRelative = mCurrentState.zOrderRelativeOf.promote();
if (strongRelative != nullptr) {
@@ -143,7 +151,7 @@
}
mCurrentState.zOrderRelativeOf = nullptr;
}
-
+
for (const auto& child : mCurrentChildren) {
child->onRemovedFromCurrentState();
}
@@ -155,13 +163,8 @@
destroyAllHwcLayers();
- mRemoved = true;
-
- for (auto& point : mRemoteSyncPoints) {
- point->setTransactionApplied();
- }
- for (auto& point : mLocalSyncPoints) {
- point->setFrameAvailable();
+ for (const auto& child : mCurrentChildren) {
+ child->onRemoved();
}
}
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 3e6eba4..874b551 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -341,7 +341,7 @@
virtual bool isCreatedFromMainThread() const { return false; }
- bool isRemoved() const { return mRemoved; }
+ bool isPendingRemoval() const { return mPendingRemoval; }
void writeToProto(LayerProto* layerInfo,
LayerVector::StateSet stateSet = LayerVector::StateSet::Drawing);
@@ -589,12 +589,12 @@
*/
class LayerCleaner {
sp<SurfaceFlinger> mFlinger;
- sp<Layer> mLayer;
+ wp<Layer> mLayer;
protected:
~LayerCleaner() {
// destroy client resources
- mFlinger->removeLayer(mLayer, true);
+ mFlinger->onLayerDestroyed(mLayer);
}
public:
@@ -739,7 +739,7 @@
// Whether filtering is needed b/c of the drawingstate
bool mNeedsFiltering{false};
- bool mRemoved{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 7ccf8bc..9410cdb 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2833,7 +2833,6 @@
recordBufferingStats(l->getName().string(),
l->getOccupancyHistory(true));
l->onRemoved();
- mNumLayers -= 1;
}
mLayersPendingRemoval.clear();
}
@@ -3249,7 +3248,7 @@
if (parent == nullptr) {
mCurrentState.layersSortedByZ.add(lbc);
} else {
- if (parent->isRemoved()) {
+ if (parent->isPendingRemoval()) {
ALOGE("addClientLayer called with a removed parent");
return NAME_NOT_FOUND;
}
@@ -3281,7 +3280,7 @@
status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp<Layer>& layer,
bool topLevelOnly) {
- if (layer->isRemoved()) {
+ if (layer->isPendingRemoval()) {
return NO_ERROR;
}
@@ -3291,14 +3290,39 @@
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);
}
+ // 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;
+ }
+
layer->onRemovedFromCurrentState();
mLayersPendingRemoval.add(layer);
mLayersRemoved = true;
+ mNumLayers -= 1 + layer->getChildrenCount();
setTransactionFlags(eTransactionNeeded);
return NO_ERROR;
}
@@ -3501,7 +3525,7 @@
return 0;
}
- if (layer->isRemoved()) {
+ if (layer->isPendingRemoval()) {
ALOGW("Attempting to set client state on removed layer: %s", layer->getName().string());
return 0;
}
@@ -3704,7 +3728,7 @@
return;
}
- if (layer->isRemoved()) {
+ if (layer->isPendingRemoval()) {
ALOGW("Attempting to destroy on removed layer: %s", layer->getName().string());
return;
}
@@ -3881,6 +3905,19 @@
return err;
}
+status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& 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);
+}
+
// ---------------------------------------------------------------------------
void SurfaceFlinger::onInitializeDisplays() {
@@ -5254,7 +5291,7 @@
auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
auto parent = layerHandle->owner.promote();
- if (parent == nullptr || parent->isRemoved()) {
+ 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 1bf1c5a..b77bf48 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -559,6 +559,11 @@
// ISurfaceComposerClient::destroySurface()
status_t onLayerRemoved(const sp<Client>& client, const sp<IBinder>& handle);
+ // 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.
+ status_t onLayerDestroyed(const wp<Layer>& layer);
+
// remove a layer from SurfaceFlinger immediately
status_t removeLayer(const sp<Layer>& layer, bool topLevelOnly = false);
status_t removeLayerLocked(const Mutex&, const sp<Layer>& layer, bool topLevelOnly = false);
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index 5e37332..3af98e5 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -2554,37 +2554,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, DetachChildrenDifferentClient) {
sp<SurfaceComposerClient> mNewComposerClient = new SurfaceComposerClient;
sp<SurfaceControl> mChildNewClient =