From 85578e498c4d8243e44904e47cfc9d2615846213 Mon Sep 17 00:00:00 2001 From: Jonathan Almeida Date: Wed, 3 Aug 2022 21:01:15 -0400 Subject: [PATCH] [fenix] Close https://github.com/mozilla-mobile/fenix/issues/26041: Re-set TrackingProtectionPolicy after Nimbus SDK is initialized There are three issues here that we have uncovered while investigating this bug: 1. Settings.kt has a lazy block around `enabledTotalCookieProtection` which ends up caching the first result it evaluates. 3. The `FeatureHolder` within FxNimbus caches the incorrectly evaluated value and returns this value hence forth. 4. Nimbus is not ready to return a result for an engine experiment when we need it early on in the dependency tree initialization. There are multiple systems that require engine to be initialized for them to work (e.g. Glean, Profiler, concept-fetch). In our TCP, experiment, we need to apply these engine settings during the engine initialization. So when we try and evaluate Nimbus that early on, it has not had time to initialize itself correctly or even use the engine's concept-fetch client to return the correct experiment result. This bug is made worse because of the first two caching bugs where we are always holding onto a cached value of the wrong result. Our temporary solution is to: 1. Remove the `lazy` around `Settings.enabledTotalCookieProtection`. 2. Set the `FxNimbus.api` value right after we are done initializing `FxNimbus` and `NimbusApi` so that all future queries to FxNimbus will be made against a real instance of `NimbusApi`. This is a short-term fix for the `FeatureHolder` caching bug. 3. Set a new TrackingProtectionPolicy that will evaluate Nimbus now that it is in the correct state when receive the `NimbusInterface.Observer.onUpdatesApplied`. Co-authored-by: jhugman Co-authored-by: Christian Sadilek --- .../java/org/mozilla/fenix/FenixApplication.kt | 16 ++++++++++++++++ .../java/org/mozilla/fenix/utils/Settings.kt | 5 ++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt index 3a6c31d271..c7d9b2f356 100644 --- a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt +++ b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -434,12 +434,28 @@ open class FenixApplication : LocaleAwareApplication(), Provider { private fun setupNimbusObserver(nimbus: Observable) { nimbus.register(object : NimbusInterface.Observer { override fun onUpdatesApplied(updated: List) { + // To workaround a caching bug in Nimbus that appears when we try to query an + // experiment **before** `FxNimbus.initialize` is called we need to set the `api` + // value again so that we can still access the NimbusApi that is wrapped + // in `FxNimbus.initialize.getSdk`. + // + // We set this value here to minimize the race between setting the `api` value and + // the callers of FxNimbus. + // See: https://github.com/mozilla/application-services/issues/5075 + FxNimbus.api = components.analytics.experiments onNimbusStartupAndUpdate() } }) } 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 (FeatureFlags.messagingFeature && settings.isExperimentationEnabled) { components.appStore.dispatch(AppAction.MessagingAction.Restore) diff --git a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt index 156ce0c353..bce2db2a24 100644 --- a/app/src/main/java/org/mozilla/fenix/utils/Settings.kt +++ b/app/src/main/java/org/mozilla/fenix/utils/Settings.kt @@ -581,9 +581,8 @@ class Settings(private val appContext: Context) : PreferencesHolder { true ) - val enabledTotalCookieProtection: Boolean by lazy { - FxNimbus.features.engineSettings.value().totalCookieProtectionEnabled - } + val enabledTotalCookieProtection: Boolean + get() = FxNimbus.features.engineSettings.value().totalCookieProtectionEnabled val blockCookiesSelectionInCustomTrackingProtection by stringPreference( appContext.getPreferenceKey(R.string.pref_key_tracking_protection_custom_cookies_select),