From dab5b3c152dfedea2e47b8d2556706b29eb28337 Mon Sep 17 00:00:00 2001 From: tonihei Date: Tue, 15 Nov 2022 14:48:02 +0000 Subject: [PATCH] Mark iterationFinished when triggering release event. When we currently trigger the iteration finished event during the release, we don't mark the event as triggered. This means that someone can trigger another release from within the callback, which then tries to resend the event. Issue: google/ExoPlayer#10758 PiperOrigin-RevId: 488645089 (cherry picked from commit 1def68bf3c00035aad0821d3997187317ce099dc) --- RELEASENOTES.md | 3 + .../media3/common/util/ListenerSet.java | 1 + .../media3/common/util/ListenerSetTest.java | 58 +++++++++++++++---- 3 files changed, 51 insertions(+), 11 deletions(-) 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 ListenerHolder(T listener) { 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 void queueEvent_withoutFlush_sendsNoEvents() { 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 void flushEvents_sendsPreviouslyQueuedEventsToAllListeners() { 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 void flushEvents_sendsPreviouslyQueuedEventsToAllListeners() { 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 void callback1() { 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 void callback1() { 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 void callback3() { // 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 void callback3() { 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 void iterationFinished(FlagSet flags) { 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 void flushEvents_withUnsetEventFlag_doesNotThrow() { 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 void callback1() { // 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 void add_withQueueing_onlyReceivesUpdatesForFutureEvents() { 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 void callback1() { 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 void remove_withQueueing_stopsReceivingEventsImmediately() { 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,10 +353,40 @@ public void callback1() { // 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); }