For #13959: add MozillaStrictModeSuppression detekt check.
This commit is contained in:
parent
66f220c22a
commit
3b5d6d58d9
|
@ -2,6 +2,11 @@
|
||||||
* License, v. 2.0. If a copy of the MPL was not distributed with this
|
* 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/. */
|
* 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
|
package org.mozilla.fenix
|
||||||
|
|
||||||
import android.os.Build
|
import android.os.Build
|
||||||
|
@ -26,6 +31,14 @@ class StrictModeManager(config: Config) {
|
||||||
// This is public so it can be used by inline functions.
|
// This is public so it can be used by inline functions.
|
||||||
val isEnabledByBuildConfig = config.channel.isDebug
|
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)
|
@VisibleForTesting(otherwise = PRIVATE)
|
||||||
var suppressionCount: Long = 0
|
var suppressionCount: Long = 0
|
||||||
|
|
||||||
|
@ -78,12 +91,14 @@ class StrictModeManager(config: Config) {
|
||||||
/**
|
/**
|
||||||
* Runs the given [functionBlock] and sets the given [StrictMode.ThreadPolicy] after its
|
* 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
|
* 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 function contains perf improvements so it should be
|
||||||
*
|
* called instead of [mozilla.components.support.ktx.android.os.resetAfter] (using the wrong
|
||||||
* This is significantly less convenient to run than when it was written as an extension function
|
* method should be prevented by a lint check). This is significantly less convenient to run than
|
||||||
* on [StrictMode.ThreadPolicy] but I think this is okay: it shouldn't be easy to ignore StrictMode.
|
* 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].
|
* @return the value returned by [functionBlock].
|
||||||
*/
|
*/
|
||||||
|
|
|
@ -112,6 +112,8 @@ mozilla-detekt-rules:
|
||||||
# with the debuggable flag or not. Use a check for different build variants,
|
# with the debuggable flag or not. Use a check for different build variants,
|
||||||
# instead.
|
# instead.
|
||||||
bannedProperties: "BuildConfig.DEBUG"
|
bannedProperties: "BuildConfig.DEBUG"
|
||||||
|
MozillaStrictModeSuppression:
|
||||||
|
active: true
|
||||||
MozillaCorrectUnitTestRunner:
|
MozillaCorrectUnitTestRunner:
|
||||||
active: true
|
active: true
|
||||||
|
|
||||||
|
|
|
@ -15,6 +15,7 @@ class CustomRulesetProvider : RuleSetProvider {
|
||||||
ruleSetId,
|
ruleSetId,
|
||||||
listOf(
|
listOf(
|
||||||
MozillaBannedPropertyAccess(config),
|
MozillaBannedPropertyAccess(config),
|
||||||
|
MozillaStrictModeSuppression(config),
|
||||||
MozillaCorrectUnitTestRunner(config)
|
MozillaCorrectUnitTestRunner(config)
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
|
|
|
@ -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))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
Loading…
Reference in New Issue