From 6a8f8af9d0b28dac6231622ed3048e5cc7616d50 Mon Sep 17 00:00:00 2001 From: Valerie Hau Date: Fri, 20 Dec 2019 11:39:23 -0800 Subject: RESTRICT AUTOMERGE: Fix removal of handle from map Bug: 144667224, 137284057 Test: build, boot, SurfaceFlinger_test, libgui_test, manual Change-Id: Ie0f111a6b8f4424375cc124ccd2683f5bfad7759 --- services/surfaceflinger/Client.cpp | 25 +++++--------- services/surfaceflinger/Client.h | 2 +- services/surfaceflinger/Layer.cpp | 9 ++--- services/surfaceflinger/SurfaceFlinger.cpp | 55 +++++++++++++++++++++--------- services/surfaceflinger/SurfaceFlinger.h | 10 ++++-- 5 files changed, 57 insertions(+), 44 deletions(-) diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp index 09d87b29d6..6b54a142e9 100644 --- a/services/surfaceflinger/Client.cpp +++ b/services/surfaceflinger/Client.cpp @@ -106,16 +106,16 @@ void Client::detachLayer(const Layer* layer) } } } -sp Client::getLayerUser(const sp& handle) const + +bool Client::isAttached(const sp& handle) const { Mutex::Autolock _l(mLock); sp lbc; wp layer(mLayers.valueFor(handle)); if (layer != 0) { - lbc = layer.promote(); - ALOGE_IF(lbc==0, "getLayerUser(name=%p) is dead", handle.get()); + return true; } - return lbc; + return false; } status_t Client::onTransact( @@ -150,7 +150,8 @@ status_t Client::createSurface( sp* handle, sp* gbp) { bool parentDied; - sp parentLayer = getParentLayer(&parentDied); + sp parentLayer; + if (!parentHandle) parentLayer = getParentLayer(&parentDied); if (parentHandle == nullptr && parentDied) { return NAME_NOT_FOUND; } @@ -163,21 +164,11 @@ status_t Client::destroySurface(const sp& handle) { } status_t Client::clearLayerFrameStats(const sp& handle) const { - sp layer = getLayerUser(handle); - if (layer == nullptr) { - return NAME_NOT_FOUND; - } - layer->clearFrameStats(); - return NO_ERROR; + return mFlinger->clearLayerFrameStats(this, handle); } status_t Client::getLayerFrameStats(const sp& handle, FrameStats* outStats) const { - sp layer = getLayerUser(handle); - if (layer == nullptr) { - return NAME_NOT_FOUND; - } - layer->getFrameStats(outStats); - return NO_ERROR; + return mFlinger->getLayerFrameStats(this, handle, outStats); } // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/Client.h b/services/surfaceflinger/Client.h index 36b685db58..c8da5283c0 100644 --- a/services/surfaceflinger/Client.h +++ b/services/surfaceflinger/Client.h @@ -49,7 +49,7 @@ public: void detachLayer(const Layer* layer); - sp getLayerUser(const sp& handle) const; + bool isAttached (const sp& handle) const; void updateParent(const sp& parentLayer); diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 13b12e29bd..a062aa1fad 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -178,13 +178,8 @@ Layer::~Layer() { void Layer::onLayerDisplayed(const sp& /*releaseFence*/) {} void Layer::onRemovedFromCurrentState() { - if (!mPendingRemoval) { - // the layer is removed from SF mCurrentState to mLayersPendingRemoval - mPendingRemoval = true; - - // remove from sf mapping - mFlinger->removeLayerFromMap(this); - } + // the layer is removed from SF mCurrentState to mLayersPendingRemoval + mPendingRemoval = true; if (mCurrentState.zOrderRelativeOf != nullptr) { sp strongRelative = mCurrentState.zOrderRelativeOf.promote(); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 6ce2cb8e30..2c7f8c4c1d 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3187,16 +3187,10 @@ status_t SurfaceFlinger::addClientLayer(const sp& client, const sp& layer, bool topLevelOnly) { - Mutex::Autolock _l(mStateLock); - return removeLayerLocked(mStateLock, layer, topLevelOnly); -} - -status_t SurfaceFlinger::removeLayerFromMap(Layer* layer) { +status_t SurfaceFlinger::removeLayerFromMap(const wp& layer) { auto it = mLayersByLocalBinderToken.begin(); while (it != mLayersByLocalBinderToken.end()) { - auto strongRef = it->second.promote(); - if (strongRef != nullptr && strongRef.get() == layer) { + if (it->second == layer) { it = mLayersByLocalBinderToken.erase(it); break; } else { @@ -3210,6 +3204,11 @@ status_t SurfaceFlinger::removeLayerFromMap(Layer* layer) { return NO_ERROR; } +status_t SurfaceFlinger::removeLayer(const sp& layer, bool topLevelOnly) { + Mutex::Autolock _l(mStateLock); + return removeLayerLocked(mStateLock, layer, topLevelOnly); +} + status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp& layer, bool topLevelOnly) { if (layer->isPendingRemoval()) { @@ -3455,8 +3454,8 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState const layer_state_t& s = composerState.state; sp client(static_cast(composerState.client.get())); - sp layer(client->getLayerUser(s.surface)); - if (layer == nullptr) { + sp layer = fromHandle(s.surface); + if (layer == nullptr || !(client->isAttached(s.surface))) { return 0; } @@ -3631,8 +3630,8 @@ void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) { const layer_state_t& state = composerState.state; sp client(static_cast(composerState.client.get())); - sp layer(client->getLayerUser(state.surface)); - if (layer == nullptr) { + sp layer = fromHandle(state.surface); + if (layer == nullptr || !(client->isAttached(state.surface))) { return; } @@ -3767,14 +3766,35 @@ status_t SurfaceFlinger::createColorLayer(const sp& client, return NO_ERROR; } +status_t SurfaceFlinger::clearLayerFrameStats(const sp& client, const sp& handle) { + Mutex::Autolock _l(mStateLock); + sp layer = fromHandle(handle); + if (layer == nullptr || !(client->isAttached(handle))) { + return NAME_NOT_FOUND; + } + layer->clearFrameStats(); + return NO_ERROR; +} + +status_t SurfaceFlinger::getLayerFrameStats(const sp& client, const sp& handle, FrameStats* outStats) { + Mutex::Autolock _l(mStateLock); + sp layer = fromHandle(handle); + if (layer == nullptr || !(client->isAttached(handle))) { + return NAME_NOT_FOUND; + } + layer->getFrameStats(outStats); + return NO_ERROR; +} + status_t SurfaceFlinger::onLayerRemoved(const sp& client, const sp& handle) { + Mutex::Autolock _l(mStateLock); // called by a client when it wants to remove a Layer status_t err = NO_ERROR; - sp l(client->getLayerUser(handle)); - if (l != nullptr) { + sp l = fromHandle(handle); + if (l != nullptr || client->isAttached(handle)) { mInterceptor->saveSurfaceDeletion(l); - err = removeLayer(l); + err = removeLayerLocked(mStateLock, l); ALOGE_IF(err<0 && err != NAME_NOT_FOUND, "error removing layer=%p (%s)", l.get(), strerror(-err)); } @@ -3783,15 +3803,18 @@ status_t SurfaceFlinger::onLayerRemoved(const sp& client, const sp& layer) { + Mutex::Autolock _l(mStateLock); // called by ~LayerCleaner() when all references to the IBinder (handle) // are gone sp l = layer.promote(); if (l == nullptr) { + removeLayerFromMap(layer); // The layer has already been removed, carry on return NO_ERROR; } + removeLayerFromMap(layer); // If we have a parent, then we can continue to live as long as it does. - return removeLayer(l, true); + return removeLayerLocked(mStateLock, l, true); } // --------------------------------------------------------------------------- diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 11cdad1f8d..1f8c205466 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -347,6 +347,10 @@ public: int getPrimaryDisplayOrientation() const { return mPrimaryDisplayOrientation; } + status_t clearLayerFrameStats(const sp& client, const sp& handle); + + status_t getLayerFrameStats(const sp& client, const sp& handle, FrameStats* outStats); + sp fromHandle(const sp& handle) REQUIRES(mStateLock); private: @@ -523,9 +527,9 @@ private: uint32_t setTransactionFlags(uint32_t flags, VSyncModulator::TransactionStart transactionStart); void commitTransaction(); bool containsAnyInvalidClientState(const Vector& states); - uint32_t setClientStateLocked(const ComposerState& composerState); + uint32_t setClientStateLocked(const ComposerState& composerState) REQUIRES(mStateLock); uint32_t setDisplayStateLocked(const DisplayState& s); - void setDestroyStateLocked(const ComposerState& composerState); + void setDestroyStateLocked(const ComposerState& composerState) REQUIRES(mStateLock); /* ------------------------------------------------------------------------ * Layer management @@ -560,7 +564,7 @@ private: status_t removeLayerLocked(const Mutex&, const sp& layer, bool topLevelOnly = false); // remove layer from mapping - status_t removeLayerFromMap(Layer* layer); + status_t removeLayerFromMap(const wp& layer); // add a layer to SurfaceFlinger status_t addClientLayer(const sp& client, const sp& handle, -- cgit v1.2.3