From c6438e9059d3e6157b3041a2028eef469516ca3f Mon Sep 17 00:00:00 2001 From: Simon Bowden Date: Wed, 31 Aug 2022 14:23:32 +0000 Subject: Fix cancelSynced cleanup: stop the vibrator. Currently, the VibratorController is not updated when a synced vibration is cancelled. Consequently, it stays "isVibrating" when the vibration is finished. This is fixed by doing the same steps cancellation of individual steps failure for the triggerSynced failure too. Add new tests to ensure that vibrators have stopped vibrating, and to stop the looper thread (this is how the issue was found). Bug: 244428021 Test: atest Change-Id: I67ffeb170ce28e1f65227079185f1cda75636a14 Merged-In: I67ffeb170ce28e1f65227079185f1cda75636a14 (cherry picked from commit fb2fc1f0c733eefbe59024207493eca1d70beb3c) --- .../server/vibrator/StartSequentialEffectStep.java | 63 +++++++++++----------- .../vibrator/VibratorManagerServiceTest.java | 59 ++++++++++++++++---- 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 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 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); -- cgit v1.2.3-59-g8ed1b