Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 746c0742 authored by Vlad Popa's avatar Vlad Popa
Browse files

Fix deadlock caused by wrong locking order

The correct order to lock is mSettingsLock -> VolumeStreamState.class.
In some cases this order was broken which lead to some deadlocks. Add
quick fix. Might still need to investigate whether we need both locks,
for details see b/287066735

Test: atest AudioManagerTest
Bug: 283969676
Bug: 283436164
Change-Id: Id9c9fe4bd78d23dff92bf3a32c6f16d8e7b96720
parent 4ac3a5d5
Loading
Loading
Loading
Loading
+131 −99
Original line number Original line Diff line number Diff line
@@ -3663,6 +3663,8 @@ public class AudioService extends IAudioService.Stub
     * and aliases before mute change changed and after.
     * and aliases before mute change changed and after.
     */
     */
    private void muteAliasStreams(int streamAlias, boolean state) {
    private void muteAliasStreams(int streamAlias, boolean state) {
        // Locking mSettingsLock to avoid inversion when calling doMute -> updateVolumeGroupIndex
        synchronized (mSettingsLock) {
            synchronized (VolumeStreamState.class) {
            synchronized (VolumeStreamState.class) {
                List<Integer> streamsToMute = new ArrayList<>();
                List<Integer> streamsToMute = new ArrayList<>();
                for (int stream = 0; stream < mStreamStates.length; stream++) {
                for (int stream = 0; stream < mStreamStates.length; stream++) {
@@ -3671,7 +3673,8 @@ public class AudioService extends IAudioService.Stub
                        if (!(mCameraSoundForced
                        if (!(mCameraSoundForced
                                && (vss.getStreamType()
                                && (vss.getStreamType()
                                == AudioSystem.STREAM_SYSTEM_ENFORCED))) {
                                == AudioSystem.STREAM_SYSTEM_ENFORCED))) {
                        boolean changed = vss.mute(state, /* apply= */ false, "muteAliasStreams");
                            boolean changed = vss.mute(state, /* apply= */ false,
                                    "muteAliasStreams");
                            if (changed) {
                            if (changed) {
                                streamsToMute.add(stream);
                                streamsToMute.add(stream);
                            }
                            }
@@ -3684,6 +3687,7 @@ public class AudioService extends IAudioService.Stub
                });
                });
            }
            }
        }
        }
    }
    private void broadcastMuteSetting(int streamType, boolean isMuted) {
    private void broadcastMuteSetting(int streamType, boolean isMuted) {
        // Stream mute changed, fire the intent.
        // Stream mute changed, fire the intent.
@@ -3696,6 +3700,9 @@ public class AudioService extends IAudioService.Stub
    // Called after a delay when volume down is pressed while muted
    // Called after a delay when volume down is pressed while muted
    private void onUnmuteStreamOnSingleVolDevice(int streamAlias, int flags) {
    private void onUnmuteStreamOnSingleVolDevice(int streamAlias, int flags) {
        boolean wasMuted;
        boolean wasMuted;
        // Locking mSettingsLock to avoid inversion when calling vss.mute -> vss.doMute ->
        // vss.updateVolumeGroupIndex
        synchronized (mSettingsLock) {
            synchronized (VolumeStreamState.class) {
            synchronized (VolumeStreamState.class) {
                final VolumeStreamState streamState = mStreamStates[streamAlias];
                final VolumeStreamState streamState = mStreamStates[streamAlias];
                // if unmuting causes a change, it was muted
                // if unmuting causes a change, it was muted
@@ -3714,6 +3721,7 @@ public class AudioService extends IAudioService.Stub
                }
                }
            }
            }
        }
        }
    }
    @GuardedBy("mHdmiClientLock")
    @GuardedBy("mHdmiClientLock")
    private void maybeSendSystemAudioStatusCommand(boolean isMuteAdjust) {
    private void maybeSendSystemAudioStatusCommand(boolean isMuteAdjust) {
@@ -7609,9 +7617,13 @@ public class AudioService extends IAudioService.Stub
                Log.i(TAG, String.format("onAccessoryPlugMediaUnmute unmuting device=%d [%s]",
                Log.i(TAG, String.format("onAccessoryPlugMediaUnmute unmuting device=%d [%s]",
                        newDevice, AudioSystem.getOutputDeviceName(newDevice)));
                        newDevice, AudioSystem.getOutputDeviceName(newDevice)));
            }
            }
            // Locking mSettingsLock to avoid inversion when calling vss.mute -> vss.doMute ->
            // vss.updateVolumeGroupIndex
            synchronized (mSettingsLock) {
                mStreamStates[AudioSystem.STREAM_MUSIC].mute(false, "onAccessoryPlugMediaUnmute");
                mStreamStates[AudioSystem.STREAM_MUSIC].mute(false, "onAccessoryPlugMediaUnmute");
            }
            }
        }
        }
    }
    /**
    /**
     * See AudioManager.hasHapticChannels(Context, Uri).
     * See AudioManager.hasHapticChannels(Context, Uri).
@@ -7644,11 +7656,16 @@ public class AudioService extends IAudioService.Stub
                continue;
                continue;
            }
            }
        }
        }
        // need mSettingsLock for vgs.applyAllVolumes -> vss.setIndex which grabs this lock after
        // VSS.class. Locking order needs to be preserved
        synchronized (mSettingsLock) {
            for (int i = 0; i < sVolumeGroupStates.size(); i++) {
            for (int i = 0; i < sVolumeGroupStates.size(); i++) {
                final VolumeGroupState vgs = sVolumeGroupStates.valueAt(i);
                final VolumeGroupState vgs = sVolumeGroupStates.valueAt(i);
                vgs.applyAllVolumes(/* userSwitch= */ false);
                vgs.applyAllVolumes(/* userSwitch= */ false);
            }
            }
        }
        }
    }
    private void ensureValidAttributes(AudioVolumeGroup avg) {
    private void ensureValidAttributes(AudioVolumeGroup avg) {
        boolean hasAtLeastOneValidAudioAttributes = avg.getAudioAttributes().stream()
        boolean hasAtLeastOneValidAudioAttributes = avg.getAudioAttributes().stream()
@@ -7683,11 +7700,16 @@ public class AudioService extends IAudioService.Stub
        if (DEBUG_VOL) {
        if (DEBUG_VOL) {
            Log.v(TAG, "restoreVolumeGroups");
            Log.v(TAG, "restoreVolumeGroups");
        }
        }
        // need mSettingsLock for vgs.applyAllVolumes -> vss.setIndex which grabs this lock after
        // VSS.class. Locking order needs to be preserved
        synchronized (mSettingsLock) {
            for (int i = 0; i < sVolumeGroupStates.size(); i++) {
            for (int i = 0; i < sVolumeGroupStates.size(); i++) {
                final VolumeGroupState vgs = sVolumeGroupStates.valueAt(i);
                final VolumeGroupState vgs = sVolumeGroupStates.valueAt(i);
                vgs.applyAllVolumes(false/*userSwitch*/);
                vgs.applyAllVolumes(false/*userSwitch*/);
            }
            }
        }
        }
    }
    private void dumpVolumeGroups(PrintWriter pw) {
    private void dumpVolumeGroups(PrintWriter pw) {
        pw.println("\nVolume Groups (device: index)");
        pw.println("\nVolume Groups (device: index)");
@@ -7822,6 +7844,7 @@ public class AudioService extends IAudioService.Stub
        }
        }
        public void adjustVolume(int direction, int flags) {
        public void adjustVolume(int direction, int flags) {
            synchronized (mSettingsLock) {
                synchronized (AudioService.VolumeStreamState.class) {
                synchronized (AudioService.VolumeStreamState.class) {
                    int device = getDeviceForVolume();
                    int device = getDeviceForVolume();
                    int previousIndex = getIndex(device);
                    int previousIndex = getIndex(device);
@@ -7868,6 +7891,7 @@ public class AudioService extends IAudioService.Stub
                    }
                    }
                }
                }
            }
            }
        }
        public int getVolumeIndex() {
        public int getVolumeIndex() {
            synchronized (AudioService.VolumeStreamState.class) {
            synchronized (AudioService.VolumeStreamState.class) {
@@ -7876,6 +7900,7 @@ public class AudioService extends IAudioService.Stub
        }
        }
        public void setVolumeIndex(int index, int flags) {
        public void setVolumeIndex(int index, int flags) {
            synchronized (mSettingsLock) {
                synchronized (AudioService.VolumeStreamState.class) {
                synchronized (AudioService.VolumeStreamState.class) {
                    if (mUseFixedVolume) {
                    if (mUseFixedVolume) {
                        return;
                        return;
@@ -7883,6 +7908,7 @@ public class AudioService extends IAudioService.Stub
                    setVolumeIndex(index, getDeviceForVolume(), flags);
                    setVolumeIndex(index, getDeviceForVolume(), flags);
                }
                }
            }
            }
        }
        @GuardedBy("AudioService.VolumeStreamState.class")
        @GuardedBy("AudioService.VolumeStreamState.class")
        private void setVolumeIndex(int index, int device, int flags) {
        private void setVolumeIndex(int index, int device, int flags) {
@@ -8687,6 +8713,10 @@ public class AudioService extends IAudioService.Stub
        // If associated to volume group, update group cache
        // If associated to volume group, update group cache
        private void updateVolumeGroupIndex(int device, boolean forceMuteState) {
        private void updateVolumeGroupIndex(int device, boolean forceMuteState) {
            // need mSettingsLock when called from setIndex for vgs.mute -> vgs.applyAllVolumes ->
            // vss.setIndex which grabs this lock after VSS.class. Locking order needs to be
            // preserved
            synchronized (mSettingsLock) {
                synchronized (VolumeStreamState.class) {
                synchronized (VolumeStreamState.class) {
                    if (mVolumeGroupState != null) {
                    if (mVolumeGroupState != null) {
                        int groupIndex = (getIndex(device) + 5) / 10;
                        int groupIndex = (getIndex(device) + 5) / 10;
@@ -8694,13 +8724,14 @@ public class AudioService extends IAudioService.Stub
                            Log.d(TAG, "updateVolumeGroupIndex for stream " + mStreamType
                            Log.d(TAG, "updateVolumeGroupIndex for stream " + mStreamType
                                    + ", muted=" + mIsMuted + ", device=" + device + ", index="
                                    + ", muted=" + mIsMuted + ", device=" + device + ", index="
                                    + getIndex(device) + ", group " + mVolumeGroupState.name()
                                    + getIndex(device) + ", group " + mVolumeGroupState.name()
                                + " Muted=" + mVolumeGroupState.isMuted() + ", Index=" + groupIndex
                                    + " Muted=" + mVolumeGroupState.isMuted() + ", Index="
                                + ", forceMuteState=" + forceMuteState);
                                    + groupIndex + ", forceMuteState=" + forceMuteState);
                        }
                        }
                        mVolumeGroupState.updateVolumeIndex(groupIndex, device);
                        mVolumeGroupState.updateVolumeIndex(groupIndex, device);
                        // Only propage mute of stream when applicable
                        // Only propage mute of stream when applicable
                        if (isMutable()) {
                        if (isMutable()) {
                        // For call stream, align mute only when muted, not when index is set to 0
                            // For call stream, align mute only when muted, not when index is set to
                            // 0
                            mVolumeGroupState.mute(
                            mVolumeGroupState.mute(
                                    forceMuteState ? mIsMuted :
                                    forceMuteState ? mIsMuted :
                                            (groupIndex == 0 && !isCallStream(mStreamType))
                                            (groupIndex == 0 && !isCallStream(mStreamType))
@@ -8709,6 +8740,7 @@ public class AudioService extends IAudioService.Stub
                    }
                    }
                }
                }
            }
            }
        }
        /**
        /**
         * Mute/unmute the stream
         * Mute/unmute the stream