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

Commit 0260938a authored by Jean-Michel Trivi's avatar Jean-Michel Trivi
Browse files

NotificationPlayer: fix race conditions 

This patch fixes two race conditions that affect the Looper used
  to signal the completion of a notification to abandon audio focus
  as well as the wakelock used between issuing a playback
  command and its actual start. Annotations are added to clarify
  which objects are used to synchronize which methods and variables.

Looper for notification playback completion:
  Before a notification starts playing, audio focus is requested,
  which causes the ducking of media apps. When the notification
  completes, audio focus is abandoned. If a new notification is
  to be played while one is playing, the current player is
  stopped and the Looper on which we expect the MediaPlayer
  completion callback is .quit(). But there is a race condition
  between the quitting of the current Looper whenever a sound
  is started (in startSound()) and when quit when playback
  is stopped (command STOP), and when created in
  CreationAndCompletionThread.run(). If the Looper is quit
  when another notification starts to play, the completion
  callback cannot be received, and audio focus will not be
  abandoned.
  The fix consists in synchronizing all access to mLooper
  on mCompletionHandlingLock.

Wakelock:
  Initializing and acquiring the wakelock, and releasing it
  are done in different threads (client thread vs CmdThread).
  There was no memory barrier between the initialization
  and release. The fix consists in making all wakelock
  operations synchronized on mCmdQueue.

Test: issue multiple notifications that interrupt eachother, verify focus is abandonned (in logs, check "abandonAudioFocus()")
Bug: 65866087
Bug: 64531811

Change-Id: Ie8f4091eaa96bd0bcb732e27423f6e31e76da98e
parent 8f22de0e

packages/SystemUI/src/com/android/systemui/media/NotificationPlayer.java

+54 −20
Original line number Diff line number Diff line
@@ -29,6 +29,8 @@ import android.os.PowerManager; 
import android.os.SystemClock;

import android.util.Log;



import com.android.internal.annotations.GuardedBy;



import java.util.LinkedList;



/**
@@ -57,8 +59,12 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
        }

    }



    private LinkedList<Command> mCmdQueue = new LinkedList();

    private final LinkedList<Command> mCmdQueue = new LinkedList<Command>();



    private final Object mCompletionHandlingLock = new Object();

    @GuardedBy("mCompletionHandlingLock")

    private CreationAndCompletionThread mCompletionThread;

    @GuardedBy("mCompletionHandlingLock")

    private Looper mLooper;



    /*
@@ -76,7 +82,10 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 


        public void run() {

            Looper.prepare();

            // ok to modify mLooper as here we are

            // synchronized on mCompletionHandlingLock due to the Object.wait() in startSound(cmd)

            mLooper = Looper.myLooper();

            if (DEBUG) Log.d(mTag, "in run: new looper " + mLooper);

            synchronized(this) {

                AudioManager audioManager =

                    (AudioManager) mCmd.context.getSystemService(Context.AUDIO_SERVICE);
@@ -129,7 +138,9 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
                        Log.e(mTag, "Exception while sleeping to sync notification playback"

                                + " with ducking", e);

                    }

                    if (DEBUG) { Log.d(mTag, "player.start"); }

                    if (mPlayer != null) {

                        if (DEBUG) { Log.d(mTag, "mPlayer.release"); }

                        mPlayer.release();

                    }

                    mPlayer = player;
@@ -148,7 +159,7 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
        // is playing, let it continue until we're done, so there

        // is less of a glitch.

        try {

            if (DEBUG) Log.d(mTag, "Starting playback");

            if (DEBUG) { Log.d(mTag, "startSound()"); }

            //-----------------------------------

            // This is were we deviate from the AsyncPlayer implementation and create the

            // MediaPlayer in a new thread with which we're synchronized
@@ -158,6 +169,7 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
                // matters

                if((mLooper != null)

                        && (mLooper.getThread().getState() != Thread.State.TERMINATED)) {

                    if (DEBUG) { Log.d(mTag, "in startSound quitting looper " + mLooper); }

                    mLooper.quit();

                }

                mCompletionThread = new CreationAndCompletionThread(cmd);
@@ -209,14 +221,19 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
                        mPlayer = null;

                        synchronized(mQueueAudioFocusLock) {

                            if (mAudioManagerWithAudioFocus != null) {

                                if (DEBUG) { Log.d(mTag, "in STOP: abandonning AudioFocus"); }

                                mAudioManagerWithAudioFocus.abandonAudioFocus(null);

                                mAudioManagerWithAudioFocus = null;

                            }

                        }

                        if((mLooper != null)

                                && (mLooper.getThread().getState() != Thread.State.TERMINATED)) {

                        synchronized (mCompletionHandlingLock) {

                            if ((mLooper != null) &&

                                    (mLooper.getThread().getState() != Thread.State.TERMINATED))

                            {

                                if (DEBUG) { Log.d(mTag, "in STOP: quitting looper "+ mLooper); }

                                mLooper.quit();

                            }

                        }

                    } else {

                        Log.w(mTag, "STOP command without a player");

                    }
@@ -250,9 +267,11 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
        }

        // if there are no more sounds to play, end the Looper to listen for media completion

        synchronized (mCmdQueue) {

            if (mCmdQueue.size() == 0) {

            synchronized(mCompletionHandlingLock) {

                if (DEBUG) { Log.d(mTag, "onCompletion queue size=" + mCmdQueue.size()); }

                if ((mCmdQueue.size() == 0)) {

                    if (mLooper != null) {

                        if (DEBUG) { Log.d(mTag, "in onCompletion quitting looper " + mLooper); }

                        mLooper.quit();

                    }

                    mCompletionThread = null;
@@ -269,13 +288,20 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
    }



    private String mTag;



    @GuardedBy("mCmdQueue")

    private CmdThread mThread;

    private CreationAndCompletionThread mCompletionThread;

    private final Object mCompletionHandlingLock = new Object();



    private MediaPlayer mPlayer;





    @GuardedBy("mCmdQueue")

    private PowerManager.WakeLock mWakeLock;



    private final Object mQueueAudioFocusLock = new Object();

    private AudioManager mAudioManagerWithAudioFocus; // synchronized on mQueueAudioFocusLock

    @GuardedBy("mQueueAudioFocusLock")

    private AudioManager mAudioManagerWithAudioFocus;



    private int mNotificationRampTimeMs = 0;



    // The current state according to the caller.  Reality lags behind
@@ -311,6 +337,7 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
     */

    @Deprecated

    public void play(Context context, Uri uri, boolean looping, int stream) {

        if (DEBUG) { Log.d(mTag, "play uri=" + uri.toString()); }

        PlayerBase.deprecateStreamTypeForPlayback(stream, "NotificationPlayer", "play");

        Command cmd = new Command();

        cmd.requestTime = SystemClock.uptimeMillis();
@@ -339,6 +366,7 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
     *          (see {@link MediaPlayer#setAudioAttributes(AudioAttributes)})

     */

    public void play(Context context, Uri uri, boolean looping, AudioAttributes attributes) {

        if (DEBUG) { Log.d(mTag, "play uri=" + uri.toString()); }

        Command cmd = new Command();

        cmd.requestTime = SystemClock.uptimeMillis();

        cmd.code = PLAY;
@@ -357,6 +385,7 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
     * at this point.  Calling this multiple times has no ill effects.

     */

    public void stop() {

        if (DEBUG) { Log.d(mTag, "stop"); }

        synchronized (mCmdQueue) {

            // This check allows stop to be called multiple times without starting

            // a thread that ends up doing nothing.
@@ -370,6 +399,7 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
        }

    }



    @GuardedBy("mCmdQueue")

    private void enqueueLocked(Command cmd) {

        mCmdQueue.add(cmd);

        if (mThread == null) {
@@ -393,6 +423,7 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
     * @hide

     */

    public void setUsesWakeLock(Context context) {

        synchronized (mCmdQueue) {

            if (mWakeLock != null || mThread != null) {

                // if either of these has happened, we've already played something.

                // and our releases will be out of sync.
@@ -402,13 +433,16 @@ public class NotificationPlayer implements OnCompletionListener, OnErrorListener 
            PowerManager pm = (PowerManager)context.getSystemService(Context.POWER_SERVICE);

            mWakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, mTag);

        }

    }



    @GuardedBy("mCmdQueue")

    private void acquireWakeLock() {

        if (mWakeLock != null) {

            mWakeLock.acquire();

        }

    }



    @GuardedBy("mCmdQueue")

    private void releaseWakeLock() {

        if (mWakeLock != null) {

            mWakeLock.release();