From 218da1673108ba2c7b26ff99314e301c1c416f67 Mon Sep 17 00:00:00 2001 From: Jean-Michel Trivi Date: Fri, 5 Apr 2019 16:05:04 -0700 Subject: [PATCH] AudioService: fix audio mode lock Fix lock "sharing" for audio mode, with locking order as 1/ AudioDeviceBroker.mSetModeLock 2/ AudioDeviceBroker.mDeviceStateLock 3/ BtHelper class The following code paths have been automatically generated as accessing BtHelper.requestScoState(int, int), which requires the lock to mSetModeLock. They have been checked for consistency of locks, and have been fixed and/or annotated: ScoClient in BtHelper.decCount() (com.android.server.audio) BtHelper.stopBluetoothScoForClient(IBinder, String) (com.android.server.audio) AudioDeviceBroker.stopBluetoothScoForClient_Sync(IBinder, String) (com.android.server.audio) AudioService.stopBluetoothSco(IBinder) (com.android.server.audio) ScoClient in BtHelper.incCount(int) (com.android.server.audio) BtHelper.startBluetoothScoForClient(IBinder, int, String) (com.android.server.audio) AudioDeviceBroker.startBluetoothScoForClient_Sync(IBinder, int, String) (com.android.server.audio) AudioService.startBluetoothScoInt(IBinder, int, String) (com.android.server.audio) ScoClient in BtHelper.clearCount(boolean) (com.android.server.audio) BtHelper.clearAllScoClients(int, boolean) (com.android.server.audio) BtHelper.disconnectBluetoothSco(int) (com.android.server.audio) BrokerHandler in AudioDeviceBroker.handleMessage(Message) (com.android.server.audio) BtHelper.resetBluetoothSco() (com.android.server.audio) BrokerHandler in AudioDeviceBroker.handleMessage(Message) (com.android.server.audio) BtHelper.setBtScoActiveDevice(BluetoothDevice) (com.android.server.audio) BtHelper.disconnectHeadset() (com.android.server.audio) BrokerHandler in AudioDeviceBroker.handleMessage(Message) (com.android.server.audio) BtHelper.receiveBtEvent(Intent) (com.android.server.audio) AudioDeviceBroker.receiveBtEvent(Intent) (com.android.server.audio) BtHelper.onHeadsetProfileConnected(BluetoothHeadset) (com.android.server.audio) BrokerHandler in AudioDeviceBroker.handleMessage(Message) (com.android.server.audio) BtHelper.onSystemReady() (com.android.server.audio) AudioDeviceBroker.onSystemReady() (com.android.server.audio) BtHelper.receiveBtEvent(Intent) (com.android.server.audio) BtHelper.scoClientDied(Object) (com.android.server.audio) BrokerHandler in AudioDeviceBroker.handleMessage(Message) (com.android.server.audio) Bug: 123769055 Test: see bug Change-Id: I5fbb5e8c56d69b8ccfc6b2f44b00169c6b75b632 --- .../server/audio/AudioDeviceBroker.java | 59 ++++-- .../android/server/audio/AudioService.java | 8 +- .../com/android/server/audio/BtHelper.java | 179 +++++++++++------- 3 files changed, 159 insertions(+), 87 deletions(-) diff --git a/services/core/java/com/android/server/audio/AudioDeviceBroker.java b/services/core/java/com/android/server/audio/AudioDeviceBroker.java index 44c1715cfed54..c573332235d88 100644 --- a/services/core/java/com/android/server/audio/AudioDeviceBroker.java +++ b/services/core/java/com/android/server/audio/AudioDeviceBroker.java @@ -114,8 +114,10 @@ import java.util.ArrayList; // All post* methods are asynchronous /*package*/ void onSystemReady() { - synchronized (mDeviceStateLock) { - mBtHelper.onSystemReady(); + synchronized (mSetModeLock) { + synchronized (mDeviceStateLock) { + mBtHelper.onSystemReady(); + } } } @@ -151,8 +153,10 @@ import java.util.ArrayList; * @param intent */ /*package*/ void receiveBtEvent(@NonNull Intent intent) { - synchronized (mDeviceStateLock) { - mBtHelper.receiveBtEvent(intent); + synchronized (mSetModeLock) { + synchronized (mDeviceStateLock) { + mBtHelper.receiveBtEvent(intent); + } } } @@ -350,13 +354,19 @@ import java.util.ArrayList; sendLMsgNoDelay(MSG_L_A2DP_DEVICE_CONFIG_CHANGE, SENDMSG_QUEUE, device); } + @GuardedBy("mSetModeLock") /*package*/ void startBluetoothScoForClient_Sync(IBinder cb, int scoAudioMode, @NonNull String eventSource) { - mBtHelper.startBluetoothScoForClient(cb, scoAudioMode, eventSource); + synchronized (mDeviceStateLock) { + mBtHelper.startBluetoothScoForClient(cb, scoAudioMode, eventSource); + } } + @GuardedBy("mSetModeLock") /*package*/ void stopBluetoothScoForClient_Sync(IBinder cb, @NonNull String eventSource) { - mBtHelper.stopBluetoothScoForClient(cb, eventSource); + synchronized (mDeviceStateLock) { + mBtHelper.stopBluetoothScoForClient(cb, eventSource); + } } //--------------------------------------------------------------------- @@ -479,6 +489,10 @@ import java.util.ArrayList; hearingAidProfile); } + /*package*/ void postScoClientDied(Object obj) { + sendLMsgNoDelay(MSG_L_SCOCLIENT_DIED, SENDMSG_QUEUE, obj); + } + //--------------------------------------------------------------------- // Method forwarding between the helper classes (BtHelper, AudioDeviceInventory) // only call from a "handle"* method or "on"* method @@ -708,8 +722,10 @@ import java.util.ArrayList; } break; case MSG_BT_HEADSET_CNCT_FAILED: - synchronized (mDeviceStateLock) { - mBtHelper.resetBluetoothSco(); + synchronized (mSetModeLock) { + synchronized (mDeviceStateLock) { + mBtHelper.resetBluetoothSco(); + } } break; case MSG_IL_BTA2DP_DOCK_TIMEOUT: @@ -742,8 +758,17 @@ import java.util.ArrayList; } break; case MSG_I_DISCONNECT_BT_SCO: - synchronized (mDeviceStateLock) { - mBtHelper.disconnectBluetoothSco(msg.arg1); + synchronized (mSetModeLock) { + synchronized (mDeviceStateLock) { + mBtHelper.disconnectBluetoothSco(msg.arg1); + } + } + break; + case MSG_L_SCOCLIENT_DIED: + synchronized (mSetModeLock) { + synchronized (mDeviceStateLock) { + mBtHelper.scoClientDied(msg.arg1); + } } break; case MSG_TOGGLE_HDMI: @@ -774,8 +799,10 @@ import java.util.ArrayList; } break; case MSG_DISCONNECT_BT_HEADSET: - synchronized (mDeviceStateLock) { - mBtHelper.disconnectHeadset(); + synchronized (mSetModeLock) { + synchronized (mDeviceStateLock) { + mBtHelper.disconnectHeadset(); + } } break; case MSG_L_BT_SERVICE_CONNECTED_PROFILE_A2DP: @@ -794,8 +821,10 @@ import java.util.ArrayList; } break; case MSG_L_BT_SERVICE_CONNECTED_PROFILE_HEADSET: - synchronized (mDeviceStateLock) { - mBtHelper.onHeadsetProfileConnected((BluetoothHeadset) msg.obj); + synchronized (mSetModeLock) { + synchronized (mDeviceStateLock) { + mBtHelper.onHeadsetProfileConnected((BluetoothHeadset) msg.obj); + } } break; case MSG_L_A2DP_DEVICE_CONNECTION_CHANGE_EXT: { @@ -892,6 +921,8 @@ import java.util.ArrayList; private static final int MSG_L_HEARING_AID_DEVICE_CONNECTION_CHANGE_EXT = 28; // process external command to (dis)connect or change active A2DP device private static final int MSG_L_A2DP_ACTIVE_DEVICE_CHANGE_EXT = 29; + // a ScoClient died in BtHelper + private static final int MSG_L_SCOCLIENT_DIED = 30; private static boolean isMessageHandledUnderWakelock(int msgId) { diff --git a/services/core/java/com/android/server/audio/AudioService.java b/services/core/java/com/android/server/audio/AudioService.java index aee08bb094017..d30a9d2b158e1 100644 --- a/services/core/java/com/android/server/audio/AudioService.java +++ b/services/core/java/com/android/server/audio/AudioService.java @@ -3484,7 +3484,9 @@ public class AudioService extends IAudioService.Stub !mSystemReady) { return; } - mDeviceBroker.startBluetoothScoForClient_Sync(cb, scoAudioMode, eventSource); + synchronized (mDeviceBroker.mSetModeLock) { + mDeviceBroker.startBluetoothScoForClient_Sync(cb, scoAudioMode, eventSource); + } } /** @see AudioManager#stopBluetoothSco() */ @@ -3496,7 +3498,9 @@ public class AudioService extends IAudioService.Stub final String eventSource = new StringBuilder("stopBluetoothSco()") .append(") from u/pid:").append(Binder.getCallingUid()).append("/") .append(Binder.getCallingPid()).toString(); - mDeviceBroker.stopBluetoothScoForClient_Sync(cb, eventSource); + synchronized (mDeviceBroker.mSetModeLock) { + mDeviceBroker.stopBluetoothScoForClient_Sync(cb, eventSource); + } } diff --git a/services/core/java/com/android/server/audio/BtHelper.java b/services/core/java/com/android/server/audio/BtHelper.java index 2d9156b8782e7..332ff362392af 100644 --- a/services/core/java/com/android/server/audio/BtHelper.java +++ b/services/core/java/com/android/server/audio/BtHelper.java @@ -36,6 +36,8 @@ import android.os.UserHandle; import android.provider.Settings; import android.util.Log; +import com.android.internal.annotations.GuardedBy; + import java.util.ArrayList; import java.util.List; import java.util.NoSuchElementException; @@ -163,6 +165,8 @@ public class BtHelper { //---------------------------------------------------------------------- // Interface for AudioDeviceBroker + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void onSystemReady() { mScoConnectionState = android.media.AudioManager.SCO_AUDIO_STATE_ERROR; resetBluetoothSco(); @@ -231,6 +235,8 @@ public class BtHelper { return mapBluetoothCodecToAudioFormat(btCodecConfig.getCodecType()); } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void receiveBtEvent(Intent intent) { final String action = intent.getAction(); if (action.equals(BluetoothHeadset.ACTION_ACTIVE_DEVICE_CHANGED)) { @@ -317,6 +323,8 @@ public class BtHelper { * * @param exceptPid pid whose SCO connections through {@link AudioManager} should be kept */ + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void disconnectBluetoothSco(int exceptPid) { checkScoAudioState(); if (mScoAudioState == SCO_STATE_ACTIVE_EXTERNAL) { @@ -325,6 +333,8 @@ public class BtHelper { clearAllScoClients(exceptPid, true); } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void startBluetoothScoForClient(IBinder cb, int scoAudioMode, @NonNull String eventSource) { ScoClient client = getScoClient(cb, true); @@ -344,6 +354,8 @@ public class BtHelper { Binder.restoreCallingIdentity(ident); } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void stopBluetoothScoForClient(IBinder cb, @NonNull String eventSource) { ScoClient client = getScoClient(cb, false); @@ -401,6 +413,8 @@ public class BtHelper { mDeviceBroker.postDisconnectHearingAid(); } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void resetBluetoothSco() { clearAllScoClients(0, false); mScoAudioState = SCO_STATE_INACTIVE; @@ -409,6 +423,8 @@ public class BtHelper { mDeviceBroker.setBluetoothScoOn(false, "resetBluetoothSco"); } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void disconnectHeadset() { setBtScoActiveDevice(null); mBluetoothHeadset = null; @@ -454,6 +470,8 @@ public class BtHelper { /*eventSource*/ "mBluetoothProfileServiceListener"); } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") /*package*/ synchronized void onHeadsetProfileConnected(BluetoothHeadset headset) { // Discard timeout message mDeviceBroker.handleCancelFailureToConnectToBtHeadsetService(); @@ -540,6 +558,9 @@ public class BtHelper { return result; } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + //@GuardedBy("AudioDeviceBroker.mDeviceStateLock") + @GuardedBy("BtHelper.this") private void setBtScoActiveDevice(BluetoothDevice btDevice) { Log.i(TAG, "setBtScoActiveDevice: " + mBluetoothHeadsetDevice + " -> " + btDevice); final BluetoothDevice previousActiveDevice = mBluetoothHeadsetDevice; @@ -621,6 +642,20 @@ public class BtHelper { }; //---------------------------------------------------------------------- + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + @GuardedBy("AudioDeviceBroker.mDeviceStateLock") + /*package*/ synchronized void scoClientDied(Object obj) { + final ScoClient client = (ScoClient) obj; + Log.w(TAG, "SCO client died"); + int index = mScoClients.indexOf(client); + if (index < 0) { + Log.w(TAG, "unregistered SCO client died"); + } else { + client.clearCount(true); + mScoClients.remove(client); + } + } + private class ScoClient implements IBinder.DeathRecipient { private IBinder mCb; // To be notified of client's death private int mCreatorPid; @@ -634,21 +669,14 @@ public class BtHelper { @Override public void binderDied() { - // this is the only place the implementation of ScoClient needs to be synchronized - // on the instance, as all other methods are directly or indirectly called from - // package-private methods, which are synchronized - synchronized (BtHelper.this) { - Log.w(TAG, "SCO client died"); - int index = mScoClients.indexOf(this); - if (index < 0) { - Log.w(TAG, "unregistered SCO client died"); - } else { - clearCount(true); - mScoClients.remove(this); - } - } + // process this from DeviceBroker's message queue to take the right locks since + // this event can impact SCO mode and requires querying audio mode stack + mDeviceBroker.postScoClientDied(this); } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + // @GuardedBy("AudioDeviceBroker.mDeviceStateLock") + @GuardedBy("BtHelper.this") void incCount(int scoAudioMode) { requestScoState(BluetoothHeadset.STATE_AUDIO_CONNECTED, scoAudioMode); if (mStartcount == 0) { @@ -663,6 +691,9 @@ public class BtHelper { mStartcount++; } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + // @GuardedBy("AudioDeviceBroker.mDeviceStateLock") + @GuardedBy("BtHelper.this") void decCount() { if (mStartcount == 0) { Log.w(TAG, "ScoClient.decCount() already 0"); @@ -679,6 +710,9 @@ public class BtHelper { } } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + // @GuardedBy("AudioDeviceBroker.mDeviceStateLock") + @GuardedBy("BtHelper.this") void clearCount(boolean stopSco) { if (mStartcount != 0) { try { @@ -714,6 +748,9 @@ public class BtHelper { return count; } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + //@GuardedBy("AudioDeviceBroker.mDeviceStateLock") + @GuardedBy("BtHelper.this") private void requestScoState(int state, int scoAudioMode) { checkScoAudioState(); int clientCount = totalCount(); @@ -728,74 +765,71 @@ public class BtHelper { broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_CONNECTING); // Accept SCO audio activation only in NORMAL audio mode or if the mode is // currently controlled by the same client process. - // TODO do not sync that way, see b/123769055 - synchronized (mDeviceBroker.mSetModeLock) { - int modeOwnerPid = mDeviceBroker.getSetModeDeathHandlers().isEmpty() - ? 0 : mDeviceBroker.getSetModeDeathHandlers().get(0).getPid(); - if (modeOwnerPid != 0 && (modeOwnerPid != mCreatorPid)) { - Log.w(TAG, "requestScoState: audio mode is not NORMAL and modeOwnerPid " - + modeOwnerPid + " != creatorPid " + mCreatorPid); - broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_DISCONNECTED); - return; - } - switch (mScoAudioState) { - case SCO_STATE_INACTIVE: - mScoAudioMode = scoAudioMode; - if (scoAudioMode == SCO_MODE_UNDEFINED) { - mScoAudioMode = SCO_MODE_VIRTUAL_CALL; - if (mBluetoothHeadsetDevice != null) { - mScoAudioMode = Settings.Global.getInt( - mDeviceBroker.getContentResolver(), - "bluetooth_sco_channel_" - + mBluetoothHeadsetDevice.getAddress(), - SCO_MODE_VIRTUAL_CALL); - if (mScoAudioMode > SCO_MODE_MAX || mScoAudioMode < 0) { - mScoAudioMode = SCO_MODE_VIRTUAL_CALL; - } + int modeOwnerPid = mDeviceBroker.getSetModeDeathHandlers().isEmpty() + ? 0 : mDeviceBroker.getSetModeDeathHandlers().get(0).getPid(); + if (modeOwnerPid != 0 && (modeOwnerPid != mCreatorPid)) { + Log.w(TAG, "requestScoState: audio mode is not NORMAL and modeOwnerPid " + + modeOwnerPid + " != creatorPid " + mCreatorPid); + broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_DISCONNECTED); + return; + } + switch (mScoAudioState) { + case SCO_STATE_INACTIVE: + mScoAudioMode = scoAudioMode; + if (scoAudioMode == SCO_MODE_UNDEFINED) { + mScoAudioMode = SCO_MODE_VIRTUAL_CALL; + if (mBluetoothHeadsetDevice != null) { + mScoAudioMode = Settings.Global.getInt( + mDeviceBroker.getContentResolver(), + "bluetooth_sco_channel_" + + mBluetoothHeadsetDevice.getAddress(), + SCO_MODE_VIRTUAL_CALL); + if (mScoAudioMode > SCO_MODE_MAX || mScoAudioMode < 0) { + mScoAudioMode = SCO_MODE_VIRTUAL_CALL; } } - if (mBluetoothHeadset == null) { - if (getBluetoothHeadset()) { - mScoAudioState = SCO_STATE_ACTIVATE_REQ; - } else { - Log.w(TAG, "requestScoState: getBluetoothHeadset failed during" - + " connection, mScoAudioMode=" + mScoAudioMode); - broadcastScoConnectionState( - AudioManager.SCO_AUDIO_STATE_DISCONNECTED); - } - break; - } - if (mBluetoothHeadsetDevice == null) { - Log.w(TAG, "requestScoState: no active device while connecting," - + " mScoAudioMode=" + mScoAudioMode); - broadcastScoConnectionState( - AudioManager.SCO_AUDIO_STATE_DISCONNECTED); - break; - } - if (connectBluetoothScoAudioHelper(mBluetoothHeadset, - mBluetoothHeadsetDevice, mScoAudioMode)) { - mScoAudioState = SCO_STATE_ACTIVE_INTERNAL; + } + if (mBluetoothHeadset == null) { + if (getBluetoothHeadset()) { + mScoAudioState = SCO_STATE_ACTIVATE_REQ; } else { - Log.w(TAG, "requestScoState: connect to " + mBluetoothHeadsetDevice - + " failed, mScoAudioMode=" + mScoAudioMode); + Log.w(TAG, "requestScoState: getBluetoothHeadset failed during" + + " connection, mScoAudioMode=" + mScoAudioMode); broadcastScoConnectionState( AudioManager.SCO_AUDIO_STATE_DISCONNECTED); } break; - case SCO_STATE_DEACTIVATING: - mScoAudioState = SCO_STATE_ACTIVATE_REQ; + } + if (mBluetoothHeadsetDevice == null) { + Log.w(TAG, "requestScoState: no active device while connecting," + + " mScoAudioMode=" + mScoAudioMode); + broadcastScoConnectionState( + AudioManager.SCO_AUDIO_STATE_DISCONNECTED); break; - case SCO_STATE_DEACTIVATE_REQ: + } + if (connectBluetoothScoAudioHelper(mBluetoothHeadset, + mBluetoothHeadsetDevice, mScoAudioMode)) { mScoAudioState = SCO_STATE_ACTIVE_INTERNAL; - broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_CONNECTED); - break; - default: - Log.w(TAG, "requestScoState: failed to connect in state " - + mScoAudioState + ", scoAudioMode=" + scoAudioMode); - broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_DISCONNECTED); - break; + } else { + Log.w(TAG, "requestScoState: connect to " + mBluetoothHeadsetDevice + + " failed, mScoAudioMode=" + mScoAudioMode); + broadcastScoConnectionState( + AudioManager.SCO_AUDIO_STATE_DISCONNECTED); + } + break; + case SCO_STATE_DEACTIVATING: + mScoAudioState = SCO_STATE_ACTIVATE_REQ; + break; + case SCO_STATE_DEACTIVATE_REQ: + mScoAudioState = SCO_STATE_ACTIVE_INTERNAL; + broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_CONNECTED); + break; + default: + Log.w(TAG, "requestScoState: failed to connect in state " + + mScoAudioState + ", scoAudioMode=" + scoAudioMode); + broadcastScoConnectionState(AudioManager.SCO_AUDIO_STATE_DISCONNECTED); + break; - } } } else if (state == BluetoothHeadset.STATE_AUDIO_DISCONNECTED) { switch (mScoAudioState) { @@ -906,6 +940,9 @@ public class BtHelper { return null; } + // @GuardedBy("AudioDeviceBroker.mSetModeLock") + //@GuardedBy("AudioDeviceBroker.mDeviceStateLock") + @GuardedBy("BtHelper.this") private void clearAllScoClients(int exceptPid, boolean stopSco) { ScoClient savedClient = null; for (ScoClient cl : mScoClients) {