From f93a416d612fcd6d5afe78cfec422c0039118abf Mon Sep 17 00:00:00 2001 From: JingMatrix Date: Thu, 20 Aug 2026 15:00:24 +0200 Subject: [PATCH 1/2] Share one installed-app enumeration between concurrent readers Asking the daemon for the installed applications is not the cheap lookup its call site reads as. It goes out as getInstalledPackagesFromAllUsers with filterNoProcess set, and the daemon answers that by querying the package manager for the full component list of every package on the device -- activities, services, receivers and providers, several hundred times over, with a four-call fallback each time a binder buffer overflows on the way back. AppRepository caches the answer for exactly that reason, and the splash prefetch pays for it once where nobody is waiting. What the cache did not have was a way to say that a read was already running. It checked a volatile field and, finding it empty, went to the daemon; two callers arriving together both found it empty and both went. The scope editor is two such callers by construction -- its load reads the list, and the module-package set it filters by reads it again -- so on a cold cache it ran two of those enumerations against each other, one racing the other for the same threads, while the screen it was opening waited on the first. Cold is not the rare case either: every install, update and uninstall drops the cache, and a package event arrives twice, once from the platform and once from the daemon's re-broadcast. So the fetch becomes a job the repository holds rather than work each caller starts. A caller that finds one running joins it, and the mutex guarding that field is never held across the fetch itself, only across the decision. The job runs on the application scope, not the caller's: leaving a screen part-way through a read now leaves the answer behind for the next visit instead of throwing away the several hundred queries it had already paid for. A finished job is retired by the next reader rather than by itself, which is what lets a failed read be retried instead of joined for the life of the process -- a successful one has filled the cache and is never consulted again. A generation counter goes in beside it. A read already in flight when a package event lands has by definition missed what that event carried, and the old code cached its answer regardless, holding a list known to be wrong until the next event -- on a device where nothing else changes, forever. The counter is sampled when a read starts and again before it publishes: the caller that asked still gets the answer, because it is the best that read can offer, but nobody else inherits it. forceRefresh goes. It had no callers, and sharing a running job leaves nothing for it to mean. --- .../manager/data/repository/AppRepository.kt | 147 ++++++++++++------ .../vector/manager/di/ServiceLocator.kt | 2 +- 2 files changed, 104 insertions(+), 45 deletions(-) diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/AppRepository.kt b/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/AppRepository.kt index 134db4a13..6865d71ba 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/AppRepository.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/data/repository/AppRepository.kt @@ -1,8 +1,14 @@ package org.matrix.vector.manager.data.repository import android.content.pm.ApplicationInfo import android.content.pm.PackageManager +import java.util.concurrent.atomic.AtomicInteger import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Deferred import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.matrix.vector.manager.data.model.AppInfo import org.matrix.vector.manager.data.model.ModuleDetectionCache @@ -15,10 +21,48 @@ class AppRepository( private val daemonClient: DaemonClient, private val packageManager: PackageManager, private val moduleDetection: ModuleDetectionCache, + private val scope: CoroutineScope, ) { @Volatile private var cachedApps: List? = null @Volatile private var cachedModulePackages: Set? = null + /** + * Bumped by every [invalidate], so a read already in flight when one lands can tell. + * + * That read has by definition missed whatever the package event carried, and writing its answer + * into the cache afterwards would hold the stale list until the *next* event — on a device + * where nothing else changes, indefinitely. The counter is sampled when a read starts and again + * before it publishes: on a mismatch the answer still goes back to the caller who asked for it, + * because it is the best that read can offer, but it is not cached for anyone else. + */ + private val generation = AtomicInteger(0) + + /** Guards [inFlight] alone. Never held across a fetch — that is what lets a fetch be shared. */ + private val lock = Mutex() + + /** + * The enumeration running right now, so concurrent callers join it rather than each start one. + * + * Enumerating is expensive in a way the call site cannot see: the daemon answers + * `getInstalledPackagesFromAllUsers` with `filterNoProcess = true` by asking the package + * manager for the full component list of every installed package, several hundred of them on a + * normal device. The scope editor asks twice at once — once from `load`, once for the module + * packages it filters by — and with a cold cache both used to go through. That is issue #917: + * any install or uninstall drops the cache, so the next module's scope screen ran two of those + * enumerations against each other and sat on its spinner until they finished. + * + * Started on the application scope rather than the caller's, so leaving the screen part-way + * through does not throw the work away: the next visit finds the cache warm rather than paying + * for the same enumeration again. + * + * Retired by the next reader that finds it finished rather than by the job itself. A fetch that + * succeeded has filled the cache, so the check above this one answers before the field is ever + * consulted; a fetch that failed deliberately cached nothing, and dropping the finished job is + * exactly what lets the next caller retry instead of joining a failure for the life of the + * process. + */ + private var inFlight: Deferred>? = null + /** * Drops the cache so the next read goes back to the daemon. * @@ -26,62 +70,76 @@ class AppRepository( * while the manager is open would not appear for the life of the process. */ fun invalidate() { + generation.incrementAndGet() cachedApps = null cachedModulePackages = null } - suspend fun getInstalledApps(forceRefresh: Boolean = false): List = - withContext(Dispatchers.IO) { - if (!forceRefresh && cachedApps != null) { - return@withContext cachedApps!! + suspend fun getInstalledApps(): List { + cachedApps?.let { + return it + } + val job = + lock.withLock { + // Re-read under the lock. A fetch that finished while this call was waiting has + // already published, and joining a job only to learn what the field beside it now + // holds is a suspension for nothing. + cachedApps?.let { + return it + } + inFlight?.takeIf { it.isActive } + ?: scope.async(Dispatchers.IO) { fetchInstalledApps() }.also { inFlight = it } } + return job.await() + } - val flags = PackageManager.MATCH_UNINSTALLED_PACKAGES or PackageManager.GET_META_DATA + private suspend fun fetchInstalledApps(): List { + val startedAt = generation.get() + val flags = PackageManager.MATCH_UNINSTALLED_PACKAGES or PackageManager.GET_META_DATA - val result = - daemonClient.getInstalledPackagesFromAllUsers(flags, filterNoProcess = true) - val failure = result.exceptionOrNull() - if (failure != null) { - if (failure !is CancellationException) { - logW("apps: installed package list unavailable from daemon", failure) - } - return@withContext emptyList() + val result = daemonClient.getInstalledPackagesFromAllUsers(flags, filterNoProcess = true) + val failure = result.exceptionOrNull() + if (failure != null) { + if (failure !is CancellationException) { + logW("apps: installed package list unavailable from daemon", failure) } + return emptyList() + } - val packages = result.getOrNull() ?: emptyList() - val PER_USER_RANGE = 100000 + val packages = result.getOrNull() ?: emptyList() + val PER_USER_RANGE = 100000 - val appList = - packages.mapNotNull { pkg -> - val appInfo = pkg.applicationInfo ?: return@mapNotNull null - val isSystem = (appInfo.flags and ApplicationInfo.FLAG_SYSTEM) != 0 - // FLAG_IS_GAME was replaced by the category in API 26 and is deprecated, but an - // app built before that still ships it and sets no category, so both are read. - @Suppress("DEPRECATION") - val isGame = - appInfo.category == ApplicationInfo.CATEGORY_GAME || - (appInfo.flags and ApplicationInfo.FLAG_IS_GAME) != 0 + val appList = + packages.mapNotNull { pkg -> + val appInfo = pkg.applicationInfo ?: return@mapNotNull null + val isSystem = (appInfo.flags and ApplicationInfo.FLAG_SYSTEM) != 0 + // FLAG_IS_GAME was replaced by the category in API 26 and is deprecated, but an + // app built before that still ships it and sets no category, so both are read. + @Suppress("DEPRECATION") + val isGame = + appInfo.category == ApplicationInfo.CATEGORY_GAME || + (appInfo.flags and ApplicationInfo.FLAG_IS_GAME) != 0 - val userId = appInfo.uid / PER_USER_RANGE + val userId = appInfo.uid / PER_USER_RANGE - AppInfo( - packageName = pkg.packageName, - userId = userId, - appName = appInfo.loadLabel(packageManager).toString(), - isSystemApp = isSystem, - isGame = isGame, - isSelectedInScope = false, // To be merged later in the ViewModel - isRecommended = false, - lastUpdateTime = pkg.lastUpdateTime, - firstInstallTime = pkg.firstInstallTime, - versionCode = pkg.versionCodeCompat, - applicationInfo = appInfo, - ) - } + AppInfo( + packageName = pkg.packageName, + userId = userId, + appName = appInfo.loadLabel(packageManager).toString(), + isSystemApp = isSystem, + isGame = isGame, + isSelectedInScope = false, // To be merged later in the ViewModel + isRecommended = false, + lastUpdateTime = pkg.lastUpdateTime, + firstInstallTime = pkg.firstInstallTime, + versionCode = pkg.versionCodeCompat, + applicationInfo = appInfo, + ) + } - cachedApps = appList - return@withContext appList - } + if (generation.get() == startedAt) cachedApps = appList + return appList + } /** * Which installed packages are themselves Xposed modules. @@ -101,6 +159,7 @@ class AppRepository( cachedModulePackages?.let { return@withContext it } + val startedAt = generation.get() val packages = getInstalledApps() .asSequence() @@ -116,7 +175,7 @@ class AppRepository( } .map { it.packageName } .toSet() - cachedModulePackages = packages + if (generation.get() == startedAt) cachedModulePackages = packages packages } } diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt b/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt index 802e1270e..707369c56 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/di/ServiceLocator.kt @@ -117,7 +117,7 @@ object ServiceLocator { val modules: ModuleRepository by lazy { ModuleRepository(daemon, appScope) } val apps: AppRepository by lazy { - AppRepository(daemon, context.packageManager, moduleDetection) + AppRepository(daemon, context.packageManager, moduleDetection, appScope) } /** From 0ee0f531458425f75546745ab5a20ac3c353943c Mon Sep 17 00:00:00 2001 From: JingMatrix Date: Thu, 20 Aug 2026 15:00:37 +0200 Subject: [PATCH 2/2] Never leave the scope editor on a spinner it cannot clear ScopeUiState starts with loading set, and the state built on the last line of load was the only other thing that ever wrote it. Every exit that skipped that line therefore left the flag standing, and the screen is drawn entirely behind it: a throw from one of the reads, or the reader leaving while they were still running, and the spinner was there for as long as the view model was. There is no second load to correct it -- load runs once, from init. A try around the body and the flag cleared in the finally, only when it is still set, so the success path stays the single writer of the state it builds. What a failure now shows is the empty list, which is wrong but says so, rather than a wait that never ends. The one read in that body that could throw is the manifest inspection, which opens the module's APK. The package info fetched immediately above it is already guarded for the same hazard -- a package removed or replaced between one call and the next -- and this one was not, so an APK that went away mid-load took the whole manager down from a viewModelScope coroutine. It recovers the way its neighbour does, as no recommended scope, which costs the reader a few rows that would have arrived pre-ticked. --- .../ui/screens/modules/ScopeViewModel.kt | 184 ++++++++++-------- 1 file changed, 106 insertions(+), 78 deletions(-) diff --git a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt index 89d066309..f476e2005 100644 --- a/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt +++ b/manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/modules/ScopeViewModel.kt @@ -450,87 +450,115 @@ class ScopeViewModel( viewModelScope.launch { _uiState.value = _uiState.value.copy(loading = true) - // Only the apps belonging to the user this module is installed for. A module in a - // work profile can only hook that profile's apps, so listing the owner's alongside - // them offers choices the framework will not honour — and the same package appears - // once per user, so an unfiltered list shows visible duplicates. - val apps = - withContext(Dispatchers.IO) { - appRepository.getInstalledApps().filter { it.userId == userId } - } + // `loading` starts true and the state built at the end of this body is the only + // other writer of it, so any exit that skips that line leaves the spinner up for + // good: a throw from one of the reads below, or the screen being left while they + // are still running. Issue #917 was that window held open for minutes by a cold + // app list, and nothing about it on screen could be told from a permanent hang. + try { + // Only the apps belonging to the user this module is installed for. A module in a + // work profile can only hook that profile's apps, so listing the owner's alongside + // them offers choices the framework will not honour — and the same package appears + // once per user, so an unfiltered list shows visible duplicates. + val apps = + withContext(Dispatchers.IO) { + appRepository.getInstalledApps().filter { it.userId == userId } + } - // The system server is a hook target like any other and modules ask for it by name, - // but it is not an installed package so it never appears in the package list. Without - // this entry a module whose entire recommended scope is the framework — Core Patch, - // for one — offers the user nothing to tick. - // - // Offered to every user, not only the owner. There is exactly one system_server on the - // device, so it is not a per-user target that other users happen to lack — it is one - // process they all share, and a module in a work profile or a private space would - // otherwise have no way to ask for the only target it may need (issue #136). The - // daemon agrees: `ModuleDatabase.setModuleScope` stores a `system` row under user 0 - // whoever asked, and `ConfigCache` maps it to system_server without looking at whose - // module it was. - val withFramework = listOf(systemFrameworkEntry(apps)) + apps - allApps.value = withFramework - - // Asked once per load rather than per row, and held here until the state is built at - // the end of this function — anything written into `_uiState` before then is discarded - // when that fresh ScopeUiState replaces it. A failure to ask means not explaining, - // never explaining wrongly. - val userCount = - withContext(Dispatchers.IO) { daemonClient.getUsers().getOrNull()?.size ?: 1 } - - // A scope the daemon will not hand over shows as none rather than keeping the screen - // shut; [readSavedScope] logs why, and keeps that case apart from the empty list a - // module with nothing ticked legitimately has. - val saved = readSavedScope() ?: emptySet() - savedScope.value = saved - draftScope.value = saved - - val info = - withContext(Dispatchers.IO) { - runCatching { - packageManager.getApplicationInfo( - modulePackageName, - android.content.pm.PackageManager.GET_META_DATA, - ) - } - .onFailure { e -> - logW( - "scope: package info for $modulePackageName (user $userId) " + - "unavailable, no recommended scope", - e, - ) + // The system server is a hook target like any other and modules ask for it by name, + // but it is not an installed package so it never appears in the package list. + // Without this entry a module whose entire recommended scope is the framework — + // Core Patch, for one — offers the user nothing to tick. + // + // Offered to every user, not only the owner. There is exactly one system_server on + // the device, so it is not a per-user target that other users happen to lack — it + // is one process they all share, and a module in a work profile or a private space + // would otherwise have no way to ask for the only target it may need (issue #136). + // The daemon agrees: `ModuleDatabase.setModuleScope` stores a `system` row under + // user 0 whoever asked, and `ConfigCache` maps it to system_server without looking + // at whose module it was. + val withFramework = listOf(systemFrameworkEntry(apps)) + apps + allApps.value = withFramework + + // Asked once per load rather than per row, and held here until the state is built + // at the end of this function — anything written into `_uiState` before then is + // discarded when that fresh ScopeUiState replaces it. A failure to ask means not + // explaining, never explaining wrongly. + val userCount = + withContext(Dispatchers.IO) { daemonClient.getUsers().getOrNull()?.size ?: 1 } + + // A scope the daemon will not hand over shows as none rather than keeping the + // screen shut; [readSavedScope] logs why, and keeps that case apart from the empty + // list a module with nothing ticked legitimately has. + val saved = readSavedScope() ?: emptySet() + savedScope.value = saved + draftScope.value = saved + + val info = + withContext(Dispatchers.IO) { + runCatching { + packageManager.getApplicationInfo( + modulePackageName, + android.content.pm.PackageManager.GET_META_DATA, + ) + } + .onFailure { e -> + logW( + "scope: package info for $modulePackageName (user $userId) " + + "unavailable, no recommended scope", + e, + ) + } + .getOrNull() + } + // One inspection, two answers: what the module asks to hook, and which generation + // of module it is. Both come out of the same pass over the APK, and opening it is + // the expensive part. + val manifest = + info?.let { + withContext(Dispatchers.IO) { + // Guarded like the package info above it, and for the same reason: this + // opens the module's APK, so a package removed or replaced between that + // lookup and this one throws here. Recovering as "no recommended scope" + // costs the reader a few pre-ticked rows; letting it out of a + // `viewModelScope` coroutine takes the whole manager down. + runCatching { ModuleDetection.inspect(it, packageManager) } + .onFailure { e -> + logW( + "scope: reading the manifest of $modulePackageName " + + "failed, no recommended scope", + e, + ) + } + .getOrNull() } - .getOrNull() - } - // One inspection, two answers: what the module asks to hook, and which generation of - // module it is. Both come out of the same pass over the APK, and opening it is the - // expensive part. - val manifest = - info?.let { - withContext(Dispatchers.IO) { ModuleDetection.inspect(it, packageManager) } + } + val recommended = + manifest?.let { RecommendedScope(it.scope, it.staticScope) } + ?: RecommendedScope.NONE + + _uiState.value = + ScopeUiState( + moduleName = + info?.loadLabel(packageManager)?.toString() ?: modulePackageName, + isEnabled = modulePackageName in moduleRepository.enabledModulesState.value, + includeNewApps = + daemonClient.getIncludeNewApps(modulePackageName).getOrDefault(false), + recommended = recommended, + loading = false, + multipleUsers = userCount > 1, + // The manager's own reading of the APK, not the daemon's. The daemon + // settles this while it loads the module and never tells anyone — and it + // only holds an answer for a module that is enabled, which is precisely not + // the state a module is in while its scope is being chosen for the first + // time. + selfHooked = manifest?.isLegacy == true, + ) + } finally { + if (_uiState.value.loading) { + _uiState.value = _uiState.value.copy(loading = false) } - val recommended = - manifest?.let { RecommendedScope(it.scope, it.staticScope) } ?: RecommendedScope.NONE - - _uiState.value = - ScopeUiState( - moduleName = - info?.loadLabel(packageManager)?.toString() ?: modulePackageName, - isEnabled = modulePackageName in moduleRepository.enabledModulesState.value, - includeNewApps = - daemonClient.getIncludeNewApps(modulePackageName).getOrDefault(false), - recommended = recommended, - loading = false, - multipleUsers = userCount > 1, - // The manager's own reading of the APK, not the daemon's. The daemon settles - // this while it loads the module and never tells anyone — and it only holds an - // answer for a module that is enabled, which is precisely not the state a - // module is in while its scope is being chosen for the first time. - selfHooked = manifest?.isLegacy == true, - ) + } } }