diff options
3 files changed, 148 insertions, 35 deletions
diff --git a/packages/SystemUI/multivalentTests/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManagerTest.kt b/packages/SystemUI/multivalentTests/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManagerTest.kt new file mode 100644 index 000000000000..e8504563b4ae --- /dev/null +++ b/packages/SystemUI/multivalentTests/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManagerTest.kt @@ -0,0 +1,40 @@ +package com.android.systemui.keyguard.ui.preview + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.filters.SmallTest +import com.android.systemui.SysuiTestCase +import com.android.systemui.util.mockito.mock +import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runTest +import org.junit.Test +import org.junit.runner.RunWith + +@SmallTest +@RunWith(AndroidJUnit4::class) +class KeyguardRemotePreviewManagerTest : SysuiTestCase() { + + private val testDispatcher = StandardTestDispatcher() + private val testScope = TestScope(testDispatcher) + + @Test + fun onDestroy_clearsReferencesToRenderer() = + testScope.runTest { + val renderer = mock<KeyguardPreviewRenderer>() + val onDestroy: (PreviewLifecycleObserver) -> Unit = {} + + val observer = PreviewLifecycleObserver(this, testDispatcher, renderer, onDestroy) + + // Precondition check. + assertThat(observer.renderer).isNotNull() + assertThat(observer.onDestroy).isNotNull() + + observer.onDestroy() + + // The verification checks renderer/requestDestruction lambda because they-re + // non-singletons which can't leak KeyguardPreviewRenderer. + assertThat(observer.renderer).isNull() + assertThat(observer.onDestroy).isNull() + } +} diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManager.kt b/packages/SystemUI/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManager.kt index acfd3b0bcf57..24240dfe7402 100644 --- a/packages/SystemUI/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManager.kt +++ b/packages/SystemUI/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManager.kt @@ -30,6 +30,7 @@ import com.android.systemui.dagger.qualifiers.Application import com.android.systemui.dagger.qualifiers.Background import com.android.systemui.dagger.qualifiers.Main import com.android.systemui.shared.quickaffordance.shared.model.KeyguardPreviewConstants +import com.android.systemui.util.kotlin.logD import javax.inject.Inject import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope @@ -58,11 +59,14 @@ constructor( observer = PreviewLifecycleObserver( - renderer, applicationScope, mainDispatcher, + renderer, ::destroyObserver, ) + + logD(TAG) { "Created observer $observer" } + // Destroy any previous renderer associated with this token. activePreviews[renderer.id]?.let { destroyObserver(it) } activePreviews[renderer.id] = observer @@ -80,6 +84,8 @@ constructor( observer, ) ) + // NOTE: The process on the other side can retain messenger indefinitely. + // (e.g. GC might not trigger and cleanup the reference) val msg = Message.obtain() msg.replyTo = messenger result.putParcelable(KEY_PREVIEW_CALLBACK, msg) @@ -99,57 +105,84 @@ constructor( } } - private class PreviewLifecycleObserver( - private val renderer: KeyguardPreviewRenderer, - private val scope: CoroutineScope, - private val mainDispatcher: CoroutineDispatcher, - private val requestDestruction: (PreviewLifecycleObserver) -> Unit, - ) : Handler.Callback, IBinder.DeathRecipient { + companion object { + internal const val TAG = "KeyguardRemotePreviewManager" + @VisibleForTesting const val KEY_PREVIEW_SURFACE_PACKAGE = "surface_package" + @VisibleForTesting const val KEY_PREVIEW_CALLBACK = "callback" + } +} - private var isDestroyedOrDestroying = false +/** + * Handles messages from the other process and handles cleanup. + * + * NOTE: The other process might hold on to reference of this class indefinitely. It's entirely + * possible that GC won't trigger and we'll leak this for all times even if [onDestroy] was called. + * This helps make sure no non-Singleton objects are retained beyond destruction to prevent leaks. + */ +@VisibleForTesting(VisibleForTesting.PRIVATE) +class PreviewLifecycleObserver( + private val scope: CoroutineScope, + private val mainDispatcher: CoroutineDispatcher, + renderer: KeyguardPreviewRenderer, + onDestroy: (PreviewLifecycleObserver) -> Unit, +) : Handler.Callback, IBinder.DeathRecipient { + + private var isDestroyedOrDestroying = false + // These two are null after destruction + @VisibleForTesting var renderer: KeyguardPreviewRenderer? + @VisibleForTesting var onDestroy: ((PreviewLifecycleObserver) -> Unit)? + + init { + this.renderer = renderer + this.onDestroy = onDestroy + } - override fun handleMessage(message: Message): Boolean { - if (isDestroyedOrDestroying) { - return true - } + override fun handleMessage(message: Message): Boolean { + if (isDestroyedOrDestroying) { + return true + } - when (message.what) { - KeyguardPreviewConstants.MESSAGE_ID_SLOT_SELECTED -> { - message.data.getString(KeyguardPreviewConstants.KEY_SLOT_ID)?.let { slotId -> - renderer.onSlotSelected(slotId = slotId) - } + when (message.what) { + KeyguardPreviewConstants.MESSAGE_ID_SLOT_SELECTED -> { + message.data.getString(KeyguardPreviewConstants.KEY_SLOT_ID)?.let { slotId -> + checkNotNull(renderer).onSlotSelected(slotId = slotId) } - KeyguardPreviewConstants.MESSAGE_ID_HIDE_SMART_SPACE -> { - renderer.hideSmartspace( + } + KeyguardPreviewConstants.MESSAGE_ID_HIDE_SMART_SPACE -> { + checkNotNull(renderer) + .hideSmartspace( message.data.getBoolean(KeyguardPreviewConstants.KEY_HIDE_SMART_SPACE) ) - } - else -> requestDestruction(this) } - - return true + else -> checkNotNull(onDestroy).invoke(this) } - override fun binderDied() { - requestDestruction(this) + return true + } + + override fun binderDied() { + onDestroy?.invoke(this) + } + + fun onDestroy(): Pair<IBinder?, Int>? { + if (isDestroyedOrDestroying) { + return null } - fun onDestroy(): Pair<IBinder?, Int>? { - if (isDestroyedOrDestroying) { - return null - } + logD(TAG) { "Destroying $this" } - isDestroyedOrDestroying = true - val hostToken = renderer.hostToken + isDestroyedOrDestroying = true + return renderer?.let { rendererToDestroy -> + this.renderer = null + this.onDestroy = null + val hostToken = rendererToDestroy.hostToken hostToken?.unlinkToDeath(this, 0) - scope.launch(mainDispatcher) { renderer.destroy() } - return renderer.id + scope.launch(mainDispatcher) { rendererToDestroy.destroy() } + rendererToDestroy.id } } companion object { private const val TAG = "KeyguardRemotePreviewManager" - @VisibleForTesting const val KEY_PREVIEW_SURFACE_PACKAGE = "surface_package" - @VisibleForTesting const val KEY_PREVIEW_CALLBACK = "callback" } } diff --git a/packages/SystemUI/src/com/android/systemui/util/kotlin/Log.kt b/packages/SystemUI/src/com/android/systemui/util/kotlin/Log.kt new file mode 100644 index 000000000000..2f6c450e924c --- /dev/null +++ b/packages/SystemUI/src/com/android/systemui/util/kotlin/Log.kt @@ -0,0 +1,40 @@ +/* + * Copyright (C) 2023 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.util.kotlin + +import android.util.Log + +/** Logs message at [Log.DEBUG] level. Won't call the lambda if [DEBUG] is not loggable. */ +inline fun logD(tag: String, messageLambda: () -> String) { + if (Log.isLoggable(tag, Log.DEBUG)) { + Log.d(tag, messageLambda.invoke()) + } +} + +/** Logs message at [Log.VERBOSE] level. Won't call the lambda if [VERBOSE] is not loggable. */ +inline fun logV(tag: String, messageLambda: () -> String) { + if (Log.isLoggable(tag, Log.VERBOSE)) { + Log.v(tag, messageLambda.invoke()) + } +} + +/** Logs message at [Log.INFO] level. Won't call the lambda if [INFO] is not loggable. */ +inline fun logI(tag: String, messageLambda: () -> String) { + if (Log.isLoggable(tag, Log.INFO)) { + Log.i(tag, messageLambda.invoke()) + } +} |