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

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)
+                }
             }
         }
 

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