From 86b2a3e7236e0efca59754da4ce7ae81258c5718 Mon Sep 17 00:00:00 2001 From: Caitlin Cassidy Date: Wed, 11 May 2022 14:25:54 +0000 Subject: [PATCH] [Media] Add logs for potential memory leaks in MediaCarouselController. Bug: 231625697 Test: atest SystemUITests Change-Id: I0a308795f5c17dc40c915d756e0e22dc1cfc10e5 --- .../systemui/log/dagger/LogModule.java | 12 +++++ .../dagger/MediaCarouselControllerLog.java | 35 ++++++++++++++ .../systemui/media/MediaCarouselController.kt | 48 ++++++++++++++----- .../media/MediaCarouselControllerLogger.kt | 45 +++++++++++++++++ .../media/MediaCarouselControllerTest.kt | 4 +- 5 files changed, 131 insertions(+), 13 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/log/dagger/MediaCarouselControllerLog.java create mode 100644 packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt diff --git a/packages/SystemUI/src/com/android/systemui/log/dagger/LogModule.java b/packages/SystemUI/src/com/android/systemui/log/dagger/LogModule.java index f72f1bb474681..1e7a292dadbac 100644 --- a/packages/SystemUI/src/com/android/systemui/log/dagger/LogModule.java +++ b/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 diff --git a/packages/SystemUI/src/com/android/systemui/log/dagger/MediaCarouselControllerLog.java b/packages/SystemUI/src/com/android/systemui/log/dagger/MediaCarouselControllerLog.java new file mode 100644 index 0000000000000..b03655a543f70 --- /dev/null +++ b/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 { +} diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt index 3483bc39b9438..3b8b260411eb7 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt +++ b/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() @@ -882,7 +890,8 @@ class MediaCarouselController @Inject constructor( override fun dump(pw: PrintWriter, args: Array) { 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") @@ -945,9 +954,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) @@ -959,10 +972,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) @@ -970,13 +987,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) } } @@ -1004,6 +1026,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 diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt new file mode 100644 index 0000000000000..04ebd5a71137a --- /dev/null +++ b/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" diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt index 0d917e3b19a87..ceb811b8d8aae 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt +++ b/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()