Merge changes from topic 'layer-wp-race' into oc-dev
* changes:
surfaceflinger: Layer::getParent requires state lock held
surfaceflinger: distinguish mCurrentParent/mDrawingParent
surfaceflinger: protect Client::mParentLayer with a lock
diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp
index e9a2513..8ba6cb9 100644
--- a/services/surfaceflinger/Client.cpp
+++ b/services/surfaceflinger/Client.cpp
@@ -57,9 +57,19 @@
}
void Client::setParentLayer(const sp<Layer>& parentLayer) {
+ Mutex::Autolock _l(mLock);
mParentLayer = parentLayer;
}
+sp<Layer> Client::getParentLayer(bool* outParentDied) const {
+ Mutex::Autolock _l(mLock);
+ sp<Layer> parent = mParentLayer.promote();
+ if (outParentDied != nullptr) {
+ *outParentDied = (mParentLayer != nullptr && parent == nullptr);
+ }
+ return parent;
+}
+
status_t Client::initCheck() const {
return NO_ERROR;
}
@@ -108,7 +118,7 @@
// We grant an exception in the case that the Client has a "parent layer", as its
// effects will be scoped to that layer.
if (CC_UNLIKELY(pid != self_pid && uid != AID_GRAPHICS && uid != AID_SYSTEM && uid != 0)
- && (mParentLayer.promote() == nullptr)) {
+ && (getParentLayer() == nullptr)) {
// we're called from a different process, do the real check
if (!PermissionCache::checkCallingPermission(sAccessSurfaceFlinger))
{
@@ -135,11 +145,12 @@
return NAME_NOT_FOUND;
}
}
- if (parent == nullptr && mParentLayer != nullptr) {
- parent = mParentLayer.promote();
+ if (parent == nullptr) {
+ bool parentDied;
+ parent = getParentLayer(&parentDied);
// If we had a parent, but it died, we've lost all
// our capabilities.
- if (parent == nullptr) {
+ if (parentDied) {
return NAME_NOT_FOUND;
}
}
diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h
index b5f98b8..2aab28f 100644
--- a/services/surfaceflinger/Client.h
+++ b/services/surfaceflinger/Client.h
@@ -71,12 +71,13 @@
virtual status_t onTransact(
uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags);
+ sp<Layer> getParentLayer(bool* outParentDied = nullptr) const;
+
// constant
sp<SurfaceFlinger> mFlinger;
// protected by mLock
DefaultKeyedVector< wp<IBinder>, wp<Layer> > mLayers;
-
wp<Layer> mParentLayer;
// thread-safe
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 2ff14aa..022b416 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -403,7 +403,7 @@
win.intersect(s.finalCrop, &win);
}
- const sp<Layer>& p = getParent();
+ const sp<Layer>& p = mDrawingParent.promote();
// Now we need to calculate the parent bounds, so we can clip ourselves to those.
// When calculating the parent bounds for purposes of clipping,
// we don't need to constrain the parent to its transparent region.
@@ -440,7 +440,7 @@
}
Rect bounds = win;
- const auto& p = getParent();
+ const auto& p = mDrawingParent.promote();
if (p != nullptr) {
// Look in computeScreenBounds recursive call for explanation of
// why we pass false here.
@@ -498,7 +498,7 @@
// Screen space to make reduction to parent crop clearer.
Rect activeCrop = computeInitialCrop(hw);
- const auto& p = getParent();
+ const auto& p = mDrawingParent.promote();
if (p != nullptr) {
auto parentCrop = p->computeInitialCrop(hw);
activeCrop.intersect(parentCrop, &activeCrop);
@@ -710,7 +710,7 @@
int type = s.type;
int appId = s.appId;
- sp<Layer> parent = mParent.promote();
+ sp<Layer> parent = mDrawingParent.promote();
if (parent.get()) {
auto& parentState = parent->getDrawingState();
type = parentState.type;
@@ -1109,8 +1109,9 @@
* of a camera where the buffer remains in native orientation,
* we want the pixels to always be upright.
*/
- if (getParent() != nullptr) {
- const auto parentTransform = getParent()->getTransform();
+ sp<Layer> p = mDrawingParent.promote();
+ if (p != nullptr) {
+ const auto parentTransform = p->getTransform();
tr = tr * inverseOrientation(parentTransform.getOrientation());
}
@@ -1928,7 +1929,7 @@
}
uint32_t Layer::getLayerStack() const {
- auto p = getParent();
+ auto p = mDrawingParent.promote();
if (p == nullptr) {
return getDrawingState().layerStack;
}
@@ -2071,7 +2072,7 @@
bool Layer::isHiddenByPolicy() const {
const Layer::State& s(mDrawingState);
- const auto& parent = getParent();
+ const auto& parent = mDrawingParent.promote();
if (parent != nullptr && parent->isHiddenByPolicy()) {
return true;
}
@@ -2555,25 +2556,7 @@
}
for (const sp<Layer>& child : mCurrentChildren) {
- // We don't call addChild as we need to delay updating the child's parent pointer until
- // a transaction occurs. Remember a refresh could occur in between now and the next
- // transaction, in which case the Layer's parent pointer would be updated, but changes
- // made to the parent in the same transaction would not have applied.
- // This means that the following kind of scenario wont work:
- //
- // 1. Existing and visible child and parent surface exist
- // 2. Create new surface hidden
- // 3. Open transaction
- // 4. Show the new surface, and reparent the old surface's children to it.
- // 5. Close transaction.
- //
- // If we were to update the parent pointer immediately, then the child surface
- // could disappear for one frame as it pointed at the new parent which
- // hasn't yet become visible as the transaction hasn't yet occurred.
- //
- // Instead we defer the reparenting to commitChildList which happens as part
- // of the global transaction.
- newParent->mCurrentChildren.add(child);
+ newParent->addChild(child);
sp<Client> client(child->mClientRef.promote());
if (client != nullptr) {
@@ -2601,7 +2584,7 @@
}
void Layer::setParent(const sp<Layer>& layer) {
- mParent = layer;
+ mCurrentParent = layer;
}
void Layer::clearSyncPoints() {
@@ -2691,7 +2674,7 @@
Transform Layer::getTransform() const {
Transform t;
- const auto& p = getParent();
+ const auto& p = mDrawingParent.promote();
if (p != nullptr) {
t = p->getTransform();
@@ -2724,14 +2707,14 @@
#ifdef USE_HWC2
float Layer::getAlpha() const {
- const auto& p = getParent();
+ const auto& p = mDrawingParent.promote();
float parentAlpha = (p != nullptr) ? p->getAlpha() : 1.0;
return parentAlpha * getDrawingState().alpha;
}
#else
uint8_t Layer::getAlpha() const {
- const auto& p = getParent();
+ const auto& p = mDrawingParent.promote();
float parentAlpha = (p != nullptr) ? (p->getAlpha() / 255.0f) : 1.0;
float drawingAlpha = getDrawingState().alpha / 255.0f;
@@ -2743,11 +2726,10 @@
void Layer::commitChildList() {
for (size_t i = 0; i < mCurrentChildren.size(); i++) {
const auto& child = mCurrentChildren[i];
- child->setParent(this);
-
child->commitChildList();
}
mDrawingChildren = mCurrentChildren;
+ mDrawingParent = mCurrentParent;
}
// ---------------------------------------------------------------------------
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 5335fff..24fc10d 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -524,7 +524,7 @@
// Returns index if removed, or negative value otherwise
// for symmetry with Vector::remove
ssize_t removeChild(const sp<Layer>& layer);
- sp<Layer> getParent() const { return mParent.promote(); }
+ sp<Layer> getParent() const { return mCurrentParent.promote(); }
bool hasParent() const { return getParent() != nullptr; }
Rect computeScreenBounds(bool reduceTransparentRegion = true) const;
@@ -801,7 +801,8 @@
// Child list used for rendering.
LayerVector mDrawingChildren;
- wp<Layer> mParent;
+ wp<Layer> mCurrentParent;
+ wp<Layer> mDrawingParent;
};
// ---------------------------------------------------------------------------
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index a49e8f4..6174185 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2679,14 +2679,18 @@
return NO_ERROR;
}
-status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer) {
+status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly) {
Mutex::Autolock _l(mStateLock);
const auto& p = layer->getParent();
- const ssize_t index = (p != nullptr) ? p->removeChild(layer) :
- mCurrentState.layersSortedByZ.remove(layer);
-
+ ssize_t index;
if (p != nullptr) {
+ if (topLevelOnly) {
+ return NO_ERROR;
+ }
+
+ index = p->removeChild(layer);
+
sp<Layer> ancestor = p;
while (ancestor->getParent() != nullptr) {
ancestor = ancestor->getParent();
@@ -2695,6 +2699,8 @@
ALOGE("removeLayer called with a layer whose parent has been removed");
return NAME_NOT_FOUND;
}
+ } else {
+ index = mCurrentState.layersSortedByZ.remove(layer);
}
// As a matter of normal operation, the LayerCleaner will produce a second
@@ -3125,11 +3131,9 @@
if (l == nullptr) {
// The layer has already been removed, carry on
return NO_ERROR;
- } if (l->getParent() != nullptr) {
- // If we have a parent, then we can continue to live as long as it does.
- return NO_ERROR;
}
- return removeLayer(l);
+ // If we have a parent, then we can continue to live as long as it does.
+ return removeLayer(l, true);
}
// ---------------------------------------------------------------------------
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index c89e26f..9239538 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -396,7 +396,7 @@
status_t onLayerDestroyed(const wp<Layer>& layer);
// remove a layer from SurfaceFlinger immediately
- status_t removeLayer(const sp<Layer>& layer);
+ status_t removeLayer(const sp<Layer>& layer, bool topLevelOnly = false);
// add a layer to SurfaceFlinger
status_t addClientLayer(const sp<Client>& client,
diff --git a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
index e19e021..3d421d2 100644
--- a/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
+++ b/services/surfaceflinger/SurfaceFlinger_hwc1.cpp
@@ -2339,12 +2339,20 @@
return NO_ERROR;
}
-status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer) {
+status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly) {
Mutex::Autolock _l(mStateLock);
const auto& p = layer->getParent();
- const ssize_t index = (p != nullptr) ? p->removeChild(layer) :
- mCurrentState.layersSortedByZ.remove(layer);
+ ssize_t index;
+ if (p != nullptr) {
+ if (topLevelOnly) {
+ return NO_ERROR;
+ }
+
+ 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
@@ -2769,11 +2777,9 @@
if (l == nullptr) {
// The layer has already been removed, carry on
return NO_ERROR;
- } if (l->getParent() != nullptr) {
- // If we have a parent, then we can continue to live as long as it does.
- return NO_ERROR;
}
- return removeLayer(l);
+ // If we have a parent, then we can continue to live as long as it does.
+ return removeLayer(l, true);
}
// ---------------------------------------------------------------------------