From 76e124bcd3ee2e5ba98dcbdb99965a4c133fe7e4 Mon Sep 17 00:00:00 2001 From: Jean-Michel Trivi Date: Tue, 10 Apr 2018 14:06:07 -0700 Subject: [PATCH] PlayerBase: fix deadlock Source of deadlock between PlayerBase.mLock and PlaybackActivityMonitor.mPlayerLock: android.media.MediaPlayer.release() > android.media.PlayerBase.baseRelease() > synchronized (mLock) > com.android.server.audio.PlaybackActivityMonitor.releasePlayer() > synchronized(mPlayerLock) and: com.android.server.audio.PlaybackActivityMonitor.unmutePlayersForCall() > synchronized (mPlayerLock) > android.media.PlayerProxy.setVolume() > android.media.PlayerBase$IPlayerWrapper.setVolume() > android.media.PlayerBase.baseSetVolume() > synchronized (mLock) playerSetVolume() Since system_server can have its own players, the calls to AudioService from PlayerBase can be synchronous, hence the deadlock. The fix consists in never holding the lock in PlayerBase while calling into AudioService. Refactor the playstate update into a method used for start / stop / pause. Bug: 72294559 Test: see bug Change-Id: I6451aa3bf19a0365472ba007b116a9e6151ab33e --- .../media/AudioPlaybackConfiguration.java | 2 + media/java/android/media/PlayerBase.java | 84 ++++++++++--------- 2 files changed, 48 insertions(+), 38 deletions(-) diff --git a/media/java/android/media/AudioPlaybackConfiguration.java b/media/java/android/media/AudioPlaybackConfiguration.java index 8a36f91cfff66..7dfdb20a98fe2 100644 --- a/media/java/android/media/AudioPlaybackConfiguration.java +++ b/media/java/android/media/AudioPlaybackConfiguration.java @@ -43,6 +43,8 @@ public final class AudioPlaybackConfiguration implements Parcelable { /** @hide */ public static final int PLAYER_PIID_INVALID = -1; /** @hide */ + public static final int PLAYER_PIID_UNASSIGNED = 0; + /** @hide */ public static final int PLAYER_UPID_INVALID = -1; // information about the implementation diff --git a/media/java/android/media/PlayerBase.java b/media/java/android/media/PlayerBase.java index 80049ba5614a6..7c6367e8123ac 100644 --- a/media/java/android/media/PlayerBase.java +++ b/media/java/android/media/PlayerBase.java @@ -31,6 +31,7 @@ import android.os.RemoteException; import android.os.ServiceManager; import android.util.Log; +import com.android.internal.annotations.GuardedBy; import com.android.internal.app.IAppOpsCallback; import com.android.internal.app.IAppOpsService; @@ -58,20 +59,29 @@ public abstract class PlayerBase { protected float mRightVolume = 1.0f; protected float mAuxEffectSendLevel = 0.0f; - // for AppOps - private IAppOpsService mAppOps; // may be null - private IAppOpsCallback mAppOpsCallback; - private boolean mHasAppOpsPlayAudio = true; // sync'd on mLock + // NEVER call into AudioService (see getService()) with mLock held: PlayerBase can run in + // the same process as AudioService, which can synchronously call back into this class, + // causing deadlocks between the two private final Object mLock = new Object(); + // for AppOps + private @Nullable IAppOpsService mAppOps; + private IAppOpsCallback mAppOpsCallback; + @GuardedBy("mLock") + private boolean mHasAppOpsPlayAudio = true; + private final int mImplType; // uniquely identifies the Player Interface throughout the system (P I Id) - private int mPlayerIId; + private int mPlayerIId = AudioPlaybackConfiguration.PLAYER_PIID_UNASSIGNED; - private int mState; // sync'd on mLock - private int mStartDelayMs = 0; // sync'd on mLock - private float mPanMultiplierL = 1.0f; // sync'd on mLock - private float mPanMultiplierR = 1.0f; // sync'd on mLock + @GuardedBy("mLock") + private int mState; + @GuardedBy("mLock") + private int mStartDelayMs = 0; + @GuardedBy("mLock") + private float mPanMultiplierL = 1.0f; + @GuardedBy("mLock") + private float mPanMultiplierR = 1.0f; /** * Constructor. Must be given audio attributes, as they are required for AppOps. @@ -134,16 +144,24 @@ public abstract class PlayerBase { } } + private void updateState(int state) { + final int piid; + synchronized (mLock) { + mState = state; + piid = mPlayerIId; + } + try { + getService().playerEvent(piid, state); + } catch (RemoteException e) { + Log.e(TAG, "Error talking to audio service, " + + AudioPlaybackConfiguration.toLogFriendlyPlayerState(state) + + " state will not be tracked for piid=" + piid, e); + } + } + void baseStart() { if (DEBUG) { Log.v(TAG, "baseStart() piid=" + mPlayerIId); } - try { - synchronized (mLock) { - mState = AudioPlaybackConfiguration.PLAYER_STATE_STARTED; - getService().playerEvent(mPlayerIId, mState); - } - } catch (RemoteException e) { - Log.e(TAG, "Error talking to audio service, STARTED state will not be tracked", e); - } + updateState(AudioPlaybackConfiguration.PLAYER_STATE_STARTED); synchronized (mLock) { if (isRestricted_sync()) { playerSetVolume(true/*muting*/,0, 0); @@ -165,26 +183,12 @@ public abstract class PlayerBase { void basePause() { if (DEBUG) { Log.v(TAG, "basePause() piid=" + mPlayerIId); } - try { - synchronized (mLock) { - mState = AudioPlaybackConfiguration.PLAYER_STATE_PAUSED; - getService().playerEvent(mPlayerIId, mState); - } - } catch (RemoteException e) { - Log.e(TAG, "Error talking to audio service, PAUSED state will not be tracked", e); - } + updateState(AudioPlaybackConfiguration.PLAYER_STATE_PAUSED); } void baseStop() { if (DEBUG) { Log.v(TAG, "baseStop() piid=" + mPlayerIId); } - try { - synchronized (mLock) { - mState = AudioPlaybackConfiguration.PLAYER_STATE_STOPPED; - getService().playerEvent(mPlayerIId, mState); - } - } catch (RemoteException e) { - Log.e(TAG, "Error talking to audio service, STOPPED state will not be tracked", e); - } + updateState(AudioPlaybackConfiguration.PLAYER_STATE_STOPPED); } void baseSetPan(float pan) { @@ -228,12 +232,16 @@ public abstract class PlayerBase { */ void baseRelease() { if (DEBUG) { Log.v(TAG, "baseRelease() piid=" + mPlayerIId + " state=" + mState); } + boolean releasePlayer = false; + synchronized (mLock) { + if (mState != AudioPlaybackConfiguration.PLAYER_STATE_RELEASED) { + releasePlayer = true; + mState = AudioPlaybackConfiguration.PLAYER_STATE_RELEASED; + } + } try { - synchronized (mLock) { - if (mState != AudioPlaybackConfiguration.PLAYER_STATE_RELEASED) { - getService().releasePlayer(mPlayerIId); - mState = AudioPlaybackConfiguration.PLAYER_STATE_RELEASED; - } + if (releasePlayer) { + getService().releasePlayer(mPlayerIId); } } catch (RemoteException e) { Log.e(TAG, "Error talking to audio service, the player will still be tracked", e);