From db1a1dc40cea4d3fc6a3bc239488e9e00bd9adff Mon Sep 17 00:00:00 2001 From: Haruki Hasegawa Date: Sun, 27 Mar 2022 00:10:16 +0900 Subject: [PATCH 1/2] Add more test cases for checking the MediaController#release() behavior Current implementation of the release() method have a bug; it does not clear pending messages/callbacks queued to the applicationHandler. --- .../media3/session/MediaControllerTest.java | 38 +++++++++++++- .../session/MediaControllerTestRule.java | 51 +++++++++++++++++-- ...aControllerWithMediaSessionCompatTest.java | 17 +++++++ 3 files changed, 100 insertions(+), 6 deletions(-) diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java index 4fc4b33a6d..1088be232f 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java @@ -192,7 +192,7 @@ public class MediaControllerTest { } @Test - public void isConnected_afterDisconnection_returnsFalse() throws Exception { + public void isConnected_afterDisconnectionBySessionRelease_returnsFalse() throws Exception { CountDownLatch disconnectedLatch = new CountDownLatch(1); MediaController controller = controllerTestRule.createController( @@ -210,6 +210,42 @@ public class MediaControllerTest { assertThat(controller.isConnected()).isFalse(); } + @Test + public void isConnected_afterDisconnectionByControllerRelease_returnsFalse() throws Exception { + CountDownLatch latch = new CountDownLatch(1); + MediaController controller = + controllerTestRule.createController( + remoteSession.getToken(), + null, + new MediaController.Listener() { + @Override + public void onDisconnected(MediaController controller) { + latch.countDown(); + } + }); + threadTestRule.getHandler().postAndSync(controller::release); + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + assertThat(controller.isConnected()).isFalse(); + } + + @Test + public void isConnected_afterDisconnectionByControllerReleaseRightAfterCreated_returnsFalse() throws Exception { + CountDownLatch latch = new CountDownLatch(1); + MediaController controller = + controllerTestRule.createController( + remoteSession.getToken(), + null, + new MediaController.Listener() { + @Override + public void onDisconnected(MediaController controller) { + latch.countDown(); + } + }, + MediaController::release); + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + assertThat(controller.isConnected()).isFalse(); + } + @Test public void close_twice() throws Exception { MediaController controller = controllerTestRule.createController(remoteSession.getToken()); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTestRule.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTestRule.java index d629083b15..f1d0b7a758 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTestRule.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTestRule.java @@ -22,13 +22,17 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS; import android.content.Context; import android.os.Bundle; import android.support.v4.media.session.MediaSessionCompat; +import androidx.annotation.MainThread; import androidx.annotation.Nullable; import androidx.collection.ArrayMap; import androidx.media3.common.util.Log; import androidx.media3.test.session.common.HandlerThreadTestRule; import androidx.test.core.app.ApplicationProvider; import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.MoreExecutors; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import org.junit.rules.ExternalResource; /** @@ -44,6 +48,15 @@ public final class MediaControllerTestRule extends ExternalResource { private volatile Class controllerType; private volatile long timeoutMs; + public interface MediaControllerCreateListener { + /** + * This callback is invoked immediately after the {@link MediaController} instance is created. + * @param controller {@link MediaController} instance + */ + @MainThread + void onCreated(MediaController controller); + } + public MediaControllerTestRule(HandlerThreadTestRule handlerThreadTestRule) { this.handlerThreadTestRule = handlerThreadTestRule; controllers = new ArrayMap<>(); @@ -96,17 +109,24 @@ public final class MediaControllerTestRule extends ExternalResource { public MediaController createController( MediaSessionCompat.Token token, @Nullable MediaController.Listener listener) throws Exception { + return createController(token, listener, null); + } + + /** Creates {@link MediaController} from {@link MediaSessionCompat.Token}. */ + public MediaController createController( + MediaSessionCompat.Token token, @Nullable MediaController.Listener listener, @Nullable MediaControllerCreateListener controllerCreateListener) + throws Exception { TestMediaBrowserListener testListener = new TestMediaBrowserListener(listener); - MediaController controller = createControllerOnHandler(token, testListener); + MediaController controller = createControllerOnHandler(token, testListener, controllerCreateListener); controllers.put(controller, testListener); return controller; } private MediaController createControllerOnHandler( - MediaSessionCompat.Token token, TestMediaBrowserListener listener) throws Exception { + MediaSessionCompat.Token token, TestMediaBrowserListener listener, @Nullable MediaControllerCreateListener controllerCreateListener) throws Exception { SessionToken sessionToken = SessionToken.createSessionToken(context, token).get(TIMEOUT_MS, MILLISECONDS); - return createControllerOnHandler(sessionToken, /* connectionHints= */ null, listener); + return createControllerOnHandler(sessionToken, /* connectionHints= */ null, listener, controllerCreateListener); } /** Creates {@link MediaController} from {@link SessionToken} with default options. */ @@ -120,14 +140,24 @@ public final class MediaControllerTestRule extends ExternalResource { @Nullable Bundle connectionHints, @Nullable MediaController.Listener listener) throws Exception { + return createController(token, connectionHints, listener, null); + } + + /** Creates {@link MediaController} from {@link SessionToken}. */ + public MediaController createController( + SessionToken token, + @Nullable Bundle connectionHints, + @Nullable MediaController.Listener listener, + @Nullable MediaControllerCreateListener controllerCreateListener) + throws Exception { TestMediaBrowserListener testListener = new TestMediaBrowserListener(listener); - MediaController controller = createControllerOnHandler(token, connectionHints, testListener); + MediaController controller = createControllerOnHandler(token, connectionHints, testListener, controllerCreateListener); controllers.put(controller, testListener); return controller; } private MediaController createControllerOnHandler( - SessionToken token, @Nullable Bundle connectionHints, TestMediaBrowserListener listener) + SessionToken token, @Nullable Bundle connectionHints, TestMediaBrowserListener listener, @Nullable MediaControllerCreateListener controllerCreateListener) throws Exception { // Create controller on the test handler, for changing MediaBrowserCompat's Handler // Looper. Otherwise, MediaBrowserCompat will post all the commands to the handler @@ -153,6 +183,17 @@ public final class MediaControllerTestRule extends ExternalResource { return builder.buildAsync(); } }); + + if (controllerCreateListener != null) { + future.addListener(() -> { + try { + MediaController controller = future.get(); + controllerCreateListener.onCreated(controller); + } catch (ExecutionException ignored) { + } catch (InterruptedException ignored) { + } + }, MoreExecutors.directExecutor()); + } return future.get(timeoutMs, MILLISECONDS); } diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java index 886091efe2..066c4b4a3d 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java @@ -170,6 +170,23 @@ public class MediaControllerWithMediaSessionCompatTest { assertThat(controller.isConnected()).isFalse(); } + @Test + public void disconnected_byControllerReleaseRightAfterCreated() throws Exception { + CountDownLatch latch = new CountDownLatch(1); + MediaController controller = + controllerTestRule.createController( + session.getSessionToken(), + new MediaController.Listener() { + @Override + public void onDisconnected(MediaController controller) { + latch.countDown(); + } + }, + MediaController::release); + assertThat(latch.await(TIMEOUT_MS, MILLISECONDS)).isTrue(); + assertThat(controller.isConnected()).isFalse(); + } + @Test public void close_twice_doesNotCrash() throws Exception { MediaController controller = controllerTestRule.createController(session.getSessionToken()); From 7536a25bf5c8637bdcd710161022fb1468774446 Mon Sep 17 00:00:00 2001 From: Haruki Hasegawa Date: Sun, 27 Mar 2022 00:19:57 +0900 Subject: [PATCH 2/2] Call removeCallbacksAndMessages() in MediaController#release() This fixes the NPE ocuured in the MediaControllerImplLegacy#connectToSession() right after MediaController#release() is called. --- .../src/main/java/androidx/media3/session/MediaController.java | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaController.java b/libraries/session/src/main/java/androidx/media3/session/MediaController.java index 53d7b912c1..ac0c416845 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaController.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaController.java @@ -417,6 +417,7 @@ public class MediaController implements Player { return; } released = true; + applicationHandler.removeCallbacksAndMessages(null); try { impl.release(); } catch (Exception e) {