diff --git a/RELEASENOTES.md b/RELEASENOTES.md index fddcf73ef3..e7f9e53a8a 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -30,6 +30,9 @@ Release notes ([#10684](https://github.com/google/ExoPlayer/issues/10684)). * Add `Player.getVideoSurfaceSize` that returns the size of the surface on which the video is rendered. + * Fix bug where removing listeners during the player release can cause an + `IllegalStateException` + ([#10758](https://github.com/google/ExoPlayer/issues/10758)). * Build: * Avoid publishing block when included in another gradle build. * Downloads: diff --git a/libraries/common/src/main/java/androidx/media3/common/util/ListenerSet.java b/libraries/common/src/main/java/androidx/media3/common/util/ListenerSet.java index 077141b05c..78e529ae3a 100644 --- a/libraries/common/src/main/java/androidx/media3/common/util/ListenerSet.java +++ b/libraries/common/src/main/java/androidx/media3/common/util/ListenerSet.java @@ -270,6 +270,7 @@ public final class ListenerSet { public void release(IterationFinishedEvent event) { released = true; if (needsIterationFinishedEvent) { + needsIterationFinishedEvent = false; event.invoke(listener, flagsBuilder.build()); } } diff --git a/libraries/common/src/test/java/androidx/media3/common/util/ListenerSetTest.java b/libraries/common/src/test/java/androidx/media3/common/util/ListenerSetTest.java index 211e0e4ae4..a67da240ca 100644 --- a/libraries/common/src/test/java/androidx/media3/common/util/ListenerSetTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/util/ListenerSetTest.java @@ -49,7 +49,7 @@ public class ListenerSetTest { listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1); listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); verifyNoMoreInteractions(listener); } @@ -67,6 +67,7 @@ public class ListenerSetTest { listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2); listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1); listenerSet.flushEvents(); + ShadowLooper.idleMainLooper(); InOrder inOrder = Mockito.inOrder(listener1, listener2); inOrder.verify(listener1).callback1(); @@ -75,6 +76,8 @@ public class ListenerSetTest { inOrder.verify(listener2).callback2(); inOrder.verify(listener1).callback1(); inOrder.verify(listener2).callback1(); + inOrder.verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2)); + inOrder.verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2)); inOrder.verifyNoMoreInteractions(); } @@ -99,6 +102,7 @@ public class ListenerSetTest { listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1); listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2); listenerSet.flushEvents(); + ShadowLooper.idleMainLooper(); InOrder inOrder = Mockito.inOrder(listener1, listener2); inOrder.verify(listener1).callback1(); @@ -107,6 +111,8 @@ public class ListenerSetTest { inOrder.verify(listener2).callback2(); inOrder.verify(listener1).callback3(); inOrder.verify(listener2).callback3(); + inOrder.verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2, EVENT_ID_3)); + inOrder.verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2, EVENT_ID_3)); inOrder.verifyNoMoreInteractions(); } @@ -131,7 +137,7 @@ public class ListenerSetTest { // Iteration with single flush. listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2); listenerSet.flushEvents(); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); // Iteration with multiple flushes. listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1); @@ -139,11 +145,11 @@ public class ListenerSetTest { listenerSet.flushEvents(); listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1); listenerSet.flushEvents(); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); // Iteration with recursive call. listenerSet.sendEvent(EVENT_ID_3, TestListener::callback3); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); InOrder inOrder = Mockito.inOrder(listener1, listener2); inOrder.verify(listener1).callback2(); @@ -192,7 +198,7 @@ public class ListenerSetTest { listenerSet.add(listener3); listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); InOrder inOrder = Mockito.inOrder(listener1, listener2, listener3); inOrder.verify(listener1).callback2(); @@ -216,7 +222,7 @@ public class ListenerSetTest { listenerSet.queueEvent(/* eventFlag= */ C.INDEX_UNSET, TestListener::callback1); listenerSet.flushEvents(); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); // Asserts that negative event flag (INDEX_UNSET) can be used without throwing. } @@ -242,7 +248,7 @@ public class ListenerSetTest { // listener2 was added. listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1); listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); InOrder inOrder = Mockito.inOrder(listener1, listener2); inOrder.verify(listener1).callback1(); @@ -267,7 +273,7 @@ public class ListenerSetTest { listenerSet.add(listener2); listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2); listenerSet.flushEvents(); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); InOrder inOrder = Mockito.inOrder(listener1, listener2); inOrder.verify(listener1).callback1(); @@ -299,7 +305,7 @@ public class ListenerSetTest { listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1); listenerSet.remove(listener1); listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); verify(listener1).callback1(); verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1)); @@ -320,7 +326,7 @@ public class ListenerSetTest { listenerSet.remove(listener1); listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1); listenerSet.flushEvents(); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); verify(listener2, times(2)).callback1(); verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1)); @@ -347,13 +353,43 @@ public class ListenerSetTest { // Listener2 shouldn't even get this event as it's released before the event can be invoked. listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1); listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2); - ShadowLooper.runMainLooperToNextTask(); + ShadowLooper.idleMainLooper(); verify(listener1).callback1(); verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1)); verifyNoMoreInteractions(listener1, listener2); } + @Test + public void remove_withRecursionDuringRelease_callsAllPendingEventsAndIterationFinished() { + ListenerSet listenerSet = + new ListenerSet<>(Looper.myLooper(), Clock.DEFAULT, TestListener::iterationFinished); + TestListener listener2 = mock(TestListener.class); + // Listener1 removes Listener2 from within the callback triggered by release(). + TestListener listener1 = + spy( + new TestListener() { + @Override + public void iterationFinished(FlagSet flags) { + listenerSet.remove(listener2); + } + }); + listenerSet.add(listener1); + listenerSet.add(listener2); + + // Listener2 should still get the event and iterationFinished callback because it was triggered + // before the release and the listener removal. + listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1); + listenerSet.release(); + ShadowLooper.idleMainLooper(); + + verify(listener1).callback1(); + verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1)); + verify(listener2).callback1(); + verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1)); + verifyNoMoreInteractions(listener1, listener2); + } + @Test public void release_preventsRegisteringNewListeners() { ListenerSet listenerSet =