From 7ab12ba890c7559b81dd627f4aad5aca7a58d281 Mon Sep 17 00:00:00 2001
From: Simon Bowden <sbowden@google.com>
Date: Thu, 5 Aug 2021 12:46:45 +0000
Subject: [PATCH] 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.

This caused flaky tests as the extra delay caused the thread to
take longer to complete, and the newly extended callback delay
happened to coincide with the test's wait timeout. Consequently,
the test's wait timeout is shortened slightly to make it clearer
when the issue is caused by "falling into" the OffStep callback
delay, which isn't intended to be a normal behavior.

Test: manual atest
Bug: 194408223
Change-Id: I97ec74bbc26171a4384ed9ddd7e2130fb8004b9b
---
 .../server/vibrator/VibrationThread.java      | 30 +++++++++++--------
 .../server/vibrator/VibrationThreadTest.java  |  2 +-
 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 e85fa234a4ce9..0f4f58b32c1b1 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 61b5c2b09bb2d..4cc4d55f228da 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";