RESTRICT AUTOMERGE: Fix removal of handle from map

Bug: 144667224, 137284057
Test: build, boot, SurfaceFlinger_test, libgui_test, manual
Change-Id: Ie0f111a6b8f4424375cc124ccd2683f5bfad7759

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<Layer> Client::getLayerUser(const sp<IBinder>& handle) const
+
+bool Client::isAttached(const sp<IBinder>& handle) const
 {
     Mutex::Autolock _l(mLock);
     sp<Layer> lbc;
     wp<Layer> 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<IBinder>* handle,
         sp<IGraphicBufferProducer>* gbp) {
     bool parentDied;
-    sp<Layer> parentLayer = getParentLayer(&parentDied);
+    sp<Layer> parentLayer;
+    if (!parentHandle) parentLayer = getParentLayer(&parentDied);
     if (parentHandle == nullptr && parentDied) {
         return NAME_NOT_FOUND;
     }
@@ -163,21 +164,11 @@ status_t Client::destroySurface(const sp<IBinder>& handle) {
 }
 
 status_t Client::clearLayerFrameStats(const sp<IBinder>& handle) const {
-    sp<Layer> 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<IBinder>& handle, FrameStats* outStats) const {
-    sp<Layer> 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<Layer> getLayerUser(const sp<IBinder>& handle) const;
+    bool isAttached (const sp<IBinder>& handle) const;
 
     void updateParent(const sp<Layer>& 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<Fence>& /*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<Layer> 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>& client, const sp<IBind
     return NO_ERROR;
 }
 
-status_t SurfaceFlinger::removeLayer(const sp<Layer>& 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>& 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>& layer, bool topLevelOnly) {
+    Mutex::Autolock _l(mStateLock);
+    return removeLayerLocked(mStateLock, layer, topLevelOnly);
+}
+
 status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp<Layer>& 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> client(static_cast<Client*>(composerState.client.get()));
 
-    sp<Layer> layer(client->getLayerUser(s.surface));
-    if (layer == nullptr) {
+    sp<Layer> 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> client(static_cast<Client*>(composerState.client.get()));
 
-    sp<Layer> layer(client->getLayerUser(state.surface));
-    if (layer == nullptr) {
+    sp<Layer> layer = fromHandle(state.surface);
+    if (layer == nullptr || !(client->isAttached(state.surface))) {
         return;
     }
 
@@ -3767,14 +3766,35 @@ status_t SurfaceFlinger::createColorLayer(const sp<Client>& client,
     return NO_ERROR;
 }
 
+status_t SurfaceFlinger::clearLayerFrameStats(const sp<const Client>& client, const sp<IBinder>& handle) {
+    Mutex::Autolock _l(mStateLock);
+    sp<Layer> layer = fromHandle(handle);
+    if (layer == nullptr || !(client->isAttached(handle))) {
+        return NAME_NOT_FOUND;
+    }
+    layer->clearFrameStats();
+    return NO_ERROR;
+}
+
+status_t SurfaceFlinger::getLayerFrameStats(const sp<const Client>& client, const sp<IBinder>& handle, FrameStats* outStats) {
+    Mutex::Autolock _l(mStateLock);
+    sp<Layer> 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>& client, const sp<IBinder>& handle)
 {
+    Mutex::Autolock _l(mStateLock);
     // called by a client when it wants to remove a Layer
     status_t err = NO_ERROR;
-    sp<Layer> l(client->getLayerUser(handle));
-    if (l != nullptr) {
+    sp<Layer> 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>& client, const sp<IBind
 
 status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer)
 {
+    Mutex::Autolock _l(mStateLock);
     // called by ~LayerCleaner() when all references to the IBinder (handle)
     // are gone
     sp<Layer> 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<const Client>& client, const sp<IBinder>& handle);
+
+    status_t getLayerFrameStats(const sp<const Client>& client, const sp<IBinder>& handle, FrameStats* outStats);
+
     sp<Layer> fromHandle(const sp<IBinder>& handle) REQUIRES(mStateLock);
 
 private:
@@ -523,9 +527,9 @@ private:
     uint32_t setTransactionFlags(uint32_t flags, VSyncModulator::TransactionStart transactionStart);
     void commitTransaction();
     bool containsAnyInvalidClientState(const Vector<ComposerState>& 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>& layer, bool topLevelOnly = false);
 
     // remove layer from mapping
-    status_t removeLayerFromMap(Layer* layer);
+    status_t removeLayerFromMap(const wp<Layer>& layer);
 
     // add a layer to SurfaceFlinger
     status_t addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,