Don't invoke transactions on layers that will be removed.
A layer can be marked as removed, but still be present in memory.
This check ensures that transactions aren't invoked on layers that
will be removed on the next commitTransaction.
Normally, this would be harmless since the layer will get removed
as soon as a commitTransaction is called. However, for cases like
re-parenting, a removed child layer can be re-parented to a non-removed
layer, which prevents the child from getting removed.
Test: Added code that would destroy a layer before the re-parent was
called. Ensure that the re-parent was ignored. There doesn't
seem to be an easy way to write a test case right now.
Change-Id: I17614447fc4253bdbbb0c06469bb09117b55c1ab
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index e40bbd5..4ca9acb 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2857,6 +2857,10 @@
status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly) {
Mutex::Autolock _l(mStateLock);
+ if (layer->isPendingRemoval()) {
+ return NO_ERROR;
+ }
+
const auto& p = layer->getParent();
ssize_t index;
if (p != nullptr) {
@@ -3060,131 +3064,139 @@
const sp<Client>& client,
const layer_state_t& s)
{
- uint32_t flags = 0;
sp<Layer> layer(client->getLayerUser(s.surface));
- if (layer != 0) {
- const uint32_t what = s.what;
- bool geometryAppliesWithResize =
- what & layer_state_t::eGeometryAppliesWithResize;
- if (what & layer_state_t::ePositionChanged) {
- if (layer->setPosition(s.x, s.y, !geometryAppliesWithResize)) {
- flags |= eTraversalNeeded;
- }
+ if (layer == nullptr) {
+ 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;
+ bool geometryAppliesWithResize =
+ what & layer_state_t::eGeometryAppliesWithResize;
+ if (what & layer_state_t::ePositionChanged) {
+ if (layer->setPosition(s.x, s.y, !geometryAppliesWithResize)) {
+ flags |= eTraversalNeeded;
}
- if (what & layer_state_t::eLayerChanged) {
- // NOTE: index needs to be calculated before we update the state
- const auto& p = layer->getParent();
- if (p == nullptr) {
- ssize_t idx = mCurrentState.layersSortedByZ.indexOf(layer);
- if (layer->setLayer(s.z) && idx >= 0) {
- mCurrentState.layersSortedByZ.removeAt(idx);
- mCurrentState.layersSortedByZ.add(layer);
- // we need traversal (state changed)
- // AND transaction (list changed)
- flags |= eTransactionNeeded|eTraversalNeeded;
- }
- } else {
- if (p->setChildLayer(layer, s.z)) {
- flags |= eTransactionNeeded|eTraversalNeeded;
- }
- }
- }
- if (what & layer_state_t::eRelativeLayerChanged) {
+ }
+ if (what & layer_state_t::eLayerChanged) {
+ // NOTE: index needs to be calculated before we update the state
+ const auto& p = layer->getParent();
+ if (p == nullptr) {
ssize_t idx = mCurrentState.layersSortedByZ.indexOf(layer);
- if (layer->setRelativeLayer(s.relativeLayerHandle, s.z)) {
- mCurrentState.layersSortedByZ.removeAt(idx);
- mCurrentState.layersSortedByZ.add(layer);
- flags |= eTransactionNeeded|eTraversalNeeded;
- }
- }
- if (what & layer_state_t::eSizeChanged) {
- if (layer->setSize(s.w, s.h)) {
- flags |= eTraversalNeeded;
- }
- }
- if (what & layer_state_t::eAlphaChanged) {
- if (layer->setAlpha(s.alpha))
- flags |= eTraversalNeeded;
- }
- if (what & layer_state_t::eColorChanged) {
- if (layer->setColor(s.color))
- flags |= eTraversalNeeded;
- }
- if (what & layer_state_t::eMatrixChanged) {
- if (layer->setMatrix(s.matrix))
- flags |= eTraversalNeeded;
- }
- if (what & layer_state_t::eTransparentRegionChanged) {
- if (layer->setTransparentRegionHint(s.transparentRegion))
- flags |= eTraversalNeeded;
- }
- if (what & layer_state_t::eFlagsChanged) {
- if (layer->setFlags(s.flags, s.mask))
- flags |= eTraversalNeeded;
- }
- if (what & layer_state_t::eCropChanged) {
- if (layer->setCrop(s.crop, !geometryAppliesWithResize))
- flags |= eTraversalNeeded;
- }
- if (what & layer_state_t::eFinalCropChanged) {
- if (layer->setFinalCrop(s.finalCrop, !geometryAppliesWithResize))
- flags |= eTraversalNeeded;
- }
- if (what & layer_state_t::eLayerStackChanged) {
- ssize_t idx = mCurrentState.layersSortedByZ.indexOf(layer);
- // We only allow setting layer stacks for top level layers,
- // everything else inherits layer stack from its parent.
- if (layer->hasParent()) {
- ALOGE("Attempt to set layer stack on layer with parent (%s) is invalid",
- layer->getName().string());
- } else if (idx < 0) {
- ALOGE("Attempt to set layer stack on layer without parent (%s) that "
- "that also does not appear in the top level layer list. Something"
- " has gone wrong.", layer->getName().string());
- } else if (layer->setLayerStack(s.layerStack)) {
+ if (layer->setLayer(s.z) && idx >= 0) {
mCurrentState.layersSortedByZ.removeAt(idx);
mCurrentState.layersSortedByZ.add(layer);
// we need traversal (state changed)
// AND transaction (list changed)
flags |= eTransactionNeeded|eTraversalNeeded;
}
- }
- if (what & layer_state_t::eDeferTransaction) {
- if (s.barrierHandle != nullptr) {
- layer->deferTransactionUntil(s.barrierHandle, s.frameNumber);
- } else if (s.barrierGbp != nullptr) {
- const sp<IGraphicBufferProducer>& gbp = s.barrierGbp;
- if (authenticateSurfaceTextureLocked(gbp)) {
- const auto& otherLayer =
- (static_cast<MonitoredProducer*>(gbp.get()))->getLayer();
- layer->deferTransactionUntil(otherLayer, s.frameNumber);
- } else {
- ALOGE("Attempt to defer transaction to to an"
- " unrecognized GraphicBufferProducer");
- }
- }
- // We don't trigger a traversal here because if no other state is
- // changed, we don't want this to cause any more work
- }
- if (what & layer_state_t::eReparent) {
- if (layer->reparent(s.parentHandleForChild)) {
+ } else {
+ if (p->setChildLayer(layer, s.z)) {
flags |= eTransactionNeeded|eTraversalNeeded;
}
}
- if (what & layer_state_t::eReparentChildren) {
- if (layer->reparentChildren(s.reparentHandle)) {
- flags |= eTransactionNeeded|eTraversalNeeded;
+ }
+ if (what & layer_state_t::eRelativeLayerChanged) {
+ ssize_t idx = mCurrentState.layersSortedByZ.indexOf(layer);
+ if (layer->setRelativeLayer(s.relativeLayerHandle, s.z)) {
+ mCurrentState.layersSortedByZ.removeAt(idx);
+ mCurrentState.layersSortedByZ.add(layer);
+ flags |= eTransactionNeeded|eTraversalNeeded;
+ }
+ }
+ if (what & layer_state_t::eSizeChanged) {
+ if (layer->setSize(s.w, s.h)) {
+ flags |= eTraversalNeeded;
+ }
+ }
+ if (what & layer_state_t::eAlphaChanged) {
+ if (layer->setAlpha(s.alpha))
+ flags |= eTraversalNeeded;
+ }
+ if (what & layer_state_t::eColorChanged) {
+ if (layer->setColor(s.color))
+ flags |= eTraversalNeeded;
+ }
+ if (what & layer_state_t::eMatrixChanged) {
+ if (layer->setMatrix(s.matrix))
+ flags |= eTraversalNeeded;
+ }
+ if (what & layer_state_t::eTransparentRegionChanged) {
+ if (layer->setTransparentRegionHint(s.transparentRegion))
+ flags |= eTraversalNeeded;
+ }
+ if (what & layer_state_t::eFlagsChanged) {
+ if (layer->setFlags(s.flags, s.mask))
+ flags |= eTraversalNeeded;
+ }
+ if (what & layer_state_t::eCropChanged) {
+ if (layer->setCrop(s.crop, !geometryAppliesWithResize))
+ flags |= eTraversalNeeded;
+ }
+ if (what & layer_state_t::eFinalCropChanged) {
+ if (layer->setFinalCrop(s.finalCrop, !geometryAppliesWithResize))
+ flags |= eTraversalNeeded;
+ }
+ if (what & layer_state_t::eLayerStackChanged) {
+ ssize_t idx = mCurrentState.layersSortedByZ.indexOf(layer);
+ // We only allow setting layer stacks for top level layers,
+ // everything else inherits layer stack from its parent.
+ if (layer->hasParent()) {
+ ALOGE("Attempt to set layer stack on layer with parent (%s) is invalid",
+ layer->getName().string());
+ } else if (idx < 0) {
+ ALOGE("Attempt to set layer stack on layer without parent (%s) that "
+ "that also does not appear in the top level layer list. Something"
+ " has gone wrong.", layer->getName().string());
+ } else if (layer->setLayerStack(s.layerStack)) {
+ mCurrentState.layersSortedByZ.removeAt(idx);
+ mCurrentState.layersSortedByZ.add(layer);
+ // we need traversal (state changed)
+ // AND transaction (list changed)
+ flags |= eTransactionNeeded|eTraversalNeeded;
+ }
+ }
+ if (what & layer_state_t::eDeferTransaction) {
+ if (s.barrierHandle != nullptr) {
+ layer->deferTransactionUntil(s.barrierHandle, s.frameNumber);
+ } else if (s.barrierGbp != nullptr) {
+ const sp<IGraphicBufferProducer>& gbp = s.barrierGbp;
+ if (authenticateSurfaceTextureLocked(gbp)) {
+ const auto& otherLayer =
+ (static_cast<MonitoredProducer*>(gbp.get()))->getLayer();
+ layer->deferTransactionUntil(otherLayer, s.frameNumber);
+ } else {
+ ALOGE("Attempt to defer transaction to to an"
+ " unrecognized GraphicBufferProducer");
}
}
- if (what & layer_state_t::eDetachChildren) {
- layer->detachChildren();
+ // We don't trigger a traversal here because if no other state is
+ // changed, we don't want this to cause any more work
+ }
+ if (what & layer_state_t::eReparent) {
+ if (layer->reparent(s.parentHandleForChild)) {
+ flags |= eTransactionNeeded|eTraversalNeeded;
}
- if (what & layer_state_t::eOverrideScalingModeChanged) {
- layer->setOverrideScalingMode(s.overrideScalingMode);
- // We don't trigger a traversal here because if no other state is
- // changed, we don't want this to cause any more work
+ }
+ if (what & layer_state_t::eReparentChildren) {
+ if (layer->reparentChildren(s.reparentHandle)) {
+ flags |= eTransactionNeeded|eTraversalNeeded;
}
}
+ if (what & layer_state_t::eDetachChildren) {
+ layer->detachChildren();
+ }
+ if (what & layer_state_t::eOverrideScalingModeChanged) {
+ layer->setOverrideScalingMode(s.overrideScalingMode);
+ // We don't trigger a traversal here because if no other state is
+ // changed, we don't want this to cause any more work
+ }
return flags;
}