DO NOT MERGE: 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: I9987614ade29456453de3610782a645ea9db0892
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index e9fbf6e..b0d442f 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2116,8 +2116,16 @@
bool needsTraversal = false;
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 = flushTransactions();
needsTraversal |= commitCreatedLayers();
- needsTraversal |= flushTransactionQueues(vsyncId);
+ needsTraversal |= applyTransactions(transactions, vsyncId);
}
const bool shouldCommit =
@@ -3774,7 +3782,7 @@
return transactionsPendingBarrier;
}
-bool SurfaceFlinger::flushTransactionQueues(int64_t vsyncId) {
+std::vector<TransactionState> SurfaceFlinger::flushTransactions() {
// 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
@@ -3868,14 +3876,25 @@
flushUnsignaledPendingTransactionQueues(transactions, bufferLayersReadyToPresent,
applyTokensWithUnsignaledTransactions);
}
-
- return applyTransactions(transactions, vsyncId);
}
}
+ return transactions;
+}
+
+// for test only
+bool SurfaceFlinger::flushTransactionQueues(int64_t vsyncId) {
+ std::vector<TransactionState> transactions = flushTransactions();
+ return applyTransactions(transactions, vsyncId);
}
bool SurfaceFlinger::applyTransactions(std::vector<TransactionState>& transactions,
int64_t vsyncId) {
+ Mutex::Autolock _l(mStateLock);
+ return applyTransactionsLocked(transactions, vsyncId);
+}
+
+bool SurfaceFlinger::applyTransactionsLocked(std::vector<TransactionState>& transactions,
+ int64_t vsyncId) {
bool needsTraversal = false;
// Now apply all transactions.
for (auto& transaction : transactions) {
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 46f0e6b..ea0e780 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -770,6 +770,9 @@
REQUIRES(mStateLock);
// flush pending transaction that was presented after desiredPresentTime.
bool flushTransactionQueues(int64_t vsyncId);
+
+ std::vector<TransactionState> flushTransactions();
+
// Returns true if there is at least one transaction that needs to be flushed
bool transactionFlushNeeded();
@@ -818,7 +821,8 @@
size_t totalTXapplied) const;
bool stopTransactionProcessing(const std::unordered_set<sp<IBinder>, SpHash<IBinder>>&
applyTokensWithUnsignaledTransactions) const;
- bool applyTransactions(std::vector<TransactionState>& transactions, int64_t vsyncId)
+ bool applyTransactions(std::vector<TransactionState>& transactions, int64_t vsyncId);
+ bool applyTransactionsLocked(std::vector<TransactionState>& transactions, int64_t vsyncId)
REQUIRES(mStateLock);
uint32_t setDisplayStateLocked(const DisplayState& s) REQUIRES(mStateLock);
uint32_t addInputWindowCommands(const InputWindowCommands& inputWindowCommands)