commit 255acdca7bb9ad76dea94d006238831f66e4e8dd
author Robert Carr <racarr@google.com> Fri Apr 17 14:08:55 2020 -0700
committer Robert Carr <racarr@google.com> Mon Apr 20 12:21:36 2020 -0700
tree 5a68d49937e65916d19dd0a980badb0e07445c85
parent 9bb4cfb009f1317605e8ff090e0ce930e15d4d40

BLASTBufferQueue: Don't use mNextTransaction on Binder thread.

The "mUseNextTransaction" boolean was intended to ensure that we only
used mNextTransaction when calling processBuffer from frameAvailable
and not when processing the queue from the transaction callback. However
this didn't quite work because we can end up returning early from
processNextBuffer() and then the next call (coming from the Transation callback)
will use mNextTransaction from the binder thread. If we randomly access
mNextTransaction from random threads it's very difficult for other threads
to use, so we avoid this directly.

Test: Existing tests pass
Bug: 153120755
Change-Id: Ic261d1fdf3f1458f208face572f2780c1043192c

libs/gui/BLASTBufferQueue.cpp

diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index a8384ac..b0d9521 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -189,13 +189,13 @@
     mPendingReleaseItem.item = std::move(mSubmitted.front());
     mSubmitted.pop();
 
-    processNextBufferLocked();
+    processNextBufferLocked(false);
 
     mCallbackCV.notify_all();
     decStrong((void*)transactionCallbackThunk);
 }
 
-void BLASTBufferQueue::processNextBufferLocked() {
+void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) {
     ATRACE_CALL();
     if (mNumFrameAvailable == 0 || mNumAcquired == MAX_ACQUIRED_BUFFERS + 1) {
         return;
@@ -209,7 +209,7 @@
     SurfaceComposerClient::Transaction localTransaction;
     bool applyTransaction = true;
     SurfaceComposerClient::Transaction* t = &localTransaction;
-    if (mNextTransaction != nullptr && mUseNextTransaction) {
+    if (mNextTransaction != nullptr && useNextTransaction) {
         t = mNextTransaction;
         mNextTransaction = nullptr;
         applyTransaction = false;
@@ -274,16 +274,14 @@
         while (mNumFrameAvailable > 0 || mNumAcquired == MAX_ACQUIRED_BUFFERS + 1) {
             mCallbackCV.wait(_lock);
         }
-        mUseNextTransaction = true;
     }
     // add to shadow queue
     mNumFrameAvailable++;
-    processNextBufferLocked();
+    processNextBufferLocked(true);
 }
 
 void BLASTBufferQueue::setNextTransaction(SurfaceComposerClient::Transaction* t) {
     std::lock_guard _lock{mMutex};
-    mUseNextTransaction = false;
     mNextTransaction = t;
 }