Ensure unlinkToDeath when cancelling external vibrations.

Remove the ExternalVibrationDeathRecipient, and use the ExternalVibrationHolder directly. Ensure all resets of mCurrentExternalVibration pass through the end method that unlinks.

Add some test coverage.

Bug: 222300386
Test: atest
Change-Id: I7da926af98e5b9e6a80f50c27e1655e02308710f

3 files changed, 81 insertions, 52 deletions

diff --git a/core/java/android/os/ExternalVibration.java b/core/java/android/os/ExternalVibration.java
index 4e0995fb8792..db5bc7024e0b 100644
--- a/core/java/android/os/ExternalVibration.java
+++ b/core/java/android/os/ExternalVibration.java
@@ -47,7 +47,13 @@ public class ExternalVibration implements Parcelable {
         this(uid, pkg, attrs, controller, new Binder());
     }
 
-    private ExternalVibration(int uid, @NonNull String pkg, @NonNull AudioAttributes attrs,
+    /**
+     * Full constructor, but exposed to construct the ExternalVibration with an explicit binder
+     * token (for mocks).
+     *
+     * @hide
+     */
+    public ExternalVibration(int uid, @NonNull String pkg, @NonNull AudioAttributes attrs,
             @NonNull IExternalVibrationController controller, @NonNull IBinder token) {
         mUid = uid;
         mPkg = Preconditions.checkNotNull(pkg);
@@ -166,7 +172,7 @@ public class ExternalVibration implements Parcelable {
             + "pkg=" + mPkg + ", "
             + "attrs=" + mAttrs + ", "
             + "controller=" + mController
-            + "token=" + mController
+            + "token=" + mToken
             + "}";
     }
 
diff --git a/services/core/java/com/android/server/vibrator/VibratorManagerService.java b/services/core/java/com/android/server/vibrator/VibratorManagerService.java
index 9ec1079e46cc..1260e5dbc9f7 100644
--- a/services/core/java/com/android/server/vibrator/VibratorManagerService.java
+++ b/services/core/java/com/android/server/vibrator/VibratorManagerService.java
@@ -464,10 +464,9 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
                             && shouldCancelVibration(
                             mCurrentExternalVibration.externalVibration.getVibrationAttributes(),
                             usageFilter)) {
-                        endVibrationLocked(mCurrentExternalVibration, Vibration.Status.CANCELLED);
                         mCurrentExternalVibration.externalVibration.mute();
-                        mCurrentExternalVibration = null;
-                        setExternalControl(false);
+                        endExternalVibrateLocked(Vibration.Status.CANCELLED,
+                                /* continueExternalControl= */ false);
                     }
                 } finally {
                     Binder.restoreCallingIdentity(ident);
@@ -1283,7 +1282,7 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
     }
 
     /** Holder for a {@link ExternalVibration}. */
-    private final class ExternalVibrationHolder {
+    private final class ExternalVibrationHolder implements IBinder.DeathRecipient {
 
         public final ExternalVibration externalVibration;
         public int scale;
@@ -1308,6 +1307,18 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
             mEndTimeDebug = System.currentTimeMillis();
         }
 
+        public void binderDied() {
+            synchronized (mLock) {
+                if (mCurrentExternalVibration != null) {
+                    if (DEBUG) {
+                        Slog.d(TAG, "External vibration finished because binder died");
+                    }
+                    endExternalVibrateLocked(Vibration.Status.CANCELLED,
+                            /* continueExternalControl= */ false);
+                }
+            }
+        }
+
         public Vibration.DebugInfo getDebugInfo() {
             return new Vibration.DebugInfo(
                     mStartTimeDebug, mEndTimeDebug, /* effect= */ null, /* originalEffect= */ null,
@@ -1450,10 +1461,36 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
         }
     }
 
+    /**
+     * Ends the external vibration, and clears related service state.
+     *
+     * @param status the status to end the associated Vibration with
+     * @param continueExternalControl indicates whether external control will continue. If not, the
+     *                                HAL will have external control turned off.
+     */
+    @GuardedBy("mLock")
+    private void endExternalVibrateLocked(Vibration.Status status,
+            boolean continueExternalControl) {
+        Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "endExternalVibrateLocked");
+        try {
+            if (mCurrentExternalVibration == null) {
+                return;
+            }
+            endVibrationLocked(mCurrentExternalVibration, status);
+            mCurrentExternalVibration.externalVibration.unlinkToDeath(
+                    mCurrentExternalVibration);
+            mCurrentExternalVibration = null;
+            if (!continueExternalControl) {
+                setExternalControl(false);
+            }
+        } finally {
+            Trace.traceEnd(Trace.TRACE_TAG_VIBRATOR);
+        }
+    }
+
     /** Implementation of {@link IExternalVibratorService} to be triggered on external control. */
     @VisibleForTesting
     final class ExternalVibratorService extends IExternalVibratorService.Stub {
-        ExternalVibrationDeathRecipient mCurrentExternalDeathRecipient;
 
         @Override
         public int onExternalVibrationStart(ExternalVibration vib) {
@@ -1469,7 +1506,7 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
                 return IExternalVibratorService.SCALE_MUTE;
             }
 
-            ExternalVibrationHolder cancelingExternalVibration = null;
+            boolean alreadyUnderExternalControl = false;
             boolean waitForCompletion = false;
             int scale;
             synchronized (mLock) {
@@ -1504,13 +1541,13 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
                     //
                     // Note that this doesn't support multiple concurrent external controls, as we
                     // would need to mute the old one still if it came from a different controller.
+                    alreadyUnderExternalControl = true;
                     mCurrentExternalVibration.externalVibration.mute();
-                    endVibrationLocked(mCurrentExternalVibration, Vibration.Status.CANCELLED);
-                    cancelingExternalVibration = mCurrentExternalVibration;
+                    endExternalVibrateLocked(Vibration.Status.CANCELLED,
+                            /* continueExternalControl= */ true);
                 }
                 mCurrentExternalVibration = new ExternalVibrationHolder(vib);
-                mCurrentExternalDeathRecipient = new ExternalVibrationDeathRecipient();
-                vib.linkToDeath(mCurrentExternalDeathRecipient);
+                vib.linkToDeath(mCurrentExternalVibration);
                 mCurrentExternalVibration.scale = mVibrationScaler.getExternalVibrationScale(
                         vib.getVibrationAttributes().getUsage());
                 scale = mCurrentExternalVibration.scale;
@@ -1520,14 +1557,13 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
                 if (!mVibrationThread.waitForThreadIdle(VIBRATION_CANCEL_WAIT_MILLIS)) {
                     Slog.e(TAG, "Timed out waiting for vibration to cancel");
                     synchronized (mLock) {
-                        stopExternalVibrateLocked(Vibration.Status.IGNORED_ERROR_CANCELLING);
+                        endExternalVibrateLocked(Vibration.Status.IGNORED_ERROR_CANCELLING,
+                                /* continueExternalControl= */ false);
                     }
                     return IExternalVibratorService.SCALE_MUTE;
                 }
             }
-            if (cancelingExternalVibration == null) {
-                // We only need to set external control if it was not already set by another
-                // external vibration.
+            if (!alreadyUnderExternalControl) {
                 if (DEBUG) {
                     Slog.d(TAG, "Vibrator going under external control.");
                 }
@@ -1547,29 +1583,12 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
                     if (DEBUG) {
                         Slog.e(TAG, "Stopping external vibration" + vib);
                     }
-                    stopExternalVibrateLocked(Vibration.Status.FINISHED);
+                    endExternalVibrateLocked(Vibration.Status.FINISHED,
+                            /* continueExternalControl= */ false);
                 }
             }
         }
 
-        @GuardedBy("mLock")
-        private void stopExternalVibrateLocked(Vibration.Status status) {
-            Trace.traceBegin(Trace.TRACE_TAG_VIBRATOR, "stopExternalVibrateLocked");
-            try {
-                if (mCurrentExternalVibration == null) {
-                    return;
-                }
-                endVibrationLocked(mCurrentExternalVibration, status);
-                mCurrentExternalVibration.externalVibration.unlinkToDeath(
-                        mCurrentExternalDeathRecipient);
-                mCurrentExternalDeathRecipient = null;
-                mCurrentExternalVibration = null;
-                setExternalControl(false);
-            } finally {
-                Trace.traceEnd(Trace.TRACE_TAG_VIBRATOR);
-            }
-        }
-
         private boolean hasExternalControlCapability() {
             for (int i = 0; i < mVibrators.size(); i++) {
                 if (mVibrators.valueAt(i).hasCapability(IVibrator.CAP_EXTERNAL_CONTROL)) {
@@ -1578,19 +1597,6 @@ public class VibratorManagerService extends IVibratorManagerService.Stub {
             }
             return false;
         }
-
-        private class ExternalVibrationDeathRecipient implements IBinder.DeathRecipient {
-            public void binderDied() {
-                synchronized (mLock) {
-                    if (mCurrentExternalVibration != null) {
-                        if (DEBUG) {
-                            Slog.d(TAG, "External vibration finished because binder died");
-                        }
-                        stopExternalVibrateLocked(Vibration.Status.CANCELLED);
-                    }
-                }
-            }
-        }
     }
 
     /** Provide limited functionality from {@link VibratorManagerService} as shell commands. */
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 e46e28199d5a..92736c517782 100644
--- a/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java
+++ b/services/tests/servicestests/src/com/android/server/vibrator/VibratorManagerServiceTest.java
@@ -1136,19 +1136,23 @@ public class VibratorManagerServiceTest {
     }
 
     @Test
-    public void onExternalVibration_setsExternalControl() {
+    public void onExternalVibration_setsExternalControl() throws Exception {
         mockVibrators(1);
         mVibratorProviders.get(1).setCapabilities(IVibrator.CAP_EXTERNAL_CONTROL);
         createSystemReadyService();
 
+        IBinder binderToken = mock(IBinder.class);
         ExternalVibration externalVibration = new ExternalVibration(UID, PACKAGE_NAME, AUDIO_ATTRS,
-                mock(IExternalVibrationController.class));
+                mock(IExternalVibrationController.class), binderToken);
         int scale = mExternalVibratorService.onExternalVibrationStart(externalVibration);
         mExternalVibratorService.onExternalVibrationStop(externalVibration);
 
         assertNotEquals(IExternalVibratorService.SCALE_MUTE, scale);
         assertEquals(Arrays.asList(false, true, false),
                 mVibratorProviders.get(1).getExternalControlStates());
+
+        verify(binderToken).linkToDeath(any(), eq(0));
+        verify(binderToken).unlinkToDeath(any(), eq(0));
     }
 
     @Test
@@ -1160,17 +1164,19 @@ public class VibratorManagerServiceTest {
         setUserSetting(Settings.System.VIBRATE_WHEN_RINGING, 1);
         createSystemReadyService();
 
+        IBinder firstToken = mock(IBinder.class);
+        IBinder secondToken = mock(IBinder.class);
         IExternalVibrationController firstController = mock(IExternalVibrationController.class);
         IExternalVibrationController secondController = mock(IExternalVibrationController.class);
         ExternalVibration firstVibration = new ExternalVibration(UID, PACKAGE_NAME, AUDIO_ATTRS,
-                firstController);
+                firstController, firstToken);
         int firstScale = mExternalVibratorService.onExternalVibrationStart(firstVibration);
 
         AudioAttributes ringtoneAudioAttrs = new AudioAttributes.Builder()
                 .setUsage(AudioAttributes.USAGE_NOTIFICATION_RINGTONE)
                 .build();
         ExternalVibration secondVibration = new ExternalVibration(UID, PACKAGE_NAME,
-                ringtoneAudioAttrs, secondController);
+                ringtoneAudioAttrs, secondController, secondToken);
         int secondScale = mExternalVibratorService.onExternalVibrationStart(secondVibration);
 
         assertNotEquals(IExternalVibratorService.SCALE_MUTE, firstScale);
@@ -1180,6 +1186,17 @@ public class VibratorManagerServiceTest {
         // Set external control called only once.
         assertEquals(Arrays.asList(false, true),
                 mVibratorProviders.get(1).getExternalControlStates());
+
+        mExternalVibratorService.onExternalVibrationStop(secondVibration);
+        mExternalVibratorService.onExternalVibrationStop(firstVibration);
+        assertEquals(Arrays.asList(false, true, false),
+                mVibratorProviders.get(1).getExternalControlStates());
+
+        verify(firstToken).linkToDeath(any(), eq(0));
+        verify(firstToken).unlinkToDeath(any(), eq(0));
+
+        verify(secondToken).linkToDeath(any(), eq(0));
+        verify(secondToken).unlinkToDeath(any(), eq(0));
     }
 
     @Test