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

Commit 42ad3196 authored by Vishnu Nair's avatar Vishnu Nair
Browse files

SF: Fix a race between layer creation and apply transaction 

Between commitCreatedLayers and applyTransactions in the main
thread, the client could create a new layer and queue a transaction.
This will mean a layer transaction can be applied before the layer
can be committed.

Fix this by flushing the transactions to be applied before
committing any new layers.

Test: presubmit
Fixes: b/262336014

Change-Id: Id2848af4fbb7afe4e6a20a48f6a8a13f322e1cd7
parent 08b80233

services/surfaceflinger/SurfaceFlinger.cpp

+17 −8
Original line number Original line Diff line number Diff line
@@ -2250,9 +2250,17 @@ bool SurfaceFlinger::commit(TimePoint frameTime, VsyncId vsyncId, TimePoint expe 




        bool needsTraversal = false;

        bool needsTraversal = false;

        if (clearTransactionFlags(eTransactionFlushNeeded)) {

        if (clearTransactionFlags(eTransactionFlushNeeded)) {

            // Locking:

            // 1. to prevent onHandleDestroyed from being called while the state lock is held,

            // we must keep a copy of the transactions (specifically the composer

            // states) around outside the scope of the lock.

            // 2. Transactions and created layers do not share a lock. To prevent applying

            // transactions with layers still in the createdLayer queue, flush the transactions

            // before committing the created layers.

            std::vector<TransactionState> transactions = mTransactionHandler.flushTransactions();

            needsTraversal |= commitMirrorDisplays(vsyncId);

            needsTraversal |= commitMirrorDisplays(vsyncId);

            needsTraversal |= commitCreatedLayers(vsyncId);

            needsTraversal |= commitCreatedLayers(vsyncId);

            needsTraversal |= flushTransactionQueues(vsyncId);

            needsTraversal |= applyTransactions(transactions, vsyncId);

        }

        }





        const bool shouldCommit =

        const bool shouldCommit =
@@ -3948,19 +3956,20 @@ void SurfaceFlinger::addTransactionReadyFilters() { 
            std::bind(&SurfaceFlinger::transactionReadyBufferCheck, this, std::placeholders::_1));

            std::bind(&SurfaceFlinger::transactionReadyBufferCheck, this, std::placeholders::_1));

}

}





// For tests only

bool SurfaceFlinger::flushTransactionQueues(VsyncId vsyncId) {

bool SurfaceFlinger::flushTransactionQueues(VsyncId vsyncId) {

    // to prevent onHandleDestroyed from being called while the lock is held,

    // we must keep a copy of the transactions (specifically the composer

    // states) around outside the scope of the lock

    std::vector<TransactionState> transactions = mTransactionHandler.flushTransactions();

    std::vector<TransactionState> transactions = mTransactionHandler.flushTransactions();

    {

        Mutex::Autolock _l(mStateLock);

    return applyTransactions(transactions, vsyncId);

    return applyTransactions(transactions, vsyncId);

}

}

}





bool SurfaceFlinger::applyTransactions(std::vector<TransactionState>& transactions,

bool SurfaceFlinger::applyTransactions(std::vector<TransactionState>& transactions,

                                       VsyncId vsyncId) {

                                       VsyncId vsyncId) {

    Mutex::Autolock _l(mStateLock);

    return applyTransactionsLocked(transactions, vsyncId);

}



bool SurfaceFlinger::applyTransactionsLocked(std::vector<TransactionState>& transactions,

                                             VsyncId vsyncId) {

    bool needsTraversal = false;

    bool needsTraversal = false;

    // Now apply all transactions.

    // Now apply all transactions.

    for (auto& transaction : transactions) {

    for (auto& transaction : transactions) {

services/surfaceflinger/SurfaceFlinger.h

+5 −1
Original line number Original line Diff line number Diff line
@@ -725,7 +725,11 @@ private: 
                               int originPid, int originUid, uint64_t transactionId)

                               int originPid, int originUid, uint64_t transactionId)

            REQUIRES(mStateLock);

            REQUIRES(mStateLock);

    // Flush pending transactions that were presented after desiredPresentTime.

    // Flush pending transactions that were presented after desiredPresentTime.

    // For test only

    bool flushTransactionQueues(VsyncId) REQUIRES(kMainThreadContext);

    bool flushTransactionQueues(VsyncId) REQUIRES(kMainThreadContext);



    bool applyTransactions(std::vector<TransactionState>&, VsyncId) REQUIRES(kMainThreadContext);



    // Returns true if there is at least one transaction that needs to be flushed

    // Returns true if there is at least one transaction that needs to be flushed

    bool transactionFlushNeeded();

    bool transactionFlushNeeded();

    void addTransactionReadyFilters();

    void addTransactionReadyFilters();
@@ -756,7 +760,7 @@ private: 
    static LatchUnsignaledConfig getLatchUnsignaledConfig();

    static LatchUnsignaledConfig getLatchUnsignaledConfig();

    bool shouldLatchUnsignaled(const sp<Layer>& layer, const layer_state_t&, size_t numStates,

    bool shouldLatchUnsignaled(const sp<Layer>& layer, const layer_state_t&, size_t numStates,

                               bool firstTransaction) const;

                               bool firstTransaction) const;

    bool applyTransactions(std::vector<TransactionState>& transactions, VsyncId)

    bool applyTransactionsLocked(std::vector<TransactionState>& transactions, VsyncId)

            REQUIRES(mStateLock);

            REQUIRES(mStateLock);

    uint32_t setDisplayStateLocked(const DisplayState& s) REQUIRES(mStateLock);

    uint32_t setDisplayStateLocked(const DisplayState& s) REQUIRES(mStateLock);

    uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands)

    uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands)