Merge "Fix a race condition between calculateWaitTime and actually waiting. When the race was lost with a completion callback, the operation being waited upon may have been de-queued by the completion callback for immediate execution, but the thread was still proceeding with the wait." into sc-dev am: 47fc76e4f2

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/15472895

Change-Id: Ibaae504192cfc6c18536fcd0f6e6de0c31f231bd

2 files changed, 18 insertions, 14 deletions

diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java
index e85fa234a4ce..0f4f58b32c1b 100644
--- a/services/core/java/com/android/server/vibrator/VibrationThread.java
+++ b/services/core/java/com/android/server/vibrator/VibrationThread.java
@@ -228,17 +228,20 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
 
             Vibration.Status status = null;
             while (!mStepQueue.isEmpty()) {
-                long waitTime = mStepQueue.calculateWaitTime();
-                if (waitTime <= 0) {
-                    mStepQueue.consumeNext();
-                } else {
-                    synchronized (mLock) {
+                long waitTime;
+                synchronized (mLock) {
+                    waitTime = mStepQueue.calculateWaitTime();
+                    if (waitTime > 0) {
                         try {
                             mLock.wait(waitTime);
                         } catch (InterruptedException e) {
                         }
                     }
                 }
+                // If we waited, the queue may have changed, so let the loop run again.
+                if (waitTime <= 0) {
+                    mStepQueue.consumeNext();
+                }
                 Vibration.Status currentStatus = mStop ? Vibration.Status.CANCELLED
                         : mStepQueue.calculateVibrationStatus(sequentialEffectSize);
                 if (status == null && currentStatus != Vibration.Status.RUNNING) {
@@ -387,15 +390,13 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
         }
 
         /** Returns the time in millis to wait before calling {@link #consumeNext()}. */
+        @GuardedBy("mLock")
         public long calculateWaitTime() {
-            Step nextStep;
-            synchronized (mLock) {
-                if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
-                    // Steps anticipated by vibrator complete callback should be played right away.
-                    return 0;
-                }
-                nextStep = mNextSteps.peek();
+            if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
+                // Steps anticipated by vibrator complete callback should be played right away.
+                return 0;
             }
+            Step nextStep = mNextSteps.peek();
             return nextStep == null ? 0 : nextStep.calculateWaitTime();
         }
 
@@ -603,7 +604,10 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
             return false;
         }
 
-        /** Returns the time in millis to wait before playing this step. */
+        /**
+         * Returns the time in millis to wait before playing this step. This is performed
+         * while holding the queue lock, so should not rely on potentially slow operations.
+         */
         public long calculateWaitTime() {
             if (startTime == Long.MAX_VALUE) {
                 // This step don't have a predefined start time, it's just marked to be executed
diff --git a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java
index 61b5c2b09bb2..4cc4d55f228d 100644
--- a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java
+++ b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java
@@ -83,7 +83,7 @@ import java.util.stream.Collectors;
 @Presubmit
 public class VibrationThreadTest {
 
-    private static final int TEST_TIMEOUT_MILLIS = 1_000;
+    private static final int TEST_TIMEOUT_MILLIS = 900;
     private static final int UID = Process.ROOT_UID;
     private static final int VIBRATOR_ID = 1;
     private static final String PACKAGE_NAME = "package";