Merge "[Media] Add logs for potential memory leaks in MediaCarouselController." into tm-dev

packages/SystemUI/src/com/android/systemui/log/dagger/LogModule.java

@@ -230,6 +230,18 @@ public class LogModule {
         return factory.create("MediaBrowser", 100);
     }
 
+    /**
+     * Provides a buffer for updates to the media carousel.
+     *
+     * See {@link com.android.systemui.media.MediaCarouselController}.
+     */
+    @Provides
+    @SysUISingleton
+    @MediaCarouselControllerLog
+    public static LogBuffer provideMediaCarouselControllerBuffer(LogBufferFactory factory) {
+        return factory.create("MediaCarouselCtlrLog", 20);
+    }
+
     /** Allows logging buffers to be tweaked via adb on debug builds but not on prod builds. */
     @Provides
     @SysUISingleton

packages/SystemUI/src/com/android/systemui/log/dagger/MediaCarouselControllerLog.java

@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.systemui.log.dagger;
+
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import com.android.systemui.log.LogBuffer;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.Retention;
+
+import javax.inject.Qualifier;
+
+/**
+ * A {@link LogBuffer} for {@link com.android.systemui.media.MediaCarouselController}
+ */
+@Qualifier
+@Documented
+@Retention(RUNTIME)
+public @interface MediaCarouselControllerLog {
+}

packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt

@@ -59,7 +59,8 @@ class MediaCarouselController @Inject constructor(
     falsingCollector: FalsingCollector,
     falsingManager: FalsingManager,
     dumpManager: DumpManager,
-    private val logger: MediaUiEventLogger
+    private val logger: MediaUiEventLogger,
+    private val debugLogger: MediaCarouselControllerLogger
 ) : Dumpable {
     /**
      * The current width of the carousel
@@ -439,12 +440,16 @@ class MediaCarouselController @Inject constructor(
             newPlayer.mediaViewHolder?.player?.setLayoutParams(lp)
             newPlayer.bindPlayer(data, key)
             newPlayer.setListening(currentlyExpanded)
-            MediaPlayerData.addMediaPlayer(key, data, newPlayer, systemClock, isSsReactivated)
+            MediaPlayerData.addMediaPlayer(
+                key, data, newPlayer, systemClock, isSsReactivated, debugLogger
+            )
             updatePlayerToState(newPlayer, noAnimation = true)
             reorderAllPlayers(curVisibleMediaKey)
         } else {
             existingPlayer.bindPlayer(data, key)
-            MediaPlayerData.addMediaPlayer(key, data, existingPlayer, systemClock, isSsReactivated)
+            MediaPlayerData.addMediaPlayer(
+                key, data, existingPlayer, systemClock, isSsReactivated, debugLogger
+            )
             if (isReorderingAllowed || shouldScrollToActivePlayer) {
                 reorderAllPlayers(curVisibleMediaKey)
             } else {
@@ -475,7 +480,8 @@ class MediaCarouselController @Inject constructor(
 
         val existingSmartspaceMediaKey = MediaPlayerData.smartspaceMediaKey()
         existingSmartspaceMediaKey?.let {
-            MediaPlayerData.removeMediaPlayer(existingSmartspaceMediaKey)
+            val removedPlayer = MediaPlayerData.removeMediaPlayer(existingSmartspaceMediaKey)
+            removedPlayer?.run { debugLogger.logPotentialMemoryLeak(existingSmartspaceMediaKey) }
         }
 
         val newRecs = mediaControlPanelFactory.get()
@@ -488,7 +494,9 @@ class MediaCarouselController @Inject constructor(
         newRecs.bindRecommendation(data)
         val curVisibleMediaKey = MediaPlayerData.playerKeys()
                 .elementAtOrNull(mediaCarouselScrollHandler.visibleMediaIndex)
-        MediaPlayerData.addMediaRecommendation(key, data, newRecs, shouldPrioritize, systemClock)
+        MediaPlayerData.addMediaRecommendation(
+            key, data, newRecs, shouldPrioritize, systemClock, debugLogger
+        )
         updatePlayerToState(newRecs, noAnimation = true)
         reorderAllPlayers(curVisibleMediaKey)
         updatePageIndicator()
@@ -883,7 +891,8 @@ class MediaCarouselController @Inject constructor(
     override fun dump(pw: PrintWriter, args: Array<out String>) {
         pw.apply {
             println("keysNeedRemoval: $keysNeedRemoval")
-            println("playerKeys: ${MediaPlayerData.playerKeys()}")
+            println("dataKeys: ${MediaPlayerData.dataKeys()}")
+            println("playerSortKeys: ${MediaPlayerData.playerKeys()}")
             println("smartspaceMediaData: ${MediaPlayerData.smartspaceMediaData}")
             println("shouldPrioritizeSs: ${MediaPlayerData.shouldPrioritizeSs}")
             println("current size: $currentCarouselWidth x $currentCarouselHeight")
@@ -946,9 +955,13 @@ internal object MediaPlayerData {
         data: MediaData,
         player: MediaControlPanel,
         clock: SystemClock,
-        isSsReactivated: Boolean
+        isSsReactivated: Boolean,
+        debugLogger: MediaCarouselControllerLogger? = null
     ) {
-        removeMediaPlayer(key)
+        val removedPlayer = removeMediaPlayer(key)
+        if (removedPlayer != null && removedPlayer != player) {
+            debugLogger?.logPotentialMemoryLeak(key)
+        }
         val sortKey = MediaSortKey(isSsMediaRec = false,
                 data, clock.currentTimeMillis(), isSsReactivated = isSsReactivated)
         mediaData.put(key, sortKey)
@@ -960,10 +973,14 @@ internal object MediaPlayerData {
         data: SmartspaceMediaData,
         player: MediaControlPanel,
         shouldPrioritize: Boolean,
-        clock: SystemClock
+        clock: SystemClock,
+        debugLogger: MediaCarouselControllerLogger? = null
     ) {
         shouldPrioritizeSs = shouldPrioritize
-        removeMediaPlayer(key)
+        val removedPlayer = removeMediaPlayer(key)
+        if (removedPlayer != null && removedPlayer != player) {
+            debugLogger?.logPotentialMemoryLeak(key)
+        }
         val sortKey = MediaSortKey(isSsMediaRec = true,
             EMPTY.copy(isPlaying = false), clock.currentTimeMillis(), isSsReactivated = true)
         mediaData.put(key, sortKey)
@@ -971,13 +988,18 @@ internal object MediaPlayerData {
         smartspaceMediaData = data
     }
 
-    fun moveIfExists(oldKey: String?, newKey: String) {
+    fun moveIfExists(
+        oldKey: String?,
+        newKey: String,
+        debugLogger: MediaCarouselControllerLogger? = null
+    ) {
         if (oldKey == null || oldKey == newKey) {
             return
         }
 
         mediaData.remove(oldKey)?.let {
-            removeMediaPlayer(newKey)
+            val removedPlayer = removeMediaPlayer(newKey)
+            removedPlayer?.run { debugLogger?.logPotentialMemoryLeak(newKey) }
             mediaData.put(newKey, it)
         }
     }
@@ -1005,6 +1027,8 @@ internal object MediaPlayerData {
 
     fun mediaData() = mediaData.entries.map { e -> Triple(e.key, e.value.data, e.value.isSsMediaRec) }
 
+    fun dataKeys() = mediaData.keys
+
     fun players() = mediaPlayers.values
 
     fun playerKeys() = mediaPlayers.keys

packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt

@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.systemui.media
+
+import com.android.systemui.dagger.SysUISingleton
+import com.android.systemui.log.LogBuffer
+import com.android.systemui.log.LogLevel
+import com.android.systemui.log.dagger.MediaCarouselControllerLog
+import javax.inject.Inject
+
+/** A debug logger for [MediaCarouselController]. */
+@SysUISingleton
+class MediaCarouselControllerLogger @Inject constructor(
+    @MediaCarouselControllerLog private val buffer: LogBuffer
+) {
+    /**
+     * Log that there might be a potential memory leak for the [MediaControlPanel] and/or
+     * [MediaViewController] related to [key].
+     */
+    fun logPotentialMemoryLeak(key: String) = buffer.log(
+        TAG,
+        LogLevel.DEBUG,
+        { str1 = key },
+        {
+            "Potential memory leak: " +
+                    "Removing control panel for $str1 from map without calling #onDestroy"
+        }
+    )
+}
+
+private const val TAG = "MediaCarouselCtlrLog"

packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt

@@ -63,6 +63,7 @@ class MediaCarouselControllerTest : SysuiTestCase() {
     @Mock lateinit var falsingManager: FalsingManager
     @Mock lateinit var dumpManager: DumpManager
     @Mock lateinit var logger: MediaUiEventLogger
+    @Mock lateinit var debugLogger: MediaCarouselControllerLogger
 
     private val clock = FakeSystemClock()
     private lateinit var mediaCarouselController: MediaCarouselController
@@ -83,7 +84,8 @@ class MediaCarouselControllerTest : SysuiTestCase() {
             falsingCollector,
             falsingManager,
             dumpManager,
-            logger
+            logger,
+            debugLogger
         )
 
         MediaPlayerData.clear()