diff --git a/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt b/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt index 13448b305..16ffa3d79 100644 --- a/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt +++ b/app/src/main/java/org/mozilla/fenix/StrictModeManager.kt @@ -2,6 +2,11 @@ * 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 suppress StrictMode with performance +// monitoring by wrapping the raw methods. This lint check tells us not to use the raw +// methods so we suppress the check. +@file:Suppress("MozillaStrictModeSuppression") + package org.mozilla.fenix import android.os.Build @@ -26,6 +31,14 @@ class StrictModeManager(config: Config) { // This is public so it can be used by inline functions. val isEnabledByBuildConfig = config.channel.isDebug + /** + * The number of times StrictMode has been suppressed. StrictMode can be used to prevent main + * thread IO but it's easy to suppress. We use this value, in combination with: + * - a test: that fails if the suppression count increases + * - a lint check: to ensure this value gets used instead of functions that work around it + * - code owners: to prevent modifications to these above items without perf knowing + * to make suppressions a more deliberate act. + */ @VisibleForTesting(otherwise = PRIVATE) var suppressionCount: Long = 0 @@ -78,12 +91,14 @@ class StrictModeManager(config: Config) { /** * 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]. + * not enabled, simply runs the [functionBlock]. This function is written in the style of + * [AutoCloseable.use]. * - * 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. + * This function contains perf improvements so it should be + * called instead of [mozilla.components.support.ktx.android.os.resetAfter] (using the wrong + * method should be prevented by a lint check). 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]. */ diff --git a/config/detekt.yml b/config/detekt.yml index 180847b99..d178ee322 100644 --- a/config/detekt.yml +++ b/config/detekt.yml @@ -112,6 +112,8 @@ mozilla-detekt-rules: # with the debuggable flag or not. Use a check for different build variants, # instead. bannedProperties: "BuildConfig.DEBUG" + MozillaStrictModeSuppression: + active: true MozillaCorrectUnitTestRunner: active: true diff --git a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt index 1a4c0f67e..509b4efe8 100644 --- a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt +++ b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt @@ -15,6 +15,7 @@ class CustomRulesetProvider : RuleSetProvider { ruleSetId, listOf( MozillaBannedPropertyAccess(config), + MozillaStrictModeSuppression(config), MozillaCorrectUnitTestRunner(config) ) ) diff --git a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaStrictModeSuppression.kt b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaStrictModeSuppression.kt new file mode 100644 index 000000000..00fe2e8e7 --- /dev/null +++ b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaStrictModeSuppression.kt @@ -0,0 +1,69 @@ +/* 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.detektrules + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import org.jetbrains.kotlin.psi.KtCallExpression +import org.jetbrains.kotlin.psi.KtImportDirective + +private const val VIOLATION_MSG = "Please use `components.strictMode.resetAfter` instead because it has " + + "performance improvements and additional code to monitor for performance regressions." + +/** + * A check to prevent us from working around mechanisms we implemented to prevent suppressing StrictMode. + * + * IF YOU UPDATE THIS FILE NAME, UPDATE CODE OWNERS. + */ +class MozillaStrictModeSuppression(config: Config) : Rule(config) { + override val issue = Issue( + "MozillaStrictModeSuppression", + Severity.Performance, + "Prevents us from working around mechanisms we implemented to prevent suppressing StrictMode", + Debt.TEN_MINS + ) + + override fun visitImportDirective(importDirective: KtImportDirective) { + super.visitImportDirective(importDirective) + reportIfImportAcResetAfter(importDirective) + } + + override fun visitCallExpression(expression: KtCallExpression) { + super.visitCallExpression(expression) + reportIfCallStrictModeSetPolicy(expression) + } + + private fun reportIfImportAcResetAfter(importDirective: KtImportDirective) { + if (importDirective.importPath?.toString() == "mozilla.components.support.ktx.android.os.resetAfter") { + report(CodeSmell(issue, Entity.from(importDirective), VIOLATION_MSG)) + } + } + + private fun reportIfCallStrictModeSetPolicy(expression: KtCallExpression) { + // There is probably a more correct way of doing this but this API is not well documented + // so it's not worth our time, I think. + val receiver = expression.parent?.firstChild?.node?.chars + val calledMethod = expression.calleeExpression?.firstChild?.node?.chars + + if (receiver == "StrictMode") { + val violationMsg = when (calledMethod) { + "setThreadPolicy" -> VIOLATION_MSG + "setVmPolicy" -> "NOT YET IMPLEMENTED: please consult the perf team about implementing" + + "`StrictModeManager.resetAfter`: we want to understand the performance implications " + + "of suppressing setVmPolicy before allowing it." + else -> null + } + + violationMsg?.let { msg -> + report(CodeSmell(issue, Entity.from(expression), msg)) + } + } + } +}