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

Commit 76e124bc authored by Jean-Michel Trivi's avatar Jean-Michel Trivi
Browse files

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
parent 42c68686

media/java/android/media/AudioPlaybackConfiguration.java

+2 −0
Original line number Diff line number Diff line
@@ -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

media/java/android/media/PlayerBase.java

+44 −36
Original line number Diff line number Diff line
@@ -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;



    // 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 IAppOpsService mAppOps; // may be null

    private @Nullable IAppOpsService mAppOps;

    private IAppOpsCallback mAppOpsCallback;

    private boolean mHasAppOpsPlayAudio = true; // sync'd on mLock

    private final Object mLock = new Object();

    @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 { 
        }

    }



    void baseStart() {

        if (DEBUG) { Log.v(TAG, "baseStart() piid=" + mPlayerIId); }

        try {

    private void updateState(int state) {

        final int piid;

        synchronized (mLock) {

                mState = AudioPlaybackConfiguration.PLAYER_STATE_STARTED;

                getService().playerEvent(mPlayerIId, mState);

            mState = state;

            piid = mPlayerIId;

        }

        try {

            getService().playerEvent(piid, state);

        } catch (RemoteException e) {

            Log.e(TAG, "Error talking to audio service, STARTED state will not be tracked", 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); }

        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,13 +232,17 @@ public abstract class PlayerBase { 
     */

    void baseRelease() {

        if (DEBUG) { Log.v(TAG, "baseRelease() piid=" + mPlayerIId + " state=" + mState); }

        try {

        boolean releasePlayer = false;

        synchronized (mLock) {

            if (mState != AudioPlaybackConfiguration.PLAYER_STATE_RELEASED) {

                    getService().releasePlayer(mPlayerIId);

                releasePlayer = true;

                mState = AudioPlaybackConfiguration.PLAYER_STATE_RELEASED;

            }

        }

        try {

            if (releasePlayer) {

                getService().releasePlayer(mPlayerIId);

            }

        } catch (RemoteException e) {

            Log.e(TAG, "Error talking to audio service, the player will still be tracked", e);

        }