From 776a39931499d7d118eba916aba017032cde49a9 Mon Sep 17 00:00:00 2001 From: Jean-Michel Trivi Date: Tue, 12 Sep 2017 16:45:34 -0700 Subject: [PATCH] AudioPlaybackConfiguration: prevent race condition on mIPlayerShell Synchronize changes to mIPlayerShell after release of corresponding player. Flush binder commands when a player is released, in AudioService and in the clients that have an AudioPlaybackCallback implementation. Do the same in MediaSessionService, which directly implements the IPlaybackConfigDispatcher interface, without going through the AudioPlaybackCallback registration and notification mechanisms. Test: adb shell /system/bin/write_sine_callback -m2 -pl Bug: 65450109 Change-Id: I2f0697e0e164283284ce30d2cc736c4f8df270c4 --- media/java/android/media/AudioManager.java | 6 ++- .../media/AudioPlaybackConfiguration.java | 52 ++++++++++++++----- .../media/IPlaybackConfigDispatcher.aidl | 2 +- .../server/audio/PlaybackActivityMonitor.java | 27 +++++++--- .../server/media/AudioPlaybackMonitor.java | 6 ++- 5 files changed, 71 insertions(+), 22 deletions(-) diff --git a/media/java/android/media/AudioManager.java b/media/java/android/media/AudioManager.java index eea4628e83af1..3df17068725e3 100644 --- a/media/java/android/media/AudioManager.java +++ b/media/java/android/media/AudioManager.java @@ -3058,7 +3058,11 @@ public class AudioManager { private final IPlaybackConfigDispatcher mPlayCb = new IPlaybackConfigDispatcher.Stub() { @Override - public void dispatchPlaybackConfigChange(List configs) { + public void dispatchPlaybackConfigChange(List configs, + boolean flush) { + if (flush) { + Binder.flushPendingCommands(); + } synchronized(mPlaybackCallbackLock) { if (mPlaybackCallbackList != null) { for (int i=0 ; i < mPlaybackCallbackList.size() ; i++) { diff --git a/media/java/android/media/AudioPlaybackConfiguration.java b/media/java/android/media/AudioPlaybackConfiguration.java index 14bc5551ba491..8a36f91cfff66 100644 --- a/media/java/android/media/AudioPlaybackConfiguration.java +++ b/media/java/android/media/AudioPlaybackConfiguration.java @@ -212,8 +212,10 @@ public final class AudioPlaybackConfiguration implements Parcelable { * @hide */ public void init() { - if (mIPlayerShell != null) { - mIPlayerShell.monitorDeath(); + synchronized (this) { + if (mIPlayerShell != null) { + mIPlayerShell.monitorDeath(); + } } } @@ -322,7 +324,11 @@ public final class AudioPlaybackConfiguration implements Parcelable { */ @SystemApi public PlayerProxy getPlayerProxy() { - return mIPlayerShell == null ? null : new PlayerProxy(this); + final IPlayerShell ips; + synchronized (this) { + ips = mIPlayerShell; + } + return ips == null ? null : new PlayerProxy(this); } /** @@ -330,7 +336,11 @@ public final class AudioPlaybackConfiguration implements Parcelable { * @return the IPlayer interface for the associated player */ IPlayer getIPlayer() { - return mIPlayerShell == null ? null : mIPlayerShell.getIPlayer(); + final IPlayerShell ips; + synchronized (this) { + ips = mIPlayerShell; + } + return ips == null ? null : ips.getIPlayer(); } /** @@ -351,10 +361,14 @@ public final class AudioPlaybackConfiguration implements Parcelable { * @return true if the state changed, false otherwise */ public boolean handleStateEvent(int event) { - final boolean changed = (mPlayerState != event); - mPlayerState = event; - if ((event == PLAYER_STATE_RELEASED) && (mIPlayerShell != null)) { - mIPlayerShell.release(); + final boolean changed; + synchronized (this) { + changed = (mPlayerState != event); + mPlayerState = event; + if (changed && (event == PLAYER_STATE_RELEASED) && (mIPlayerShell != null)) { + mIPlayerShell.release(); + mIPlayerShell = null; + } } return changed; } @@ -447,7 +461,11 @@ public final class AudioPlaybackConfiguration implements Parcelable { dest.writeInt(mClientPid); dest.writeInt(mPlayerState); mPlayerAttr.writeToParcel(dest, 0); - dest.writeStrongInterface(mIPlayerShell == null ? null : mIPlayerShell.getIPlayer()); + final IPlayerShell ips; + synchronized (this) { + ips = mIPlayerShell; + } + dest.writeStrongInterface(ips == null ? null : ips.getIPlayer()); } private AudioPlaybackConfiguration(Parcel in) { @@ -479,14 +497,17 @@ public final class AudioPlaybackConfiguration implements Parcelable { static final class IPlayerShell implements IBinder.DeathRecipient { final AudioPlaybackConfiguration mMonitor; // never null - private IPlayer mIPlayer; + private volatile IPlayer mIPlayer; IPlayerShell(@NonNull AudioPlaybackConfiguration monitor, @NonNull IPlayer iplayer) { mMonitor = monitor; mIPlayer = iplayer; } - void monitorDeath() { + synchronized void monitorDeath() { + if (mIPlayer == null) { + return; + } try { mIPlayer.asBinder().linkToDeath(this, 0); } catch (RemoteException e) { @@ -509,8 +530,13 @@ public final class AudioPlaybackConfiguration implements Parcelable { } else if (DEBUG) { Log.i(TAG, "IPlayerShell binderDied"); } } - void release() { + synchronized void release() { + if (mIPlayer == null) { + return; + } mIPlayer.asBinder().unlinkToDeath(this, 0); + mIPlayer = null; + Binder.flushPendingCommands(); } } @@ -532,7 +558,7 @@ public final class AudioPlaybackConfiguration implements Parcelable { case PLAYER_TYPE_HW_SOURCE: return "hardware source"; case PLAYER_TYPE_EXTERNAL_PROXY: return "external proxy"; default: - return "unknown player type - FIXME"; + return "unknown player type " + type + " - FIXME"; } } diff --git a/media/java/android/media/IPlaybackConfigDispatcher.aidl b/media/java/android/media/IPlaybackConfigDispatcher.aidl index 3cb52161d9bf7..150ff26043626 100644 --- a/media/java/android/media/IPlaybackConfigDispatcher.aidl +++ b/media/java/android/media/IPlaybackConfigDispatcher.aidl @@ -25,6 +25,6 @@ import android.media.AudioPlaybackConfiguration; */ oneway interface IPlaybackConfigDispatcher { - void dispatchPlaybackConfigChange(in List configs); + void dispatchPlaybackConfigChange(in List configs, in boolean flush); } diff --git a/services/core/java/com/android/server/audio/PlaybackActivityMonitor.java b/services/core/java/com/android/server/audio/PlaybackActivityMonitor.java index 27e4d14ee94b3..d1a37af5e7a8d 100644 --- a/services/core/java/com/android/server/audio/PlaybackActivityMonitor.java +++ b/services/core/java/com/android/server/audio/PlaybackActivityMonitor.java @@ -171,7 +171,7 @@ public final class PlaybackActivityMonitor } } if (change) { - dispatchPlaybackChange(); + dispatchPlaybackChange(false); } } @@ -210,7 +210,7 @@ public final class PlaybackActivityMonitor } } if (change) { - dispatchPlaybackChange(); + dispatchPlaybackChange(event == AudioPlaybackConfiguration.PLAYER_STATE_RELEASED); } } @@ -244,6 +244,14 @@ public final class PlaybackActivityMonitor pw.println("\nPlaybackActivityMonitor dump time: " + DateFormat.getTimeInstance().format(new Date())); synchronized(mPlayerLock) { + pw.println("\n playback listeners:"); + synchronized(mClients) { + for (PlayMonitorClient pmc : mClients) { + pw.print(" " + (pmc.mIsPrivileged ? "(S)" : "(P)") + + pmc.toString()); + } + } + pw.println("\n"); // all players pw.println("\n players:"); final List piidIntList = new ArrayList(mPlayers.keySet()); @@ -268,7 +276,7 @@ public final class PlaybackActivityMonitor for (int uid : mBannedUids) { pw.print(" " + uid); } - pw.println(); + pw.println("\n"); // log mEventLogger.dump(pw); } @@ -293,7 +301,11 @@ public final class PlaybackActivityMonitor return true; } - private void dispatchPlaybackChange() { + /** + * Sends new list after update of playback configurations + * @param iplayerReleased indicates if the change was due to a player being released + */ + private void dispatchPlaybackChange(boolean iplayerReleased) { synchronized (mClients) { // typical use case, nobody is listening, don't do any work if (mClients.isEmpty()) { @@ -324,9 +336,12 @@ public final class PlaybackActivityMonitor // do not spam the logs if there are problems communicating with this client if (pmc.mErrorCount < PlayMonitorClient.MAX_ERRORS) { if (pmc.mIsPrivileged) { - pmc.mDispatcherCb.dispatchPlaybackConfigChange(configsSystem); + pmc.mDispatcherCb.dispatchPlaybackConfigChange(configsSystem, + iplayerReleased); } else { - pmc.mDispatcherCb.dispatchPlaybackConfigChange(configsPublic); + // non-system clients don't have the control interface IPlayer, so + // they don't need to flush commands when a player was released + pmc.mDispatcherCb.dispatchPlaybackConfigChange(configsPublic, false); } } } catch (RemoteException e) { diff --git a/services/core/java/com/android/server/media/AudioPlaybackMonitor.java b/services/core/java/com/android/server/media/AudioPlaybackMonitor.java index f6f767653c832..791ee821b357d 100644 --- a/services/core/java/com/android/server/media/AudioPlaybackMonitor.java +++ b/services/core/java/com/android/server/media/AudioPlaybackMonitor.java @@ -100,7 +100,11 @@ class AudioPlaybackMonitor extends IPlaybackConfigDispatcher.Stub { * @param configs List of the current audio playback configuration */ @Override - public void dispatchPlaybackConfigChange(List configs) { + public void dispatchPlaybackConfigChange(List configs, + boolean flush) { + if (flush) { + Binder.flushPendingCommands(); + } final long token = Binder.clearCallingIdentity(); try { List newActiveAudioPlaybackClientUids = new ArrayList<>();