Clarify and correct allowed multi-threading for some Player methods

Some Player methods like getting the Looper and adding listeners
were always allowed to be called from any thread, but this is
undocumented. This change makes the threading rules of these
methods more explicit.

Removing listeners was never meant to be called from another thread
and we also don't support it safely because final callbacks may
be triggered from the wrong thread. To find potential issues, we
can assert the correct thread when releasing listeners.

Finally, there is a potential race condition when calling addListener
from a different thread at the same time as release, which may lead to
a registered listener that could receive callbacks after the player is
released.

PiperOrigin-RevId: 493843981

library/common/src/main/java/com/google/android/exoplayer2/Player.java

@@ -53,6 +53,10 @@ import java.util.List;
  * A media player interface defining traditional high-level functionality, such as the ability to
  * play, pause, seek and query properties of the currently playing media.
  *
+ * <p>All methods must be called from a single {@linkplain #getApplicationLooper() application
+ * thread} unless indicated otherwise. Callbacks in registered listeners are called on the same
+ * thread.
+ *
  * <p>This interface includes some convenience methods that can be implemented by calling other
  * methods in the interface. {@link BasePlayer} implements these convenience methods so inheriting
  * {@link BasePlayer} is recommended when implementing the interface so that only the minimal set of
@@ -1546,6 +1550,8 @@ public interface Player {
   /**
    * Returns the {@link Looper} associated with the application thread that's used to access the
    * player and on which player events are received.
+   *
+   * <p>This method can be called from any thread.
    */
   Looper getApplicationLooper();
 
@@ -1555,6 +1561,8 @@ public interface Player {
    * <p>The listener's methods will be called on the thread associated with {@link
    * #getApplicationLooper()}.
    *
+   * <p>This method can be called from any thread.
+   *
    * @param listener The listener to register.
    */
   void addListener(Listener listener);

library/common/src/main/java/com/google/android/exoplayer2/SimpleBasePlayer.java

@@ -1981,8 +1981,7 @@ public abstract class SimpleBasePlayer extends BasePlayer {
 
   @Override
   public final void removeListener(Listener listener) {
-    // Don't verify application thread. We allow calls to this method from any thread.
+    verifyApplicationThreadAndInitState();
-    checkNotNull(listener);
     listeners.remove(listener);
   }
 

library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java

@@ -15,9 +15,12 @@
  */
 package com.google.android.exoplayer2.util;
 
+import static com.google.android.exoplayer2.util.Assertions.checkState;
+
 import android.os.Looper;
 import android.os.Message;
 import androidx.annotation.CheckResult;
+import androidx.annotation.GuardedBy;
 import androidx.annotation.Nullable;
 import com.google.android.exoplayer2.C;
 import java.util.ArrayDeque;
@@ -33,6 +36,9 @@ import org.checkerframework.checker.nullness.qual.NonNull;
  * <p>Events are also guaranteed to be only sent to the listeners registered at the time the event
  * was enqueued and haven't been removed since.
  *
+ * <p>All methods must be called on the {@link Looper} passed to the constructor unless indicated
+ * otherwise.
+ *
  * @param <T> The listener type.
  */
 public final class ListenerSet<T extends @NonNull Object> {
@@ -74,14 +80,18 @@ public final class ListenerSet<T extends @NonNull Object> {
   private final CopyOnWriteArraySet<ListenerHolder<T>> listeners;
   private final ArrayDeque<Runnable> flushingEvents;
   private final ArrayDeque<Runnable> queuedEvents;
+  private final Object releasedLock;
 
+  @GuardedBy("releasedLock")
   private boolean released;
 
+  private boolean throwsWhenUsingWrongThread;
+
   /**
    * Creates a new listener set.
    *
    * @param looper A {@link Looper} used to call listeners on. The same {@link Looper} must be used
-   *     to call all other methods of this class.
+   *     to call all other methods of this class unless indicated otherwise.
    * @param clock A {@link Clock}.
    * @param iterationFinishedEvent An {@link IterationFinishedEvent} sent when all other events sent
    *     during one {@link Looper} message queue iteration were handled by the listeners.
@@ -98,17 +108,21 @@ public final class ListenerSet<T extends @NonNull Object> {
     this.clock = clock;
     this.listeners = listeners;
     this.iterationFinishedEvent = iterationFinishedEvent;
+    releasedLock = new Object();
     flushingEvents = new ArrayDeque<>();
     queuedEvents = new ArrayDeque<>();
     // It's safe to use "this" because we don't send a message before exiting the constructor.
     @SuppressWarnings("nullness:methodref.receiver.bound")
     HandlerWrapper handler = clock.createHandler(looper, this::handleMessage);
     this.handler = handler;
+    throwsWhenUsingWrongThread = true;
   }
 
   /**
    * Copies the listener set.
    *
+   * <p>This method can be called from any thread.
+   *
    * @param looper The new {@link Looper} for the copied listener set.
    * @param iterationFinishedEvent The new {@link IterationFinishedEvent} sent when all other events
    *     sent during one {@link Looper} message queue iteration were handled by the listeners.
@@ -122,6 +136,8 @@ public final class ListenerSet<T extends @NonNull Object> {
   /**
    * Copies the listener set.
    *
+   * <p>This method can be called from any thread.
+   *
    * @param looper The new {@link Looper} for the copied listener set.
    * @param clock The new {@link Clock} for the copied listener set.
    * @param iterationFinishedEvent The new {@link IterationFinishedEvent} sent when all other events
@@ -139,14 +155,18 @@ public final class ListenerSet<T extends @NonNull Object> {
    *
    * <p>If a listener is already present, it will not be added again.
    *
+   * <p>This method can be called from any thread.
+   *
    * @param listener The listener to be added.
    */
   public void add(T listener) {
-    if (released) {
-      return;
-    }
     Assertions.checkNotNull(listener);
-    listeners.add(new ListenerHolder<>(listener));
+    synchronized (releasedLock) {
+      if (released) {
+        return;
+      }
+      listeners.add(new ListenerHolder<>(listener));
+    }
   }
 
   /**
@@ -157,6 +177,7 @@ public final class ListenerSet<T extends @NonNull Object> {
    * @param listener The listener to be removed.
    */
   public void remove(T listener) {
+    verifyCurrentThread();
     for (ListenerHolder<T> listenerHolder : listeners) {
       if (listenerHolder.listener.equals(listener)) {
         listenerHolder.release(iterationFinishedEvent);
@@ -167,11 +188,13 @@ public final class ListenerSet<T extends @NonNull Object> {
 
   /** Removes all listeners from the set. */
   public void clear() {
+    verifyCurrentThread();
     listeners.clear();
   }
 
   /** Returns the number of added listeners. */
   public int size() {
+    verifyCurrentThread();
     return listeners.size();
   }
 
@@ -183,6 +206,7 @@ public final class ListenerSet<T extends @NonNull Object> {
    * @param event The event.
    */
   public void queueEvent(int eventFlag, Event<T> event) {
+    verifyCurrentThread();
     CopyOnWriteArraySet<ListenerHolder<T>> listenerSnapshot = new CopyOnWriteArraySet<>(listeners);
     queuedEvents.add(
         () -> {
@@ -194,6 +218,7 @@ public final class ListenerSet<T extends @NonNull Object> {
 
   /** Notifies listeners of events previously enqueued with {@link #queueEvent(int, Event)}. */
   public void flushEvents() {
+    verifyCurrentThread();
     if (queuedEvents.isEmpty()) {
       return;
     }
@@ -232,11 +257,27 @@ public final class ListenerSet<T extends @NonNull Object> {
    * <p>This will ensure no events are sent to any listener after this method has been called.
    */
   public void release() {
+    verifyCurrentThread();
+    synchronized (releasedLock) {
+      released = true;
+    }
     for (ListenerHolder<T> listenerHolder : listeners) {
       listenerHolder.release(iterationFinishedEvent);
     }
     listeners.clear();
-    released = true;
+  }
+
+  /**
+   * Sets whether methods throw when using the wrong thread.
+   *
+   * <p>Do not use this method unless to support legacy use cases.
+   *
+   * @param throwsWhenUsingWrongThread Whether to throw when using the wrong thread.
+   * @deprecated Do not use this method and ensure all calls are made from the correct thread.
+   */
+  @Deprecated
+  public void setThrowsWhenUsingWrongThread(boolean throwsWhenUsingWrongThread) {
+    this.throwsWhenUsingWrongThread = throwsWhenUsingWrongThread;
   }
 
   private boolean handleMessage(Message message) {
@@ -252,6 +293,13 @@ public final class ListenerSet<T extends @NonNull Object> {
     return true;
   }
 
+  private void verifyCurrentThread() {
+    if (!throwsWhenUsingWrongThread) {
+      return;
+    }
+    checkState(Thread.currentThread() == handler.getLooper().getThread());
+  }
+
   private static final class ListenerHolder<T extends @NonNull Object> {
 
     public final T listener;

library/core/src/main/java/com/google/android/exoplayer2/ExoPlayer.java

@@ -125,15 +125,15 @@ import java.util.List;
  * threading model">
  *
  * <ul>
- *   <li>ExoPlayer instances must be accessed from a single application thread. For the vast
+ *   <li>ExoPlayer instances must be accessed from a single application thread unless indicated
- *       majority of cases this should be the application's main thread. Using the application's
+ *       otherwise. For the vast majority of cases this should be the application's main thread.
- *       main thread is also a requirement when using ExoPlayer's UI components or the IMA
+ *       Using the application's main thread is also a requirement when using ExoPlayer's UI
- *       extension. The thread on which an ExoPlayer instance must be accessed can be explicitly
+ *       components or the IMA extension. The thread on which an ExoPlayer instance must be accessed
- *       specified by passing a `Looper` when creating the player. If no `Looper` is specified, then
+ *       can be explicitly specified by passing a `Looper` when creating the player. If no `Looper`
- *       the `Looper` of the thread that the player is created on is used, or if that thread does
+ *       is specified, then the `Looper` of the thread that the player is created on is used, or if
- *       not have a `Looper`, the `Looper` of the application's main thread is used. In all cases
+ *       that thread does not have a `Looper`, the `Looper` of the application's main thread is
- *       the `Looper` of the thread from which the player must be accessed can be queried using
+ *       used. In all cases the `Looper` of the thread from which the player must be accessed can be
- *       {@link #getApplicationLooper()}.
+ *       queried using {@link #getApplicationLooper()}.
  *   <li>Registered listeners are called on the thread associated with {@link
  *       #getApplicationLooper()}. Note that this means registered listeners are called on the same
  *       thread which must be used to access the player.
@@ -1187,6 +1187,8 @@ public interface ExoPlayer extends Player {
   /**
    * Adds a listener to receive audio offload events.
    *
+   * <p>This method can be called from any thread.
+   *
    * @param listener The listener to register.
    */
   void addAudioOffloadListener(AudioOffloadListener listener);
@@ -1204,6 +1206,8 @@ public interface ExoPlayer extends Player {
   /**
    * Adds an {@link AnalyticsListener} to receive analytics events.
    *
+   * <p>This method can be called from any thread.
+   *
    * @param listener The listener to be added.
    */
   void addAnalyticsListener(AnalyticsListener listener);
@@ -1263,10 +1267,18 @@ public interface ExoPlayer extends Player {
   @Deprecated
   TrackSelectionArray getCurrentTrackSelections();
 
-  /** Returns the {@link Looper} associated with the playback thread. */
+  /**
+   * Returns the {@link Looper} associated with the playback thread.
+   *
+   * <p>This method may be called from any thread.
+   */
   Looper getPlaybackLooper();
 
-  /** Returns the {@link Clock} used for playback. */
+  /**
+   * Returns the {@link Clock} used for playback.
+   *
+   * <p>This method can be called from any thread.
+   */
   Clock getClock();
 
   /**

library/core/src/main/java/com/google/android/exoplayer2/ExoPlayerImpl.java

@@ -59,6 +59,7 @@ import com.google.android.exoplayer2.PlayerMessage.Target;
 import com.google.android.exoplayer2.Renderer.MessageType;
 import com.google.android.exoplayer2.analytics.AnalyticsCollector;
 import com.google.android.exoplayer2.analytics.AnalyticsListener;
+import com.google.android.exoplayer2.analytics.DefaultAnalyticsCollector;
 import com.google.android.exoplayer2.analytics.MediaMetricsListener;
 import com.google.android.exoplayer2.analytics.PlayerId;
 import com.google.android.exoplayer2.audio.AudioAttributes;
@@ -469,7 +470,7 @@ import java.util.concurrent.TimeoutException;
 
   @Override
   public void removeAudioOffloadListener(AudioOffloadListener listener) {
-    // Don't verify application thread. We allow calls to this method from any thread.
+    verifyApplicationThread();
     audioOffloadListeners.remove(listener);
   }
 
@@ -1477,7 +1478,7 @@ import java.util.concurrent.TimeoutException;
 
   @Override
   public void removeAnalyticsListener(AnalyticsListener listener) {
-    // Don't verify application thread. We allow calls to this method from any thread.
+    verifyApplicationThread();
     analyticsCollector.removeListener(checkNotNull(listener));
   }
 
@@ -1594,9 +1595,8 @@ import java.util.concurrent.TimeoutException;
 
   @Override
   public void removeListener(Listener listener) {
-    // Don't verify application thread. We allow calls to this method from any thread.
+    verifyApplicationThread();
-    checkNotNull(listener);
+    listeners.remove(checkNotNull(listener));
-    listeners.remove(listener);
   }
 
   @Override
@@ -1679,8 +1679,14 @@ import java.util.concurrent.TimeoutException;
     return false;
   }
 
+  @SuppressWarnings("deprecation") // Calling deprecated methods.
   /* package */ void setThrowsWhenUsingWrongThread(boolean throwsWhenUsingWrongThread) {
     this.throwsWhenUsingWrongThread = throwsWhenUsingWrongThread;
+    listeners.setThrowsWhenUsingWrongThread(throwsWhenUsingWrongThread);
+    if (analyticsCollector instanceof DefaultAnalyticsCollector) {
+      ((DefaultAnalyticsCollector) analyticsCollector)
+          .setThrowsWhenUsingWrongThread(throwsWhenUsingWrongThread);
+    }
   }
 
   /**

library/core/src/main/java/com/google/android/exoplayer2/analytics/DefaultAnalyticsCollector.java

@@ -94,6 +94,20 @@ public class DefaultAnalyticsCollector implements AnalyticsCollector {
     eventTimes = new SparseArray<>();
   }
 
+  /**
+   * Sets whether methods throw when using the wrong thread.
+   *
+   * <p>Do not use this method unless to support legacy use cases.
+   *
+   * @param throwsWhenUsingWrongThread Whether to throw when using the wrong thread.
+   * @deprecated Do not use this method and ensure all calls are made from the correct thread.
+   */
+  @SuppressWarnings("deprecation") // Calling deprecated method.
+  @Deprecated
+  public void setThrowsWhenUsingWrongThread(boolean throwsWhenUsingWrongThread) {
+    listeners.setThrowsWhenUsingWrongThread(throwsWhenUsingWrongThread);
+  }
+
   @Override
   @CallSuper
   public void addListener(AnalyticsListener listener) {

library/core/src/test/java/com/google/android/exoplayer2/ExoPlayerTest.java

@@ -11996,10 +11996,20 @@ public final class ExoPlayerTest {
 
   @Test
   @Config(sdk = Config.ALL_SDKS)
-  public void builder_inBackgroundThread_doesNotThrow() throws Exception {
+  public void builder_inBackgroundThreadWithAllowedAnyThreadMethods_doesNotThrow()
+      throws Exception {
     Thread builderThread =
         new Thread(
-            () -> new ExoPlayer.Builder(ApplicationProvider.getApplicationContext()).build());
+            () -> {
+              ExoPlayer player =
+                  new ExoPlayer.Builder(ApplicationProvider.getApplicationContext()).build();
+              player.addListener(new Listener() {});
+              player.addAnalyticsListener(new AnalyticsListener() {});
+              player.addAudioOffloadListener(new ExoPlayer.AudioOffloadListener() {});
+              player.getClock();
+              player.getApplicationLooper();
+              player.getPlaybackLooper();
+            });
     AtomicReference<Throwable> builderThrow = new AtomicReference<>();
     builderThread.setUncaughtExceptionHandler((thread, throwable) -> builderThrow.set(throwable));