Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 7aaa9bca authored by Jernej Virag's avatar Jernej Virag
Browse files

Release references to KeyguardRemotePreview after onDestroy 

The process on the other side of the IPC can keep PreviewLifecycleObserver for any time - until the GC triggers or even longer if there's an accidental leak. This can clocks and other heavy objects on our side.
PreviewLifecycleObserver is moved outside the KeyguardRemotePreviewManager to prevent accidental capturing of the manager class via this references.

Flag: NONE
Bug: 315637349
Test: newly added unit test and verified with Studio profiler
Change-Id: If8515407aa75a754d94cff92965d9ee6b713001c
parent 9d708f7e

packages/SystemUI/multivalentTests/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManagerTest.kt

0 → 100644
+40 −0
Original line number Diff line number Diff line 
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()

        }

}

packages/SystemUI/src/com/android/systemui/keyguard/ui/preview/KeyguardRemotePreviewManager.kt

+68 −35
Original line number Diff line number Diff line
@@ -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,14 +105,37 @@ constructor( 
        }

    }



    private class PreviewLifecycleObserver(

        private val renderer: KeyguardPreviewRenderer,

    companion object {

        internal const val TAG = "KeyguardRemotePreviewManager"

        @VisibleForTesting const val KEY_PREVIEW_SURFACE_PACKAGE = "surface_package"

        @VisibleForTesting const val KEY_PREVIEW_CALLBACK = "callback"

    }

}



/**

 * 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,

        private val requestDestruction: (PreviewLifecycleObserver) -> Unit,

    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) {
@@ -116,22 +145,23 @@ constructor( 
        when (message.what) {

            KeyguardPreviewConstants.MESSAGE_ID_SLOT_SELECTED -> {

                message.data.getString(KeyguardPreviewConstants.KEY_SLOT_ID)?.let { slotId ->

                        renderer.onSlotSelected(slotId = slotId)

                    checkNotNull(renderer).onSlotSelected(slotId = slotId)

                }

            }

            KeyguardPreviewConstants.MESSAGE_ID_HIDE_SMART_SPACE -> {

                    renderer.hideSmartspace(

                checkNotNull(renderer)

                    .hideSmartspace(

                        message.data.getBoolean(KeyguardPreviewConstants.KEY_HIDE_SMART_SPACE)

                    )

            }

                else -> requestDestruction(this)

            else -> checkNotNull(onDestroy).invoke(this)

        }



        return true

    }



    override fun binderDied() {

            requestDestruction(this)

        onDestroy?.invoke(this)

    }



    fun onDestroy(): Pair<IBinder?, Int>? {
@@ -139,17 +169,20 @@ constructor( 
            return null

        }



        logD(TAG) { "Destroying $this" }



        isDestroyedOrDestroying = true

            val hostToken = renderer.hostToken

        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"

    }

}

packages/SystemUI/src/com/android/systemui/util/kotlin/Log.kt

0 → 100644
+40 −0
Original line number Diff line number Diff line 
/*

 * 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())

    }

}