Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 22851c3b authored by Dan Stoza's avatar Dan Stoza Committed by Irvel
Browse files

SF: Fix a couple of Layer ref count issues 

This is an attempt at fixing two reference counting issues for Layers.

The first issue is that since we were holding an sp<IBinder> (really a
reference to a LayerCleaner) inside the layer state for deferred
transactions, there was a possibility that it could end up being the
last strong reference to the LayerCleaner such that when it was
destroyed while applying a non-deferred transaction, it would attempt
to grab the SurfaceFlinger main lock to destroy its Layer. Since this
occurred in the main SurfaceFlinger loop, which was already holding
the lock to process transactions, this would cause a deadlock.

To fix this, the sp<IBinder> inside the layer state was changed to a
wp<IBinder>, only being promoted when it actually needs to be accessed
(i.e., when the deferred transaction is created).

The second issue is that we were promoting and holding a strong
reference to a Layer before calling into SurfaceFlinger to destroy it
on the onLayerDestroyed path (triggered when a LayerCleaner is
destroyed). After returning from the attempt to grab the SurfaceFlinger
main lock, it was possible that this strong reference was the last one
keeping the Layer alive, and destroying it at this point could cause
the HWC2 version of the layer to be destroyed at effectively any point,
even between validate/present.

To fix this, the promotion of the weak Layer reference was moved inside
the critical section where the SurfaceFlinger main lock is held.

Bug: 30503916
Bug: 30281222
Change-Id: I1c6a271f9a7b5d6eea9a9db61d971f262d0cfe84
parent ab046855

services/surfaceflinger/Client.cpp

+1 −4
Original line number Original line Diff line number Diff line
@@ -43,10 +43,7 @@ Client::~Client() 
{

{

    const size_t count = mLayers.size();

    const size_t count = mLayers.size();

    for (size_t i=0 ; i<count ; i++) {

    for (size_t i=0 ; i<count ; i++) {

        sp<Layer> layer(mLayers.valueAt(i).promote());

        mFlinger->removeLayer(mLayers.valueAt(i));

        if (layer != 0) {

            mFlinger->removeLayer(layer);

        }

    }

    }

}

}

services/surfaceflinger/Layer.cpp

+8 −3
Original line number Original line Diff line number Diff line
@@ -1313,9 +1313,14 @@ void Layer::pushPendingState() { 
    // If this transaction is waiting on the receipt of a frame, generate a sync

    // If this transaction is waiting on the receipt of a frame, generate a sync

    // point and send it to the remote layer.

    // point and send it to the remote layer.

    if (mCurrentState.handle != nullptr) {

    if (mCurrentState.handle != nullptr) {

        sp<Handle> handle = static_cast<Handle*>(mCurrentState.handle.get());

        sp<IBinder> strongBinder = mCurrentState.handle.promote();

        sp<Layer> handleLayer = handle->owner.promote();

        sp<Handle> handle = nullptr;

        if (handleLayer == nullptr) {

        sp<Layer> handleLayer = nullptr;

        if (strongBinder != nullptr) {

            handle = static_cast<Handle*>(strongBinder.get());

            handleLayer = handle->owner.promote();

        }

        if (strongBinder == nullptr || handleLayer == nullptr) {

            ALOGE("[%s] Unable to promote Layer handle", mName.string());

            ALOGE("[%s] Unable to promote Layer handle", mName.string());

            // If we can't promote the layer we are intended to wait on,

            // If we can't promote the layer we are intended to wait on,

            // then it is expired or otherwise invalid. Allow this transaction

            // then it is expired or otherwise invalid. Allow this transaction

services/surfaceflinger/Layer.h

+1 −1
Original line number Original line Diff line number Diff line
@@ -128,7 +128,7 @@ public: 




        // If set, defers this state update until the Layer identified by handle

        // If set, defers this state update until the Layer identified by handle

        // receives a frame with the given frameNumber

        // receives a frame with the given frameNumber

        sp<IBinder> handle;

        wp<IBinder> handle;

        uint64_t frameNumber;

        uint64_t frameNumber;





        // the transparentRegion hint is a bit special, it's latched only

        // the transparentRegion hint is a bit special, it's latched only

services/surfaceflinger/SurfaceFlinger.cpp

+8 −9
Original line number Original line Diff line number Diff line
@@ -2283,8 +2283,14 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, 
    return NO_ERROR;

    return NO_ERROR;

}

}





status_t SurfaceFlinger::removeLayer(const sp<Layer>& layer) {

status_t SurfaceFlinger::removeLayer(const wp<Layer>& weakLayer) {

    Mutex::Autolock _l(mStateLock);

    Mutex::Autolock _l(mStateLock);

    sp<Layer> layer = weakLayer.promote();

    if (layer == nullptr) {

        // The layer has already been removed, carry on

        return NO_ERROR;

    }



    ssize_t index = mCurrentState.layersSortedByZ.remove(layer);

    ssize_t index = mCurrentState.layersSortedByZ.remove(layer);

    if (index >= 0) {

    if (index >= 0) {

        mLayersPendingRemoval.push(layer);

        mLayersPendingRemoval.push(layer);
@@ -2630,14 +2636,7 @@ status_t SurfaceFlinger::onLayerDestroyed(const wp<Layer>& layer) 
{

{

    // called by ~LayerCleaner() when all references to the IBinder (handle)

    // called by ~LayerCleaner() when all references to the IBinder (handle)

    // are gone

    // are gone

    status_t err = NO_ERROR;

    return removeLayer(layer);

    sp<Layer> l(layer.promote());

    if (l != NULL) {

        err = removeLayer(l);

        ALOGE_IF(err<0 && err != NAME_NOT_FOUND,

                "error removing layer=%p (%s)", l.get(), strerror(-err));

    }

    return err;

}

}





// ---------------------------------------------------------------------------

// ---------------------------------------------------------------------------

services/surfaceflinger/SurfaceFlinger.h

+1 −1
Original line number Original line Diff line number Diff line
@@ -314,7 +314,7 @@ private: 
    status_t onLayerDestroyed(const wp<Layer>& layer);

    status_t onLayerDestroyed(const wp<Layer>& layer);





    // remove a layer from SurfaceFlinger immediately

    // remove a layer from SurfaceFlinger immediately

    status_t removeLayer(const sp<Layer>& layer);

    status_t removeLayer(const wp<Layer>& layer);





    // add a layer to SurfaceFlinger

    // add a layer to SurfaceFlinger

    status_t addClientLayer(const sp<Client>& client,

    status_t addClientLayer(const sp<Client>& client,