From e7cb53f0b85a9fb872e79de316888eb8948f1485 Mon Sep 17 00:00:00 2001 From: darken Date: Tue, 4 Aug 2026 20:23:50 +0200 Subject: [PATCH] fix: Harden the error dialog's Google Play fix action The dialog's fix dispatch ran unguarded: a throwing action crashed the UI thread from inside a click handler and skipped onDismiss(), leaving the dialog latched on the current error. The dispatch is now wrapped in try/catch with onDismiss() in a finally block. Google Play fix action: - Drop FLAG_ACTIVITY_NEW_TASK. The action runs on an activity context, so the flag only detached Play's app info from the caller's task and back stack. - Catch SecurityException next to ActivityNotFoundException: Play can be installed but blocked (disabled app, restricted profile, guarding ROM), which denies the launch instead of failing to resolve it. - The fallback toast is now a translatable string resource instead of a hardcoded literal. New coverage: ComposeErrorDialogGuardTest pins that a throwing fix action still dismisses the dialog (shared source set, so both flavors run it), GplayFixActionTest pins the denied and unresolvable launches showing a toast instead of crashing, and ComposeErrorDialogTest now asserts the launch intent carries no NEW_TASK flag. --- .../GplayServiceUnavailableException.kt | 24 ++++-- app/src/gplay/res/values/strings.xml | 2 + .../capod/common/error/ErrorEventHandler.kt | 18 ++++- .../error/ComposeErrorDialogGuardTest.kt | 73 +++++++++++++++++++ .../common/error/ComposeErrorDialogTest.kt | 4 + .../core/billing/GplayFixActionTest.kt | 57 +++++++++++++++ 6 files changed, 168 insertions(+), 10 deletions(-) create mode 100644 app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt create mode 100644 app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt diff --git a/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt b/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt index 2997ae6a..7fb93c96 100644 --- a/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt +++ b/app/src/gplay/java/eu/darken/capod/common/upgrade/core/billing/GplayServiceUnavailableException.kt @@ -1,5 +1,6 @@ package eu.darken.capod.common.upgrade.core.billing +import android.app.Activity import android.content.ActivityNotFoundException import android.content.Context import android.content.Intent @@ -25,21 +26,28 @@ class GplayServiceUnavailableException(cause: Throwable) : // action is a GENERIC troubleshooting affordance (open Play's app info), not a diagnosis of // the cause. Harmless for a transient blip, and it matches the fleet's dialog. fixAction = { activity -> - try { - val intent = Intent().apply { - action = Settings.ACTION_APPLICATION_DETAILS_SETTINGS - data = Uri.fromParts("package", GPLAY_PKG, null) - addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) - } + val intent = Intent().apply { + action = Settings.ACTION_APPLICATION_DETAILS_SETTINGS + data = Uri.fromParts("package", GPLAY_PKG, null) + } + try { activity.startActivity(intent) } catch (e: ActivityNotFoundException) { - log(ERROR) { "Can't launch settings intent for Google Play: $e" } - Toast.makeText(activity, "Google Play is not installed", Toast.LENGTH_SHORT).show() + onLaunchFailed(activity, e) + } catch (e: SecurityException) { + // Play can be installed but unreachable: disabled app, work/restricted profile or a + // ROM that guards the settings screen. The launch is denied, not unresolvable. + onLaunchFailed(activity, e) } }, ) + private fun onLaunchFailed(activity: Activity, e: Exception) { + log(ERROR) { "Can't launch settings intent for Google Play: $e" } + Toast.makeText(activity, R.string.upgrades_gplay_not_installed_message, Toast.LENGTH_SHORT).show() + } + companion object { private const val GPLAY_PKG = "com.android.vending" } diff --git a/app/src/gplay/res/values/strings.xml b/app/src/gplay/res/values/strings.xml index 52494098..c45bb9ad 100644 --- a/app/src/gplay/res/values/strings.xml +++ b/app/src/gplay/res/values/strings.xml @@ -69,6 +69,8 @@ CAPod Pro Your upgrade and purchase status. CAPod can\'t connect to Google Play. Is Google Play installed and up to date? Is your Google Account logged in? Try clearing the cache of the Google Play app and rebooting your device. + + Google Play is not installed. Google Play error An internal Google Play error occurred. Please try the following:\n\n• Restart your device\n• Clear Google Play Store cache\n• Try again later Connection error diff --git a/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt b/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt index 54906c90..6bcd993a 100644 --- a/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt +++ b/app/src/main/java/eu/darken/capod/common/error/ErrorEventHandler.kt @@ -13,6 +13,10 @@ import androidx.compose.runtime.setValue import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.res.stringResource import eu.darken.capod.R +import eu.darken.capod.common.debug.logging.Logging.Priority.ERROR +import eu.darken.capod.common.debug.logging.asLog +import eu.darken.capod.common.debug.logging.log +import eu.darken.capod.common.debug.logging.logTag @Composable fun ErrorEventHandler(source: ErrorEventSource2) { @@ -52,8 +56,16 @@ private fun ComposeErrorDialog( if (hasFix) { TextButton( onClick = { - localizedError.fixAction!!.invoke(activity!!) - onDismiss() + // Error actions are arbitrary code (intent launches): a throw here would + // crash the UI thread from inside a click handler, and skipping onDismiss() + // would leave the dialog latched on the current error with no way out. + try { + localizedError.fixAction!!.invoke(activity!!) + } catch (e: Exception) { + log(TAG, ERROR) { "Error action failed: ${e.asLog()}" } + } finally { + onDismiss() + } }, ) { Text(text = localizedError.fixActionLabel ?: stringResource(android.R.string.ok)) @@ -75,3 +87,5 @@ private fun ComposeErrorDialog( }, ) } + +private val TAG = logTag("Error", "Dialog", "Compose") diff --git a/app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt b/app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt new file mode 100644 index 00000000..de06b755 --- /dev/null +++ b/app/src/test/java/eu/darken/capod/common/error/ComposeErrorDialogGuardTest.kt @@ -0,0 +1,73 @@ +package eu.darken.capod.common.error + +import android.app.Activity +import android.content.Context +import androidx.compose.ui.test.assertCountEquals +import androidx.compose.ui.test.onAllNodesWithText +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import eu.darken.capod.common.compose.PreviewWrapper +import eu.darken.capod.common.flow.SingleEventFlow +import io.kotest.matchers.shouldBe +import org.junit.Test +import testhelpers.compose.BaseComposeRobolectricTest + +/** + * The shared error dialog dispatches arbitrary fix actions: one that blows up must never take the + * UI — or the dialog's exit — down with it. Flavor-independent, so it runs on both legs. + */ +class ComposeErrorDialogGuardTest : BaseComposeRobolectricTest() { + + private class FakeErrorSource : ErrorEventSource2 { + override val errorEvents = SingleEventFlow() + } + + private class TestError( + private val fixAction: (Activity) -> Unit, + ) : Exception(ERROR_BODY), HasLocalizedError { + override fun getLocalizedError(context: Context): LocalizedError = LocalizedError( + throwable = this, + label = ERROR_TITLE, + description = ERROR_BODY, + fixActionLabel = FIX_LABEL, + fixAction = fixAction, + ) + } + + private fun showError(error: Throwable) { + val source = FakeErrorSource() + composeRule.setContent { + PreviewWrapper { + ErrorEventHandler(source) + } + } + // Buffered channel: the event survives until the handler's collector attaches. + source.errorEvents.tryEmit(error) + composeRule.waitForIdle() + } + + @Test + fun `a throwing fix action still dismisses the dialog`() { + var invoked = false + showError( + TestError { + // Flag first: the assertion below has to distinguish "action ran and threw" from + // "action was never dispatched". + invoked = true + throw IllegalStateException("fix action exploded") + } + ) + + composeRule.onNodeWithText(FIX_LABEL).performClick() + composeRule.waitForIdle() + + invoked shouldBe true + // The handler latches on the current error: the dialog only goes away if the throw was + // caught and onDismiss still ran. + composeRule.onAllNodesWithText(FIX_LABEL).assertCountEquals(0) + } +} + +private const val ERROR_TITLE = "Test error title" +private const val ERROR_BODY = "Test error description" +private const val FIX_LABEL = "Fix it" diff --git a/app/src/testGplay/java/eu/darken/capod/common/error/ComposeErrorDialogTest.kt b/app/src/testGplay/java/eu/darken/capod/common/error/ComposeErrorDialogTest.kt index 75e4e23a..7ac7d2d1 100644 --- a/app/src/testGplay/java/eu/darken/capod/common/error/ComposeErrorDialogTest.kt +++ b/app/src/testGplay/java/eu/darken/capod/common/error/ComposeErrorDialogTest.kt @@ -1,6 +1,7 @@ package eu.darken.capod.common.error import android.content.Context +import android.content.Intent import android.provider.Settings import androidx.activity.ComponentActivity import androidx.compose.ui.test.assertCountEquals @@ -76,6 +77,9 @@ class ComposeErrorDialogTest : BaseTest() { val started = shadowOf(composeRule.activity).nextStartedActivity.shouldNotBeNull() started.action shouldBe Settings.ACTION_APPLICATION_DETAILS_SETTINGS started.data.toString() shouldBe "package:com.android.vending" + // NEW_TASK on an activity context detaches Play's app info from our task: the user loses the + // back path to the screen they came from and the settings screen lingers in recents. + (started.flags and Intent.FLAG_ACTIVITY_NEW_TASK) shouldBe 0 // The user acted: leaving the dialog up would greet them again on the way back. composeRule.onAllNodesWithText("Google Play").assertCountEquals(0) } diff --git a/app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt b/app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt new file mode 100644 index 00000000..c08080fc --- /dev/null +++ b/app/src/testGplay/java/eu/darken/capod/common/upgrade/core/billing/GplayFixActionTest.kt @@ -0,0 +1,57 @@ +package eu.darken.capod.common.upgrade.core.billing + +import android.app.Activity +import android.content.ActivityNotFoundException +import android.content.Intent +import eu.darken.capod.R +import io.kotest.matchers.nulls.shouldNotBeNull +import io.kotest.matchers.shouldBe +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.robolectric.shadows.ShadowToast +import testhelpers.BaseTest +import testhelpers.TestApplication + +/** + * The error dialog's "Google Play" button runs on an activity context: a device where the launch is + * refused must get a toast, not a crash. The successful launch is covered by ComposeErrorDialogTest. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33], application = TestApplication::class) +class GplayFixActionTest : BaseTest() { + + /** Play is installed but unreachable: disabled app, restricted profile or a guarding ROM. */ + class DeniedLaunchActivity : Activity() { + override fun startActivity(intent: Intent): Unit = throw SecurityException("Permission Denial") + } + + /** Play isn't on the device at all, so nothing resolves the app info intent. */ + class MissingPlayActivity : Activity() { + override fun startActivity(intent: Intent): Unit = throw ActivityNotFoundException("No Activity found") + } + + private fun activityOf(clazz: Class): T = Robolectric.buildActivity(clazz).setup().get() + + private fun assertToastInsteadOfCrash(activity: Activity) { + val fixAction = GplayServiceUnavailableException(RuntimeException("Play hiccup")) + .getLocalizedError(activity).fixAction.shouldNotBeNull() + + fixAction.invoke(activity) + + val expected = activity.getString(R.string.upgrades_gplay_not_installed_message) + ShadowToast.getTextOfLatestToast() shouldBe expected + } + + @Test + fun `a denied launch shows the not-installed toast instead of crashing`() { + assertToastInsteadOfCrash(activityOf(DeniedLaunchActivity::class.java)) + } + + @Test + fun `an unresolvable launch shows the not-installed toast instead of crashing`() { + assertToastInsteadOfCrash(activityOf(MissingPlayActivity::class.java)) + } +}