From b3ec3062cc13eaa62c254112f08d6c39ac13081a Mon Sep 17 00:00:00 2001 From: James Hugman Date: Tue, 1 Nov 2022 20:42:44 +0000 Subject: [PATCH] Fixes: #26320 Reorganize Nimbus Startup --- .../perf/StartupExcessiveResourceUseTest.kt | 2 +- .../org/mozilla/fenix/FenixApplication.kt | 81 +++++++------- .../mozilla/fenix/experiments/NimbusSetup.kt | 102 +++++++++--------- .../FenixRobolectricTestApplication.kt | 2 + 4 files changed, 91 insertions(+), 96 deletions(-) diff --git a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt index e2aae7321..7e8a10263 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt @@ -33,7 +33,7 @@ import org.mozilla.fenix.helpers.HomeActivityTestRule * * Say no to main thread IO! 🙅 */ -private const val EXPECTED_SUPPRESSION_COUNT = 19 +private const val EXPECTED_SUPPRESSION_COUNT = 18 /** * The number of times we call the `runBlocking` coroutine method on the main thread during this diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt index 1c547ad16..2c15b0633 100644 --- a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt +++ b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -50,7 +50,6 @@ import mozilla.components.service.glean.net.ConceptFetchHttpUploader import mozilla.components.support.base.facts.register import mozilla.components.support.base.log.Log import mozilla.components.support.base.log.logger.Logger -import mozilla.components.support.base.observer.Observable import mozilla.components.support.ktx.android.content.isMainProcess import mozilla.components.support.ktx.android.content.runOnlyInMainProcess import mozilla.components.support.locale.LocaleAwareApplication @@ -59,8 +58,6 @@ import mozilla.components.support.rusthttp.RustHttpConfig import mozilla.components.support.rustlog.RustLog import mozilla.components.support.utils.logElapsedTime import mozilla.components.support.webextensions.WebExtensionSupport -import org.mozilla.experiments.nimbus.NimbusInterface -import org.mozilla.experiments.nimbus.internal.EnrolledExperiment import org.mozilla.fenix.GleanMetrics.Addons import org.mozilla.fenix.GleanMetrics.AndroidAutofill import org.mozilla.fenix.GleanMetrics.CustomizeHome @@ -134,6 +131,10 @@ open class FenixApplication : LocaleAwareApplication(), Provider { return } + // We can initialize Nimbus before Glean because Glean will queue messages + // before it's initialized. + initializeNimbus() + // We need to always initialize Glean and do it early here. initializeGlean() @@ -201,7 +202,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider { run { // Attention: Do not invoke any code from a-s in this scope. - val megazordSetup = setupMegazord() + val megazordSetup = finishSetupMegazord() setDayNightTheme() components.strictMode.enableStrictMode(true) @@ -222,6 +223,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider { } restoreBrowserState() restoreDownloads() + restoreMessaging() // Just to make sure it is impossible for any application-services pieces // to invoke parts of itself that require complete megazord initialization @@ -415,6 +417,15 @@ open class FenixApplication : LocaleAwareApplication(), Provider { .install(this) } + protected open fun initializeNimbus() { + beginSetupMegazord() + + // This lazily constructs the Nimbus object… + val nimbus = components.analytics.experiments + // … which we then can populate the feature configuration. + FxNimbus.initialize { nimbus } + } + /** * Initiate Megazord sequence! Megazord Battle Mode! * @@ -424,54 +435,40 @@ open class FenixApplication : LocaleAwareApplication(), Provider { * Documentation on what megazords are, and why they're needed: * - https://github.com/mozilla/application-services/blob/master/docs/design/megazords.md * - https://mozilla.github.io/application-services/docs/applications/consuming-megazord-libraries.html + * + * This is the initialization of the megazord without setting up networking, i.e. needing the + * engine for networking. This should do the minimum work necessary as it is done on the main + * thread, early in the app startup sequence. */ - @OptIn(DelicateCoroutinesApi::class) // GlobalScope usage - private fun setupMegazord(): Deferred { + private fun beginSetupMegazord() { // Note: Megazord.init() must be called as soon as possible ... Megazord.init() - // Give the generated FxNimbus a closure to lazily get the Nimbus object - FxNimbus.initialize { components.analytics.experiments } + + initializeRustErrors(components.analytics.crashReporter) + // ... but RustHttpConfig.setClient() and RustLog.enable() can be called later. + + // Once application-services has switched to using the new + // error reporting system, RustLog shouldn't input a CrashReporter + // anymore. + // (https://github.com/mozilla/application-services/issues/4981). + RustLog.enable(components.analytics.crashReporter) + } + + @OptIn(DelicateCoroutinesApi::class) // GlobalScope usage + private fun finishSetupMegazord(): Deferred { return GlobalScope.async(Dispatchers.IO) { - initializeRustErrors(components.analytics.crashReporter) - // ... but RustHttpConfig.setClient() and RustLog.enable() can be called later. RustHttpConfig.setClient(lazy { components.core.client }) - // Once application-services has switched to using the new - // error reporting system, RustLog shouldn't input a CrashReporter - // anymore. - // (https://github.com/mozilla/application-services/issues/4981). - RustLog.enable(components.analytics.crashReporter) - // We want to ensure Nimbus is initialized as early as possible so we can - // experiment on features close to startup. - // But we need viaduct (the RustHttp client) to be ready before we do. - components.analytics.experiments.apply { - setupNimbusObserver(this) - } - } - } - private fun setupNimbusObserver(nimbus: Observable) { - nimbus.register( - object : NimbusInterface.Observer { - override fun onUpdatesApplied(updated: List) { - onNimbusStartupAndUpdate() - } - }, - ) + // Now viaduct (the RustHttp client) is initialized we can ask Nimbus to fetch + // experiments recipes from the server. + components.analytics.experiments.fetchExperiments() + } } - private fun onNimbusStartupAndUpdate() { - // When Nimbus has successfully started up, we can apply our engine settings experiment. - // Any previous value that was set on the engine will be overridden from those set in - // Core.Engine.DefaultSettings. - // NOTE ⚠️: Any startup experiment we want to run needs to have it's value re-applied here. - components.core.engine.settings.trackingProtectionPolicy = - components.core.trackingProtectionPolicyFactory.createTrackingProtectionPolicy() - - val settings = settings() - if (settings.isExperimentationEnabled) { + private fun restoreMessaging() { + if (settings().isExperimentationEnabled) { components.appStore.dispatch(AppAction.MessagingAction.Restore) } - reportHomeScreenSectionMetrics(settings) } override fun onTrimMemory(level: Int) { diff --git a/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt b/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt index 72b9cac81..ae212b5fb 100644 --- a/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt +++ b/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt @@ -6,7 +6,6 @@ package org.mozilla.fenix.experiments import android.content.Context import android.net.Uri -import android.os.StrictMode import mozilla.components.service.nimbus.Nimbus import mozilla.components.service.nimbus.NimbusApi import mozilla.components.service.nimbus.NimbusAppInfo @@ -46,6 +45,9 @@ private const val TIME_OUT_LOADING_EXPERIMENT_FROM_DISK_MS = 200L @Suppress("TooGenericExceptionCaught") fun createNimbus(context: Context, url: String?): NimbusApi { + // Once application-services has switched to using the new + // error reporting system, we may not need this anymore. + // https://mozilla-hub.atlassian.net/browse/EXP-2868 val errorReporter: ((String, Throwable) -> Unit) = reporter@{ message, e -> Logger.error("Nimbus error: $message", e) @@ -55,75 +57,69 @@ fun createNimbus(context: Context, url: String?): NimbusApi { context.components.analytics.crashReporter.submitCaughtException(e) } - return try { - // Eventually we'll want to use `NimbusDisabled` when we have no NIMBUS_ENDPOINT. - // but we keep this here to not mix feature flags and how we configure Nimbus. - val serverSettings = if (!url.isNullOrBlank()) { - if (context.settings().nimbusUsePreview) { - NimbusServerSettings(url = Uri.parse(url), collection = "nimbus-preview") - } else { - NimbusServerSettings(url = Uri.parse(url)) - } + // Eventually we'll want to use `NimbusDisabled` when we have no NIMBUS_ENDPOINT. + // but we keep this here to not mix feature flags and how we configure Nimbus. + val serverSettings = if (!url.isNullOrBlank()) { + if (context.settings().nimbusUsePreview) { + NimbusServerSettings(url = Uri.parse(url), collection = "nimbus-preview") } else { - null + NimbusServerSettings(url = Uri.parse(url)) } + } else { + null + } - // Global opt out state is stored in Nimbus, and shouldn't be toggled to `true` - // from the app unless the user does so from a UI control. - // However, the user may have opt-ed out of mako experiments already, so - // we should respect that setting here. - val enabled = - context.components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) { - context.settings().isExperimentationEnabled - } + val isFirstNimbusRun = context.settings().isFirstNimbusRun + if (isFirstNimbusRun) { + context.settings().isFirstNimbusRun = false + } - // The name "fenix" here corresponds to the app_name defined for the family of apps - // that encompasses all of the channels for the Fenix app. This is defined upstream in - // the telemetry system. For more context on where the app_name come from see: - // https://probeinfo.telemetry.mozilla.org/v2/glean/app-listings - // and - // https://github.com/mozilla/probe-scraper/blob/master/repositories.yaml - val appInfo = NimbusAppInfo( - appName = "fenix", - // Note: Using BuildConfig.BUILD_TYPE is important here so that it matches the value - // passed into Glean. `Config.channel.toString()` turned out to be non-deterministic - // and would mostly produce the value `Beta` and rarely would produce `beta`. - channel = BuildConfig.BUILD_TYPE, - customTargetingAttributes = mapOf( - "isFirstRun" to context.settings().isFirstNimbusRun.toString(), - ), - ) + // The name "fenix" here corresponds to the app_name defined for the family of apps + // that encompasses all of the channels for the Fenix app. This is defined upstream in + // the telemetry system. For more context on where the app_name come from see: + // https://probeinfo.telemetry.mozilla.org/v2/glean/app-listings + // and + // https://github.com/mozilla/probe-scraper/blob/master/repositories.yaml + val appInfo = NimbusAppInfo( + appName = "fenix", + // Note: Using BuildConfig.BUILD_TYPE is important here so that it matches the value + // passed into Glean. `Config.channel.toString()` turned out to be non-deterministic + // and would mostly produce the value `Beta` and rarely would produce `beta`. + channel = BuildConfig.BUILD_TYPE.let { if (it == "debug") "developer" else it }, + customTargetingAttributes = mapOf( + "isFirstRun" to isFirstNimbusRun.toString(), + ), + ) + return try { Nimbus(context, appInfo, serverSettings, errorReporter).apply { // We register our own internal observer for housekeeping the Nimbus SDK and // generated code. register(observer) - val isFirstNimbusRun = context.settings().isFirstNimbusRun + // Apply any experiment recipes we downloaded last time, or + // if this is the first time, we load the ones bundled in the res/raw + // directory. + val job = if (isFirstNimbusRun || url.isNullOrBlank()) { + applyLocalExperiments(R.raw.initial_experiments) + } else { + applyPendingExperiments() + } - // We always want `Nimbus.initialize` to happen ASAP and before any features (engine/UI) + // We always want initialize Nimbus to happen ASAP and before any features (engine/UI) // have been initialized. For that reason, we use runBlocking here to avoid // inconsistency in the experiments. - // We can safely do this because Nimbus does most of it's work on background threads, - // except for loading the initial experiments from disk. For this reason, we have a + // We can safely do this because Nimbus does most of its work on background threads, + // including the loading the initial experiments from disk. For this reason, we have a // `joinOrTimeout` to limit the blocking until TIME_OUT_LOADING_EXPERIMENT_FROM_DISK_MS. runBlockingIncrement { - val job = initialize( - isFirstNimbusRun || url.isNullOrBlank(), - R.raw.initial_experiments, - ) // We only read from disk when loading first-run experiments. This is the only time // that we should join and block. Otherwise, we don't want to wait. - if (isFirstNimbusRun) { - context.settings().isFirstNimbusRun = false - job.joinOrTimeout(TIME_OUT_LOADING_EXPERIMENT_FROM_DISK_MS) - } - } - - if (!enabled) { - // This opts out of nimbus experiments. It involves writing to disk, so does its - // work on the db thread. - globalUserParticipation = enabled + job.joinOrTimeout(TIME_OUT_LOADING_EXPERIMENT_FROM_DISK_MS) } + // By now, on this thread, we have a fully initialized Nimbus object, ready for use: + // * we gave a 200ms timeout to the loading of a file from res/raw + // * on completion or cancellation, applyPendingExperiments or initialize was + // called, and this thread waited for that to complete. } } catch (e: Throwable) { // Something went wrong. We'd like not to, but stability of the app is more important than diff --git a/app/src/test/java/org/mozilla/fenix/helpers/FenixRobolectricTestApplication.kt b/app/src/test/java/org/mozilla/fenix/helpers/FenixRobolectricTestApplication.kt index 72cc5ace7..fa16108be 100644 --- a/app/src/test/java/org/mozilla/fenix/helpers/FenixRobolectricTestApplication.kt +++ b/app/src/test/java/org/mozilla/fenix/helpers/FenixRobolectricTestApplication.kt @@ -23,6 +23,8 @@ class FenixRobolectricTestApplication : FenixApplication() { override val components = mockk() + override fun initializeNimbus() = Unit + override fun initializeGlean() = Unit override fun setupInAllProcesses() = Unit