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

Commit a79435bd authored by Rob Carr's avatar Rob Carr
Browse files

SurfaceFlinger: Avoid destroying Layer on Binder thread 

BufferQueueLayer::onFrameAvailable passes 'this' as an sp<Layer>
to SurfaceInterceptor. This constructs a temporary sp<Layer>. We are
on a binder thread and not holding any locks, so at this point the main
thread could drop it's last references. Then when we destroy our
temporary sp<Layer> it is the last reference and we end up
invoking ~Layer from the Binder thread, an invalid operation
which in this case leads to dead-lock (as we attempt to reacquire
the already acquired BufferQueue mutex from the BufferQueueLayer d'tor)

Bug: 149473038
Test: Existing tests pass
Change-Id: I77a20bedf2db3b974ac03d804f70993514478fb2
parent 34560b6d

services/surfaceflinger/BufferQueueLayer.cpp

+1 −1
Original line number Diff line number Diff line
@@ -441,7 +441,7 @@ void BufferQueueLayer::onFrameAvailable(const BufferItem& item) { 
        mQueueItemCondition.broadcast();

    }



    mFlinger->mInterceptor->saveBufferUpdate(this, item.mGraphicBuffer->getWidth(),

    mFlinger->mInterceptor->saveBufferUpdate(layerId, item.mGraphicBuffer->getWidth(),

                                             item.mGraphicBuffer->getHeight(), item.mFrameNumber);



    mFlinger->signalLayerUpdate();

services/surfaceflinger/SurfaceInterceptor.cpp

+12 −5
Original line number Diff line number Diff line
@@ -524,11 +524,11 @@ void SurfaceInterceptor::addSurfaceDeletionLocked(Increment* increment, 
    deletion->set_id(getLayerId(layer));

}



void SurfaceInterceptor::addBufferUpdateLocked(Increment* increment, const sp<const Layer>& layer,

void SurfaceInterceptor::addBufferUpdateLocked(Increment* increment, int32_t layerId,

        uint32_t width, uint32_t height, uint64_t frameNumber)

{

    BufferUpdate* update(increment->mutable_buffer_update());

    update->set_id(getLayerId(layer));

    update->set_id(layerId);

    update->set_w(width);

    update->set_h(height);

    update->set_frame_number(frameNumber);
@@ -644,15 +644,22 @@ void SurfaceInterceptor::saveSurfaceDeletion(const sp<const Layer>& layer) { 
    addSurfaceDeletionLocked(createTraceIncrementLocked(), layer);

}



void SurfaceInterceptor::saveBufferUpdate(const sp<const Layer>& layer, uint32_t width,

/**

 * Here we pass the layer by ID instead of by sp<> since this is called without

 * holding the state-lock from a Binder thread. If we required the caller

 * to pass 'this' by sp<> the temporary sp<> constructed could end up

 * being the last reference and we might accidentally destroy the Layer

 * from this binder thread.

 */

void SurfaceInterceptor::saveBufferUpdate(int32_t layerId, uint32_t width,

        uint32_t height, uint64_t frameNumber)

{

    if (!mEnabled || layer == nullptr) {

    if (!mEnabled) {

        return;

    }

    ATRACE_CALL();

    std::lock_guard<std::mutex> protoGuard(mTraceMutex);

    addBufferUpdateLocked(createTraceIncrementLocked(), layer, width, height, frameNumber);

    addBufferUpdateLocked(createTraceIncrementLocked(), layerId, width, height, frameNumber);

}



void SurfaceInterceptor::saveVSyncEvent(nsecs_t timestamp) {

services/surfaceflinger/SurfaceInterceptor.h

+3 −3
Original line number Diff line number Diff line
@@ -67,7 +67,7 @@ public: 
    // Intercept surface data

    virtual void saveSurfaceCreation(const sp<const Layer>& layer) = 0;

    virtual void saveSurfaceDeletion(const sp<const Layer>& layer) = 0;

    virtual void saveBufferUpdate(const sp<const Layer>& layer, uint32_t width, uint32_t height,

    virtual void saveBufferUpdate(int32_t layerId, uint32_t width, uint32_t height,

                                  uint64_t frameNumber) = 0;



    // Intercept display data
@@ -102,7 +102,7 @@ public: 
    // Intercept surface data

    void saveSurfaceCreation(const sp<const Layer>& layer) override;

    void saveSurfaceDeletion(const sp<const Layer>& layer) override;

    void saveBufferUpdate(const sp<const Layer>& layer, uint32_t width, uint32_t height,

    void saveBufferUpdate(int32_t layerId, uint32_t width, uint32_t height,

                          uint64_t frameNumber) override;



    // Intercept display data
@@ -130,7 +130,7 @@ private: 
    Increment* createTraceIncrementLocked();

    void addSurfaceCreationLocked(Increment* increment, const sp<const Layer>& layer);

    void addSurfaceDeletionLocked(Increment* increment, const sp<const Layer>& layer);

    void addBufferUpdateLocked(Increment* increment, const sp<const Layer>& layer, uint32_t width,

    void addBufferUpdateLocked(Increment* increment, int32_t layerId, uint32_t width,

            uint32_t height, uint64_t frameNumber);

    void addVSyncUpdateLocked(Increment* increment, nsecs_t timestamp);

    void addDisplayCreationLocked(Increment* increment, const DisplayDeviceState& info);

services/surfaceflinger/tests/unittests/mock/MockSurfaceInterceptor.h

+1 −1
Original line number Diff line number Diff line
@@ -39,7 +39,7 @@ public: 
                      const Vector<DisplayState>&, uint32_t));

    MOCK_METHOD1(saveSurfaceCreation, void(const sp<const Layer>&));

    MOCK_METHOD1(saveSurfaceDeletion, void(const sp<const Layer>&));

    MOCK_METHOD4(saveBufferUpdate, void(const sp<const Layer>&, uint32_t, uint32_t, uint64_t));

    MOCK_METHOD4(saveBufferUpdate, void(int32_t, uint32_t, uint32_t, uint64_t));

    MOCK_METHOD1(saveDisplayCreation, void(const DisplayDeviceState&));

    MOCK_METHOD1(saveDisplayDeletion, void(int32_t));

    MOCK_METHOD2(savePowerModeUpdate, void(int32_t, int32_t));