From cf1b224a789f8412211d22fba9551ce01e54be14 Mon Sep 17 00:00:00 2001 From: Jean-Michel Trivi Date: Tue, 9 May 2017 11:19:42 -0700 Subject: [PATCH 1/2] IPlayer has weak reference to PlayerBase The implementation of the IPlayer interface was an inner class that implicitly held a strong reference to the PlayerBase instance, preventing subclasses of PlayerBase to be GC'd. The fix consists in making the IPlayer implementation be a static class and hold a weak reference to PlayerBase. Test: see bug Bug: 35359144 Change-Id: I5f7d658f4bda07c92cfdb437b42d3f78213ab552 --- media/java/android/media/PlayerBase.java | 55 ++++++++++++++++++------ 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/media/java/android/media/PlayerBase.java b/media/java/android/media/PlayerBase.java index dd666493c346f..72be9f71d3a81 100644 --- a/media/java/android/media/PlayerBase.java +++ b/media/java/android/media/PlayerBase.java @@ -35,6 +35,7 @@ import com.android.internal.app.IAppOpsCallback; import com.android.internal.app.IAppOpsService; import java.lang.IllegalArgumentException; +import java.lang.ref.WeakReference; import java.util.Objects; /** @@ -114,10 +115,8 @@ public abstract class PlayerBase { mHasAppOpsPlayAudio = false; } try { - if (mIPlayer == null) { - throw new IllegalStateException("Cannot register a player with a null mIPlayer"); - } - newPiid = getService().trackPlayer(new PlayerIdCard(mImplType, mAttributes, mIPlayer)); + newPiid = getService().trackPlayer( + new PlayerIdCard(mImplType, mAttributes, new IPlayerWrapper(this))); } catch (RemoteException e) { Log.e(TAG, "Error talking to audio service, player will not be tracked", e); } @@ -407,46 +406,74 @@ public abstract class PlayerBase { //===================================================================== /** - * Implementation of IPlayer for all subclasses of PlayerBase + * Wrapper around an implementation of IPlayer for all subclasses of PlayerBase + * that doesn't keep a strong reference on PlayerBase */ - private IPlayer mIPlayer = new IPlayer.Stub() { + private static class IPlayerWrapper extends IPlayer.Stub { + private final WeakReference mWeakPB; + + public IPlayerWrapper(PlayerBase pb) { + mWeakPB = new WeakReference(pb); + } + @Override public void start() { - playerStart(); + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.playerStart(); + } } @Override public void pause() { - playerPause(); + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.playerPause(); + } } @Override public void stop() { - playerStop(); + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.playerStop(); + } } @Override public void setVolume(float vol) { - baseSetVolume(vol, vol); + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.baseSetVolume(vol, vol); + } } @Override public void setPan(float pan) { - baseSetPan(pan); + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.baseSetPan(pan); + } } @Override public void setStartDelayMs(int delayMs) { - baseSetStartDelayMs(delayMs); + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.baseSetStartDelayMs(delayMs); + } } @Override public void applyVolumeShaper( @NonNull VolumeShaper.Configuration configuration, @NonNull VolumeShaper.Operation operation) { - /* void */ playerApplyVolumeShaper(configuration, operation); + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.playerApplyVolumeShaper(configuration, operation); + } } - }; + } //===================================================================== /** From aee6ee94675d56e71a42d52b16b8d8e5fa6ea3ff Mon Sep 17 00:00:00 2001 From: Jean-Michel Trivi Date: Tue, 9 May 2017 16:19:36 -0700 Subject: [PATCH 2/2] IAppOpsCallback has weak reference to PlayerBase The implementation of the IAppOpsCallback interface was an inner class that implicitly held a strong reference to the PlayerBase instance, preventing subclasses of PlayerBase to be GC'd. The fix consists in making the IAppOpsCallback implementation be a static class and hold a weak reference to PlayerBase. Test: see bug Bug: 35359144 Change-Id: Ic97d07dad0be2376eef160d01ff4e4a9e5ee0bcd --- media/java/android/media/PlayerBase.java | 50 ++++++++++++++++-------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/media/java/android/media/PlayerBase.java b/media/java/android/media/PlayerBase.java index 72be9f71d3a81..9bd93aa31abf8 100644 --- a/media/java/android/media/PlayerBase.java +++ b/media/java/android/media/PlayerBase.java @@ -46,11 +46,11 @@ import java.util.Objects; */ public abstract class PlayerBase { - private final static String TAG = "PlayerBase"; - private final static boolean DEBUG = false; + private static final String TAG = "PlayerBase"; + private static final boolean DEBUG = false; private static IAudioService sService; //lazy initialization, use getService() /** Debug app ops */ - protected static final boolean DEBUG_APP_OPS = Log.isLoggable(TAG + ".AO", Log.DEBUG); + private static final boolean DEBUG_APP_OPS = false; // parameters of the player that affect AppOps protected AudioAttributes mAttributes; @@ -95,19 +95,9 @@ public abstract class PlayerBase { IBinder b = ServiceManager.getService(Context.APP_OPS_SERVICE); mAppOps = IAppOpsService.Stub.asInterface(b); // initialize mHasAppOpsPlayAudio - synchronized (mLock) { - updateAppOpsPlayAudio_sync(); - } + updateAppOpsPlayAudio(); // register a callback to monitor whether the OP_PLAY_AUDIO is still allowed - mAppOpsCallback = new IAppOpsCallback.Stub() { - public void opChanged(int op, int uid, String packageName) { - synchronized (mLock) { - if (op == AppOpsManager.OP_PLAY_AUDIO) { - updateAppOpsPlayAudio_sync(); - } - } - } - }; + mAppOpsCallback = new IAppOpsCallbackWrapper(this); try { mAppOps.startWatchingMode(AppOpsManager.OP_PLAY_AUDIO, ActivityThread.currentPackageName(), mAppOpsCallback); @@ -258,6 +248,12 @@ public abstract class PlayerBase { } } + private void updateAppOpsPlayAudio() { + synchronized (mLock) { + updateAppOpsPlayAudio_sync(); + } + } + /** * To be called whenever a condition that might affect audibility of this player is updated. * Must be called synchronized on mLock. @@ -404,6 +400,26 @@ public abstract class PlayerBase { abstract void playerPause(); abstract void playerStop(); + //===================================================================== + private static class IAppOpsCallbackWrapper extends IAppOpsCallback.Stub { + private final WeakReference mWeakPB; + + public IAppOpsCallbackWrapper(PlayerBase pb) { + mWeakPB = new WeakReference(pb); + } + + @Override + public void opChanged(int op, int uid, String packageName) { + if (op == AppOpsManager.OP_PLAY_AUDIO) { + if (DEBUG_APP_OPS) { Log.v(TAG, "opChanged: op=PLAY_AUDIO pack=" + packageName); } + final PlayerBase pb = mWeakPB.get(); + if (pb != null) { + pb.updateAppOpsPlayAudio(); + } + } + } + } + //===================================================================== /** * Wrapper around an implementation of IPlayer for all subclasses of PlayerBase @@ -482,8 +498,8 @@ public abstract class PlayerBase { public static class PlayerIdCard implements Parcelable { public final int mPlayerType; - public final static int AUDIO_ATTRIBUTES_NONE = 0; - public final static int AUDIO_ATTRIBUTES_DEFINED = 1; + public static final int AUDIO_ATTRIBUTES_NONE = 0; + public static final int AUDIO_ATTRIBUTES_DEFINED = 1; public final AudioAttributes mAttributes; public final IPlayer mIPlayer;