Merge "Fix cancelSynced cleanup: stop the vibrator." into tm-qpr-dev am: d250dd0eb6

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

Change-Id: Id97b69f8fbe0f613427b3e0922f4185d56b1a7b7
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

2 files changed, 81 insertions, 41 deletions

diff --git a/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java b/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
index 2c6fbbc945fa..fd1a3acd4cd7 100644
--- a/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
+++ b/services/core/java/com/android/server/vibrator/StartSequentialEffectStep.java
@@ -192,41 +192,44 @@ final class StartSequentialEffectStep extends Step {
         // delivered asynchronously but enqueued until the step processing is finished.
         boolean hasPrepared = false;
         boolean hasTriggered = false;
+        boolean hasFailed = 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);
-            }
+        hasPrepared = conductor.vibratorManagerHooks.prepareSyncedVibration(
+                effectMapping.getRequiredSyncCapabilities(),
+                effectMapping.getVibratorIds());
 
-            // 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);
+        for (AbstractVibratorStep step : steps) {
+            long duration = startVibrating(step, nextSteps);
+            if (duration < 0) {
+                // One vibrator has failed, fail this entire sync attempt.
+                hasFailed = true;
+                break;
             }
-            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();
-                }
+            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 && !hasFailed && maxDuration > 0) {
+            hasTriggered = conductor.vibratorManagerHooks.triggerSyncedVibration(getVibration().id);
+            hasFailed &= hasTriggered;
+        }
+
+        if (hasFailed) {
+            // Something failed, possibly after other vibrators were activated.
+            // Cancel and remove every pending step from output list.
+            for (int i = nextSteps.size() - 1; i >= 0; i--) {
+                nextSteps.remove(i).cancelImmediately();
             }
         }
+
+        // Cancel the preparation if trigger failed or all
+        if (hasPrepared && !hasTriggered) {
+            // Trigger has failed or was skipped, so abort the synced vibration.
+            conductor.vibratorManagerHooks.cancelSyncedVibration();
+        }
+
+        return hasFailed ? -1 : maxDuration;
     }
 
     private long startVibrating(AbstractVibratorStep step, List<Step> nextSteps) {
diff --git a/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java
index 039e159a347b..7c2cd9418745 100644
--- a/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java
+++ b/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java
@@ -117,6 +117,10 @@ import java.util.function.Predicate;
 public class VibratorManagerServiceTest {
 
     private static final int TEST_TIMEOUT_MILLIS = 1_000;
+    // Time to allow for a cancellation to complete (notably including system ramp down), but not so
+    // long that tests easily get really slow or flaky. If a vibration is close to this, it should
+    // be cancelled in the body of the individual test.
+    private static final int CLEANUP_TIMEOUT_MILLIS = 100;
     private static final int UID = Process.ROOT_UID;
     private static final int VIRTUAL_DISPLAY_ID = 1;
     private static final String PACKAGE_NAME = "package";
@@ -162,6 +166,7 @@ public class VibratorManagerServiceTest {
 
     private final Map<Integer, FakeVibratorControllerProvider> mVibratorProviders = new HashMap<>();
 
+    private VibratorManagerService mService;
     private Context mContextSpy;
     private TestLooper mTestLooper;
     private FakeVibrator mVibrator;
@@ -226,8 +231,27 @@ public class VibratorManagerServiceTest {
 
     @After
     public void tearDown() throws Exception {
+        if (mService != null) {
+            // Wait until all vibrators have stopped vibrating, with a bit of flexibility for tests
+            // that just do a click or have cancelled at the end (waiting for ramp-down).
+            //
+            // Note: if a test is flaky here, check whether a VibrationEffect duration is close to
+            // CLEANUP_TIMEOUT_MILLIS - in which case it's probably best to just cancel that effect
+            // explicitly at the end of the test case (rather than letting it run and race flakily).
+            assertTrue(waitUntil(s -> {
+                for (int vibratorId : mService.getVibratorIds()) {
+                    if (s.isVibrating(vibratorId)) {
+                        return false;
+                    }
+                }
+                return true;
+            }, mService, CLEANUP_TIMEOUT_MILLIS));
+        }
+
         LocalServices.removeServiceForTest(PackageManagerInternal.class);
         LocalServices.removeServiceForTest(PowerManagerInternal.class);
+        // Ignore potential exceptions about the looper having never dispatched any messages.
+        mTestLooper.stopAutoDispatchAndIgnoreExceptions();
     }
 
     private VibratorManagerService createSystemReadyService() {
@@ -237,7 +261,7 @@ public class VibratorManagerServiceTest {
     }
 
     private VibratorManagerService createService() {
-        return new VibratorManagerService(
+        mService = new VibratorManagerService(
                 mContextSpy,
                 new VibratorManagerService.Injector() {
                     @Override
@@ -274,6 +298,7 @@ public class VibratorManagerServiceTest {
                                 (VibratorManagerService.ExternalVibratorService) serviceInstance;
                     }
                 });
+        return mService;
     }
 
     @Test
@@ -474,6 +499,7 @@ public class VibratorManagerServiceTest {
         verify(listeners[0]).onVibrating(eq(true));
         verify(listeners[1]).onVibrating(eq(true));
         verify(listeners[2], never()).onVibrating(eq(true));
+        cancelVibrate(service);
     }
 
     @Test
@@ -821,7 +847,7 @@ public class VibratorManagerServiceTest {
         assertFalse(mVibratorProviders.get(1).getAllEffectSegments().stream()
                 .anyMatch(PrebakedSegment.class::isInstance));
         // Clean up repeating effect.
-        service.cancelVibrate(VibrationAttributes.USAGE_FILTER_MATCH_ALL, service);
+        cancelVibrate(service);
     }
 
     @Test
@@ -876,6 +902,8 @@ public class VibratorManagerServiceTest {
 
         // The second vibration should have recorded that the vibrators were turned on.
         verify(mBatteryStatsMock, times(2)).noteVibratorOn(anyInt(), anyLong());
+
+        cancelVibrate(service);  // Clean up repeating effect.
     }
 
     @Test
@@ -904,7 +932,7 @@ public class VibratorManagerServiceTest {
         assertFalse(mVibratorProviders.get(1).getAllEffectSegments().stream()
                 .anyMatch(PrebakedSegment.class::isInstance));
         // Clean up long effect.
-        service.cancelVibrate(VibrationAttributes.USAGE_FILTER_MATCH_ALL, service);
+        cancelVibrate(service);
     }
 
     @Test
@@ -912,6 +940,7 @@ public class VibratorManagerServiceTest {
             throws Exception {
         mockVibrators(1);
         mVibratorProviders.get(1).setCapabilities(IVibrator.CAP_AMPLITUDE_CONTROL);
+        mVibratorProviders.get(1).setSupportedEffects(VibrationEffect.EFFECT_CLICK);
         VibratorManagerService service = createSystemReadyService();
 
         VibrationEffect effect = VibrationEffect.createWaveform(
@@ -922,14 +951,16 @@ public class VibratorManagerServiceTest {
         assertTrue(waitUntil(s -> !mVibratorProviders.get(1).getAllEffectSegments().isEmpty(),
                 service, TEST_TIMEOUT_MILLIS));
 
-        vibrate(service, effect, RINGTONE_ATTRS);
-
-        // VibrationThread will start this vibration async, so wait before checking it started.
-        assertTrue(waitUntil(s -> mVibratorProviders.get(1).getAllEffectSegments().size() > 1,
-                service, TEST_TIMEOUT_MILLIS));
+        vibrateAndWaitUntilFinished(service, VibrationEffect.get(VibrationEffect.EFFECT_CLICK),
+                RINGTONE_ATTRS);
 
         // The second vibration should have recorded that the vibrators were turned on.
         verify(mBatteryStatsMock, times(2)).noteVibratorOn(anyInt(), anyLong());
+        // One segment played is the prebaked CLICK from the second vibration.
+        assertEquals(1,
+                mVibratorProviders.get(1).getAllEffectSegments().stream()
+                        .filter(PrebakedSegment.class::isInstance)
+                        .count());
     }
 
     @Test
@@ -1220,10 +1251,11 @@ public class VibratorManagerServiceTest {
         service.updateServiceState();
         // Vibration is not stopped nearly after updating service.
         assertFalse(waitUntil(s -> !s.isVibrating(1), service, 50));
+        cancelVibrate(service);
     }
 
     @Test
-    public void vibrate_withVitualDisplayChange_ignoreVibrationFromVirtualDisplay()
+    public void vibrate_withVirtualDisplayChange_ignoreVibrationFromVirtualDisplay()
             throws Exception {
         mockVibrators(1);
         VibratorManagerService service = createSystemReadyService();
@@ -1249,10 +1281,11 @@ public class VibratorManagerServiceTest {
         // Haptic feedback played normally when the virtual display is removed.
         assertTrue(waitUntil(s -> s.isVibrating(1), service, TEST_TIMEOUT_MILLIS));
 
+        cancelVibrate(service);  // Clean up long-ish effect.
     }
 
     @Test
-    public void vibrate_withAppsOnVitualDisplayChange_ignoreVibrationFromVirtualDisplay()
+    public void vibrate_withAppsOnVirtualDisplayChange_ignoreVibrationFromVirtualDisplay()
             throws Exception {
         mockVibrators(1);
         VibratorManagerService service = createSystemReadyService();
@@ -1277,7 +1310,7 @@ public class VibratorManagerServiceTest {
                 HAPTIC_FEEDBACK_ATTRS);
         // Haptic feedback played normally when the same app no long runs on a virtual display.
         assertTrue(waitUntil(s -> s.isVibrating(1), service, TEST_TIMEOUT_MILLIS));
-
+        cancelVibrate(service);  // Clean up long-ish effect.
     }
 
     @Test
@@ -1916,6 +1949,10 @@ public class VibratorManagerServiceTest {
         when(mNativeWrapperMock.getVibratorIds()).thenReturn(vibratorIds);
     }
 
+    private void cancelVibrate(VibratorManagerService service) {
+        service.cancelVibrate(VibrationAttributes.USAGE_FILTER_MATCH_ALL, service);
+    }
+
     private IVibratorStateListener mockVibratorStateListener() {
         IVibratorStateListener listenerMock = mock(IVibratorStateListener.class);
         IBinder binderMock = mock(IBinder.class);