Merge "Simplify MediaSessionRecord callback invocations" into main am: b92cea9bda am: f848944b1c

Original change: https://android-review.googlesource.com/c/platform/frameworks/base/+/3043212

Change-Id: I3f204d23f13a7fcb4c9d3d5077d22bad0abcf4ae
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

1 files changed, 54 insertions, 151 deletions

diff --git a/services/core/java/com/android/server/media/MediaSessionRecord.java b/services/core/java/com/android/server/media/MediaSessionRecord.java
index 3d6855547bcd..a3c5d2d336f2 100644
--- a/services/core/java/com/android/server/media/MediaSessionRecord.java
+++ b/services/core/java/com/android/server/media/MediaSessionRecord.java
@@ -66,7 +66,6 @@ import android.net.Uri;
 import android.os.Binder;
 import android.os.Build;
 import android.os.Bundle;
-import android.os.DeadObjectException;
 import android.os.Handler;
 import android.os.IBinder;
 import android.os.Looper;
@@ -90,7 +89,6 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collection;
 import java.util.List;
 import java.util.NoSuchElementException;
 import java.util.Objects;
@@ -191,6 +189,9 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
     private final ForegroundServiceDelegationOptions mForegroundServiceDelegationOptions;
 
     private final Object mLock = new Object();
+    // This field is partially guarded by mLock. Writes and non-atomic iterations (for example:
+    // index-based-iterations) must be guarded by mLock. But it is safe to acquire an iterator
+    // without acquiring mLock.
     private final CopyOnWriteArrayList<ISessionControllerCallbackHolder>
             mControllerCallbackHolders = new CopyOnWriteArrayList<>();
 
@@ -886,24 +887,9 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
             }
             playbackState = mPlaybackState;
         }
-        Collection<ISessionControllerCallbackHolder> deadCallbackHolders = null;
-        for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
-            try {
-                holder.mCallback.onPlaybackStateChanged(playbackState);
-            } catch (DeadObjectException e) {
-                if (deadCallbackHolders == null) {
-                    deadCallbackHolders = new ArrayList<>();
-                }
-                deadCallbackHolders.add(holder);
-                logCallbackException("Removing dead callback in pushPlaybackStateUpdate", holder,
-                        e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushPlaybackStateUpdate", holder, e);
-            }
-        }
-        if (deadCallbackHolders != null) {
-            removeControllerHoldersSafely(deadCallbackHolders);
-        }
+        performOnCallbackHolders(
+                "pushPlaybackStateUpdate",
+                holder -> holder.mCallback.onPlaybackStateChanged(playbackState));
     }
 
     private void pushMetadataUpdate() {
@@ -914,23 +900,8 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
             }
             metadata = mMetadata;
         }
-        Collection<ISessionControllerCallbackHolder> deadCallbackHolders = null;
-        for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
-            try {
-                holder.mCallback.onMetadataChanged(metadata);
-            } catch (DeadObjectException e) {
-                if (deadCallbackHolders == null) {
-                    deadCallbackHolders = new ArrayList<>();
-                }
-                deadCallbackHolders.add(holder);
-                logCallbackException("Removing dead callback in pushMetadataUpdate", holder, e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushMetadataUpdate", holder, e);
-            }
-        }
-        if (deadCallbackHolders != null) {
-            removeControllerHoldersSafely(deadCallbackHolders);
-        }
+        performOnCallbackHolders(
+                "pushMetadataUpdate", holder -> holder.mCallback.onMetadataChanged(metadata));
     }
 
     private void pushQueueUpdate() {
@@ -941,31 +912,18 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
             }
             toSend = mQueue == null ? null : new ArrayList<>(mQueue);
         }
-        Collection<ISessionControllerCallbackHolder> deadCallbackHolders = null;
-        for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
-            ParceledListSlice<QueueItem> parcelableQueue = null;
-            if (toSend != null) {
-                parcelableQueue = new ParceledListSlice<>(toSend);
-                // Limit the size of initial Parcel to prevent binder buffer overflow
-                // as onQueueChanged is an async binder call.
-                parcelableQueue.setInlineCountLimit(1);
-            }
-
-            try {
-                holder.mCallback.onQueueChanged(parcelableQueue);
-            } catch (DeadObjectException e) {
-                if (deadCallbackHolders == null) {
-                    deadCallbackHolders = new ArrayList<>();
-                }
-                deadCallbackHolders.add(holder);
-                logCallbackException("Removing dead callback in pushQueueUpdate", holder, e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushQueueUpdate", holder, e);
-            }
-        }
-        if (deadCallbackHolders != null) {
-            removeControllerHoldersSafely(deadCallbackHolders);
-        }
+        performOnCallbackHolders(
+                "pushQueueUpdate",
+                holder -> {
+                    ParceledListSlice<QueueItem> parcelableQueue = null;
+                    if (toSend != null) {
+                        parcelableQueue = new ParceledListSlice<>(toSend);
+                        // Limit the size of initial Parcel to prevent binder buffer overflow
+                        // as onQueueChanged is an async binder call.
+                        parcelableQueue.setInlineCountLimit(1);
+                    }
+                    holder.mCallback.onQueueChanged(parcelableQueue);
+                });
     }
 
     private void pushQueueTitleUpdate() {
@@ -976,23 +934,8 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
             }
             queueTitle = mQueueTitle;
         }
-        Collection<ISessionControllerCallbackHolder> deadCallbackHolders = null;
-        for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
-            try {
-                holder.mCallback.onQueueTitleChanged(queueTitle);
-            } catch (DeadObjectException e) {
-                if (deadCallbackHolders == null) {
-                    deadCallbackHolders = new ArrayList<>();
-                }
-                deadCallbackHolders.add(holder);
-                logCallbackException("Removing dead callback in pushQueueTitleUpdate", holder, e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushQueueTitleUpdate", holder, e);
-            }
-        }
-        if (deadCallbackHolders != null) {
-            removeControllerHoldersSafely(deadCallbackHolders);
-        }
+        performOnCallbackHolders(
+                "pushQueueTitleUpdate", holder -> holder.mCallback.onQueueTitleChanged(queueTitle));
     }
 
     private void pushExtrasUpdate() {
@@ -1003,23 +946,8 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
             }
             extras = mExtras;
         }
-        Collection<ISessionControllerCallbackHolder> deadCallbackHolders = null;
-        for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
-            try {
-                holder.mCallback.onExtrasChanged(extras);
-            } catch (DeadObjectException e) {
-                if (deadCallbackHolders == null) {
-                    deadCallbackHolders = new ArrayList<>();
-                }
-                deadCallbackHolders.add(holder);
-                logCallbackException("Removing dead callback in pushExtrasUpdate", holder, e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushExtrasUpdate", holder, e);
-            }
-        }
-        if (deadCallbackHolders != null) {
-            removeControllerHoldersSafely(deadCallbackHolders);
-        }
+        performOnCallbackHolders(
+                "pushExtrasUpdate", holder -> holder.mCallback.onExtrasChanged(extras));
     }
 
     private void pushVolumeUpdate() {
@@ -1030,23 +958,8 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
             }
             info = getVolumeAttributes();
         }
-        Collection<ISessionControllerCallbackHolder> deadCallbackHolders = null;
-        for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
-            try {
-                holder.mCallback.onVolumeInfoChanged(info);
-            } catch (DeadObjectException e) {
-                if (deadCallbackHolders == null) {
-                    deadCallbackHolders = new ArrayList<>();
-                }
-                deadCallbackHolders.add(holder);
-                logCallbackException("Removing dead callback in pushVolumeUpdate", holder, e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushVolumeUpdate", holder, e);
-            }
-        }
-        if (deadCallbackHolders != null) {
-            removeControllerHoldersSafely(deadCallbackHolders);
-        }
+        performOnCallbackHolders(
+                "pushVolumeUpdate", holder -> holder.mCallback.onVolumeInfoChanged(info));
     }
 
     private void pushEvent(String event, Bundle data) {
@@ -1055,23 +968,7 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
                 return;
             }
         }
-        Collection<ISessionControllerCallbackHolder> deadCallbackHolders = null;
-        for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
-            try {
-                holder.mCallback.onEvent(event, data);
-            } catch (DeadObjectException e) {
-                if (deadCallbackHolders == null) {
-                    deadCallbackHolders = new ArrayList<>();
-                }
-                deadCallbackHolders.add(holder);
-                logCallbackException("Removing dead callback in pushEvent", holder, e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushEvent", holder, e);
-            }
-        }
-        if (deadCallbackHolders != null) {
-            removeControllerHoldersSafely(deadCallbackHolders);
-        }
+        performOnCallbackHolders("pushEvent", holder -> holder.mCallback.onEvent(event, data));
     }
 
     private void pushSessionDestroyed() {
@@ -1082,20 +979,37 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
                 return;
             }
         }
+        performOnCallbackHolders(
+                "pushSessionDestroyed",
+                holder -> {
+                    holder.mCallback.asBinder().unlinkToDeath(holder.mDeathMonitor, 0);
+                    holder.mCallback.onSessionDestroyed();
+                });
+        // After notifying clear all listeners
+        synchronized (mLock) {
+            mControllerCallbackHolders.clear();
+        }
+    }
+
+    private interface ControllerCallbackCall {
+
+        void performOn(ISessionControllerCallbackHolder holder) throws RemoteException;
+    }
+
+    private void performOnCallbackHolders(String operationName, ControllerCallbackCall call) {
+        ArrayList<ISessionControllerCallbackHolder> deadCallbackHolders = new ArrayList<>();
         for (ISessionControllerCallbackHolder holder : mControllerCallbackHolders) {
             try {
-                holder.mCallback.asBinder().unlinkToDeath(holder.mDeathMonitor, 0);
-                holder.mCallback.onSessionDestroyed();
-            } catch (NoSuchElementException e) {
-                logCallbackException("error unlinking to binder death", holder, e);
-            } catch (DeadObjectException e) {
-                logCallbackException("Removing dead callback in pushSessionDestroyed", holder, e);
-            } catch (RemoteException e) {
-                logCallbackException("unexpected exception in pushSessionDestroyed", holder, e);
+                call.performOn(holder);
+            } catch (RemoteException | NoSuchElementException exception) {
+                deadCallbackHolders.add(holder);
+                logCallbackException(
+                        "Exception while executing: " + operationName, holder, exception);
             }
         }
-        // After notifying clear all listeners
-        removeControllerHoldersSafely(null);
+        synchronized (mLock) {
+            mControllerCallbackHolders.removeAll(deadCallbackHolders);
+        }
     }
 
     private PlaybackState getStateWithUpdatedPosition() {
@@ -1143,17 +1057,6 @@ public class MediaSessionRecord extends MediaSessionRecordImpl implements IBinde
         return -1;
     }
 
-    private void removeControllerHoldersSafely(
-            Collection<ISessionControllerCallbackHolder> holders) {
-        synchronized (mLock) {
-            if (holders == null) {
-                mControllerCallbackHolders.clear();
-            } else {
-                mControllerCallbackHolders.removeAll(holders);
-            }
-        }
-    }
-
     private PlaybackInfo getVolumeAttributes() {
         int volumeType;
         AudioAttributes attributes;