From 54506b506bb6506c6389dd6700cd179e42fff576 Mon Sep 17 00:00:00 2001
From: tonihei <tonihei@google.com>
Date: Wed, 21 Oct 2020 09:02:47 +0100
Subject: [PATCH] Explicitly prevent callbacks after player is released

We currently implicitly rely on the internal playback thread to not send
new updates after the player got released. This may not always be ensured
since we let the release call timeout. For the timeout case, there may
still be a pending operation returning much later when it unstuck itself.

Fix this and potential other edge cases by explicitly removing all listeners
and preventing new listeners from being added after the release.

PiperOrigin-RevId: 338217220
---
 .../android/exoplayer2/ExoPlayerImpl.java     |  1 +
 .../android/exoplayer2/util/ListenerSet.java  | 18 +++++++++
 .../exoplayer2/util/ListenerSetTest.java      | 38 +++++++++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
index dcc28f974e..1636275109 100644
--- a/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
+++ b/library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java
@@ -721,6 +721,7 @@ import java.util.concurrent.TimeoutException;
                       new TimeoutException("Player release timed out."),
                       ExoPlaybackException.TIMEOUT_OPERATION_RELEASE)));
     }
+    listeners.release();
     playbackInfoUpdateHandler.removeCallbacksAndMessages(null);
     if (analyticsCollector != null) {
       bandwidthMeter.removeEventListener(analyticsCollector);
diff --git a/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java b/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java
index 12b73ec94f..499e05acf0 100644
--- a/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java
+++ b/library/core/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java
@@ -48,6 +48,8 @@ public final class ListenerSet<T> {
   private final ArrayDeque<Runnable> flushingEvents;
   private final ArrayDeque<Runnable> queuedEvents;
 
+  private boolean released;
+
   /** Creates the listener set. */
   public ListenerSet() {
     listeners = new CopyOnWriteArraySet<>();
@@ -63,6 +65,9 @@ public final class ListenerSet<T> {
    * @param listener The listener to be added.
    */
   public void add(T listener) {
+    if (released) {
+      return;
+    }
     Assertions.checkNotNull(listener);
     listeners.add(new ListenerHolder<T>(listener));
   }
@@ -124,6 +129,19 @@ public final class ListenerSet<T> {
     flushEvents();
   }
 
+  /**
+   * Releases the set of listeners.
+   *
+   * <p>This will ensure no events are sent to any listener after this method has been called.
+   */
+  public void release() {
+    for (ListenerHolder<T> listenerHolder : listeners) {
+      listenerHolder.release();
+    }
+    listeners.clear();
+    released = true;
+  }
+
   private static final class ListenerHolder<T> {
 
     @Nonnull public final T listener;
diff --git a/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java b/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java
index 36f20c065e..aaa299ecd7 100644
--- a/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java
+++ b/library/core/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java
@@ -184,6 +184,44 @@ public class ListenerSetTest {
     verify(listener2, times(2)).callback1();
   }
 
+  @Test
+  public void release_stopsForwardingEventsImmediately() {
+    ListenerSet<TestListener> listenerSet = new ListenerSet<>();
+    TestListener listener2 = mock(TestListener.class);
+    // Listener1 releases the set from within the callback.
+    TestListener listener1 =
+        spy(
+            new TestListener() {
+              @Override
+              public void callback1() {
+                listenerSet.release();
+              }
+            });
+    listenerSet.add(listener1);
+    listenerSet.add(listener2);
+
+    // Listener2 shouldn't even get this event as it's released before the event can be invoked.
+    listenerSet.sendEvent(TestListener::callback1);
+    listenerSet.sendEvent(TestListener::callback2);
+
+    verify(listener1).callback1();
+    verify(listener2, never()).callback1();
+    verify(listener1, never()).callback2();
+    verify(listener2, never()).callback2();
+  }
+
+  @Test
+  public void release_preventsRegisteringNewListeners() {
+    ListenerSet<TestListener> listenerSet = new ListenerSet<>();
+    TestListener listener = mock(TestListener.class);
+
+    listenerSet.release();
+    listenerSet.add(listener);
+    listenerSet.sendEvent(TestListener::callback1);
+
+    verify(listener, never()).callback1();
+  }
+
   private interface TestListener {
     default void callback1() {}