Address experiments activation issues from manual test

- Update ExperimentsTests to work with changes

components/service/experiments/src/main/java/mozilla/components/service/experiments/ExperimentEvaluator.kt

@@ -24,6 +24,16 @@ internal class ExperimentEvaluator(
 ) {
     private val logger: Logger = Logger(LOG_TAG)
 
+    internal fun findActiveExperiment(context: Context, experiments: List<Experiment>): ActiveExperiment? {
+        for (experiment in experiments) {
+            evaluate(context, ExperimentDescriptor(experiment.id), experiments)?.let {
+                return it
+            }
+        }
+
+        return null
+    }
+
     /**
      * Determines if a specific experiment should be enabled or not for the device
      *
@@ -93,10 +103,10 @@ internal class ExperimentEvaluator(
             matchesExperiment(match.deviceModel, valuesProvider.getDevice(context))
     }
 
-    private fun matchesExperiment(experimentValue: String?, deviceValue: String): Boolean {
+    private fun matchesExperiment(experimentValue: String?, deviceValue: String?): Boolean {
         return !(experimentValue != null &&
             !TextUtils.isEmpty(experimentValue) &&
-            !deviceValue.matches(experimentValue.toRegex()))
+            !(deviceValue?.matches(experimentValue.toRegex()) ?: false))
     }
 
     private fun isInBucket(userBucket: Int, experiment: Experiment): Boolean {

components/service/experiments/src/main/java/mozilla/components/service/experiments/Experiments.kt

@@ -4,6 +4,7 @@
 
 package mozilla.components.service.experiments
 
+import android.annotation.SuppressLint
 import android.content.Context
 import mozilla.components.support.base.log.logger.Logger
 import mozilla.components.service.glean.Glean
@@ -39,13 +40,17 @@ open class ExperimentsInternalAPI internal constructor() {
 
     internal lateinit var configuration: Configuration
 
+    private lateinit var context: Context
+
     /**
      * Initialize the experiments library.
      *
      * This should only be initialized once by the application.
      *
      * @param applicationContext [Context] to access application features, such
-     * as shared preferences.
+     * as shared preferences.  As we cannot enforce through the compiler that the context pass to
+     * the initialize function is a applicationContext, there could potentially be a memory leak
+     * if the initializing application doesn't comply.
      */
     fun initialize(
         applicationContext: Context,
@@ -66,18 +71,23 @@ open class ExperimentsInternalAPI internal constructor() {
 
         experimentsResult = ExperimentsSnapshot(listOf(), null)
         experimentsLoaded = false
-
+        context = applicationContext
         storage = getExperimentsStorage(applicationContext)
 
         isInitialized = true
 
-        // Load cached experiments from storage. After this, experiments status
-        // is available.
+        // Load cached experiments from storage. After this, experiments status is available.
         loadExperiments()
 
-        // Load active experiment from cache, if any.
+        // Load the active experiment from cache, if any.
         activeExperiment = loadActiveExperiment(applicationContext, experiments)
 
+        // If no active experiment was loaded from cache, check the cached experiments list for any
+        // that should be launched now.
+        if (activeExperiment == null) {
+            findAndStartActiveExperiment()
+        }
+
         // We now have the last known experiment state loaded for product code
         // that needs to check it early in startup.
         // Next we need to update the experiments list from the server async,
@@ -122,20 +132,6 @@ open class ExperimentsInternalAPI internal constructor() {
         experimentsLoaded = true
     }
 
-    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-    internal fun loadActiveExperiment(
-        context: Context,
-        experiments: List<Experiment>
-    ): ActiveExperiment? {
-        val activeExperiment = ActiveExperiment.load(context, experiments)
-        logger.info(activeExperiment?.let
-            { """Loaded active experiment - id="${it.experiment.id}", branch="${it.branch}"""" }
-            ?: "No active experiment"
-        )
-
-        return activeExperiment
-    }
-
     /**
      * Requests new experiments from the server and
      * saves them to local storage
@@ -146,35 +142,100 @@ open class ExperimentsInternalAPI internal constructor() {
 
         experimentsResult = serverState
         storage.save(serverState)
+
+        // TODO
+        // Choices here:
+        // 1) There currently is an active experiment.
+        // 1a) Should it stop? E.g. because it was deleted. If so, continue with 2.
+        // 1b) Should it continue? Then nothing else happens here.
+        // 2) There is no currently active experiment. Find one in the list, if any.
+        // 2a) If there is one...
+        // 2b) If there is none, nothing else happens.
+
+        activeExperiment?.let { active ->
+            if (experimentsResult.experiments.any { it.id == active.experiment.id }) {
+                // This covers 1b) - the active experiment should continue, no action needed.
+                logger.info("onExperimentsUpdated - currently active experiment will stay active")
+                return
+            } else {
+                // This covers 1a) - the experiment was removed.
+                // Afterwards, fall through to 2) below, which possibly starts a new experiment.
+                logger.info("onExperimentsUpdated - currently active experiment will be stopped")
+                stopActiveExperiment()
+            }
+        }
+
+        // This covers 2) - no experiment is currently active, so activate one if any match.
+        if (activeExperiment == null) {
+            logger.info("onExperimentsUpdated - no experiment currently active, looking for match")
+            findAndStartActiveExperiment()
+        }
+    }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal fun findAndStartActiveExperiment() {
+        assert(activeExperiment == null) { "Should not have an active experiment" }
+
+        evaluator.findActiveExperiment(context, experimentsResult.experiments)?.let {
+            logger.info("""Activating experiment - id="${it.experiment.id}", branch="${it.branch}"""")
+            activeExperiment = it
+            it.save(context)
+            Glean.setExperimentActive(it.experiment.id, it.branch)
+        }
+    }
+
+    private fun stopActiveExperiment() {
+        assert(activeExperiment != null) { "Should have an active experiment" }
+
+        ActiveExperiment.clear(context)
+        activeExperiment = null
+    }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    internal fun loadActiveExperiment(
+        context: Context,
+        experiments: List<Experiment>
+    ): ActiveExperiment? {
+        assert(activeExperiment == null) { "Should not have an active experiment" }
+
+        val activeExperiment = ActiveExperiment.load(context, experiments)
+        logger.info(activeExperiment?.let
+        { """Loaded active experiment from cache - id="${it.experiment.id}", branch="${it.branch}"""" }
+            ?: "No active experiment in cache"
+        )
+
+        return activeExperiment
     }
 
     /**
      * Checks if the user is part of
      * the specified experiment
      *
-     * @param context context
      * @param experimentId the id of the experiment
+     * @param branchName the name of the branch for the experiment
      *
      * @return true if the user is part of the specified experiment, false otherwise
      */
     @VisibleForTesting(otherwise = VisibleForTesting.NONE)
-    internal fun isInExperiment(context: Context, experimentId: String): Boolean {
-        return evaluator.evaluate(context, ExperimentDescriptor(experimentId), experimentsResult.experiments) != null
+    internal fun isInExperiment(experimentId: String, branchName: String): Boolean {
+        return activeExperiment?.let {
+            (it.experiment.id == experimentId) &&
+            (it.branch == branchName)
+        } ?: false
     }
 
     /**
      * Performs an action if the user is part of the specified experiment
      *
-     * @param context context
      * @param experimentId the id of the experiment
      * @param block block of code to be executed if the user is part of the experiment
      */
-    fun withExperiment(context: Context, experimentId: String, block: (branch: String) -> Unit) {
-        val activeExperiment = evaluator.evaluate(
-            context,
-            ExperimentDescriptor(experimentId), experimentsResult.experiments
-        )
-        activeExperiment?.let { block(it.branch) }
+    fun withExperiment(experimentId: String, block: (branch: String) -> Unit) {
+        activeExperiment?.let {
+            if (it.experiment.id == experimentId) {
+                block(it.branch)
+            }
+        }
     }
 
     /**
@@ -189,30 +250,11 @@ open class ExperimentsInternalAPI internal constructor() {
         return evaluator.getExperiment(ExperimentDescriptor(experimentId), experimentsResult.experiments)
     }
 
-    /**
-     * Provides the list of active experiments
-     *
-     * @param context context
-     *
-     * @return active experiments
-     */
-    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-    internal fun getActiveExperiments(context: Context): List<Experiment> {
-        return experiments.filter { isInExperiment(context, it.id) }
-    }
-
-    /**
-     * Provides a map of active/inactive experiments
-     *
-     * @param context context
-     *
-     * @return map of experiments to A/B state
-     */
-    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-    internal fun getExperimentsMap(context: Context): Map<String, Boolean> {
-        return experiments.associate {
-            it.id to
-                    isInExperiment(context, it.id)
+    private fun overrideActiveExperiment() {
+        evaluator.findActiveExperiment(context, experimentsResult.experiments)?.let {
+            logger.info("""Setting override experiment - id="${it.experiment.id}", branch="${it.branch}"""")
+            activeExperiment = it
+            Glean.setExperimentActive(it.experiment.id, it.branch)
         }
     }
 
@@ -231,6 +273,7 @@ open class ExperimentsInternalAPI internal constructor() {
         branchName: String
     ) {
         evaluator.setOverride(context, ExperimentDescriptor(experimentId), active, branchName)
+        overrideActiveExperiment()
     }
 
     /**
@@ -249,6 +292,7 @@ open class ExperimentsInternalAPI internal constructor() {
         branchName: String
     ) {
         evaluator.setOverrideNow(context, ExperimentDescriptor(experimentId), active, branchName)
+        overrideActiveExperiment()
     }
 
     /**
@@ -260,6 +304,8 @@ open class ExperimentsInternalAPI internal constructor() {
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     internal fun clearOverride(context: Context, experimentId: String) {
         evaluator.clearOverride(context, ExperimentDescriptor(experimentId))
+        activeExperiment = null
+        findAndStartActiveExperiment()
     }
 
     /**
@@ -273,6 +319,8 @@ open class ExperimentsInternalAPI internal constructor() {
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     internal fun clearOverrideNow(context: Context, experimentId: String) {
         evaluator.clearOverrideNow(context, ExperimentDescriptor(experimentId))
+        activeExperiment = null
+        findAndStartActiveExperiment()
     }
 
     /**
@@ -283,6 +331,8 @@ open class ExperimentsInternalAPI internal constructor() {
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     internal fun clearAllOverrides(context: Context) {
         evaluator.clearAllOverrides(context)
+        activeExperiment = null
+        findAndStartActiveExperiment()
     }
 
     /**
@@ -293,6 +343,8 @@ open class ExperimentsInternalAPI internal constructor() {
      */
     internal fun clearAllOverridesNow(context: Context) {
         evaluator.clearAllOverridesNow(context)
+        activeExperiment = null
+        findAndStartActiveExperiment()
     }
 
     /**
@@ -314,6 +366,7 @@ open class ExperimentsInternalAPI internal constructor() {
     }
 }
 
+@SuppressLint("StaticFieldLeak")
 object Experiments : ExperimentsInternalAPI() {
     internal const val SCHEMA_VERSION = 1
 }

components/service/experiments/src/main/java/mozilla/components/service/experiments/ExperimentsUpdater.kt

@@ -42,10 +42,12 @@ internal class ExperimentsUpdater(
     private val experiments: ExperimentsInternalAPI
 ) {
     private val logger: Logger = Logger(LOG_TAG)
+    private lateinit var config: Configuration
 
     internal lateinit var source: KintoExperimentSource
 
     internal fun initialize(configuration: Configuration) {
+        config = configuration
         source = getExperimentSource(configuration)
 
         // Schedule the periodic experiment updates
@@ -117,7 +119,7 @@ internal class ExperimentsUpdater(
     @Synchronized
     internal fun updateExperiments(): Boolean {
         return try {
-            val serverExperiments = source.getExperiments(experiments.experimentsResult)
+            val serverExperiments = getExperimentSource(config).getExperiments(experiments.experimentsResult)
             logger.info("Experiments update from server: $serverExperiments")
             experiments.onExperimentsUpdated(serverExperiments)
             true

components/service/experiments/src/main/java/mozilla/components/service/experiments/FlatFileExperimentStorage.kt

@@ -5,6 +5,7 @@
 package mozilla.components.service.experiments
 
 import android.util.AtomicFile
+import mozilla.components.support.base.log.logger.Logger
 import org.json.JSONException
 import java.io.FileNotFoundException
 import java.io.File
@@ -17,16 +18,19 @@ import java.io.IOException
  */
 internal class FlatFileExperimentStorage(file: File) {
     private val atomicFile: AtomicFile = AtomicFile(file)
+    private val logger: Logger = Logger(LOG_TAG)
 
     fun retrieve(): ExperimentsSnapshot {
         return try {
             val experimentsJson = String(atomicFile.readFully())
             ExperimentsSerializer().fromJson(experimentsJson)
         } catch (e: FileNotFoundException) {
+            logger.error("Caught error trying to retrieve experiments from storage: $e")
             ExperimentsSnapshot(listOf(), null)
         } catch (e: JSONException) {
             // The JSON we read from disk is corrupt. There's nothing we can do here and therefore
             // we just continue as if the file wouldn't exist.
+            logger.error("Caught error trying to retrieve experiments from storage: $e")
             ExperimentsSnapshot(listOf(), null)
         }
     }
@@ -45,4 +49,12 @@ internal class FlatFileExperimentStorage(file: File) {
             atomicFile.failWrite(stream)
         }
     }
+
+    fun delete() {
+        atomicFile.delete()
+    }
+
+    companion object {
+        private const val LOG_TAG = "experiments"
+    }
 }

components/service/experiments/src/main/java/mozilla/components/service/experiments/JSONExperimentParser.kt

@@ -9,6 +9,7 @@ import mozilla.components.support.ktx.android.org.json.toList
 import mozilla.components.support.ktx.android.org.json.tryGetLong
 import mozilla.components.support.ktx.android.org.json.tryGetString
 import org.json.JSONArray
+import org.json.JSONException
 import org.json.JSONObject
 
 /**
@@ -35,6 +36,9 @@ internal class JSONExperimentParser {
                 it.getInt(BRANCHES_RATIO_KEY)
             )
         }
+        if (branches.isEmpty()) {
+            throw JSONException("Branches array should not be empty")
+        }
 
         val matchObject: JSONObject = jsonObject.getJSONObject(MATCH_KEY)
         val regions: List<String>? = matchObject.optJSONArray(MATCH_REGIONS_KEY)?.toList()

components/service/experiments/src/main/java/mozilla/components/service/experiments/ValuesProvider.kt

@@ -52,7 +52,7 @@ internal open class ValuesProvider {
      *
      * @return app version name
      */
-    open fun getVersion(context: Context): String {
+    open fun getVersion(context: Context): String? {
         return context.packageManager.getPackageInfo(context.packageName, 0).versionName
     }
 

components/service/experiments/src/test/java/mozilla/components/service/experiments/JSONExperimentParserTest.kt

@@ -252,4 +252,24 @@ class JSONExperimentParserTest {
         experiment = createDefaultExperiment(id = "sample-id")
         assertEquals(experiment, JSONExperimentParser().fromJson(JSONObject(emptyObjects)))
     }
+
+    @Test(expected = JSONException::class)
+    fun `fromJson with empty branches list`() {
+        val json = """
+            {
+              "id": "some-id",
+              "match": {
+              },
+              "buckets": {
+                "start": 0,
+                "count": 0
+              },
+              "branches": [
+              ],
+              "description": "",
+              "last_modified": 1234
+            }
+        """.trimIndent()
+        JSONExperimentParser().fromJson(JSONObject(json))
+    }
 }

components/service/experiments/src/test/java/mozilla/components/service/experiments/SignatureVerifierTest.kt

@@ -880,6 +880,10 @@ class SignatureVerifierTest {
                     "count": "50"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -895,6 +899,10 @@ class SignatureVerifierTest {
                     "count": "100"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -908,6 +916,10 @@ class SignatureVerifierTest {
                     "count": "50"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -921,6 +933,10 @@ class SignatureVerifierTest {
                     "count": "100"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -933,6 +949,10 @@ class SignatureVerifierTest {
                     "count": "100"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -945,6 +965,10 @@ class SignatureVerifierTest {
                     "count": "0"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -957,6 +981,10 @@ class SignatureVerifierTest {
                     "count": "0"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -969,6 +997,10 @@ class SignatureVerifierTest {
                     "count": "0"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 },
                 {
@@ -981,6 +1013,10 @@ class SignatureVerifierTest {
                     "count": "0"
                   },
                   "branches": [
+                    {
+                      "name": "some-branch",
+                      "ratio": 2
+                    }
                   ]
                 }
               ],

components/service/experiments/src/test/java/mozilla/components/service/experiments/debug/ExperimentsDebugActivityTest.kt

@@ -18,7 +18,6 @@ import mozilla.components.service.experiments.Experiments
 import mozilla.components.service.experiments.ExperimentsSnapshot
 import mozilla.components.service.experiments.ExperimentsUpdater
 import mozilla.components.service.glean.Glean
-import org.junit.Assert
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertNotNull
@@ -37,13 +36,13 @@ class ExperimentsDebugActivityTest {
 
     private val testPackageName = "mozilla.components.service.experiments.test"
     private val context: Context = ApplicationProvider.getApplicationContext()
+    private lateinit var configuration: Configuration
 
     @Before
     fun setup() {
         WorkManagerTestInitHelper.initializeTestWorkManager(context)
 
         Glean.initialize(context)
-        Experiments.initialize(context)
 
         // This makes sure we have a "launch" intent in our package, otherwise
         // it will fail looking for it in `GleanDebugActivityTest`.
@@ -68,7 +67,7 @@ class ExperimentsDebugActivityTest {
         activity.create().start().resume()
 
         // Check that our main activity was launched.
-        Assert.assertEquals(testPackageName,
+        assertEquals(testPackageName,
             Shadows.shadowOf(activity.get()).peekNextStartedActivityForResult().intent.`package`!!)
     }
 
@@ -105,9 +104,9 @@ class ExperimentsDebugActivityTest {
             ExperimentsDebugActivity::class.java)
         var activity = Robolectric.buildActivity(ExperimentsDebugActivity::class.java, intent)
 
-        val updater: ExperimentsUpdater = spy(ExperimentsUpdater(ApplicationProvider.getApplicationContext<Context>(), Experiments))
-        updater.initialize(Configuration())
-        Experiments.updater = updater
+        configuration = Configuration()
+
+        Experiments.initialize(context, configuration)
 
         activity.create().start().resume()
 
@@ -183,7 +182,7 @@ class ExperimentsDebugActivityTest {
         // Fake some experiments to test whether the correct one is active
         val experiment1 = Experiment(
             id