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

Commit 2d9a429a authored by chaviw's avatar chaviw
Browse files

Avoid promoting weak reference of Layer on Binder thread. 

BufferQueueLayer registers itself to ConsumerBase so it can receive
callbacks about incoming frames. When doing this, ConsumerBase holds a
weak reference to BufferQueueLayer. This would normally be fine except
ConsumerBase promotes the weak pointer on a Binder thread.

This leads to a race where the layer is ready to get destroyed in
SurfaceFlinger, but ConsumerBase promotes the layer. The layer will get
destroyed after the function finishes, but now it gets destroyed off the
main thread. When this happens, a lot of unexpected behavior occurs
since the layer destructor expects the call to only happen on the main
thread and therefore, doesn't hold any locks.

Instead of giving ConsumerBase a weak reference to the layer, we can
give it a reference to a listener. This listener can be managed by
BufferQueueLayer and will only hold a raw reference to the Layer.
ConsumerBase can promote the listener, but it will not be holding a
strong reference to the Layer, allowing the layer to get destroyed on
the main thread.

We hold a lock when accessing the raw reference to ensure it's not being
destroyed. The raw reference is nulled when the layer is getting
destroyed to ensure no more calls are made to the layer after it's
destroyed.

Fixes: 143735125
Fixes: 143200062
Test: go/wm-smoke
Change-Id: I47122b0e4c6d73f23a64027fd3622b2d7fd28871
parent 28d8c7db

services/surfaceflinger/BufferQueueLayer.cpp

+57 −1
Original line number Diff line number Diff line
@@ -35,6 +35,7 @@ namespace android { 
BufferQueueLayer::BufferQueueLayer(const LayerCreationArgs& args) : BufferLayer(args) {}



BufferQueueLayer::~BufferQueueLayer() {

    mContentsChangedListener->abandon();

    mConsumer->abandon();

}
@@ -480,7 +481,9 @@ void BufferQueueLayer::onFirstRef() { 
            mFlinger->getFactory().createBufferLayerConsumer(consumer, mFlinger->getRenderEngine(),

                                                             mTextureName, this);

    mConsumer->setConsumerUsageBits(getEffectiveUsage(0));

    mConsumer->setContentsChangedListener(this);



    mContentsChangedListener = new ContentsChangedListener(this);

    mConsumer->setContentsChangedListener(mContentsChangedListener);

    mConsumer->setName(String8(mName.data(), mName.size()));



    // BufferQueueCore::mMaxDequeuedBufferCount is default to 1
@@ -552,4 +555,57 @@ sp<Layer> BufferQueueLayer::createClone() { 
    return layer;

}



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

// Interface implementation for BufferLayerConsumer::ContentsChangedListener

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



void BufferQueueLayer::ContentsChangedListener::onFrameAvailable(const BufferItem& item) {

    Mutex::Autolock lock(mMutex);

    if (mBufferQueueLayer != nullptr) {

        mBufferQueueLayer->onFrameAvailable(item);

    }

}



void BufferQueueLayer::ContentsChangedListener::onFrameReplaced(const BufferItem& item) {

    Mutex::Autolock lock(mMutex);

    if (mBufferQueueLayer != nullptr) {

        mBufferQueueLayer->onFrameReplaced(item);

    }

}



void BufferQueueLayer::ContentsChangedListener::onSidebandStreamChanged() {

    Mutex::Autolock lock(mMutex);

    if (mBufferQueueLayer != nullptr) {

        mBufferQueueLayer->onSidebandStreamChanged();

    }

}



void BufferQueueLayer::ContentsChangedListener::onFrameDequeued(const uint64_t bufferId) {

    Mutex::Autolock lock(mMutex);

    if (mBufferQueueLayer != nullptr) {

        mBufferQueueLayer->onFrameDequeued(bufferId);

    }

}



void BufferQueueLayer::ContentsChangedListener::onFrameDetached(const uint64_t bufferId) {

    Mutex::Autolock lock(mMutex);

    if (mBufferQueueLayer != nullptr) {

        mBufferQueueLayer->onFrameDetached(bufferId);

    }

}



void BufferQueueLayer::ContentsChangedListener::onFrameCancelled(const uint64_t bufferId) {

    Mutex::Autolock lock(mMutex);

    if (mBufferQueueLayer != nullptr) {

        mBufferQueueLayer->onFrameCancelled(bufferId);

    }

}



void BufferQueueLayer::ContentsChangedListener::abandon() {

    Mutex::Autolock lock(mMutex);

    mBufferQueueLayer = nullptr;

}



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



} // namespace android

services/surfaceflinger/BufferQueueLayer.h

+31 −10
Original line number Diff line number Diff line
@@ -29,7 +29,7 @@ namespace android { 
 * This also implements onFrameAvailable(), which notifies SurfaceFlinger

 * that new data has arrived.

 */

class BufferQueueLayer : public BufferLayer, public BufferLayerConsumer::ContentsChangedListener {

class BufferQueueLayer : public BufferLayer {

public:

    // Only call while mStateLock is held

    explicit BufferQueueLayer(const LayerCreationArgs&);
@@ -84,18 +84,37 @@ private: 
    void latchPerFrameState(compositionengine::LayerFECompositionState&) const override;

    sp<Layer> createClone() override;



    void onFrameAvailable(const BufferItem& item);

    void onFrameReplaced(const BufferItem& item);

    void onSidebandStreamChanged();

    void onFrameDequeued(const uint64_t bufferId);

    void onFrameDetached(const uint64_t bufferId);

    void onFrameCancelled(const uint64_t bufferId);



protected:

    void gatherBufferInfo() override;



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

    // Interface implementation for BufferLayerConsumer::ContentsChangedListener

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

protected:

    void gatherBufferInfo() override;

    class ContentsChangedListener : public BufferLayerConsumer::ContentsChangedListener {

    public:

        ContentsChangedListener(BufferQueueLayer* bufferQueueLayer)

              : mBufferQueueLayer(bufferQueueLayer) {}

        void abandon();



    protected:

        void onFrameAvailable(const BufferItem& item) override;

        void onFrameReplaced(const BufferItem& item) override;

        void onSidebandStreamChanged() override;

        void onFrameDequeued(const uint64_t bufferId) override;

        void onFrameDetached(const uint64_t bufferId) override;

        void onFrameCancelled(const uint64_t bufferId) override;



    private:

        BufferQueueLayer* mBufferQueueLayer = nullptr;

        Mutex mMutex;

    };

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



public:
@@ -130,6 +149,8 @@ private: 
    // thread-safe

    std::atomic<int32_t> mQueuedFrames{0};

    std::atomic<bool> mSidebandStreamChanged{false};



    sp<ContentsChangedListener> mContentsChangedListener;

};



} // namespace android