From fb4c7ebaa9dec7a9c4cdfc0d955ca946fa9aecc6 Mon Sep 17 00:00:00 2001 From: Lucas Dupin Date: Mon, 29 Jun 2020 17:20:41 -0700 Subject: [PATCH] Avoid reentrant callback when setting up listeners When registering new media players for timeouts, we should schedule their timeouts but not trigger the listeners yet. Otherwise we would end up on states where listeners would be ask to update players that were not initialized yet. One exception is when a playing is migrating from "resumable" to active. We'll then delay the state to the next Looper loop. Test: atest MediaTimeoutListenerTest Test: manually expire players Bug: 160036959 Change-Id: I9302dee6190e08c0e845b6f28dfcc249b636d90d --- .../systemui/media/MediaTimeoutListener.kt | 45 ++++++++++++++++--- .../media/MediaTimeoutListenerTest.kt | 21 +++++++++ 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt index 9a134dbe02647..8662aacfdab20 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt @@ -54,7 +54,32 @@ class MediaTimeoutListener @Inject constructor( if (mediaListeners.containsKey(key)) { return } + // Having an old key means that we're migrating from/to resumption. We should invalidate + // the old listener and create a new one. + val migrating = oldKey != null && key != oldKey + var wasPlaying = false + if (migrating) { + if (mediaListeners.containsKey(oldKey)) { + val oldListener = mediaListeners.remove(oldKey) + wasPlaying = oldListener?.playing ?: false + oldListener?.destroy() + if (DEBUG) Log.d(TAG, "migrating key $oldKey to $key, for resumption") + } else { + Log.w(TAG, "Old key $oldKey for player $key doesn't exist. Continuing...") + } + } mediaListeners[key] = PlaybackStateListener(key, data) + + // If a player becomes active because of a migration, we'll need to broadcast its state. + // Doing it now would lead to reentrant callbacks, so let's wait until we're done. + if (migrating && mediaListeners[key]?.playing != wasPlaying) { + mainExecutor.execute { + if (mediaListeners[key]?.playing == true) { + if (DEBUG) Log.d(TAG, "deliver delayed playback state for $key") + timeoutCallback.invoke(key, false /* timedOut */) + } + } + } } override fun onMediaDataRemoved(key: String) { @@ -71,7 +96,7 @@ class MediaTimeoutListener @Inject constructor( ) : MediaController.Callback() { var timedOut = false - private var playing: Boolean? = null + var playing: Boolean? = null // Resume controls may have null token private val mediaController = if (data.token != null) { @@ -83,7 +108,9 @@ class MediaTimeoutListener @Inject constructor( init { mediaController?.registerCallback(this) - onPlaybackStateChanged(mediaController?.playbackState) + // Let's register the cancellations, but not dispatch events now. + // Timeouts didn't happen yet and reentrant events are troublesome. + processState(mediaController?.playbackState, dispatchEvents = false) } fun destroy() { @@ -91,8 +118,12 @@ class MediaTimeoutListener @Inject constructor( } override fun onPlaybackStateChanged(state: PlaybackState?) { + processState(state, dispatchEvents = true) + } + + private fun processState(state: PlaybackState?, dispatchEvents: Boolean) { if (DEBUG) { - Log.v(TAG, "onPlaybackStateChanged: $state") + Log.v(TAG, "processState: $state") } val isPlaying = state != null && isPlayingState(state.state) @@ -116,12 +147,16 @@ class MediaTimeoutListener @Inject constructor( Log.v(TAG, "Execute timeout for $key") } timedOut = true - timeoutCallback(key, timedOut) + if (dispatchEvents) { + timeoutCallback(key, timedOut) + } }, PAUSED_MEDIA_TIMEOUT) } else { expireMediaTimeout(key, "playback started - $state, $key") timedOut = false - timeoutCallback(key, timedOut) + if (dispatchEvents) { + timeoutCallback(key, timedOut) + } } } diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt index 916fd0fe11b78..a76b20683a457 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt @@ -32,7 +32,9 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentCaptor import org.mockito.ArgumentMatchers.any +import org.mockito.ArgumentMatchers.anyBoolean import org.mockito.ArgumentMatchers.anyLong +import org.mockito.ArgumentMatchers.anyString import org.mockito.Captor import org.mockito.Mock import org.mockito.Mockito @@ -118,6 +120,7 @@ class MediaTimeoutListenerTest : SysuiTestCase() { mediaTimeoutListener.onMediaDataLoaded(KEY, null, mediaData) verify(mediaController).registerCallback(capture(mediaCallbackCaptor)) verify(executor).executeDelayed(capture(timeoutCaptor), anyLong()) + verify(timeoutCallback, never()).invoke(anyString(), anyBoolean()) } @Test @@ -132,6 +135,24 @@ class MediaTimeoutListenerTest : SysuiTestCase() { verify(mediaController, never()).unregisterCallback(anyObject()) } + @Test + fun testOnMediaDataLoaded_migratesKeys() { + // From not playing + mediaTimeoutListener.onMediaDataLoaded(KEY, null, mediaData) + clearInvocations(mediaController) + + // To playing + val playingState = mock(android.media.session.PlaybackState::class.java) + `when`(playingState.state).thenReturn(PlaybackState.STATE_PLAYING) + `when`(mediaController.playbackState).thenReturn(playingState) + mediaTimeoutListener.onMediaDataLoaded("NEWKEY", KEY, mediaData) + verify(mediaController).unregisterCallback(anyObject()) + verify(mediaController).registerCallback(anyObject()) + + // Enqueues callback + verify(executor).execute(anyObject()) + } + @Test fun testOnPlaybackStateChanged_schedulesTimeout_whenPaused() { // Assuming we're registered