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

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();
+                long waitTime;
-                if (waitTime <= 0) {
-                    mStepQueue.consumeNext();
-                } else {
                 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();
+            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

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";