Don't take the conductor lock for syncing vibrations.

The process of preparing and trigggering should be automatically
serialized and exclusive because no other step can be executing
on the VibrationThread, and only one VibrationThread is active
at any time.

Bug: 193792066
Test: presubmit
Change-Id: I93e6bdf5ea68c3602cda962fb32030c8695d5fa8

services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java

@@ -189,44 +189,42 @@ final class StartSequentialEffectStep extends Step {
         // vibrating different sets of vibrators in parallel. The manager can only prepareSynced
         // one set of vibrators at a time.
         // This property is guaranteed by there only being one thread (VibrationThread) executing
-        // one Step at a time, so there's no need to hold the state lock.
-        // TODO: remove the large locked block in a dedicated change.
-        synchronized (conductor.mLock) {
-            boolean hasPrepared = false;
-            boolean hasTriggered = false;
-            long maxDuration = 0;
-            try {
-                hasPrepared = conductor.vibratorManagerHooks.prepareSyncedVibration(
-                        effectMapping.getRequiredSyncCapabilities(),
-                        effectMapping.getVibratorIds());
+        // one Step at a time, so there's no need to hold the state lock. Callbacks will be
+        // delivered asynchronously but enqueued until the step processing is finished.
+        boolean hasPrepared = false;
+        boolean hasTriggered = false;
+        long maxDuration = 0;
+        try {
+            hasPrepared = conductor.vibratorManagerHooks.prepareSyncedVibration(
+                    effectMapping.getRequiredSyncCapabilities(),
+                    effectMapping.getVibratorIds());
 
-                for (AbstractVibratorStep step : steps) {
-                    long duration = startVibrating(step, nextSteps);
-                    if (duration < 0) {
-                        // One vibrator has failed, fail this entire sync attempt.
-                        return maxDuration = -1;
-                    }
-                    maxDuration = Math.max(maxDuration, duration);
+            for (AbstractVibratorStep step : steps) {
+                long duration = startVibrating(step, nextSteps);
+                if (duration < 0) {
+                    // One vibrator has failed, fail this entire sync attempt.
+                    return maxDuration = -1;
                 }
+                maxDuration = Math.max(maxDuration, duration);
+            }
 
-                // Check if sync was prepared and if any step was accepted by a vibrator,
-                // otherwise there is nothing to trigger here.
-                if (hasPrepared && maxDuration > 0) {
-                    hasTriggered = conductor.vibratorManagerHooks.triggerSyncedVibration(
-                            getVibration().id);
-                }
-                return maxDuration;
-            } finally {
-                if (hasPrepared && !hasTriggered) {
-                    // Trigger has failed or all steps were ignored by the vibrators.
-                    conductor.vibratorManagerHooks.cancelSyncedVibration();
-                    nextSteps.clear();
-                } else if (maxDuration < 0) {
-                    // Some vibrator failed without being prepared so other vibrators might be
-                    // active. Cancel and remove every pending step from output list.
-                    for (int i = nextSteps.size() - 1; i >= 0; i--) {
-                        nextSteps.remove(i).cancelImmediately();
-                    }
+            // Check if sync was prepared and if any step was accepted by a vibrator,
+            // otherwise there is nothing to trigger here.
+            if (hasPrepared && maxDuration > 0) {
+                hasTriggered = conductor.vibratorManagerHooks.triggerSyncedVibration(
+                        getVibration().id);
+            }
+            return maxDuration;
+        } finally {
+            if (hasPrepared && !hasTriggered) {
+                // Trigger has failed or all steps were ignored by the vibrators.
+                conductor.vibratorManagerHooks.cancelSyncedVibration();
+                nextSteps.clear();
+            } else if (maxDuration < 0) {
+                // Some vibrator failed without being prepared so other vibrators might be
+                // active. Cancel and remove every pending step from output list.
+                for (int i = nextSteps.size() - 1; i >= 0; i--) {
+                    nextSteps.remove(i).cancelImmediately();
                 }
             }
         }

services/core/java/com/android/server/vibrator/VibrationThread.java

@@ -242,8 +242,9 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
                     }
                 }
                 // Only run the next vibration step if we didn't have to wait in this loop.
-                // If we waited then the queue may have changed, so loop again to re-evaluate
-                // the scheduling of the queue top element.
+                // If we waited then the queue may have changed or the wait could have been
+                // interrupted by a cancel call, so loop again to re-evaluate the scheduling of
+                // the queue top element.
                 if (waitMillisBeforeNextStep <= 0) {
                     if (DEBUG) {
                         Slog.d(TAG, "Play vibration consuming next step...");