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) } /** 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, - ) + } } }