Mark children as detached instead of actually detaching them.
When detachChildren is called, mark children as detached instead of removing
them from the client. This is so the children can be re-attached again when
they're reparented. Check if layer is detached before copying currentState
to drawingState so detached layers aren't updated. Also check if layer
is detached before removing the layer since they should no longer be
reachable if they're detached.
Test: DetachChildrenThenAttach
Bug: 111297488
Change-Id: I31b9eb1398f02e9d68dd3f7a1f231e1cef9fb0f8
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 1e910ce..f3182be 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -1021,6 +1021,10 @@
uint32_t Layer::doTransaction(uint32_t flags) {
ATRACE_CALL();
+ if (mLayerDetached) {
+ return 0;
+ }
+
pushPendingState();
State c = getCurrentState();
if (!applyPendingStates(&c)) {
@@ -1553,6 +1557,9 @@
return false;
}
+ if (attachChildren()) {
+ setTransactionFlags(eTransactionNeeded);
+ }
for (const sp<Layer>& child : mCurrentChildren) {
newParent->addChild(child);
@@ -1597,6 +1604,13 @@
client->updateParent(newParent);
}
+ if (mLayerDetached) {
+ mLayerDetached = false;
+ setTransactionFlags(eTransactionNeeded);
+ }
+ if (attachChildren()) {
+ setTransactionFlags(eTransactionNeeded);
+ }
return true;
}
@@ -1605,7 +1619,7 @@
sp<Client> parentClient = mClientRef.promote();
sp<Client> client(child->mClientRef.promote());
if (client != nullptr && parentClient != client) {
- client->detachLayer(child.get());
+ child->mLayerDetached = true;
child->detachChildren();
}
}
@@ -1613,6 +1627,23 @@
return true;
}
+bool Layer::attachChildren() {
+ bool changed = false;
+ for (const sp<Layer>& child : mCurrentChildren) {
+ sp<Client> parentClient = mClientRef.promote();
+ sp<Client> client(child->mClientRef.promote());
+ if (client != nullptr && parentClient != client) {
+ if (child->mLayerDetached) {
+ child->mLayerDetached = false;
+ changed = true;
+ }
+ changed |= child->attachChildren();
+ }
+ }
+
+ return changed;
+}
+
bool Layer::setColorTransform(const mat4& matrix) {
static const mat4 identityMatrix = mat4();
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 40ebe1e..ed51c61 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -263,6 +263,8 @@
virtual void setChildrenDrawingParent(const sp<Layer>& layer);
virtual bool reparent(const sp<IBinder>& newParentHandle);
virtual bool detachChildren();
+ bool attachChildren();
+ bool isLayerDetached() const { return mLayerDetached; }
virtual bool setColorTransform(const mat4& matrix);
virtual const mat4& getColorTransform() const;
virtual bool hasColorTransform() const;
@@ -355,7 +357,6 @@
// to avoid grabbing the lock again to avoid deadlock
virtual bool isCreatedFromMainThread() const { return false; }
-
bool isRemovedFromCurrentState() const;
void writeToProto(LayerProto* layerInfo,
@@ -776,6 +777,9 @@
mutable LayerBE mBE;
+ // Can only be accessed with the SF state lock held.
+ bool mLayerDetached{false};
+
private:
/**
* Returns an unsorted vector of all layers that are part of this tree.
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 7150660..2f5ee8a 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3198,6 +3198,10 @@
}
status_t SurfaceFlinger::removeLayerLocked(const Mutex& lock, const sp<Layer>& layer) {
+ if (layer->isLayerDetached()) {
+ return NO_ERROR;
+ }
+
const auto& p = layer->getParent();
ssize_t index;
if (p != nullptr) {
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index 988454a..a600a0a 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -3356,6 +3356,61 @@
}
}
+TEST_F(ChildLayerTest, DetachChildrenThenAttach) {
+ sp<SurfaceComposerClient> newComposerClient = new SurfaceComposerClient;
+ sp<SurfaceControl> childNewClient =
+ newComposerClient->createSurface(String8("New Child Test Surface"), 10, 10,
+ PIXEL_FORMAT_RGBA_8888, 0, mFGSurfaceControl.get());
+
+ ASSERT_TRUE(childNewClient != nullptr);
+ ASSERT_TRUE(childNewClient->isValid());
+
+ fillSurfaceRGBA8(childNewClient, 200, 200, 200);
+
+ Transaction()
+ .hide(mChild)
+ .show(childNewClient)
+ .setPosition(childNewClient, 10, 10)
+ .setPosition(mFGSurfaceControl, 64, 64)
+ .apply();
+
+ {
+ mCapture = screenshot();
+ // Top left of foreground must now be visible
+ mCapture->expectFGColor(64, 64);
+ // But 10 pixels in we should see the child surface
+ mCapture->expectChildColor(74, 74);
+ // And 10 more pixels we should be back to the foreground surface
+ mCapture->expectFGColor(84, 84);
+ }
+
+ Transaction().detachChildren(mFGSurfaceControl).apply();
+ Transaction().hide(childNewClient).apply();
+
+ // Nothing should have changed.
+ {
+ mCapture = screenshot();
+ mCapture->expectFGColor(64, 64);
+ mCapture->expectChildColor(74, 74);
+ mCapture->expectFGColor(84, 84);
+ }
+
+ sp<SurfaceControl> newParentSurface = createLayer(String8("New Parent Surface"), 32, 32, 0);
+ fillLayerColor(ISurfaceComposerClient::eFXSurfaceBufferQueue, newParentSurface, Color::RED, 32,
+ 32);
+ Transaction()
+ .setLayer(newParentSurface, INT32_MAX - 1)
+ .show(newParentSurface)
+ .setPosition(newParentSurface, 20, 20)
+ .reparent(childNewClient, newParentSurface->getHandle())
+ .apply();
+ {
+ mCapture = screenshot();
+ // Child is now hidden.
+ mCapture->expectColor(Rect(20, 20, 52, 52), Color::RED);
+ }
+}
+
TEST_F(ChildLayerTest, ChildrenInheritNonTransformScalingFromParent) {
asTransaction([&](Transaction& t) {
t.show(mChild);