From 7756d9823d82df5fb16a4cfb712a821fa3f79eec Mon Sep 17 00:00:00 2001 From: jaewan Date: Thu, 13 Jan 2022 00:33:26 +0000 Subject: [PATCH] Fix NPE in MediaController's constructor MediaController tries to release itself when the binder to the session becomes unavailable. However, if such thing happens while connecting in the constructor, it causes NPE when accessing MediaControllerImpl which is only initialized after it's connected. This fixes random failures in existing tests. PiperOrigin-RevId: 421423381 --- .../androidx/media3/session/MediaController.java | 3 +++ .../media3/session/MediaControllerImplBase.java | 16 +++++++++++++--- .../session/MediaControllerImplLegacy.java | 4 +++- .../session/MediaSessionAndControllerTest.java | 8 +++++++- 4 files changed, 26 insertions(+), 5 deletions(-) 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 2cdbba2276..53d7b912c1 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaController.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaController.java @@ -374,6 +374,7 @@ public class MediaController implements Player { @Initialized MediaController thisRef = this; impl = thisRef.createImpl(context, thisRef, token, connectionHints); + impl.connect(); } /* package */ MediaControllerImpl createImpl( @@ -1823,6 +1824,8 @@ public class MediaController implements Player { interface MediaControllerImpl { + void connect(); + void addListener(Player.Listener listener); void removeListener(Player.Listener listener); diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java index 47d07b71e2..c2fd9f81a9 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplBase.java @@ -166,6 +166,7 @@ import org.checkerframework.checker.nullness.qual.NonNull; private final Context context; private final SessionToken token; + private final Bundle connectionHints; private final IBinder.DeathRecipient deathRecipient; private final SurfaceCallback surfaceCallback; private final ListenerSet listeners; @@ -211,12 +212,24 @@ import org.checkerframework.checker.nullness.qual.NonNull; sequencedFutureManager = new SequencedFutureManager(); controllerStub = new MediaControllerStub(this); this.token = token; + this.connectionHints = connectionHints; deathRecipient = () -> MediaControllerImplBase.this.instance.runOnApplicationLooper( MediaControllerImplBase.this.instance::release); surfaceCallback = new SurfaceCallback(); + serviceConnection = + (this.token.getType() == TYPE_SESSION) + ? null + : new SessionServiceConnection(connectionHints); + flushCommandQueueHandler = new FlushCommandQueueHandler(instance.getApplicationLooper()); + lastReturnedContentPositionMs = C.TIME_UNSET; + lastSetPlayWhenReadyCalledTimeMs = C.TIME_UNSET; + } + + @Override + public void connect() { boolean connectionRequested; if (this.token.getType() == TYPE_SESSION) { // Session @@ -229,9 +242,6 @@ import org.checkerframework.checker.nullness.qual.NonNull; if (!connectionRequested) { this.instance.runOnApplicationLooper(MediaControllerImplBase.this.instance::release); } - flushCommandQueueHandler = new FlushCommandQueueHandler(instance.getApplicationLooper()); - lastReturnedContentPositionMs = C.TIME_UNSET; - lastSetPlayWhenReadyCalledTimeMs = C.TIME_UNSET; } @Override diff --git a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java index a88f438b28..aec3f925f5 100644 --- a/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java +++ b/libraries/session/src/main/java/androidx/media3/session/MediaControllerImplLegacy.java @@ -155,9 +155,11 @@ import org.checkerframework.checker.nullness.compatqual.NullableType; this.instance = instance; controllerCompatCallback = new ControllerCompatCallback(); this.token = token; + } + @Override + public void connect() { if (this.token.getType() == SessionToken.TYPE_SESSION) { - browserCompat = null; connectToSession((MediaSessionCompat.Token) checkStateNotNull(this.token.getBinder())); } else { connectToService(); diff --git a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java index 6b04d16644..0ff9c10b0a 100644 --- a/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java +++ b/libraries/test_session_current/src/androidTest/java/androidx/media3/session/MediaSessionAndControllerTest.java @@ -38,6 +38,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Before; import org.junit.ClassRule; import org.junit.Rule; @@ -117,11 +118,15 @@ public class MediaSessionAndControllerTest { Handler mainHandler = new Handler(Looper.getMainLooper()); Executor mainExecutor = (runnable) -> Util.postOrRun(mainHandler, runnable); for (int i = 0; i < 100; i++) { - int idx = i; MediaSession session = sessionTestRule.ensureReleaseAfterTest( new MediaSession.Builder(context, player).setId(TAG).build()); CountDownLatch latch = new CountDownLatch(1); + // Keep the instance in AtomicReference to prevent GC. + // Otherwise ListenableFuture and corresponding controllers can be GCed and disconnected + // callback can be ignored. + AtomicReference> controllerFutureRef = + new AtomicReference<>(); mainHandler.post( () -> { ListenableFuture controllerFuture = @@ -134,6 +139,7 @@ public class MediaSessionAndControllerTest { } }) .buildAsync(); + controllerFutureRef.set(controllerFuture); controllerFuture.addListener( () -> { try {