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

Commit dbc93d7a authored by Garfield Tan's avatar Garfield Tan
Browse files

Save Layer instead of Handle to LayerCreatedState 

Saving Handle to LayerCreatedState can lead to the destruction of Handle
when handleLayerCreatedLocked returns if the client side releases the
Handle before that. That destruction will attempt to acquire mStateLock
in the main thread again and lead to a deadlock.

Not saving Handle to LayerCreatedState will avoid Handle being destroyed
in the main thread. The destruction of Layer in the main thread is OK.

To make that happen I moved the final resolution of addToRoot to
createLayer as well because the treatments for invalid parentHandle
and invalid parentLayer are different.

Bug: 204204635
Bug: 202621651
Test: atest SurfaceFlinger_test
Test: atest libsurfaceflinger_unittest
Change-Id: I0854203c0949302d7f514d34b956eb7ae976c51f
parent b47287e4

services/surfaceflinger/SurfaceFlinger.cpp

+18 −17
Original line number Diff line number Diff line
@@ -3356,8 +3356,7 @@ bool SurfaceFlinger::latchBuffers() { 


status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,

                                        const sp<IGraphicBufferProducer>& gbc, const sp<Layer>& lbc,

                                        const sp<IBinder>& parentHandle,

                                        const sp<Layer>& parentLayer, bool addToRoot,

                                        const wp<Layer>& parent, bool addToRoot,

                                        uint32_t* outTransformHint) {

    if (mNumLayers >= ISurfaceComposer::MAX_LAYERS) {

        ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers.load(),
@@ -3369,7 +3368,7 @@ status_t SurfaceFlinger::addClientLayer(const sp<Client>& client, const sp<IBind 
    if (gbc != nullptr) {

        initialProducer = IInterface::asBinder(gbc);

    }

    setLayerCreatedState(handle, lbc, parentHandle, parentLayer, initialProducer, addToRoot);

    setLayerCreatedState(handle, lbc, parent, initialProducer, addToRoot);



    // Create a transaction includes the initial parent and producer.

    Vector<ComposerState> states;
@@ -4227,7 +4226,7 @@ status_t SurfaceFlinger::mirrorLayer(const sp<Client>& client, const sp<IBinder> 
    }



    *outLayerId = mirrorLayer->sequence;

    return addClientLayer(client, *outHandle, nullptr, mirrorLayer, nullptr, nullptr, false,

    return addClientLayer(client, *outHandle, nullptr, mirrorLayer, nullptr, false,

                          nullptr /* outTransformHint */);

}
@@ -4296,8 +4295,15 @@ status_t SurfaceFlinger::createLayer(const String8& name, const sp<Client>& clie 
    }



    bool addToRoot = callingThreadHasUnscopedSurfaceFlingerAccess();

    result = addClientLayer(client, *handle, *gbp, layer, parentHandle, parentLayer, addToRoot,

                            outTransformHint);

    wp<Layer> parent(parentHandle != nullptr ? fromHandle(parentHandle) : parentLayer);

    if (parentHandle != nullptr && parent == nullptr) {

        ALOGE("Invalid parent handle %p.", parentHandle.get());

        addToRoot = false;

    }

    if (parentLayer != nullptr) {

        addToRoot = false;

    }

    result = addClientLayer(client, *handle, *gbp, layer, parent, addToRoot, outTransformHint);

    if (result != NO_ERROR) {

        return result;

    }
@@ -6777,11 +6783,11 @@ void TransactionState::traverseStatesWithBuffers( 
}



void SurfaceFlinger::setLayerCreatedState(const sp<IBinder>& handle, const wp<Layer>& layer,

                                          const sp<IBinder>& parent, const wp<Layer> parentLayer,

                                          const wp<IBinder>& producer, bool addToRoot) {

                                          const wp<Layer> parent, const wp<IBinder>& producer,

                                          bool addToRoot) {

    Mutex::Autolock lock(mCreatedLayersLock);

    mCreatedLayers[handle->localBinder()] =

            std::make_unique<LayerCreatedState>(layer, parent, parentLayer, producer, addToRoot);

            std::make_unique<LayerCreatedState>(layer, parent, producer, addToRoot);

}



auto SurfaceFlinger::getLayerCreatedState(const sp<IBinder>& handle) {
@@ -6819,19 +6825,14 @@ sp<Layer> SurfaceFlinger::handleLayerCreatedLocked(const sp<IBinder>& handle) { 
    }



    sp<Layer> parent;

    bool allowAddRoot = state->addToRoot;

    if (state->initialParent != nullptr) {

        parent = fromHandle(state->initialParent).promote();

        parent = state->initialParent.promote();

        if (parent == nullptr) {

            ALOGE("Invalid parent %p", state->initialParent.get());

            allowAddRoot = false;

            ALOGE("Invalid parent %p", state->initialParent.unsafe_get());

        }

    } else if (state->initialParentLayer != nullptr) {

        parent = state->initialParentLayer.promote();

        allowAddRoot = false;

    }



    if (parent == nullptr && allowAddRoot) {

    if (parent == nullptr && state->addToRoot) {

        layer->setIsAtRoot(true);

        mCurrentState.layersSortedByZ.add(layer);

    } else if (parent == nullptr) {

services/surfaceflinger/SurfaceFlinger.h

+6 −9
Original line number Diff line number Diff line
@@ -795,8 +795,8 @@ private: 
    // add a layer to SurfaceFlinger

    status_t addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,

                            const sp<IGraphicBufferProducer>& gbc, const sp<Layer>& lbc,

                            const sp<IBinder>& parentHandle, const sp<Layer>& parentLayer,

                            bool addToRoot, uint32_t* outTransformHint);

                            const wp<Layer>& parentLayer, bool addToRoot,

                            uint32_t* outTransformHint);



    // Traverse through all the layers and compute and cache its bounds.

    void computeLayerBounds();
@@ -1345,18 +1345,16 @@ private: 
            GUARDED_BY(mStateLock);

    mutable Mutex mCreatedLayersLock;

    struct LayerCreatedState {

        LayerCreatedState(const wp<Layer>& layer, const sp<IBinder>& parent,

                          const wp<Layer> parentLayer, const wp<IBinder>& producer, bool addToRoot)

        LayerCreatedState(const wp<Layer>& layer, const wp<Layer> parent,

                          const wp<IBinder>& producer, bool addToRoot)

              : layer(layer),

                initialParent(parent),

                initialParentLayer(parentLayer),

                initialProducer(producer),

                addToRoot(addToRoot) {}

        wp<Layer> layer;

        // Indicates the initial parent of the created layer, only used for creating layer in

        // SurfaceFlinger. If nullptr, it may add the created layer into the current root layers.

        sp<IBinder> initialParent;

        wp<Layer> initialParentLayer;

        wp<Layer> initialParent;

        // Indicates the initial graphic buffer producer of the created layer, only used for

        // creating layer in SurfaceFlinger.

        wp<IBinder> initialProducer;
@@ -1370,8 +1368,7 @@ private: 
    // thread.

    std::unordered_map<BBinder*, std::unique_ptr<LayerCreatedState>> mCreatedLayers;

    void setLayerCreatedState(const sp<IBinder>& handle, const wp<Layer>& layer,

                              const sp<IBinder>& parent, const wp<Layer> parentLayer,

                              const wp<IBinder>& producer, bool addToRoot);

                              const wp<Layer> parent, const wp<IBinder>& producer, bool addToRoot);

    auto getLayerCreatedState(const sp<IBinder>& handle);

    sp<Layer> handleLayerCreatedLocked(const sp<IBinder>& handle) REQUIRES(mStateLock);