Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix NPE in MediaControllerImplLegacy#connectToSession() #59

Merged
merged 2 commits into from
Apr 26, 2022
+101 −6
Merged

Fix NPE in MediaControllerImplLegacy#connectToSession() #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db1a1dc
Add more test cases for checking the MediaController#release() behavior
h6ah4i Mar 26, 2022
7536a25
Call removeCallbacksAndMessages() in MediaController#release()
h6ah4i Mar 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions libraries/session/src/main/java/androidx/media3/session/MediaController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ public void release() {
return;
}
released = true;
applicationHandler.removeCallbacksAndMessages(null);
try {
impl.release();
} catch (Exception e) {
Expand Down
38 changes: 37 additions & 1 deletion ...est_session_current/src/androidTest/java/androidx/media3/session/MediaControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ public void isConnected_afterConnection_returnsTrue() throws Exception {
}

@Test
public void isConnected_afterDisconnection_returnsFalse() throws Exception {
public void isConnected_afterDisconnectionBySessionRelease_returnsFalse() throws Exception {
CountDownLatch disconnectedLatch = new CountDownLatch(1);
MediaController controller =
controllerTestRule.createController(
Expand All @@ -210,6 +210,42 @@ public void onDisconnected(MediaController controller) {
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());
Expand Down
51 changes: 46 additions & 5 deletions ...session_current/src/androidTest/java/androidx/media3/session/MediaControllerTestRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,17 @@
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;

/**
Expand All @@ -44,6 +48,15 @@ public final class MediaControllerTestRule extends ExternalResource {
private volatile Class<? extends MediaController> 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<>();
Expand Down Expand Up @@ -96,17 +109,24 @@ public MediaController createController(MediaSessionCompat.Token token) throws E
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. */
Expand All @@ -120,14 +140,24 @@ public MediaController createController(
@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
Expand All @@ -153,6 +183,17 @@ private MediaController createControllerOnHandler(
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);
}

Expand Down
17 changes: 17 additions & 0 deletions ...c/androidTest/java/androidx/media3/session/MediaControllerWithMediaSessionCompatTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,23 @@ public void onDisconnected(MediaController controller) {
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());
Expand Down