diff --git a/app/src/androidTest/java/com/ninelivesaudio/app/ui/navigation/BackGestureRaceTest.kt b/app/src/androidTest/java/com/ninelivesaudio/app/ui/navigation/BackGestureRaceTest.kt new file mode 100644 index 00000000..be1ef1bd --- /dev/null +++ b/app/src/androidTest/java/com/ninelivesaudio/app/ui/navigation/BackGestureRaceTest.kt @@ -0,0 +1,214 @@ +package com.ninelivesaudio.app.ui.navigation + +import androidx.activity.BackEventCompat +import androidx.activity.ComponentActivity +import androidx.activity.OnBackPressedDispatcher +import androidx.activity.compose.LocalOnBackPressedDispatcherOwner +import androidx.activity.compose.setContent +import androidx.compose.material3.Text +import androidx.lifecycle.Lifecycle +import androidx.navigation.NavBackStackEntry +import androidx.navigation.NavHostController +import androidx.navigation.compose.NavHost +import androidx.navigation.compose.composable +import androidx.navigation.compose.rememberNavController +import androidx.test.core.app.ActivityScenario +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import java.util.concurrent.atomic.AtomicReference + +/** + * Issue #104. A back gesture or Back key lands, and before the navigation + * host gets to act on it the app itself changes the back stack (Play + * finishing its load and opening the Player, a toolbar back arrow, a tab). + * The host then tried to animate the page that was already gone and threw + * "Cannot transition entry that is not in the back stack", which killed + * the app. + * + * These run on a real activity with the real main-thread dispatcher, because + * the crash lives in the gap between two main-thread messages. + */ +@RunWith(AndroidJUnit4::class) +class BackGestureRaceTest { + + private val failure = AtomicReference(null) + private var previousHandler: Thread.UncaughtExceptionHandler? = null + private lateinit var scenario: ActivityScenario + private lateinit var nav: NavHostController + private lateinit var dispatcher: OnBackPressedDispatcher + + @Before fun setUp() { + previousHandler = Thread.getDefaultUncaughtExceptionHandler() + Thread.setDefaultUncaughtExceptionHandler { _, e -> failure.compareAndSet(null, e) } + scenario = ActivityScenario.launch(ComponentActivity::class.java) + scenario.onActivity { activity -> + activity.setContent { + nav = rememberNavController() + dispatcher = LocalOnBackPressedDispatcherOwner.current!!.onBackPressedDispatcher + // The same pairing NineLivesNavHost uses. + CancelPendingBackOnNavigation(nav) + NavHost(nav, startDestination = Routes.HOME) { + composable(Routes.HOME) { Text("home") } + composable(Routes.LIBRARY) { Text("library") } + composable(Routes.PLAYER) { Text("player") } + composable(Routes.BOOK_DETAIL) { Text("detail") } + } + } + } + settle() + // Library, then a book, the way a person gets to a book page. + onMain { nav.navigateToTab(Routes.LIBRARY) } + settle() + onMain { nav.navigate(Routes.bookDetail("book")) } + settle() + onMain { assertEquals(Routes.BOOK_DETAIL, nav.currentDestination?.route) } + } + + @After fun tearDown() { + scenario.close() + Thread.setDefaultUncaughtExceptionHandler(previousHandler) + } + + @Test fun backKeyThenTheAppOpensThePlayer() { + onMain { + dispatcher.onBackPressed() + // Play finished loading and opened the Player before the host + // got to the Back. + nav.navigateToTab(Routes.PLAYER) + } + settle() + assertNoCrash() + onMain { assertEquals(Routes.PLAYER, nav.currentDestination?.route) } + } + + @Test fun backKeyThenTheToolbarBackArrow() { + onMain { + dispatcher.onBackPressed() + nav.popBackStack() + } + settle() + assertNoCrash() + } + + @Test fun backSwipeStartsThenTheAppLeavesThePage() { + onMain { + dispatcher.dispatchOnBackStarted(BackEventCompat(5f, 900f, 0f, BackEventCompat.EDGE_LEFT)) + nav.popBackStack() + } + settle() + onMain { + dispatcher.dispatchOnBackProgressed(BackEventCompat(300f, 900f, 0.5f, BackEventCompat.EDGE_LEFT)) + dispatcher.onBackPressed() + } + settle() + assertNoCrash() + // The app's own pop already left the book page and cancelled the + // swipe. Lifting the finger still counts as a Back from the Library, + // so the user lands on Home. + assertSettledOn(Routes.HOME) + } + + @Test fun backSwipeInProgressThenTheAppLeavesThePage() { + onMain { dispatcher.dispatchOnBackStarted(BackEventCompat(5f, 900f, 0f, BackEventCompat.EDGE_LEFT)) } + settle() + onMain { dispatcher.dispatchOnBackProgressed(BackEventCompat(200f, 900f, 0.3f, BackEventCompat.EDGE_LEFT)) } + settle() + // The host is mid-swipe on the book page when the app pops it. + onMain { nav.popBackStack() } + settle() + onMain { + dispatcher.dispatchOnBackProgressed(BackEventCompat(300f, 900f, 0.5f, BackEventCompat.EDGE_LEFT)) + dispatcher.onBackPressed() + } + settle() + assertNoCrash() + assertSettledOn(Routes.HOME) + } + + @Test fun aPlainBackStillGoesToTheLibrary() { + val detail = currentEntry() + onMain { dispatcher.onBackPressed() } + settle() + assertNoCrash() + assertSettledOn(Routes.LIBRARY) + assertGone(detail) + } + + @Test fun aPlainBackSwipeStillGoesToTheLibrary() { + onMain { dispatcher.dispatchOnBackStarted(BackEventCompat(5f, 900f, 0f, BackEventCompat.EDGE_LEFT)) } + settle() + onMain { dispatcher.dispatchOnBackProgressed(BackEventCompat(300f, 900f, 0.5f, BackEventCompat.EDGE_LEFT)) } + settle() + val detail = currentEntry() + onMain { dispatcher.onBackPressed() } + settle() + assertNoCrash() + assertSettledOn(Routes.LIBRARY) + assertGone(detail) + } + + @Test fun twoQuickBacksGoToTheLibraryThenHome() { + onMain { dispatcher.onBackPressed() } + onMain { dispatcher.onBackPressed() } + settle() + assertNoCrash() + onMain { + val route = nav.currentDestination?.route + check(route == Routes.LIBRARY || route == Routes.HOME) { "ended on $route" } + } + } + + private fun assertNoCrash() { + failure.get()?.let { throw AssertionError("app crashed", it) } + } + + private fun currentEntry(): NavBackStackEntry { + var entry: NavBackStackEntry? = null + onMain { entry = nav.currentBackStackEntry } + return entry!! + } + + /** + * On [route], with the page animation finished: the page is RESUMED and + * it is the only page the host still draws. + */ + private fun assertSettledOn(route: String) { + waitUntil("$route never settled") { + nav.currentDestination?.route == route && + nav.currentBackStackEntry!!.lifecycle.currentState == Lifecycle.State.RESUMED && + nav.visibleEntries.value.count { it.destination.navigatorName == "composable" } == 1 + } + onMain { assertEquals(route, nav.currentDestination?.route) } + } + + /** The popped page finished its exit and was torn down. */ + private fun assertGone(entry: NavBackStackEntry) { + waitUntil("${entry.destination.route} was never destroyed") { + entry.lifecycle.currentState == Lifecycle.State.DESTROYED + } + } + + private fun waitUntil(message: String, condition: () -> Boolean) { + val deadline = System.currentTimeMillis() + 5_000 + while (true) { + var met = false + onMain { met = condition() } + if (met) return + if (System.currentTimeMillis() > deadline) throw AssertionError(message) + Thread.sleep(50) + } + } + + private fun onMain(block: () -> Unit) = scenario.onActivity { block() } + + private fun settle() { + InstrumentationRegistry.getInstrumentation().waitForIdleSync() + Thread.sleep(600) + InstrumentationRegistry.getInstrumentation().waitForIdleSync() + } +} diff --git a/app/src/main/java/com/ninelivesaudio/app/AppProcess.kt b/app/src/main/java/com/ninelivesaudio/app/AppProcess.kt new file mode 100644 index 00000000..363a1c0c --- /dev/null +++ b/app/src/main/java/com/ninelivesaudio/app/AppProcess.kt @@ -0,0 +1,11 @@ +package com.ninelivesaudio.app + +/** + * True when [processName] is the app's main process, the one named after the + * package. Side processes such as the crash dialog's ":acra" get a suffix. + * + * An unknown name counts as main, because an app that starts nothing is worse + * than one that starts everything. + */ +internal fun isMainAppProcess(processName: String?, packageName: String): Boolean = + processName == null || processName == packageName diff --git a/app/src/main/java/com/ninelivesaudio/app/NineLivesApp.kt b/app/src/main/java/com/ninelivesaudio/app/NineLivesApp.kt index 80b6ab92..230c4e89 100644 --- a/app/src/main/java/com/ninelivesaudio/app/NineLivesApp.kt +++ b/app/src/main/java/com/ninelivesaudio/app/NineLivesApp.kt @@ -34,45 +34,57 @@ import org.acra.config.mailSender import org.acra.data.StringFormat import org.acra.ktx.initAcra import com.ninelivesaudio.app.widget.WidgetRefresher +import dagger.Lazy import javax.inject.Inject @HiltAndroidApp class NineLivesApp : Application(), ImageLoaderFactory { @Inject - lateinit var connectivityMonitor: ConnectivityMonitor + lateinit var connectivityMonitorLazy: Lazy + val connectivityMonitor: ConnectivityMonitor get() = connectivityMonitorLazy.get() @Inject - lateinit var syncManager: SyncManager + lateinit var syncManagerLazy: Lazy + val syncManager: SyncManager get() = syncManagerLazy.get() @Inject - lateinit var settingsManager: SettingsManager + lateinit var settingsManagerLazy: Lazy + val settingsManager: SettingsManager get() = settingsManagerLazy.get() @Inject - lateinit var libraryRepository: LibraryRepository + lateinit var libraryRepositoryLazy: Lazy + val libraryRepository: LibraryRepository get() = libraryRepositoryLazy.get() @Inject - lateinit var apiService: ApiService + lateinit var apiServiceLazy: Lazy + val apiService: ApiService get() = apiServiceLazy.get() @Inject - lateinit var billingManager: BillingManager + lateinit var billingManagerLazy: Lazy + val billingManager: BillingManager get() = billingManagerLazy.get() @Inject - lateinit var downloadManager: DownloadManager + lateinit var downloadManagerLazy: Lazy + val downloadManager: DownloadManager get() = downloadManagerLazy.get() @Inject - lateinit var playbackManager: PlaybackManager + lateinit var playbackManagerLazy: Lazy + val playbackManager: PlaybackManager get() = playbackManagerLazy.get() @Inject - lateinit var entitlementRepository: EntitlementRepository + lateinit var entitlementRepositoryLazy: Lazy + val entitlementRepository: EntitlementRepository get() = entitlementRepositoryLazy.get() // The app's OkHttpClient carries the auth token, self-signed cert config, and // dynamic base URL, so cover requests authenticate like every other call. @Inject - lateinit var okHttpClient: OkHttpClient + lateinit var okHttpClientLazy: Lazy + val okHttpClient: OkHttpClient get() = okHttpClientLazy.get() @Inject - lateinit var widgetRefresher: WidgetRefresher + lateinit var widgetRefresherLazy: Lazy + val widgetRefresher: WidgetRefresher get() = widgetRefresherLazy.get() private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.Main) @@ -118,6 +130,14 @@ class NineLivesApp : Application(), ImageLoaderFactory { override fun onCreate() { super.onCreate() + // Android runs onCreate in every process the app has, including the + // crash dialog's own ":acra" process. Everything below belongs to the + // main process only. WorkManager, for one, is never set up in the + // crash process, so the widget refresher scheduling work there threw + // and took the crash dialog down with it. The dependencies above are + // injected lazily, so the crash process does not even build them. + if (!isMainAppProcess(getProcessName(), packageName)) return + // No grandfather writer here, deliberately. The unconditional latch that // shipped in 2.0.2 was deleted for the free build. With PAID_ERA_CUTOFF // gone, `legacy_paid` is the only grandfather signal, so any writer in @@ -220,8 +240,10 @@ class NineLivesApp : Application(), ImageLoaderFactory { } override fun onTerminate() { - connectivityMonitor.stopMonitoring() - syncManager.stop() + if (isMainAppProcess(getProcessName(), packageName)) { + connectivityMonitor.stopMonitoring() + syncManager.stop() + } super.onTerminate() } diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/navigation/BackRaceGuard.kt b/app/src/main/java/com/ninelivesaudio/app/ui/navigation/BackRaceGuard.kt new file mode 100644 index 00000000..d022214b --- /dev/null +++ b/app/src/main/java/com/ninelivesaudio/app/ui/navigation/BackRaceGuard.kt @@ -0,0 +1,51 @@ +package com.ninelivesaudio.app.ui.navigation + +import android.annotation.SuppressLint +import androidx.activity.compose.LocalOnBackPressedDispatcherOwner +import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect +import androidx.navigation.NavController + +/** + * Drops a Back that the navigation host has not acted on yet whenever the + * back stack changes under it (#104). + * + * The host in navigation-compose 2.8 does not handle Back on the spot. It + * queues the work for the next turn of the main thread and, when that runs, + * animates the page it last drew on top. If the app changes the stack in the + * gap (Play finishing its load and opening the Player, the toolbar back + * arrow, a tab tap) that page is already gone, and the host throws + * "Cannot transition entry that is not in the back stack", which kills the + * app. Navigation 2.10 reworked this, but it needs a newer build toolchain + * than this project uses. + * + * Every change to the stack, from any caller, reaches this listener before + * the queued work runs, so cancelling here means the stale Back never runs. + * The page change that cancelled it already moved the user, so nothing they + * asked for is lost. A Back still in progress when the user lifts their + * finger acts on whatever page is now on top. + * + * The listener also fires once when it is registered (Navigation reports the + * current page to every new listener), and again after a configuration change + * re-registers it. Nothing is pending at those moments, so the cancel does + * nothing. That is intended, not a stray Back being dropped. + * + * It leans on two behaviors of activity-compose 1.9 and navigation-compose + * 2.8: the dispatcher's test-only cancel reaching the host's back handler, + * and that cancel stopping the queued work. BackGestureRaceTest is the gate + * for any activity or navigation version bump. + * + * Call it next to the NavHost, with the same controller. + */ +@SuppressLint("VisibleForTests") // the dispatcher's only way to cancel a Back +@Composable +fun CancelPendingBackOnNavigation(navController: NavController) { + val dispatcher = LocalOnBackPressedDispatcherOwner.current?.onBackPressedDispatcher ?: return + DisposableEffect(navController, dispatcher) { + val listener = NavController.OnDestinationChangedListener { _, _, _ -> + dispatcher.dispatchOnBackCancelled() + } + navController.addOnDestinationChangedListener(listener) + onDispose { navController.removeOnDestinationChangedListener(listener) } + } +} diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/navigation/NineLivesNavHost.kt b/app/src/main/java/com/ninelivesaudio/app/ui/navigation/NineLivesNavHost.kt index eaf44400..25827677 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/navigation/NineLivesNavHost.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/navigation/NineLivesNavHost.kt @@ -52,6 +52,7 @@ fun NineLivesNavHost( modifier: Modifier = Modifier, startDestination: String = Routes.HOME, ) { + CancelPendingBackOnNavigation(navController) NavHost( navController = navController, startDestination = startDestination, diff --git a/app/src/test/java/com/ninelivesaudio/app/AppProcessTest.kt b/app/src/test/java/com/ninelivesaudio/app/AppProcessTest.kt new file mode 100644 index 00000000..1c5795ac --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/AppProcessTest.kt @@ -0,0 +1,38 @@ +package com.ninelivesaudio.app + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The crash dialog runs in its own ":acra" process, and Android runs + * Application.onCreate there too. Widgets, WorkManager, billing, sync and + * playback only exist in the app's main process, and starting them in the + * crash process crashed the dialog that was meant to report the first crash. + */ +class AppProcessTest { + + private val pkg = "com.ninelivesaudio.app" + + @Test fun `the app's own process is the main process`() { + assertTrue(isMainAppProcess(processName = pkg, packageName = pkg)) + } + + @Test fun `the debug build's own process is the main process`() { + assertTrue(isMainAppProcess(processName = "$pkg.debug", packageName = "$pkg.debug")) + } + + @Test fun `the crash dialog process is not`() { + assertFalse(isMainAppProcess(processName = "$pkg:acra", packageName = pkg)) + assertFalse(isMainAppProcess(processName = "$pkg.debug:acra", packageName = "$pkg.debug")) + } + + @Test fun `no other side process is either`() { + assertFalse(isMainAppProcess(processName = "$pkg:remote", packageName = pkg)) + } + + @Test fun `an unknown process name keeps the app working`() { + // Better to start everything than to boot an app with nothing running. + assertTrue(isMainAppProcess(processName = null, packageName = pkg)) + } +} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 87717b77..d4f80722 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -3,8 +3,13 @@ agp = "9.0.0" kotlin = "2.2.10" coreKtx = "1.15.0" lifecycleRuntimeKtx = "2.8.7" +# Bumping activityCompose or navigationCompose? Run BackGestureRaceTest on a +# device first. CancelPendingBackOnNavigation (#104) relies on the test-only +# OnBackPressedDispatcher.dispatchOnBackCancelled reaching NavHost's back +# handler, and a new version can quietly change that. activityCompose = "1.9.3" composeBom = "2025.06.00" +# Same gate as activityCompose above: BackGestureRaceTest. navigationCompose = "2.8.5" hilt = "2.59.1" hiltNavigationCompose = "1.2.0"