Skip to content

Commit

Permalink
For mozilla-mobile#13959: move resetAfter into StrictModeManager.
Browse files Browse the repository at this point in the history
In a followup PR, we need to add state to strictModeManager (the
number of suppressions). This is much simpler to do when this is defined
as a class rather than an object. However, when this is defined as a
class, `resetAfter` needs access to the strictModeManager. Instead of
passing it in as an argument, it made sense to move this function onto
the strictModeManager instead.

Since folks are used to calling:
```
StrictMode.ThreadPolicy.allowThreadDiskReads().resetAfter
```

We're going to have to add a lint check to prevent them from doing that.
  • Loading branch information
mcomella committed Sep 29, 2020
1 parent 6abeb2d commit f19c992
Show file tree
Hide file tree
Showing 20 changed files with 100 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,11 @@ import androidx.preference.PreferenceManager
import leakcanary.AppWatcher
import leakcanary.LeakCanary
import org.mozilla.fenix.ext.getPreferenceKey
import org.mozilla.fenix.ext.resetPoliciesAfter

class DebugFenixApplication : FenixApplication() {

override fun setupLeakCanary() {
val isEnabled = StrictMode.allowThreadDiskReads().resetPoliciesAfter {
val isEnabled = components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
PreferenceManager.getDefaultSharedPreferences(this)
.getBoolean(getPreferenceKey(R.string.pref_key_leakcanary), true)
}
Expand Down
5 changes: 2 additions & 3 deletions app/src/main/java/org/mozilla/fenix/FenixApplication.kt
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import mozilla.components.support.webextensions.WebExtensionSupport
import org.mozilla.fenix.components.Components
import org.mozilla.fenix.components.metrics.MetricServiceType
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.perf.StorageStatsMetrics
import org.mozilla.fenix.perf.StartupTimeline
Expand Down Expand Up @@ -129,7 +128,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider {
warmBrowsersCache()

// Make sure the engine is initialized and ready to use.
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
components.core.engine.warmUp()
}
initializeWebExtensionSupport()
Expand Down Expand Up @@ -435,7 +434,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider {
applicationContext.resources.configuration.uiMode = config.uiMode

// random StrictMode onDiskRead violation even when Fenix is not running in the background.
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
super.onConfigurationChanged(config)
}
}
Expand Down
5 changes: 2 additions & 3 deletions app/src/main/java/org/mozilla/fenix/HomeActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ import org.mozilla.fenix.ext.breadcrumb
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.ext.metrics
import org.mozilla.fenix.ext.nav
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.home.HomeFragmentDirections
import org.mozilla.fenix.home.intent.CrashReporterIntentProcessor
Expand Down Expand Up @@ -152,7 +151,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
final override fun onCreate(savedInstanceState: Bundle?) {
components.strictMode.attachListenerToDisablePenaltyDeath(supportFragmentManager)
// There is disk read violations on some devices such as samsung and pixel for android 9/10
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
super.onCreate(savedInstanceState)
}

Expand Down Expand Up @@ -757,7 +756,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
}

override fun attachBaseContext(base: Context) {
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
super.attachBaseContext(base)
}
}
Expand Down
5 changes: 2 additions & 3 deletions app/src/main/java/org/mozilla/fenix/IntentReceiverActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import org.mozilla.fenix.components.IntentProcessorType
import org.mozilla.fenix.components.getType
import org.mozilla.fenix.components.metrics.Event
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.perf.StartupTimeline
import org.mozilla.fenix.shortcut.NewTabShortcutIntentProcessor
Expand All @@ -28,7 +27,7 @@ class IntentReceiverActivity : Activity() {
@VisibleForTesting
override fun onCreate(savedInstanceState: Bundle?) {
// StrictMode violation on certain devices such as Samsung
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
super.onCreate(savedInstanceState)
}

Expand Down Expand Up @@ -68,7 +67,7 @@ class IntentReceiverActivity : Activity() {
)
}
// StrictMode violation on certain devices such as Samsung
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
startActivity(intent)
}
finish() // must finish() after starting the other activity
Expand Down
28 changes: 25 additions & 3 deletions app/src/main/java/org/mozilla/fenix/StrictModeManager.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import android.os.Build
import android.os.StrictMode
import androidx.fragment.app.Fragment
import androidx.fragment.app.FragmentManager
import mozilla.components.support.ktx.android.os.resetAfter

private const val MANUFACTURE_HUAWEI: String = "HUAWEI"
private const val MANUFACTURE_ONE_PLUS: String = "OnePlus"
Expand All @@ -17,9 +18,8 @@ private const val MANUFACTURE_ONE_PLUS: String = "OnePlus"
*/
class StrictModeManager(config: Config) {

// The expression in this if is duplicated in StrictMode.ThreadPolicy.resetPoliciesAfter
// because we don't want to have to pass in a dependency each time the ext fn is called.
private val isEnabledByBuildConfig = config.channel.isDebug
// This is public so it can be used by inline functions.
val isEnabledByBuildConfig = config.channel.isDebug

/***
* Enables strict mode for debug purposes. meant to be run only in the main process.
Expand Down Expand Up @@ -67,6 +67,28 @@ class StrictModeManager(config: Config) {
}, false)
}

/**
* Runs the given [functionBlock] and sets the given [StrictMode.ThreadPolicy] after its
* completion when in a build configuration that has StrictMode enabled. If StrictMode is
* not enabled, simply runs the [functionBlock].
*
* This function is written in the style of [AutoCloseable.use].
*
* This is significantly less convenient to run than when it was written as an extension function
* on [StrictMode.ThreadPolicy] but I think this is okay: it shouldn't be easy to ignore StrictMode.
*
* @return the value returned by [functionBlock].
*/
inline fun <R> resetAfter(policy: StrictMode.ThreadPolicy, functionBlock: () -> R): R {
// Calling resetAfter takes 1-2ms (unknown device) so we only execute it if StrictMode can
// actually be enabled. https://github.com/mozilla-mobile/fenix/issues/11617
return if (isEnabledByBuildConfig) {
policy.resetAfter(functionBlock)
} else {
functionBlock()
}
}

/**
* There are certain manufacturers that have custom font classes for the OS systems.
* These classes violates the [StrictMode] policies on startup. As a workaround, we create
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import org.mozilla.fenix.ext.components
import org.mozilla.fenix.ext.nav
import org.mozilla.fenix.ext.navigateSafe
import org.mozilla.fenix.ext.requireComponents
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.shortcut.PwaOnboardingObserver
import org.mozilla.fenix.trackingprotection.TrackingProtectionOverlay
Expand Down Expand Up @@ -110,7 +109,7 @@ class BrowserFragment : BaseBrowserFragment(), UserInteractionHandler {
)

readerViewFeature.set(
feature = StrictMode.allowThreadDiskReads().resetPoliciesAfter {
feature = components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
ReaderViewFeature(
context,
components.core.engine,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import mozilla.components.concept.sync.OAuthAccount
import mozilla.components.lib.crash.CrashReporter
import mozilla.components.service.fxa.manager.FxaAccountManager
import mozilla.components.support.base.log.logger.Logger
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.StrictModeManager
import kotlin.coroutines.CoroutineContext

/**
Expand Down Expand Up @@ -57,6 +57,7 @@ internal abstract class AbnormalFxaEvent : Exception() {
class AccountAbnormalities(
context: Context,
private val crashReporter: CrashReporter,
strictMode: StrictModeManager,
private val coroutineContext: CoroutineContext = Dispatchers.IO
) : AccountObserver {
companion object {
Expand All @@ -79,14 +80,14 @@ class AccountAbnormalities(
private val hadAccountPrior: Boolean

init {
val prefPair = StrictMode.allowThreadDiskReads().resetPoliciesAfter {
val prefPair = strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
val p = context.getSharedPreferences(PREF_FXA_ABNORMALITIES, Context.MODE_PRIVATE)
val a = p.getBoolean(KEY_HAS_ACCOUNT, false)
Pair(p, a)
}
prefs = prefPair.first
hadAccountPrior = prefPair.second
}
}

/**
* Once [accountManager] is initialized, queries it to detect abnormal account states.
Expand Down
9 changes: 7 additions & 2 deletions app/src/main/java/org/mozilla/fenix/components/Analytics.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import org.mozilla.fenix.Config
import org.mozilla.fenix.HomeActivity
import org.mozilla.fenix.R
import org.mozilla.fenix.ReleaseChannel
import org.mozilla.fenix.StrictModeManager
import org.mozilla.fenix.components.metrics.AdjustMetricsService
import org.mozilla.fenix.components.metrics.GleanMetricsService
import org.mozilla.fenix.components.metrics.LeanplumMetricsService
Expand All @@ -34,7 +35,8 @@ import org.mozilla.geckoview.BuildConfig.MOZ_UPDATE_CHANNEL
*/
@Mockable
class Analytics(
private val context: Context
private val context: Context,
strictMode: StrictModeManager
) {
val crashReporter: CrashReporter by lazy {
val services = mutableListOf<CrashReporterService>()
Expand Down Expand Up @@ -84,7 +86,10 @@ class Analytics(
)
}

val leanplumMetricsService by lazy { LeanplumMetricsService(context as Application) }
val leanplumMetricsService by lazy { LeanplumMetricsService(
context as Application,
strictMode
) }

val metrics: MetricController by lazy {
MetricController.create(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import mozilla.components.service.sync.logins.SyncableLoginsStorage
import mozilla.components.support.utils.RunWhenReadyQueue
import org.mozilla.fenix.Config
import org.mozilla.fenix.R
import org.mozilla.fenix.StrictModeManager
import org.mozilla.fenix.components.metrics.Event
import org.mozilla.fenix.components.metrics.MetricController
import org.mozilla.fenix.ext.components
Expand All @@ -57,7 +58,8 @@ class BackgroundServices(
historyStorage: Lazy<PlacesHistoryStorage>,
bookmarkStorage: Lazy<PlacesBookmarksStorage>,
passwordsStorage: Lazy<SyncableLoginsStorage>,
remoteTabsStorage: Lazy<RemoteTabsStorage>
remoteTabsStorage: Lazy<RemoteTabsStorage>,
strictMode: StrictModeManager
) {
// Allows executing tasks which depend on the account manager, but do not need to eagerly initialize it.
val accountManagerAvailableQueue = RunWhenReadyQueue()
Expand Down Expand Up @@ -105,7 +107,7 @@ class BackgroundServices(
context.components.analytics.metrics
)

val accountAbnormalities = AccountAbnormalities(context, crashReporter)
val accountAbnormalities = AccountAbnormalities(context, crashReporter, strictMode)

val accountManager by lazy { makeAccountManager(context, serverConfig, deviceConfig, syncConfig) }

Expand Down
7 changes: 4 additions & 3 deletions app/src/main/java/org/mozilla/fenix/components/Components.kt
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ class Components(private val context: Context) {
core.lazyHistoryStorage,
core.lazyBookmarksStorage,
core.lazyPasswordsStorage,
core.lazyRemoteTabsStorage
core.lazyRemoteTabsStorage,
strictMode
)
}
val services by lazy { Services(context, backgroundServices.accountManager) }
val core by lazy { Core(context, analytics.crashReporter) }
val core by lazy { Core(context, analytics.crashReporter, strictMode) }
val search by lazy { Search(context) }
val useCases by lazy {
UseCases(
Expand Down Expand Up @@ -120,7 +121,7 @@ class Components(private val context: Context) {
AddonManager(core.store, core.engine, addonCollectionProvider, addonUpdater)
}

val analytics by lazy { Analytics(context) }
val analytics by lazy { Analytics(context, strictMode) }
val publicSuffixList by lazy { PublicSuffixList(context) }
val clipboardHandler by lazy { ClipboardHandler(context) }
val migrationStore by lazy { MigrationStore() }
Expand Down
16 changes: 12 additions & 4 deletions app/src/main/java/org/mozilla/fenix/components/Core.kt
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ import org.mozilla.fenix.AppRequestInterceptor
import org.mozilla.fenix.Config
import org.mozilla.fenix.HomeActivity
import org.mozilla.fenix.R
import org.mozilla.fenix.StrictModeManager
import org.mozilla.fenix.downloads.DownloadService
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.media.MediaService
import org.mozilla.fenix.search.telemetry.ads.AdsTelemetry
Expand All @@ -77,7 +77,11 @@ import java.util.concurrent.TimeUnit
* Component group for all core browser functionality.
*/
@Mockable
class Core(private val context: Context, private val crashReporter: CrashReporting) {
class Core(
private val context: Context,
private val crashReporter: CrashReporting,
strictMode: StrictModeManager
) {
/**
* The browser engine component initialized based on the build
* configuration (see build variants).
Expand Down Expand Up @@ -268,7 +272,11 @@ class Core(private val context: Context, private val crashReporter: CrashReporti
val bookmarksStorage by lazy { lazyBookmarksStorage.value }
val passwordsStorage by lazy { lazyPasswordsStorage.value }

val tabCollectionStorage by lazy { TabCollectionStorage(context, sessionManager) }
val tabCollectionStorage by lazy { TabCollectionStorage(
context,
sessionManager,
strictMode
) }

/**
* A storage component for persisting thumbnail images of tabs.
Expand All @@ -280,7 +288,7 @@ class Core(private val context: Context, private val crashReporter: CrashReporti
val topSitesStorage by lazy {
val defaultTopSites = mutableListOf<Pair<String, String>>()

StrictMode.allowThreadDiskReads().resetPoliciesAfter {
strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
if (!context.settings().defaultTopSitesAdded) {
defaultTopSites.add(
Pair(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ import mozilla.components.feature.tab.collections.TabCollection
import mozilla.components.feature.tab.collections.TabCollectionStorage
import mozilla.components.support.base.observer.Observable
import mozilla.components.support.base.observer.ObserverRegistry
import org.mozilla.fenix.StrictModeManager
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.ext.toShortUrl
import org.mozilla.fenix.home.sessioncontrol.viewholders.CollectionViewHolder
import org.mozilla.fenix.utils.Mockable
Expand All @@ -29,6 +29,7 @@ import org.mozilla.fenix.utils.Mockable
class TabCollectionStorage(
private val context: Context,
private val sessionManager: SessionManager,
strictMode: StrictModeManager,
private val delegate: Observable<Observer> = ObserverRegistry()
) : Observable<org.mozilla.fenix.components.TabCollectionStorage.Observer> by delegate {

Expand Down Expand Up @@ -56,7 +57,7 @@ class TabCollectionStorage(
var cachedTabCollections = listOf<TabCollection>()

private val collectionStorage by lazy {
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
TabCollectionStorage(context, sessionManager)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import mozilla.components.support.locale.LocaleManager
import org.mozilla.fenix.BuildConfig
import org.mozilla.fenix.StrictModeManager
import org.mozilla.fenix.components.metrics.MozillaProductDetector.MozillaProducts
import org.mozilla.fenix.ext.resetPoliciesAfter
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.home.intent.DeepLinkIntentProcessor
import java.util.Locale
Expand Down Expand Up @@ -58,6 +58,7 @@ private val Event.name: String?

class LeanplumMetricsService(
private val application: Application,
strictMode: StrictModeManager,
private val deviceIdGenerator: () -> String = { randomUUID().toString() }
) : MetricsService, DeepLinkIntentProcessor.DeepLinkVerifier {
val scope = CoroutineScope(Dispatchers.IO)
Expand All @@ -83,7 +84,7 @@ class LeanplumMetricsService(
override val type = MetricServiceType.Marketing
private val token = Token(LeanplumId, LeanplumToken)

private val preferences = StrictMode.allowThreadDiskReads().resetPoliciesAfter {
private val preferences = strictMode.resetAfter(StrictMode.allowThreadDiskReads()) {
application.getSharedPreferences(PREFERENCE_NAME, MODE_PRIVATE)
}

Expand Down

0 comments on commit f19c992

Please sign in to comment.