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

Commit d2c4a4ab authored by John Reck's avatar John Reck
Browse files

Fix wrong lock scope 

mLastQueued* need to be atomic with respect to
mCore->mLastQueuedSlot, and all of them should be
guarded by the mCore->mMutex lock not the mCallbackMutex

Fixes: 311854672
Test: atest --rerun-until-failure 1000 android.view.cts.PixelCopyTest
Change-Id: Iccd7a5ca36d0ee1d87687e1643074d39ad059e01
parent 1b685dd3

libs/gui/BufferQueueProducer.cpp

+15 −13
Original line number Diff line number Diff line
@@ -887,6 +887,9 @@ status_t BufferQueueProducer::queueBuffer(int slot, 
    int callbackTicket = 0;

    uint64_t currentFrameNumber = 0;

    BufferItem item;

    int connectedApi;

    sp<Fence> lastQueuedFence;



    { // Autolock scope

        std::lock_guard<std::mutex> lock(mCore->mMutex);
@@ -1056,6 +1059,13 @@ status_t BufferQueueProducer::queueBuffer(int slot, 
        callbackTicket = mNextCallbackTicket++;



        VALIDATE_CONSISTENCY();



        connectedApi = mCore->mConnectedApi;

        lastQueuedFence = std::move(mLastQueueBufferFence);



        mLastQueueBufferFence = std::move(acquireFence);

        mLastQueuedCrop = item.mCrop;

        mLastQueuedTransform = item.mTransform;

    } // Autolock scope



    // It is okay not to clear the GraphicBuffer when the consumer is SurfaceFlinger because
@@ -1079,9 +1089,6 @@ status_t BufferQueueProducer::queueBuffer(int slot, 
    // Call back without the main BufferQueue lock held, but with the callback

    // lock held so we can ensure that callbacks occur in order



    int connectedApi;

    sp<Fence> lastQueuedFence;



    { // scope for the lock

        std::unique_lock<std::mutex> lock(mCallbackMutex);

        while (callbackTicket != mCurrentCallbackTicket) {
@@ -1094,13 +1101,6 @@ status_t BufferQueueProducer::queueBuffer(int slot, 
            frameReplacedListener->onFrameReplaced(item);

        }



        connectedApi = mCore->mConnectedApi;

        lastQueuedFence = std::move(mLastQueueBufferFence);



        mLastQueueBufferFence = std::move(acquireFence);

        mLastQueuedCrop = item.mCrop;

        mLastQueuedTransform = item.mTransform;



        ++mCurrentCallbackTicket;

        mCallbackCondition.notify_all();

    }
@@ -1653,9 +1653,10 @@ status_t BufferQueueProducer::setLegacyBufferDrop(bool drop) { 
status_t BufferQueueProducer::getLastQueuedBuffer(sp<GraphicBuffer>* outBuffer,

        sp<Fence>* outFence, float outTransformMatrix[16]) {

    ATRACE_CALL();

    BQ_LOGV("getLastQueuedBuffer");



    std::lock_guard<std::mutex> lock(mCore->mMutex);

    BQ_LOGV("getLastQueuedBuffer, slot=%d", mCore->mLastQueuedSlot);



    if (mCore->mLastQueuedSlot == BufferItem::INVALID_BUFFER_SLOT) {

        *outBuffer = nullptr;

        *outFence = Fence::NO_FENCE;
@@ -1679,10 +1680,11 @@ status_t BufferQueueProducer::getLastQueuedBuffer(sp<GraphicBuffer>* outBuffer, 
status_t BufferQueueProducer::getLastQueuedBuffer(sp<GraphicBuffer>* outBuffer, sp<Fence>* outFence,

                                                  Rect* outRect, uint32_t* outTransform) {

    ATRACE_CALL();

    BQ_LOGV("getLastQueuedBuffer");



    std::lock_guard<std::mutex> lock(mCore->mMutex);

    if (mCore->mLastQueuedSlot == BufferItem::INVALID_BUFFER_SLOT) {

    BQ_LOGV("getLastQueuedBuffer, slot=%d", mCore->mLastQueuedSlot);

    if (mCore->mLastQueuedSlot == BufferItem::INVALID_BUFFER_SLOT ||

        mSlots[mCore->mLastQueuedSlot].mBufferState.isDequeued()) {

        *outBuffer = nullptr;

        *outFence = Fence::NO_FENCE;

        return NO_ERROR;