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

Commit ab270765 authored by Nicolo' Mazzucato's avatar Nicolo' Mazzucato
Browse files

Fix race from DisplayRepository.getDisplay 

Before this cl getDisplay(id) was guaranteed to not cause any binder call, as the mapping between displayId and Display in DisplayRepository was done in the background.

However, this was causing getDisplay(id) to possibly return null after a display was added, in the case the binder call to get the display instance was in flight while getDisplay was called.

This cl regretfully makes getDisplay return the cached Display instance if available, or makes it trigger a binder call to get the Display if we know its id was added already. While this might cause regressions, it's necessary to ensure correctness. Anyway, the binder call shouldn't happen more than once per display id, so I don't expect the impact of this to be that high.

A new method with the old non-blocking behaviour has been added: getCachedDisplay.

Bug: 362719719
Bug: 410580872
Test: DisplayRepositoryTest
Flag: NONE - small performance bugfix
Change-Id: Icf2397daa604aa5f72f1962c7531130ffa634d97
parent d19e7b4c

displaylib/src/com/android/app/displaylib/DisplayRepository.kt

+37 −2
Original line number Diff line number Diff line
@@ -86,11 +86,28 @@ interface DisplayRepository { 
    /**

     * Given a display ID int, return the corresponding Display object, or null if none exist.

     *

     * This method is guaranteed to not result in any binder call.

     * This method will not result in a binder call in most cases. The only exception is if there is

     * an existing binder call ongoing to get the [Display] instance already. In that case, this

     * will wait for the end of the binder call.

     */

    fun getDisplay(displayId: Int): Display? =

    fun getDisplay(displayId: Int): Display?



    /**

     * As [getDisplay], but it's always guaranteed to not block on any binder call.

     *

     * This might return null if the display id was not mapped to a [Display] object yet.

     */

    fun getCachedDisplay(displayId: Int): Display? =

        displays.value.firstOrNull { it.displayId == displayId }



    /**

     * Returns whether the given displayId is in the set of enabled displays.

     *

     * This is guaranteed to not cause a binder call. Use this instead of [getDisplay] (see its docs

     * for why)

     */

    fun containsDisplay(displayId: Int): Boolean = displayIds.value.contains(displayId)



    /** Represents a connected display that has not been enabled yet. */

    interface PendingDisplay {

        /** Id of the pending display. */
@@ -375,6 +392,24 @@ constructor( 
            .map { defaultDisplay.state == Display.STATE_OFF }

            .distinctUntilChanged()



    override fun getDisplay(displayId: Int): Display? {

        val cachedDisplay = getCachedDisplay(displayId)

        if (cachedDisplay != null) return cachedDisplay

        // cachedDisplay could be null for 2 reasons:

        // 1. the displayId is being mapped to a display in the background, but the binder call is

        // not done

        // 2. the display is not there

        // In case of option one, let's get it synchronously from display manager to make sure for

        // this to be consistent.

        return if (displayIds.value.contains(displayId)) {

            traceSection("$TAG#getDisplayFallbackToDisplayManager") {

                getDisplayFromDisplayManager(displayId)

            }

        } else {

            null

        }

    }



    private fun <T> Flow<T>.debugLog(flowName: String): Flow<T> {

        return if (DEBUG) {

            traceEach(flowName, logcat = true, traceEmissionCount = true)

displaylib/src/com/android/app/displaylib/PerDisplayRepository.kt

+1 −1
Original line number Diff line number Diff line
@@ -192,7 +192,7 @@ constructor( 
    }



    override fun get(displayId: Int): T? {

        if (displayRepository.getDisplay(displayId) == null) {

        if (!displayRepository.containsDisplay(displayId)) {

            Log.e(TAG, "<$debugName: Display with id $displayId doesn't exist.")

            return null

        }