commit 2274a3146953176b72f2cf2cc578945d8a95f2ff
author Arthur Hung <arthurhung@google.com> Wed Apr 28 15:13:06 2021 +0000
committer Arthur Hung <arthurhung@google.com> Mon May 03 08:44:58 2021 +0000
tree 94bc623d3a3555f06163ce453926a12278b6e615
parent 0021fac86976361657fbc74bc908d548c18511ea

Fix potential synchronized race between SF and InputFlinger

The 'CountDownLatch' is used to control the synchronization behavior if
a transaction is synchronous or contains the syncInputWindow command,
while updating input windows asynchronously, if there is another
synchronized transaction queued and commited before input windows
updated, it may cause the transaction return early and cause other input
function such as 'transferTouchFocus' failed.

This CL distinguish the transaction and input windows updating
synchronization that could help to signal the CountDownLatch in correct
situation.

Test: atest SurfaceFlinger_tests
Test: atest CrossDragAndDropTests --rerun-until-failure 50
Bug: 183877670
Bug: 183877512
Bug: 183880412
Change-Id: I68e124006e86d8b9c4b8c528418b9db2065b21de

services/surfaceflinger/SurfaceFlinger.cpp

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 6497919..df98eb6 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3105,7 +3105,7 @@
 
 void SurfaceFlinger::commitTransaction() {
     commitTransactionLocked();
-    signalSynchronousTransactions();
+    signalSynchronousTransactions(CountDownLatch::eSyncTransaction);
     mAnimTransactionPending = false;
 }
 
@@ -3527,7 +3527,9 @@
     // Generate a CountDownLatch pending state if this is a synchronous transaction.
     if ((state.flags & eSynchronous) || state.inputWindowCommands.syncInputWindows) {
         state.transactionCommittedSignal = std::make_shared<CountDownLatch>(
-                (state.inputWindowCommands.syncInputWindows ? 2 : 1));
+                (state.inputWindowCommands.syncInputWindows
+                         ? (CountDownLatch::eSyncInputWindows | CountDownLatch::eSyncTransaction)
+                         : CountDownLatch::eSyncTransaction));
     }
 
     mTransactionQueue.emplace(state);
@@ -3552,10 +3554,10 @@
     }
 }
 
-void SurfaceFlinger::signalSynchronousTransactions() {
+void SurfaceFlinger::signalSynchronousTransactions(const uint32_t flag) {
     for (auto it = mTransactionCommittedSignals.begin();
          it != mTransactionCommittedSignals.end();) {
-        if ((*it)->countDown() == 0) {
+        if ((*it)->countDown(flag)) {
             it = mTransactionCommittedSignals.erase(it);
         } else {
             it++;
@@ -6175,7 +6177,7 @@
 
 void SurfaceFlinger::setInputWindowsFinished() {
     Mutex::Autolock _l(mStateLock);
-    signalSynchronousTransactions();
+    signalSynchronousTransactions(CountDownLatch::eSyncInputWindows);
 }
 
 // ---------------------------------------------------------------------------

services/surfaceflinger/SurfaceFlinger.h

diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index cf1a545..4bbdd48 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -467,24 +467,31 @@
 
     class CountDownLatch {
     public:
-        explicit CountDownLatch(int32_t count) : mCount(count) {}
+        enum {
+            eSyncTransaction = 1 << 0,
+            eSyncInputWindows = 1 << 1,
+        };
+        explicit CountDownLatch(uint32_t flags) : mFlags(flags) {}
 
-        int32_t countDown() {
+        // True if there is no waiting condition after count down.
+        bool countDown(uint32_t flag) {
             std::unique_lock<std::mutex> lock(mMutex);
-            if (mCount == 0) {
-                return 0;
+            if (mFlags == 0) {
+                return true;
             }
-            if (--mCount == 0) {
+            mFlags &= ~flag;
+            if (mFlags == 0) {
                 mCountDownComplete.notify_all();
+                return true;
             }
-            return mCount;
+            return false;
         }
 
         // Return true if triggered.
         bool wait_until(const std::chrono::seconds& timeout) const {
             std::unique_lock<std::mutex> lock(mMutex);
             const auto untilTime = std::chrono::system_clock::now() + timeout;
-            while (mCount != 0) {
+            while (mFlags != 0) {
                 // Conditional variables can be woken up sporadically, so we check count
                 // to verify the wakeup was triggered by |countDown|.
                 if (std::cv_status::timeout == mCountDownComplete.wait_until(lock, untilTime)) {
@@ -495,7 +502,7 @@
         }
 
     private:
-        int32_t mCount;
+        uint32_t mFlags;
         mutable std::condition_variable mCountDownComplete;
         mutable std::mutex mMutex;
     };
@@ -1124,7 +1131,7 @@
     // Add transaction to the Transaction Queue
     void queueTransaction(TransactionState& state) EXCLUDES(mQueueLock);
     void waitForSynchronousTransaction(const CountDownLatch& transactionCommittedSignal);
-    void signalSynchronousTransactions();
+    void signalSynchronousTransactions(const uint32_t flag);
 
     /*
      * Generic Layer Metadata