SurfaceFlinger: Share ownership of layers between State and Handle.

Currently the only strong reference to a Layer is held by their parent
or the containing layer stack. Firstly, it means that we can not
create a SurfaceControl with a null parent. For example, a
media decoding library may wish to offer an unparented SurfaceControl
representing the decoding Surface, and allow the consumer to parent
it between various SurfaceControls as they wish. Secondly it requires
lifetime coordination between various levels of the hierarchy, when adding
a child you have to be careful to observe the lifetime of the parent because
your BufferQueue may become suddenly abandoned at any point. In this change
we switch to a reference counted model, such that the Layer remains valid
as long as there is a handle to it. We also end the behavior of passing on
BufferQueue abandon to children. Layers are only abandoned/disposed when
an explicit call to remove is made, or the last reference is dropped.
In this CL we switch the handle to holding a strong pointer.
We have the handle and the current state forward their references
to the main thread to be dropped. We move the contents of onRemoved
to the destructor to ensure they are dropped when the last reference
is dropped. A second CL will replace the concept of "onRemovedFromCurrentState"
with the concept of parent=null, and remove the explicit destroy method,
replacing it with an IPC reparent to null. Two additional refactorings are
required to follow the other changes. First since we moved mNumLayers to
~Layer it will be executed for the fake parent layer created during screenshots
but mNumLayers++ (in addClientLayer) will not be incremented. The most
straightforward solution is to balance mNumLayers from the c'tor/d'tor.
Second since abandon() is virtual calling it from the d'tor is not safe.
We move the call to BufferQueueLayer and remove the abstract method entirely.

Bug: 62536731
Bug: 111373437
Bug: 111297488
Test: Transaction_test.cpp
Change-Id: I3e815eb9cee39f6def1ef459811448698a93f5c8

7 files changed, 118 insertions, 97 deletions

diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp
index e592a8bf98..c130bc5105 100644
--- a/services/surfaceflinger/BufferQueueLayer.cpp
+++ b/services/surfaceflinger/BufferQueueLayer.cpp
@@ -23,7 +23,9 @@ namespace android {
 
 BufferQueueLayer::BufferQueueLayer(const LayerCreationArgs& args) : BufferLayer(args) {}
 
-BufferQueueLayer::~BufferQueueLayer() = default;
+BufferQueueLayer::~BufferQueueLayer() {
+    mConsumer->abandon();
+}
 
 // -----------------------------------------------------------------------
 // Interface implementation for Layer
@@ -33,10 +35,6 @@ void BufferQueueLayer::onLayerDisplayed(const sp<Fence>& releaseFence) {
     mConsumer->setReleaseFence(releaseFence);
 }
 
-void BufferQueueLayer::abandon() {
-    mConsumer->abandon();
-}
-
 void BufferQueueLayer::setTransformHint(uint32_t orientation) const {
     mConsumer->setTransformHint(orientation);
 }
@@ -380,7 +378,17 @@ void BufferQueueLayer::onFrameAvailable(const BufferItem& item) {
 
     mFlinger->mInterceptor->saveBufferUpdate(this, item.mGraphicBuffer->getWidth(),
                                              item.mGraphicBuffer->getHeight(), item.mFrameNumber);
-    mFlinger->signalLayerUpdate();
+    
+    // If this layer is orphaned, then we run a fake vsync pulse so that
+    // dequeueBuffer doesn't block indefinitely.
+    if (isRemovedFromCurrentState()) {
+        bool ignored = false;
+        latchBuffer(ignored, systemTime(), Fence::NO_FENCE);
+        usleep(16000);
+        releasePendingBuffer(systemTime());
+    } else {
+        mFlinger->signalLayerUpdate();
+    }
 }
 
 void BufferQueueLayer::onFrameReplaced(const BufferItem& item) {
diff --git a/services/surfaceflinger/BufferQueueLayer.h b/services/surfaceflinger/BufferQueueLayer.h
index abe0bc7c0a..c9ebe042b8 100644
--- a/services/surfaceflinger/BufferQueueLayer.h
+++ b/services/surfaceflinger/BufferQueueLayer.h
@@ -40,8 +40,6 @@ public:
 public:
     void onLayerDisplayed(const sp<Fence>& releaseFence) override;
 
-    void abandon() override;
-
     void setTransformHint(uint32_t orientation) const override;
 
     std::vector<OccupancyTracker::Segment> getOccupancyHistory(bool forceFlush) override;
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 2e564e7d94..f29dfc0f1b 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -111,6 +111,8 @@ Layer::Layer(const LayerCreationArgs& args)
     args.flinger->getCompositorTiming(&compositorTiming);
     mFrameEventHistory.initializeCompositorTiming(compositorTiming);
     mFrameTracker.setDisplayRefreshPeriod(compositorTiming.interval);
+
+    mFlinger->onLayerCreated();
 }
 
 Layer::~Layer() {
@@ -119,13 +121,11 @@ Layer::~Layer() {
         c->detachLayer(this);
     }
 
-    for (auto& point : mRemoteSyncPoints) {
-        point->setTransactionApplied();
-    }
-    for (auto& point : mLocalSyncPoints) {
-        point->setFrameAvailable();
-    }
     mFrameTracker.logAndResetStats(mName);
+
+    destroyAllHwcLayers();
+
+    mFlinger->onLayerDestroyed();
 }
 
 // ---------------------------------------------------------------------------
@@ -140,10 +140,9 @@ Layer::~Layer() {
 void Layer::onLayerDisplayed(const sp<Fence>& /*releaseFence*/) {}
 
 void Layer::onRemovedFromCurrentState() {
-    // the layer is removed from SF mCurrentState to mLayersPendingRemoval
-
-    mPendingRemoval = true;
+    mRemovedFromCurrentState = true;
 
+    // the layer is removed from SF mCurrentState to mLayersPendingRemoval
     if (mCurrentState.zOrderRelativeOf != nullptr) {
         sp<Layer> strongRelative = mCurrentState.zOrderRelativeOf.promote();
         if (strongRelative != nullptr) {
@@ -153,19 +152,26 @@ void Layer::onRemovedFromCurrentState() {
         mCurrentState.zOrderRelativeOf = nullptr;
     }
 
-    for (const auto& child : mCurrentChildren) {
-        child->onRemovedFromCurrentState();
+    // Since we are no longer reachable from CurrentState SurfaceFlinger
+    // will no longer invoke doTransaction for us, and so we will
+    // never finish applying transactions. We signal the sync point
+    // now so that another layer will not become indefinitely
+    // blocked.
+    for (auto& point: mRemoteSyncPoints) {
+        point->setTransactionApplied();
     }
-}
+    mRemoteSyncPoints.clear();
 
-void Layer::onRemoved() {
-    // the layer is removed from SF mLayersPendingRemoval
-    abandon();
-
-    destroyAllHwcLayers();
+    {
+    Mutex::Autolock syncLock(mLocalSyncPointMutex);
+    for (auto& point : mLocalSyncPoints) {
+        point->setFrameAvailable();
+    }
+    mLocalSyncPoints.clear();
+    }
 
     for (const auto& child : mCurrentChildren) {
-        child->onRemoved();
+        child->onRemovedFromCurrentState();
     }
 }
 
@@ -228,6 +234,10 @@ void Layer::destroyAllHwcLayers() {
     }
     LOG_ALWAYS_FATAL_IF(!getBE().mHwcLayers.empty(),
                         "All hardware composer layers should have been destroyed");
+
+    for (const sp<Layer>& child : mDrawingChildren) {
+        child->destroyAllHwcLayers();
+    }
 }
 
 Rect Layer::getContentCrop() const {
@@ -752,6 +762,9 @@ bool Layer::addSyncPoint(const std::shared_ptr<SyncPoint>& point) {
         // relevant frame
         return false;
     }
+    if (isRemovedFromCurrentState()) {
+        return false;
+    }
 
     Mutex::Autolock lock(mLocalSyncPointMutex);
     mLocalSyncPoints.push_back(point);
@@ -825,7 +838,9 @@ void Layer::pushPendingState() {
 
     // If this transaction is waiting on the receipt of a frame, generate a sync
     // point and send it to the remote layer.
-    if (mCurrentState.barrierLayer_legacy != nullptr) {
+    // We don't allow installing sync points after we are removed from the current state
+    // as we won't be able to signal our end.
+    if (mCurrentState.barrierLayer_legacy != nullptr && !isRemovedFromCurrentState()) {
         sp<Layer> barrierLayer = mCurrentState.barrierLayer_legacy.promote();
         if (barrierLayer == nullptr) {
             ALOGE("[%s] Unable to promote barrier Layer.", mName.string());
@@ -1994,6 +2009,10 @@ void Layer::writeToProto(LayerProto* layerInfo, int32_t displayId) {
     }
 }
 
+bool Layer::isRemovedFromCurrentState() const  {
+    return mRemovedFromCurrentState;
+}
+
 // ---------------------------------------------------------------------------
 
 }; // namespace android
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 5d05f0530b..12671ff51d 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -346,7 +346,7 @@ public:
     virtual bool isCreatedFromMainThread() const { return false; }
 
 
-    bool isPendingRemoval() const { return mPendingRemoval; }
+    bool isRemovedFromCurrentState() const;
 
     void writeToProto(LayerProto* layerInfo,
                       LayerVector::StateSet stateSet = LayerVector::StateSet::Drawing);
@@ -394,8 +394,6 @@ public:
      */
     virtual void onLayerDisplayed(const sp<Fence>& releaseFence);
 
-    virtual void abandon() {}
-
     virtual bool shouldPresentNow(nsecs_t /*expectedPresentTime*/) const { return false; }
     virtual void setTransformHint(uint32_t /*orientation*/) const { }
 
@@ -475,12 +473,6 @@ public:
      */
     void onRemovedFromCurrentState();
 
-    /*
-     * called with the state lock from the main thread when the layer is
-     * removed from the pending removal list
-     */
-    void onRemoved();
-
     // Updates the transform hint in our SurfaceFlingerConsumer to match
     // the current orientation of the display device.
     void updateTransformHint(const sp<const DisplayDevice>& display) const;
@@ -595,12 +587,12 @@ protected:
      */
     class LayerCleaner {
         sp<SurfaceFlinger> mFlinger;
-        wp<Layer> mLayer;
+        sp<Layer> mLayer;
 
     protected:
         ~LayerCleaner() {
             // destroy client resources
-            mFlinger->onLayerDestroyed(mLayer);
+            mFlinger->onHandleDestroyed(mLayer);
         }
 
     public:
@@ -702,6 +694,8 @@ public:
     virtual PixelFormat getPixelFormat() const { return PIXEL_FORMAT_NONE; }
     bool getPremultipledAlpha() const;
 
+    bool mPendingHWCDestroy{false};
+
 protected:
     // -----------------------------------------------------------------------
     bool usingRelativeZ(LayerVector::StateSet stateSet);
@@ -745,7 +739,7 @@ protected:
     // Whether filtering is needed b/c of the drawingstate
     bool mNeedsFiltering{false};
 
-    bool mPendingRemoval{false};
+    std::atomic<bool> mRemovedFromCurrentState{false};
 
     // page-flip thread (currently main thread)
     bool mProtectedByApp{false}; // application requires protected path to external sink
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 5c31ada5ef..f8ad156254 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2735,7 +2735,15 @@ void SurfaceFlinger::commitTransaction()
         for (const auto& l : mLayersPendingRemoval) {
             recordBufferingStats(l->getName().string(),
                     l->getOccupancyHistory(true));
-            l->onRemoved();
+
+            // We need to release the HWC layers when the Layer is removed
+            // from the current state otherwise the HWC layer just continues
+            // showing at its last configured state until we eventually
+            // abandon the buffer queue.
+            if (l->isRemovedFromCurrentState()) {
+                l->destroyAllHwcLayers();
+                l->releasePendingBuffer(systemTime());
+            }
         }
         mLayersPendingRemoval.clear();
     }
@@ -3168,7 +3176,7 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client,
         if (parent == nullptr) {
             mCurrentState.layersSortedByZ.add(lbc);
         } else {
-            if (parent->isPendingRemoval()) {
+            if (parent->isRemovedFromCurrentState()) {
                 ALOGE("addClientLayer called with a removed parent");
                 return NAME_NOT_FOUND;
             }
@@ -3184,7 +3192,6 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client,
                                 mMaxGraphicBufferProducerListSize, mNumLayers);
         }
         mLayersAdded = true;
-        mNumLayers++;
     }
 
     // attach this layer to the client
@@ -3198,52 +3205,22 @@ status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer, bool topLevelOnly)
     return removeLayerLocked(mStateLock, layer, topLevelOnly);
 }
 
-status_t SurfaceFlinger::removeLayerLocked(const Mutex&, const sp<Layer>& layer,
+status_t SurfaceFlinger::removeLayerLocked(const Mutex& lock, const sp<Layer>& layer,
                                            bool topLevelOnly) {
-    if (layer->isPendingRemoval()) {
-        return NO_ERROR;
-    }
-
     const auto& p = layer->getParent();
     ssize_t index;
     if (p != nullptr) {
         if (topLevelOnly) {
             return NO_ERROR;
         }
-
-        sp<Layer> ancestor = p;
-        while (ancestor->getParent() != nullptr) {
-            ancestor = ancestor->getParent();
-        }
-        if (mCurrentState.layersSortedByZ.indexOf(ancestor) < 0) {
-            ALOGE("removeLayer called with a layer whose parent has been removed");
-            return NAME_NOT_FOUND;
-        }
-
         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
-    // so we will succeed in promoting it, but it's already been removed
-    // from mCurrentState. As long as we can find it in mDrawingState we have no problem
-    // otherwise something has gone wrong and we are leaking the layer.
-    if (index < 0 && mDrawingState.layersSortedByZ.indexOf(layer) < 0) {
-        ALOGE("Failed to find layer (%s) in layer parent (%s).",
-                layer->getName().string(),
-                (p != nullptr) ? p->getName().string() : "no-parent");
-        return BAD_VALUE;
-    } else if (index < 0) {
-        return NO_ERROR;
-    }
-
     layer->onRemovedFromCurrentState();
-    mLayersPendingRemoval.add(layer);
-    mLayersRemoved = true;
-    mNumLayers -= 1 + layer->getChildrenCount();
-    setTransactionFlags(eTransactionNeeded);
+
+    markLayerPendingRemovalLocked(lock, layer);
     return NO_ERROR;
 }
 
@@ -3444,11 +3421,6 @@ uint32_t SurfaceFlinger::setClientStateLocked(const ComposerState& composerState
         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;
@@ -3652,11 +3624,6 @@ void SurfaceFlinger::setDestroyStateLocked(const ComposerState& composerState) {
         return;
     }
 
-    if (layer->isPendingRemoval()) {
-        ALOGW("Attempting to destroy on removed layer: %s", layer->getName().string());
-        return;
-    }
-
     if (state.what & layer_state_t::eDestroySurface) {
         removeLayerLocked(mStateLock, layer);
     }
@@ -3830,17 +3797,16 @@ status_t SurfaceFlinger::onLayerRemoved(const sp<Client>& client, const sp<IBind
     return err;
 }
 
-status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer)
+void SurfaceFlinger::markLayerPendingRemovalLocked(const Mutex&, const sp<Layer>& layer) {
+    mLayersPendingRemoval.add(layer);
+    mLayersRemoved = true;
+    setTransactionFlags(eTransactionNeeded);
+}
+
+void SurfaceFlinger::onHandleDestroyed(const sp<Layer>& layer)
 {
-    // called by ~LayerCleaner() when all references to the IBinder (handle)
-    // are gone
-    sp<Layer> l = layer.promote();
-    if (l == nullptr) {
-        // The layer has already been removed, carry on
-        return NO_ERROR;
-    }
-    // If we have a parent, then we can continue to live as long as it does.
-    return removeLayer(l, true);
+    Mutex::Autolock lock(mStateLock);
+    markLayerPendingRemovalLocked(mStateLock, layer);
 }
 
 // ---------------------------------------------------------------------------
@@ -5189,7 +5155,7 @@ status_t SurfaceFlinger::captureLayers(const sp<IBinder>& layerHandleBinder,
     auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
     auto parent = layerHandle->owner.promote();
 
-    if (parent == nullptr || parent->isPendingRemoval()) {
+    if (parent == nullptr || parent->isRemovedFromCurrentState()) {
         ALOGE("captureLayers called with a removed parent");
         return NAME_NOT_FOUND;
     }
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index d60765cc1a..87ce996836 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -364,6 +364,9 @@ public:
     bool authenticateSurfaceTextureLocked(
         const sp<IGraphicBufferProducer>& bufferProducer) const;
 
+    inline void onLayerCreated() { mNumLayers++; }
+    inline void onLayerDestroyed() { mNumLayers--; }
+
 private:
     friend class Client;
     friend class DisplayEventConnection;
@@ -575,10 +578,12 @@ private:
     // ISurfaceComposerClient::destroySurface()
     status_t onLayerRemoved(const sp<Client>& client, const sp<IBinder>& handle);
 
+    void markLayerPendingRemovalLocked(const Mutex& /* mStateLock */, const sp<Layer>& layer);
+
     // called when all clients have released all their references to
     // this layer meaning it is entirely safe to destroy all
     // resources associated to this layer.
-    status_t onLayerDestroyed(const wp<Layer>& layer);
+    void onHandleDestroyed(const sp<Layer>& layer);
 
     // remove a layer from SurfaceFlinger immediately
     status_t removeLayer(const sp<Layer>& layer, bool topLevelOnly = false);
diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp
index c814142f49..57a2227593 100644
--- a/services/surfaceflinger/tests/Transaction_test.cpp
+++ b/services/surfaceflinger/tests/Transaction_test.cpp
@@ -2614,6 +2614,37 @@ TEST_F(ChildLayerTest, ReparentChildren) {
     }
 }
 
+TEST_F(ChildLayerTest, ChildrenSurviveParentDestruction) {
+    sp<SurfaceControl> mGrandChild =
+        mClient->createSurface(String8("Grand Child"), 10, 10,
+                PIXEL_FORMAT_RGBA_8888, 0, mChild.get());
+    fillSurfaceRGBA8(mGrandChild, 111, 111, 111);
+
+    {
+        SCOPED_TRACE("Grandchild visible");
+        ScreenCapture::captureScreen(&mCapture);
+        mCapture->checkPixel(64, 64, 111, 111, 111);
+    }
+
+    mChild->clear();
+
+    {
+        SCOPED_TRACE("After destroying child");
+        ScreenCapture::captureScreen(&mCapture);
+        mCapture->expectFGColor(64, 64);
+    }
+
+    asTransaction([&](Transaction& t) {
+         t.reparent(mGrandChild, mFGSurfaceControl->getHandle());
+    });
+
+    {
+        SCOPED_TRACE("After reparenting grandchild");
+        ScreenCapture::captureScreen(&mCapture);
+        mCapture->checkPixel(64, 64, 111, 111, 111);
+    }
+}
+
 TEST_F(ChildLayerTest, DetachChildrenSameClient) {
     asTransaction([&](Transaction& t) {
         t.show(mChild);