15278 detekt rule runblocking

.github/CODEOWNERS

@@ -50,6 +50,8 @@
 *Application.kt @mozilla-mobile/Performance
 *StrictMode*kt @mozilla-mobile/Performance
 *ConstraintLayoutPerfDetector* @mozilla-mobile/Performance
+*MozillaRunBlockingCheck.kt @mozilla-mobile/Performance
+*StartupExcessiveResourceUseTest.kt @mozilla-mobile/Performance
 
 # We want to be aware of new features behind flags as well as features
 # about to be enabled.

app/src/androidTest/java/org/mozilla/fenix/ui/StrictModeStartupSuppressionCountTest.kt → app/src/androidTest/java/org/mozilla/fenix/ui/StartupExcessiveResourceUseTest.kt

@@ -9,13 +9,15 @@ import androidx.test.uiautomator.UiDevice
 import org.junit.Assert.assertEquals
 import org.junit.Rule
 import org.junit.Test
+import org.mozilla.fenix.perf.RunBlockingCounter
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.helpers.HomeActivityTestRule
 
 // PLEASE CONSULT WITH PERF TEAM BEFORE CHANGING THIS VALUE.
 private const val EXPECTED_SUPPRESSION_COUNT = 11
 
-private const val FAILURE_MSG = """StrictMode startup suppression count does not match expected count.
+private const val EXPECTED_RUNBLOCKING_COUNT = 2
+private const val STRICTMODE_FAILURE_MSG = """StrictMode startup suppression count does not match expected count.
 
     If this PR removed code that suppressed StrictMode, great! Please decrement the suppression count.
 
@@ -28,31 +30,53 @@ private const val FAILURE_MSG = """StrictMode startup suppression count does not
 
 """
 
+private const val RUNBLOCKING_FAILURE_MSG = """RunblockingCounter startup count does not match expected count.
+
+    If this PR removed code that removed a RunBlocking call, great! Please decrement the suppression count.
+
+    Did this PR add or call code that incremented the RunBlockingCounter?
+    Did you know that using runBlocking can have negative perfomance implications?
+
+    If so, please do your best to implement a solution without blocking the main thread.
+    Please consult the perf team if you have questions or believe that using runBlocking
+    is the optimal solution.
+
+"""
+
 /**
- * A performance test to limit the number of StrictMode suppressions on startup.
+ * A performance test to limit the number of StrictMode suppressions and number of runBlocking used
+ * on startup.
+ *
  * This test was written by the perf team.
  *
  * StrictMode detects main thread IO, which is often indicative of a performance issue.
  * It's easy to suppress StrictMode so we wrote a test to ensure we have a discussion
- * if the StrictMode count changes. The perf team is code owners for this file so they
- * should be notified when the count is modified.
+ * if the StrictMode count changes.
+ *
+ * RunBlocking is mostly used to return values to a thread from a coroutine. However, if that
+ * coroutine takes too long, it can lead that thread to block every other operations.
+ *
+ * The perf team is code owners for this file so they should be notified when the count is modified.
  *
  * IF YOU UPDATE THE TEST NAME, UPDATE CODE OWNERS.
  */
-class StrictModeStartupSuppressionCountTest {
+class StartupExcessiveResourceUseTest {
     @get:Rule
     val activityTestRule = HomeActivityTestRule(skipOnboarding = true)
 
     private val uiDevice = UiDevice.getInstance(InstrumentationRegistry.getInstrumentation())
 
     @Test
-    fun verifyStrictModeSuppressionCount() {
+    fun verifyRunBlockingAndStrictModeSuppresionCount() {
         uiDevice.waitForIdle() // wait for async UI to load.
 
         // This might cause intermittents: at an arbitrary point after start up (such as the visual
         // completeness queue), we might run code on the main thread that suppresses StrictMode,
         // causing this number to fluctuate depending on device speed. We'll deal with it if it occurs.
-        val actual = activityTestRule.activity.components.strictMode.suppressionCount.toInt()
-        assertEquals(FAILURE_MSG, EXPECTED_SUPPRESSION_COUNT, actual)
+        val actualSuppresionCount = activityTestRule.activity.components.strictMode.suppressionCount.toInt()
+        val actualRunBlocking = RunBlockingCounter.count.get()
+
+        assertEquals(STRICTMODE_FAILURE_MSG, EXPECTED_SUPPRESSION_COUNT, actualSuppresionCount)
+        assertEquals(RUNBLOCKING_FAILURE_MSG, EXPECTED_RUNBLOCKING_COUNT, actualRunBlocking)
     }
 }

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

@@ -19,7 +19,6 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.async
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.runBlocking
 import mozilla.appservices.Megazord
 import mozilla.components.browser.session.Session
 import mozilla.components.browser.state.action.SystemAction
@@ -44,6 +43,7 @@ import org.mozilla.fenix.components.metrics.MetricServiceType
 import org.mozilla.fenix.ext.settings
 import org.mozilla.fenix.perf.StorageStatsMetrics
 import org.mozilla.fenix.perf.StartupTimeline
+import org.mozilla.fenix.perf.runBlockingIncrement
 import org.mozilla.fenix.push.PushFxaIntegration
 import org.mozilla.fenix.push.WebPushEngineIntegration
 import org.mozilla.fenix.session.PerformanceActivityLifecycleCallbacks
@@ -137,7 +137,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider {
             // to invoke parts of itself that require complete megazord initialization
             // before that process completes, we wait here, if necessary.
             if (!megazordSetup.isCompleted) {
-                runBlocking { megazordSetup.await(); }
+                runBlockingIncrement { megazordSetup.await() }
             }
         }
 

app/src/main/java/org/mozilla/fenix/components/history/PagedHistoryProvider.kt

@@ -4,10 +4,10 @@
 
 package org.mozilla.fenix.components.history
 
-import kotlinx.coroutines.runBlocking
 import mozilla.components.concept.storage.HistoryStorage
 import mozilla.components.concept.storage.VisitInfo
 import mozilla.components.concept.storage.VisitType
+import org.mozilla.fenix.perf.runBlockingIncrement
 
 /**
  * An Interface for providing a paginated list of [VisitInfo]
@@ -32,7 +32,7 @@ fun HistoryStorage.createSynchronousPagedHistoryProvider(): PagedHistoryProvider
             numberOfItems: Long,
             onComplete: (List<VisitInfo>) -> Unit
         ) {
-            runBlocking {
+            runBlockingIncrement {
                 val history = getVisitsPaginated(
                     offset,
                     numberOfItems,

app/src/main/java/org/mozilla/fenix/components/metrics/AppStartupTelemetry.kt

@@ -12,7 +12,6 @@ import androidx.lifecycle.Lifecycle
 import androidx.lifecycle.LifecycleObserver
 import androidx.lifecycle.OnLifecycleEvent
 import androidx.lifecycle.ProcessLifecycleOwner
-import kotlinx.coroutines.runBlocking
 import mozilla.components.support.utils.SafeIntent
 import org.mozilla.fenix.components.metrics.Event.AppAllStartup
 import org.mozilla.fenix.components.metrics.Event.AppAllStartup.Source
@@ -25,6 +24,7 @@ import org.mozilla.fenix.components.metrics.Event.AppAllStartup.Type.ERROR
 import org.mozilla.fenix.components.metrics.Event.AppAllStartup.Type.HOT
 import org.mozilla.fenix.components.metrics.Event.AppAllStartup.Type.COLD
 import org.mozilla.fenix.components.metrics.Event.AppAllStartup.Type.WARM
+import org.mozilla.fenix.perf.runBlockingIncrement
 import java.lang.reflect.Modifier.PRIVATE
 
 /**
@@ -186,7 +186,7 @@ class AppStartupTelemetry(
      * the application potentially closes.
      */
     fun onStop() {
-        runBlocking {
+        runBlockingIncrement {
             recordMetric()
         }
     }

app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt

@@ -12,7 +12,6 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.async
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.runBlocking
 import mozilla.components.browser.search.SearchEngine
 import mozilla.components.browser.search.provider.AssetsSearchEngineProvider
 import mozilla.components.browser.search.provider.SearchEngineList
@@ -27,6 +26,7 @@ import org.mozilla.fenix.BuildConfig
 import org.mozilla.fenix.Config
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.settings
+import org.mozilla.fenix.perf.runBlockingIncrement
 import java.util.Locale
 
 @SuppressWarnings("TooManyFunctions")
@@ -141,7 +141,7 @@ open class FenixSearchEngineProvider(
      * are readily available throughout the app. Includes all installed engines, both
      * default and custom
      */
-    fun installedSearchEngines(context: Context): SearchEngineList = runBlocking {
+    fun installedSearchEngines(context: Context): SearchEngineList = runBlockingIncrement {
         val installedIdentifiers = installedSearchEngineIdentifiers(context)
         val defaultList = searchEngines.await()
 
@@ -161,15 +161,15 @@ open class FenixSearchEngineProvider(
         )
     }
 
-    fun allSearchEngineIdentifiers() = runBlocking {
+    fun allSearchEngineIdentifiers() = runBlockingIncrement {
         loadedSearchEngines.await().list.map { it.identifier }
     }
 
-    fun uninstalledSearchEngines(context: Context): SearchEngineList = runBlocking {
+    fun uninstalledSearchEngines(context: Context): SearchEngineList = runBlockingIncrement {
         val installedIdentifiers = installedSearchEngineIdentifiers(context)
         val engineList = loadedSearchEngines.await()
 
-        return@runBlocking engineList.copy(
+        return@runBlockingIncrement engineList.copy(
             list = engineList.list.filterNot { installedIdentifiers.contains(it.identifier) }
         )
     }
@@ -182,7 +182,7 @@ open class FenixSearchEngineProvider(
         context: Context,
         searchEngine: SearchEngine,
         isCustom: Boolean = false
-    ) = runBlocking {
+    ) = runBlockingIncrement {
         if (isCustom) {
             val searchUrl = searchEngine.getSearchTemplate()
             CustomSearchEngineStore.addSearchEngine(context, searchEngine.name, searchUrl)
@@ -201,7 +201,7 @@ open class FenixSearchEngineProvider(
         context: Context,
         searchEngine: SearchEngine,
         isCustom: Boolean = false
-    ) = runBlocking {
+    ) = runBlockingIncrement {
         if (isCustom) {
             CustomSearchEngineStore.removeSearchEngine(context, searchEngine.identifier)
             reload()

app/src/main/java/org/mozilla/fenix/customtabs/FennecWebAppIntentProcessor.kt

@@ -9,7 +9,6 @@ import android.content.Intent
 import android.content.Intent.FLAG_ACTIVITY_NEW_DOCUMENT
 import androidx.annotation.VisibleForTesting
 import androidx.core.content.ContextCompat
-import kotlinx.coroutines.runBlocking
 import mozilla.components.browser.session.Session
 import mozilla.components.browser.session.SessionManager
 import mozilla.components.browser.state.state.CustomTabConfig
@@ -30,6 +29,7 @@ import mozilla.components.support.utils.toSafeIntent
 import org.json.JSONException
 import org.json.JSONObject
 import org.mozilla.fenix.R
+import org.mozilla.fenix.perf.runBlockingIncrement
 import java.io.File
 import java.io.IOException
 
@@ -61,7 +61,7 @@ class FennecWebAppIntentProcessor(
         val url = safeIntent.dataString
 
         return if (!url.isNullOrEmpty() && matches(intent)) {
-            val webAppManifest = runBlocking { loadManifest(safeIntent, url) }
+            val webAppManifest = runBlockingIncrement { loadManifest(safeIntent, url) }
 
             val session = Session(url, private = false, source = SessionState.Source.HOME_SCREEN)
             session.webAppManifest = webAppManifest

app/src/main/java/org/mozilla/fenix/ext/AtomicInteger.kt

@@ -0,0 +1,19 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.fenix.ext
+
+import java.util.concurrent.atomic.AtomicInteger
+
+/**
+ *  Increases an AtomicInteger safely.
+ */
+fun AtomicInteger.getAndIncrementNoOverflow() {
+    var prev: Int
+    var next: Int
+    do {
+        prev = this.get()
+        next = if (prev == Integer.MAX_VALUE) prev else prev + 1
+    } while (!this.compareAndSet(prev, next))
+}

app/src/main/java/org/mozilla/fenix/ext/String.kt

@@ -11,13 +11,13 @@ import android.util.Patterns
 import android.webkit.URLUtil
 import androidx.core.net.toUri
 import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.withContext
 import mozilla.components.concept.fetch.Client
 import mozilla.components.concept.fetch.Request
 import mozilla.components.lib.publicsuffixlist.PublicSuffixList
 import mozilla.components.lib.publicsuffixlist.ext.urlToTrimmedHost
 import mozilla.components.support.ktx.android.net.hostWithoutCommonPrefixes
+import org.mozilla.fenix.perf.runBlockingIncrement
 import java.io.IOException
 import java.net.IDN
 import java.util.Locale
@@ -92,9 +92,10 @@ private fun Uri.isIpv6(): Boolean {
 /**
  * Trim a host's prefix and suffix
  */
-fun String.urlToTrimmedHost(publicSuffixList: PublicSuffixList): String = runBlocking {
-    urlToTrimmedHost(publicSuffixList).await()
-}
+fun String.urlToTrimmedHost(publicSuffixList: PublicSuffixList): String =
+    runBlockingIncrement {
+        urlToTrimmedHost(publicSuffixList).await()
+    }
 
 /**
  * Trims a URL string of its scheme and common prefixes.

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

@@ -0,0 +1,45 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// This class implements the alternative ways to invoke runBlocking with some
+// monitoring by wrapping the raw methods. This lint check tells us not to use the raw
+// methods so we suppress the check.
+@file:Suppress("MozillaRunBlockingCheck")
+
+package org.mozilla.fenix.perf
+
+import androidx.annotation.VisibleForTesting
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.runBlocking
+import org.mozilla.fenix.ext.getAndIncrementNoOverflow
+import java.util.concurrent.atomic.AtomicInteger
+import kotlin.coroutines.CoroutineContext
+
+/**
+ * Counts the number of runBlocking calls made
+ */
+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+object RunBlockingCounter {
+    val count = AtomicInteger(0)
+}
+
+/**
+ * Wrapper around `runBlocking`. RunBlocking seems to be a "fix-all" to return values to the thread
+ * where the coroutine is called. The official doc explains runBlocking:  "Runs a new coroutine and
+ * blocks the current thread interruptibly until its completion`. This can block our main thread
+ * which could lead to significant jank. This wrapper aims to count the number of runBlocking call
+ * to try to limit them as much as possible to encourage alternatives solutions whenever this function
+ * might be needed.
+ */
+fun <T> runBlockingIncrement(
+    context: CoroutineContext? = null,
+    action: suspend CoroutineScope.() -> T
+): T {
+    RunBlockingCounter.count.getAndIncrementNoOverflow()
+    return if (context != null) {
+        runBlocking(context) { action() }
+    } else {
+        runBlocking { action() }
+    }
+}

app/src/main/java/org/mozilla/fenix/settings/search/AddSearchEngineFragment.kt

@@ -25,7 +25,6 @@ import kotlinx.android.synthetic.main.search_engine_radio_button.view.*
 import kotlinx.coroutines.Dispatchers.IO
 import kotlinx.coroutines.Dispatchers.Main
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.withContext
 import mozilla.components.browser.search.SearchEngine
 import org.mozilla.fenix.BrowserDirection
@@ -37,6 +36,7 @@ import org.mozilla.fenix.components.searchengine.CustomSearchEngineStore
 import org.mozilla.fenix.ext.components
 import org.mozilla.fenix.ext.requireComponents
 import org.mozilla.fenix.ext.showToolbar
+import org.mozilla.fenix.perf.runBlockingIncrement
 import org.mozilla.fenix.settings.SupportUtils
 import java.util.Locale
 
@@ -51,7 +51,7 @@ class AddSearchEngineFragment : Fragment(R.layout.fragment_add_search_engine),
         super.onCreate(savedInstanceState)
         setHasOptionsMenu(true)
 
-        availableEngines = runBlocking {
+        availableEngines = runBlockingIncrement {
             requireContext()
                 .components
                 .search

app/src/migration/java/org/mozilla/fenix/MigratingFenixApplication.kt

@@ -5,10 +5,10 @@
 package org.mozilla.fenix
 
 import android.content.Context
-import kotlinx.coroutines.runBlocking
 import mozilla.components.support.migration.FennecMigrator
 import org.mozilla.fenix.session.PerformanceActivityLifecycleCallbacks
 import org.mozilla.fenix.migration.MigrationTelemetryListener
+import org.mozilla.fenix.perf.runBlockingIncrement
 
 /**
  * An application class which knows how to migrate Fennec data.
@@ -81,7 +81,7 @@ class MigratingFenixApplication : FenixApplication() {
             .migrateSettings()
             .build()
 
-        runBlocking {
+        runBlockingIncrement {
             migrator.migrateAsync(components.migrationStore).await()
         }
     }