From da188d450b5004526cd1b6d4dfa3f7b2b9014599 Mon Sep 17 00:00:00 2001 From: Lais Andrade Date: Wed, 20 Jan 2021 13:05:58 +0000 Subject: [PATCH] Fix ANR caused by VibrationThread The awaitUntil method that waits for vibration completion is waiting on a CountDownLatch in a block synchrnized on the VibrationThread. This causes any call to VibrationThread.cancel() to lock on this wait block and triggers ANR when vibration is cancelled by the main thread. The awaitUntil methods should only wait on the VibrationThread instance so the calls to cancel() will notify and interrupt this wait. Changed the VibrationThreadTest to test the cancel call on a longer vibration pattern, that should cause the test to fail if the Thread is not cancelled right away (i.e. similar conditions that would trigger an ANR on the main thread). Fix: 177974811 Test: VibrationThread Change-Id: Id4346e60292e96df26f5c12e2e088bbf83f416c8 --- .../com/android/server/VibratorService.java | 6 +- .../server/vibrator/VibrationThread.java | 72 +++++----- .../server/vibrator/VibratorController.java | 14 +- .../server/vibrator/VibrationThreadTest.java | 130 +++++++++++++++--- 4 files changed, 157 insertions(+), 65 deletions(-) diff --git a/services/core/java/com/android/server/VibratorService.java b/services/core/java/com/android/server/VibratorService.java index 2c83da55f2750..a6a00fbe17abf 100644 --- a/services/core/java/com/android/server/VibratorService.java +++ b/services/core/java/com/android/server/VibratorService.java @@ -129,8 +129,10 @@ public class VibratorService extends IVibratorService.Stub { Slog.d(TAG, "Vibration thread finished with status " + status); } synchronized (mLock) { - mThread = null; - reportFinishVibrationLocked(status); + if (mCurrentVibration != null && mCurrentVibration.id == vibrationId) { + mThread = null; + reportFinishVibrationLocked(status); + } } } } diff --git a/services/core/java/com/android/server/vibrator/VibrationThread.java b/services/core/java/com/android/server/vibrator/VibrationThread.java index a4d888b3f9cf0..c36375ef0af5f 100644 --- a/services/core/java/com/android/server/vibrator/VibrationThread.java +++ b/services/core/java/com/android/server/vibrator/VibrationThread.java @@ -39,8 +39,6 @@ import com.google.android.collect.Lists; import java.util.ArrayList; import java.util.List; import java.util.PriorityQueue; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.TimeUnit; /** Plays a {@link Vibration} in dedicated thread. */ // TODO(b/159207608): Make this package-private once vibrator services are moved to this package @@ -76,7 +74,6 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi void onVibrationEnded(long vibrationId, Vibration.Status status); } - private final Object mLock = new Object(); private final WorkSource mWorkSource = new WorkSource(); private final PowerManager.WakeLock mWakeLock; private final IBatteryStats mBatteryStatsService; @@ -84,7 +81,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi private final VibrationCallbacks mCallbacks; private final SparseArray mVibrators; - @GuardedBy("mLock") + @GuardedBy("this") @Nullable private VibrateStep mCurrentVibrateStep; @GuardedBy("this") @@ -147,7 +144,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi /** Notify current vibration that a step has completed on given vibrator. */ public void vibratorComplete(int vibratorId) { - synchronized (mLock) { + synchronized (this) { if (mCurrentVibrateStep != null) { mCurrentVibrateStep.vibratorComplete(vibratorId); } @@ -171,7 +168,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi final int stepCount = steps.size(); for (int i = 0; i < stepCount; i++) { Step step = steps.get(i); - synchronized (mLock) { + synchronized (this) { if (step instanceof VibrateStep) { mCurrentVibrateStep = (VibrateStep) step; } else { @@ -315,27 +312,6 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi } } - /** - * Sleeps until given {@link CountDownLatch} has finished or {@code wakeUpTime} was reached. - * - *

This stops immediately when {@link #cancel()} is called. - */ - private void awaitUntil(CountDownLatch counter, long wakeUpTime) { - synchronized (this) { - long durationRemaining = wakeUpTime - SystemClock.uptimeMillis(); - while (counter.getCount() > 0 && durationRemaining > 0) { - try { - counter.await(durationRemaining, TimeUnit.MILLISECONDS); - } catch (InterruptedException e) { - } - if (mForceStop) { - break; - } - durationRemaining = wakeUpTime - SystemClock.uptimeMillis(); - } - } - } - private void noteVibratorOn(long duration) { try { mBatteryStatsService.noteVibratorOn(mVibration.uid, duration); @@ -371,12 +347,10 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi private final class SingleVibrateStep implements VibrateStep { private final VibratorController mVibrator; private final VibrationEffect mEffect; - private final CountDownLatch mCounter; SingleVibrateStep(VibratorController vibrator, VibrationEffect effect) { mVibrator = vibrator; mEffect = effect; - mCounter = new CountDownLatch(1); } @Override @@ -390,7 +364,9 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi return; } mVibrator.off(); - mCounter.countDown(); + synchronized (VibrationThread.this) { + VibrationThread.this.notify(); + } } @Override @@ -408,7 +384,11 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi noteVibratorOn(duration); // Vibration is playing with no need to control amplitudes, just wait for native // callback or timeout. - awaitUntil(mCounter, startTime + duration + CALLBACKS_EXTRA_TIMEOUT); + waitUntil(startTime + duration + CALLBACKS_EXTRA_TIMEOUT); + if (mForceStop) { + mVibrator.off(); + return Vibration.Status.CANCELLED; + } return Vibration.Status.FINISHED; } @@ -499,14 +479,15 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi /** Represent a synchronized vibration step on multiple vibrators. */ private final class SyncedVibrateStep implements VibrateStep { private final SparseArray mEffects; - private final CountDownLatch mActiveVibratorCounter; - private final int mRequiredCapabilities; private final int[] mVibratorIds; + @GuardedBy("VibrationThread.this") + private int mActiveVibratorCounter; + SyncedVibrateStep(SparseArray effects) { mEffects = effects; - mActiveVibratorCounter = new CountDownLatch(mEffects.size()); + mActiveVibratorCounter = mEffects.size(); // TODO(b/159207608): Calculate required capabilities for syncing this step. mRequiredCapabilities = 0; mVibratorIds = new int[effects.size()]; @@ -527,7 +508,11 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi return; } mVibrators.get(vibratorId).off(); - mActiveVibratorCounter.countDown(); + synchronized (VibrationThread.this) { + if (--mActiveVibratorCounter <= 0) { + VibrationThread.this.notify(); + } + } } @Override @@ -556,7 +541,9 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi AmplitudeStep nextStep = step.nextStep(); if (nextStep == null) { // This vibrator has finished playing the effect for this step. - mActiveVibratorCounter.countDown(); + synchronized (VibrationThread.this) { + mActiveVibratorCounter--; + } } else { nextSteps.add(nextStep); } @@ -564,7 +551,16 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi // All OneShot and Waveform effects have finished. Just wait for the other effects // to end via native callbacks before finishing this synced step. - awaitUntil(mActiveVibratorCounter, startTime + timeout + CALLBACKS_EXTRA_TIMEOUT); + synchronized (VibrationThread.this) { + if (mActiveVibratorCounter > 0) { + waitUntil(startTime + timeout + CALLBACKS_EXTRA_TIMEOUT); + } + } + if (mForceStop) { + stopAllVibrators(); + return Vibration.Status.CANCELLED; + } + return Vibration.Status.FINISHED; } finally { if (timeout > 0) { @@ -779,7 +775,7 @@ public final class VibrationThread extends Thread implements IBinder.DeathRecipi Slog.d(TAG, "DelayStep of " + mDelay + "ms starting..."); } waitUntil(SystemClock.uptimeMillis() + mDelay); - return Vibration.Status.FINISHED; + return mForceStop ? Vibration.Status.CANCELLED : Vibration.Status.FINISHED; } finally { if (DEBUG) { Slog.d(TAG, "DelayStep done."); diff --git a/services/core/java/com/android/server/vibrator/VibratorController.java b/services/core/java/com/android/server/vibrator/VibratorController.java index 53f52e286fbdf..9dcf12caa55b7 100644 --- a/services/core/java/com/android/server/vibrator/VibratorController.java +++ b/services/core/java/com/android/server/vibrator/VibratorController.java @@ -272,12 +272,18 @@ public final class VibratorController { return 0; } synchronized (mLock) { - mNativeWrapper.compose(effect.getPrimitiveEffects().toArray( - new VibrationEffect.Composition.PrimitiveEffect[0]), vibrationId); + VibrationEffect.Composition.PrimitiveEffect[] primitives = + effect.getPrimitiveEffects().toArray( + new VibrationEffect.Composition.PrimitiveEffect[0]); + mNativeWrapper.compose(primitives, vibrationId); notifyVibratorOnLocked(); // Compose don't actually give us an estimated duration, so we just guess here. - // TODO(b/177807015): use exposed durations from IVibrator here instead - return 20 * effect.getPrimitiveEffects().size(); + long duration = 0; + for (VibrationEffect.Composition.PrimitiveEffect primitive : primitives) { + // TODO(b/177807015): use exposed durations from IVibrator here instead + duration += 20 + primitive.delay; + } + return duration; } } 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 bee739231d3f1..0fce4baf966f1 100644 --- a/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java +++ b/services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java @@ -212,6 +212,60 @@ public class VibrationThreadTest { } } + @Test + public void vibrate_singleVibratorPredefinedCancel_cancelsVibrationImmediately() + throws Exception { + mVibratorProviders.get(VIBRATOR_ID).setCapabilities(IVibrator.CAP_COMPOSE_EFFECTS); + + long vibrationId = 1; + VibrationEffect effect = VibrationEffect.startComposition() + .addPrimitive(VibrationEffect.Composition.PRIMITIVE_CLICK, 1f, 100) + .addPrimitive(VibrationEffect.Composition.PRIMITIVE_CLICK, 1f, 100) + .addPrimitive(VibrationEffect.Composition.PRIMITIVE_CLICK, 1f, 100) + .compose(); + VibrationThread vibrationThread = startThreadAndDispatcher(vibrationId, effect); + + Thread.sleep(20); + assertTrue(vibrationThread.isAlive()); + assertTrue(vibrationThread.getVibrators().get(VIBRATOR_ID).isVibrating()); + + // Run cancel in a separate thread so if VibrationThread.cancel blocks then this test should + // fail at waitForCompletion(vibrationThread) if the vibration not cancelled immediately. + Thread cancellingThread = new Thread(() -> vibrationThread.cancel()); + cancellingThread.start(); + + waitForCompletion(vibrationThread, 20); + waitForCompletion(cancellingThread); + + verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED)); + assertFalse(vibrationThread.getVibrators().get(VIBRATOR_ID).isVibrating()); + } + + @Test + public void vibrate_singleVibratorWaveformCancel_cancelsVibrationImmediately() + throws Exception { + mVibratorProviders.get(VIBRATOR_ID).setCapabilities(IVibrator.CAP_COMPOSE_EFFECTS); + + long vibrationId = 1; + VibrationEffect effect = VibrationEffect.createWaveform(new long[]{100}, new int[]{100}, 0); + VibrationThread vibrationThread = startThreadAndDispatcher(vibrationId, effect); + + Thread.sleep(20); + assertTrue(vibrationThread.isAlive()); + assertTrue(vibrationThread.getVibrators().get(VIBRATOR_ID).isVibrating()); + + // Run cancel in a separate thread so if VibrationThread.cancel blocks then this test should + // fail at waitForCompletion(vibrationThread) if the vibration not cancelled immediately. + Thread cancellingThread = new Thread(() -> vibrationThread.cancel()); + cancellingThread.start(); + + waitForCompletion(vibrationThread, 20); + waitForCompletion(cancellingThread); + + verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED)); + assertFalse(vibrationThread.getVibrators().get(VIBRATOR_ID).isVibrating()); + } + @Test public void vibrate_singleVibratorPrebaked_runsVibration() throws Exception { mVibratorProviders.get(1).setSupportedEffects(VibrationEffect.EFFECT_THUD); @@ -544,36 +598,70 @@ public class VibrationThreadTest { } @Test - public void vibrate_multipleCancelled_allVibratorsStopped() throws Exception { - mockVibrators(1, 2, 3); + public void vibrate_multiplePredefinedCancel_cancelsVibrationImmediately() throws Exception { + mockVibrators(1, 2); + mVibratorProviders.get(1).setSupportedEffects(VibrationEffect.EFFECT_CLICK); + mVibratorProviders.get(2).setCapabilities(IVibrator.CAP_COMPOSE_EFFECTS); + + long vibrationId = 1; + CombinedVibrationEffect effect = CombinedVibrationEffect.startSynced() + .addVibrator(1, VibrationEffect.get(VibrationEffect.EFFECT_CLICK)) + .addVibrator(2, VibrationEffect.startComposition() + .addPrimitive(VibrationEffect.Composition.PRIMITIVE_CLICK, 1f, 100) + .addPrimitive(VibrationEffect.Composition.PRIMITIVE_CLICK, 1f, 100) + .addPrimitive(VibrationEffect.Composition.PRIMITIVE_CLICK, 1f, 100) + .compose()) + .combine(); + VibrationThread vibrationThread = startThreadAndDispatcher(vibrationId, effect); + + Thread.sleep(10); + assertTrue(vibrationThread.isAlive()); + assertTrue(vibrationThread.getVibrators().get(1).isVibrating()); + assertTrue(vibrationThread.getVibrators().get(2).isVibrating()); + + // Run cancel in a separate thread so if VibrationThread.cancel blocks then this test should + // fail at waitForCompletion(vibrationThread) if the vibration not cancelled immediately. + Thread cancellingThread = new Thread(() -> vibrationThread.cancel()); + cancellingThread.start(); + + waitForCompletion(vibrationThread, 20); + waitForCompletion(cancellingThread); + + verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED)); + assertFalse(vibrationThread.getVibrators().get(1).isVibrating()); + assertFalse(vibrationThread.getVibrators().get(2).isVibrating()); + } + + @Test + public void vibrate_multipleWaveformCancel_cancelsVibrationImmediately() throws Exception { + mockVibrators(1, 2); mVibratorProviders.get(1).setCapabilities(IVibrator.CAP_AMPLITUDE_CONTROL); mVibratorProviders.get(2).setCapabilities(IVibrator.CAP_AMPLITUDE_CONTROL); - mVibratorProviders.get(3).setCapabilities(IVibrator.CAP_AMPLITUDE_CONTROL); long vibrationId = 1; CombinedVibrationEffect effect = CombinedVibrationEffect.startSynced() .addVibrator(1, VibrationEffect.createWaveform( - new long[]{5, 10}, new int[]{1, 2}, 0)) - .addVibrator(2, VibrationEffect.createWaveform( - new long[]{20, 30}, new int[]{3, 4}, 0)) - .addVibrator(3, VibrationEffect.createWaveform( - new long[]{10, 40}, new int[]{5, 6}, 0)) + new long[]{100, 100}, new int[]{1, 2}, 0)) + .addVibrator(2, VibrationEffect.createOneShot(100, 100)) .combine(); - VibrationThread thread = startThreadAndDispatcher(vibrationId, effect); + VibrationThread vibrationThread = startThreadAndDispatcher(vibrationId, effect); - Thread.sleep(15); - assertTrue(thread.isAlive()); - assertTrue(thread.getVibrators().get(1).isVibrating()); - assertTrue(thread.getVibrators().get(2).isVibrating()); - assertTrue(thread.getVibrators().get(3).isVibrating()); + Thread.sleep(10); + assertTrue(vibrationThread.isAlive()); + assertTrue(vibrationThread.getVibrators().get(1).isVibrating()); + assertTrue(vibrationThread.getVibrators().get(2).isVibrating()); - thread.cancel(); - waitForCompletion(thread); - assertFalse(thread.getVibrators().get(1).isVibrating()); - assertFalse(thread.getVibrators().get(2).isVibrating()); - assertFalse(thread.getVibrators().get(3).isVibrating()); + // Run cancel in a separate thread so if VibrationThread.cancel blocks then this test should + // fail at waitForCompletion(vibrationThread) if the vibration not cancelled immediately. + Thread cancellingThread = new Thread(() -> vibrationThread.cancel()); + cancellingThread.start(); + + waitForCompletion(vibrationThread, 20); + waitForCompletion(cancellingThread); verify(mThreadCallbacks).onVibrationEnded(eq(vibrationId), eq(Vibration.Status.CANCELLED)); + assertFalse(vibrationThread.getVibrators().get(1).isVibrating()); + assertFalse(vibrationThread.getVibrators().get(2).isVibrating()); } @Test @@ -621,11 +709,11 @@ public class VibrationThreadTest { return thread; } - private void waitForCompletion(VibrationThread thread) { + private void waitForCompletion(Thread thread) { waitForCompletion(thread, TEST_TIMEOUT_MILLIS); } - private void waitForCompletion(VibrationThread thread, long timeout) { + private void waitForCompletion(Thread thread, long timeout) { try { thread.join(timeout); } catch (InterruptedException e) {