15278 detekt rule runblocking

.github/CODEOWNERS

@@ -50,6 +50,7 @@
 *Application.kt @mozilla-mobile/Performance
 *StrictMode*kt @mozilla-mobile/Performance
 *ConstraintLayoutPerfDetector.kt @mozilla-mobile/Performance
+*MozillaRunBlockingCheck.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,6 +30,19 @@ 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.
  * This test was written by the perf team.
@@ -39,20 +54,23 @@ private const val FAILURE_MSG = """StrictMode startup suppression count does not
  *
  * 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
+
+        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")
@@ -116,40 +116,47 @@ open class FenixSearchEngineProvider(
      * @return a list of all SearchEngines that are currently active. These are the engines that
      * are readily available throughout the app.
      */
-    fun installedSearchEngines(context: Context): SearchEngineList = runBlocking {
-        val installedIdentifiers = installedSearchEngineIdentifiers(context)
-        val engineList = searchEngines.await()
-
-        engineList.copy(
-            list = engineList.list.filter {
-                installedIdentifiers.contains(it.identifier)
-            }.sortedBy { it.name.toLowerCase(Locale.getDefault()) },
-            default = engineList.default?.let {
-                if (installedIdentifiers.contains(it.identifier)) {
-                    it
-                } else {
-                    null
+    fun installedSearchEngines(context: Context): SearchEngineList =
+        runBlockingIncrement {
+            val installedIdentifiers = installedSearchEngineIdentifiers(context)
+            val engineList = searchEngines.await()
+
+            engineList.copy(
+                list = engineList.list.filter {
+                    installedIdentifiers.contains(it.identifier)
+                }.sortedBy { it.name.toLowerCase(Locale.getDefault()) },
+                default = engineList.default?.let {
+                    if (installedIdentifiers.contains(it.identifier)) {
+                        it
+                    } else {
+                        null
+                    }
                 }
-            }
-        )
-    }
+            )
+        }
 
-    fun allSearchEngineIdentifiers() = runBlocking {
-        loadedSearchEngines.await().list.map { it.identifier }
-    }
+    fun allSearchEngineIdentifiers() =
+        runBlockingIncrement {
+            loadedSearchEngines.await().list.map { it.identifier }
+        }
 
-    fun uninstalledSearchEngines(context: Context): SearchEngineList = runBlocking {
-        val installedIdentifiers = installedSearchEngineIdentifiers(context)
-        val engineList = loadedSearchEngines.await()
+    fun uninstalledSearchEngines(context: Context): SearchEngineList =
+        runBlockingIncrement {
+            val installedIdentifiers = installedSearchEngineIdentifiers(context)
+            val engineList = loadedSearchEngines.await()
 
-        engineList.copy(list = engineList.list.filterNot { installedIdentifiers.contains(it.identifier) })
-    }
+            engineList.copy(list = engineList.list.filterNot { installedIdentifiers.contains(it.identifier) })
+        }
 
     override suspend fun loadSearchEngines(context: Context): SearchEngineList {
         return installedSearchEngines(context)
     }
 
-    fun installSearchEngine(context: Context, searchEngine: SearchEngine, isCustom: Boolean = false) = runBlocking {
+    fun installSearchEngine(
+        context: Context,
+        searchEngine: SearchEngine,
+        isCustom: Boolean = false
+    ) = runBlockingIncrement {
         if (isCustom) {
             val searchUrl = searchEngine.getSearchTemplate()
             CustomSearchEngineStore.addSearchEngine(context, searchEngine.name, searchUrl)
@@ -162,14 +169,19 @@ open class FenixSearchEngineProvider(
         }
     }
 
-    fun uninstallSearchEngine(context: Context, searchEngine: SearchEngine, isCustom: Boolean = false) = runBlocking {
+    fun uninstallSearchEngine(
+        context: Context,
+        searchEngine: SearchEngine,
+        isCustom: Boolean = false
+    ) = runBlockingIncrement {
         if (isCustom) {
             CustomSearchEngineStore.removeSearchEngine(context, searchEngine.identifier)
             reload()
         } else {
             val installedIdentifiers = installedSearchEngineIdentifiers(context).toMutableSet()
             installedIdentifiers.remove(searchEngine.identifier)
-            prefs(context).edit().putStringSet(localeAwareInstalledEnginesKey(), installedIdentifiers).apply()
+            prefs(context).edit()
+                .putStringSet(localeAwareInstalledEnginesKey(), installedIdentifiers).apply()
         }
     }
 

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
@@ -28,6 +27,7 @@ import mozilla.components.support.utils.SafeIntent
 import mozilla.components.support.utils.toSafeIntent
 import org.json.JSONObject
 import org.mozilla.fenix.R
+import org.mozilla.fenix.perf.runBlockingIncrement
 import java.io.File
 import java.io.IOException
 
@@ -58,7 +58,12 @@ 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/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,48 @@
+/* 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 java.util.concurrent.atomic.AtomicInteger
+import kotlin.coroutines.CoroutineContext
+
+/**
+ * Counts the number of runBlocking calls made
+ */
+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+object RunBlockingCounter {
+    var 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 have negative
+ * side-effects on the 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 {
+    synchronized(lock = Any()) {
+        if (RunBlockingCounter.count.get() >= Int.MAX_VALUE) RunBlockingCounter.count.set(0)
+        else RunBlockingCounter.count.addAndGet(1)
+    }
+
+    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