commit a07014cd6dd043d34e457ebb8f93ff9fcf61be3a parent 988aa252d2fb9866fc7f397da4ac602860dc707a Author: Matthew Tighe <matthewdtighe@gmail.com> Date: Fri, 3 Oct 2025 17:29:54 +0000 Bug 1989865 - ensure clean process starts for crash recovery flows r=android-reviewers,jonalmeida Differential Revision: https://phabricator.services.mozilla.com/D266880 Diffstat:
10 files changed, 122 insertions(+), 34 deletions(-)
diff --git a/mobile/android/fenix/app/src/main/AndroidManifest.xml b/mobile/android/fenix/app/src/main/AndroidManifest.xml @@ -490,7 +490,7 @@ <activity android:name=".startupCrash.StartupCrashActivity" - android:exported="true" + android:exported="false" > </activity> diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -140,23 +140,40 @@ open class FenixApplication : LocaleAwareApplication(), Provider { var visibilityLifecycleCallback: VisibilityLifecycleCallback? = null private set + /* + * We need to avoid initializing any components while recovering from a startup crash, + * so we use this flag in any Android override hook to ensure that we exit early if we + * are actively recovering. Transitioning between the recovery flow and the normal flow + * does so by killing the active process, so this state should get reset as the startup + * flow completes. See the README in the startup crash package for more context. + */ + private enum class StartupCrashRecoveryState { + NotNeeded, + Recovering, + } + + private var recoveryState = StartupCrashRecoveryState.NotNeeded + override fun onCreate() { super.onCreate() - - initializeWithStartupCrashCheck() + checkForStartupCrash() } /** * Initializes Fenix, unless a startup crash was detected on the previous launch, * in which case returns early to allow for the [HomeActivity] to enter the startup crash - * flow. See [HomeActivity.onCreate] for more context. + * flow. See [HomeActivity.onCreate] or the README in the startup crash package for more context. */ - open fun initializeWithStartupCrashCheck() { - if (StartupCrashCanary.build(applicationContext).startupCrashDetected) { - setupInAllProcesses() - } else { - initialize() + open fun checkForStartupCrash( + canary: StartupCrashCanary = StartupCrashCanary.build(applicationContext), + initialize: () -> Unit = ::initialize, + ) { + if (canary.startupCrashDetected) { + recoveryState = StartupCrashRecoveryState.Recovering + return } + + initialize() } /** @@ -168,7 +185,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider { setupInAllProcesses() - if (!isMainProcess()) { + if (!isMainProcess() || recoveryState == StartupCrashRecoveryState.Recovering) { // If this is not the main process then do not continue with the initialization here. Everything that // follows only needs to be done in our app's main process and should not be done in other processes like // a GeckoView child process or the crash handling process. Most importantly we never want to end up in a @@ -577,6 +594,12 @@ open class FenixApplication : LocaleAwareApplication(), Provider { override fun onTrimMemory(level: Int) { super.onTrimMemory(level) + // Guard against platform hooks initializing components while we are recovering + // from a startup crash. See the README in the startup crash package for more context. + if (recoveryState == StartupCrashRecoveryState.Recovering) { + return + } + // Additional logging and breadcrumb to debug memory issues: // https://github.com/mozilla-mobile/fenix/issues/12731 @@ -1022,6 +1045,12 @@ open class FenixApplication : LocaleAwareApplication(), Provider { } override fun onConfigurationChanged(config: android.content.res.Configuration) { + // Guard against platform hooks initializing components while we are recovering + // from a startup crash. See the README in the startup crash package for more context. + if (recoveryState == StartupCrashRecoveryState.Recovering) { + return + } + // Workaround for androidx appcompat issue where follow system day/night mode config changes // are not triggered when also using createConfigurationContext like we do in LocaleManager // https://issuetracker.google.com/issues/143570309#comment3 diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -15,6 +15,7 @@ import android.content.pm.PackageManager import android.content.res.Configuration import android.os.Build import android.os.Bundle +import android.os.Process import android.os.StrictMode import android.text.format.DateUtils import android.util.AttributeSet @@ -356,9 +357,11 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { applicationContext, StartupCrashActivity::class.java, ) - startupCrashIntent.flags = FLAG_ACTIVITY_NEW_TASK + startupCrashIntent.flags = FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_EXCLUDE_FROM_RECENTS startActivity(startupCrashIntent) - finish() + // We kill the process, because `finish` will cause `onDestroy` to run which would end up + // causing several components to be initialized and potentially cause the startup crash. + Process.killProcess(Process.myPid()) } else { initialize(savedInstanceState) } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/README.md b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/README.md @@ -0,0 +1,17 @@ +# Expected `StartupCrashActivity` behaviour + +Fenix has a challenging problem: it’s possible for crashes to happen early in startup, especially in native components; and we want to collect crash reports for these startup crashes; but we also need user consent to collect said crash reports, necessitating UX to obtain consent and control the process. + +This interacts with the regular startup lifecycle. To accommodate, Fenix writes a “crash canary” early in startup. If "early startup" completes successfully, the crash canary is removed. However, if Fenix crashes during "early startup", the next Fenix launch will witness the crash canary still present from the previous invocation. That’s our cue to display the StartupCrashActivity UX. + +However, we have a challenge. We want the StartupCrashActivity to be as minimal as possible so that it has the best chance to not itself crash. To achieve that, we prevent FenixApplication from entering its normal execution path in order to avoid initializing most components (including Gecko and application-services, the largest native-code components). + +But this is fragile: there exist many execution paths that can initialize these components and systemic protections — for example, a separate Application class that initializes less — are not provided by the Android platform. Therefore we are left carefully avoiding initialization, including in some surprising places, such as Android platform hooks like onTrimMemory. See current usages of FenixApplication.recoveryState. Generally, any overridden `Application` method carries risk of falling into this trap. + +In addition, the Android platform launch process runs FenixApplication.onCreate and Fenix’s LAUNCHER intent, currently HomeActivity. It is not currently easy to avoid launching HomeActivity (deterministically), and HomeActivity can also initialize various native-code components. Therefore we start HomeActivity but then immediately ask the Android platform to relaunch to the StartupCrashActivity, killing our own process (which might be contaminated) immediately. Note that this death must be dirty, i.e., killing ourselves uncleanly is intentional: we avoid graceful teardown which risks HomeActivity::onDestroy instantiating components. + +Finally, the StartupCrashActivity runtime environment is unusual: its FenixApplication has neutered its startup process. For safety and simplicity, launch Fenix from the StartupCrashActivity must again happen in a fresh process, which will proceed (we hope!) through the regular FenixApplication.onCreate code path. + +This diagram may help in visualize these considerations: + + diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/StartupCrashActivity.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/StartupCrashActivity.kt @@ -4,22 +4,18 @@ package org.mozilla.fenix.startupCrash -import android.content.Intent -import android.content.Intent.FLAG_ACTIVITY_NEW_TASK import android.os.Bundle +import android.os.Process import androidx.activity.enableEdgeToEdge import androidx.appcompat.app.AppCompatActivity import androidx.compose.ui.platform.ComposeView import androidx.compose.ui.platform.LocalContext import androidx.core.view.ViewCompat import androidx.core.view.WindowInsetsCompat -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.withContext -import org.mozilla.fenix.FenixApplication -import org.mozilla.fenix.HomeActivity +import mozilla.components.lib.crash.CrashReporter import org.mozilla.fenix.R -import org.mozilla.fenix.components.components import org.mozilla.fenix.crashes.StartupCrashCanary +import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.settings import org.mozilla.fenix.theme.FirefoxTheme @@ -41,8 +37,8 @@ class StartupCrashActivity : AppCompatActivity() { middleware = listOf( StartupCrashMiddleware( settings = LocalContext.current.settings(), - crashReporter = components.analytics.crashReporter, - reinitializeHandler = ::initializeAndRestartFenix, + crashReporter = installCrashReporter(), + restartHandler = ::restartFenix, startupCrashCanaryCache = StartupCrashCanary.build(applicationContext), ), @@ -58,12 +54,15 @@ class StartupCrashActivity : AppCompatActivity() { } } - private suspend fun initializeAndRestartFenix() = withContext(Dispatchers.Main) { - val fenixApplication = applicationContext as FenixApplication - fenixApplication.initialize() - val homeActivityIntent = Intent(applicationContext, HomeActivity::class.java) - homeActivityIntent.flags = FLAG_ACTIVITY_NEW_TASK - applicationContext.startActivity(homeActivityIntent) - finish() + private fun installCrashReporter(): CrashReporter = + components.analytics.crashReporter.also { + it.install(applicationContext) + } + + private fun restartFenix() { + val restartIntent = packageManager.getLaunchIntentForPackage(packageName) + startActivity(restartIntent) + // Kill the existing process to ensure we get a clean start of the application + Process.killProcess(Process.myPid()) } } diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/StartupCrashMiddleware.kt b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/StartupCrashMiddleware.kt @@ -24,7 +24,7 @@ private const val FIVE_DAYS_IN_MILLIS = DateUtils.DAY_IN_MILLIS * 5 internal class StartupCrashMiddleware( settings: Settings, private val crashReporter: CrashReporter, - private val reinitializeHandler: suspend () -> Unit, + private val restartHandler: () -> Unit, private val startupCrashCanaryCache: StartupCrashCanary, private val currentTimeInMillis: () -> TimeInMillis = { System.currentTimeMillis() }, private val cache: CrashReportCache = SettingsCrashReportCache(settings), @@ -45,9 +45,7 @@ internal class StartupCrashMiddleware( startupCrashCanaryCache.clearCanary() } } - ReopenTapped -> scope.launch { - reinitializeHandler() - } + ReopenTapped -> restartHandler() NoTapped -> { scope.launch { cache.setDeferredUntil(currentTimeInMillis() + FIVE_DAYS_IN_MILLIS) diff --git a/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/startup-crash-recovery-flow.png b/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/startupCrash/startup-crash-recovery-flow.png Binary files differ. diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/FenixApplicationTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/FenixApplicationTest.kt @@ -21,6 +21,7 @@ import mozilla.components.feature.addons.migration.DefaultSupportedAddonsChecker import mozilla.components.support.test.robolectric.testContext import mozilla.components.support.utils.BrowsersCache import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue @@ -35,6 +36,7 @@ import org.mozilla.fenix.GleanMetrics.SearchDefaultEngine import org.mozilla.fenix.GleanMetrics.TopSites import org.mozilla.fenix.components.metrics.MozillaProductDetector import org.mozilla.fenix.components.toolbar.ToolbarPosition +import org.mozilla.fenix.crashes.StartupCrashCanary import org.mozilla.fenix.distributions.DefaultDistributionBrowserStoreProvider import org.mozilla.fenix.distributions.DistributionIdManager import org.mozilla.fenix.distributions.DistributionProviderChecker @@ -87,6 +89,45 @@ class FenixApplicationTest { } @Test + fun `GIVEN a startup crash canary THEN initialize is never called`() { + var called = false + val initialize = { called = true } + + val canary = object : StartupCrashCanary { + override val startupCrashDetected: Boolean + get() = true + + override suspend fun clearCanary() { TODO("Not yet implemented") } + + override suspend fun createCanary() { TODO("Not yet implemented") } + } + + val application = FenixApplication() + application.checkForStartupCrash(canary, initialize) + + assertFalse(called) + } + + @Test + fun `GIVEN no startup crash canary THEN initialize is called`() { + var called = false + val initialize = { called = true } + val canary = object : StartupCrashCanary { + override val startupCrashDetected: Boolean + get() = false + + override suspend fun clearCanary() { TODO("Not yet implemented") } + + override suspend fun createCanary() { TODO("Not yet implemented") } + } + + val application = FenixApplication() + application.checkForStartupCrash(canary, initialize) + + assertTrue(called) + } + + @Test fun `GIVEN there are unsupported addons installed WHEN subscribing for new add-ons checks THEN register for checks`() { val checker = mockk<DefaultSupportedAddonsChecker>(relaxed = true) val unSupportedExtension: WebExtension = mockk() diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/helpers/FenixRobolectricTestApplication.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/helpers/FenixRobolectricTestApplication.kt @@ -8,6 +8,7 @@ import io.mockk.mockk import org.mozilla.fenix.FenixApplication import org.mozilla.fenix.R import org.mozilla.fenix.components.Components +import org.mozilla.fenix.crashes.StartupCrashCanary import org.mozilla.fenix.ext.settings import org.mozilla.fenix.perf.runBlockingIncrement @@ -25,7 +26,7 @@ class FenixRobolectricTestApplication : FenixApplication() { override val components = mockk<Components>() - override fun initializeWithStartupCrashCheck() = Unit + override fun checkForStartupCrash(canary: StartupCrashCanary, onCanaryMissing: () -> Unit) = Unit override fun initializeNimbus() = Unit diff --git a/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/startupCrashStore/StartupCrashMiddlewareTest.kt b/mobile/android/fenix/app/src/test/java/org/mozilla/fenix/startupCrashStore/StartupCrashMiddlewareTest.kt @@ -112,7 +112,7 @@ class StartupCrashMiddlewareTest { val middleware = StartupCrashMiddleware( settings = settings, crashReporter = crashReporter, - reinitializeHandler = { called = true }, + restartHandler = { called = true }, startupCrashCanaryCache = canaryRepo, currentTimeInMillis = currentTime, scope = scope,
