Merge "Ensure unlinkToDeath when cancelling external vibrations." into tm-dev

2022-03-07 10:57:22 +00:00
parent 429c7328bc d319a179a7
commit 4dfe4924d4
3 changed files with 81 additions and 52 deletions
--- 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
            + "}";
    }

--- 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. */
--- 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