Skip to content

Commit

Permalink
For mozilla-mobile#13959: change StrictModeManager to class from object.
Browse files Browse the repository at this point in the history
I originally tried to create this PR leaving this as an object to keep
the change simple but it wasn't worth it - once the object started to
keep state, we'd need to manually reset the state between runs. Also,
the tests were already getting hacky with static mocking so it was
easier to address some of those issues this way too.
  • Loading branch information
mcomella committed Sep 29, 2020
1 parent d4ab728 commit 6abeb2d
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 27 deletions.
3 changes: 1 addition & 2 deletions app/src/main/java/org/mozilla/fenix/FenixApplication.kt
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import mozilla.components.support.rusthttp.RustHttpConfig
import mozilla.components.support.rustlog.RustLog
import mozilla.components.support.utils.logElapsedTime
import mozilla.components.support.webextensions.WebExtensionSupport
import org.mozilla.fenix.StrictModeManager.enableStrictMode
import org.mozilla.fenix.components.Components
import org.mozilla.fenix.components.metrics.MetricServiceType
import org.mozilla.fenix.ext.components
Expand Down Expand Up @@ -126,7 +125,7 @@ open class FenixApplication : LocaleAwareApplication(), Provider {
val megazordSetup = setupMegazord()

setDayNightTheme()
enableStrictMode(true)
components.strictMode.enableStrictMode(true)
warmBrowsersCache()

// Make sure the engine is initialized and ready to use.
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/org/mozilla/fenix/HomeActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity {
private lateinit var navigationToolbar: Toolbar

final override fun onCreate(savedInstanceState: Bundle?) {
StrictModeManager.attachListenerToDisablePenaltyDeath(supportFragmentManager)
components.strictMode.attachListenerToDisablePenaltyDeath(supportFragmentManager)
// There is disk read violations on some devices such as samsung and pixel for android 9/10
StrictMode.allowThreadDiskReads().resetPoliciesAfter {
super.onCreate(savedInstanceState)
Expand Down
21 changes: 9 additions & 12 deletions app/src/main/java/org/mozilla/fenix/StrictModeManager.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,24 @@ import android.os.StrictMode
import androidx.fragment.app.Fragment
import androidx.fragment.app.FragmentManager

private const val MANUFACTURE_HUAWEI: String = "HUAWEI"
private const val MANUFACTURE_ONE_PLUS: String = "OnePlus"

/**
* Manages strict mode settings for the application.
*
* Due to the static dependencies in this class, it's getting hard to test: if this class takes any
* many more static dependencies, consider switching it, and other code that uses [StrictMode] like
* [StrictMode.ThreadPolicy].resetPoliciesAfter, to a class with dependency injection.
*/
object StrictModeManager {
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

/***
* Enables strict mode for debug purposes. meant to be run only in the main process.
* @param setPenaltyDeath boolean value to decide setting the penaltyDeath as a penalty.
*/
fun enableStrictMode(setPenaltyDeath: Boolean) {
// The expression in this if is duplicated in StrictMode.ThreadPolicy.resetPoliciesAfter
// because the tests break in unexpected ways if the value is shared as a constant in this
// class. It wasn't worth the time to address it.
if (Config.channel.isDebug) {
if (isEnabledByBuildConfig) {
val threadPolicy = StrictMode.ThreadPolicy.Builder()
.detectAll()
.penaltyLog()
Expand Down Expand Up @@ -67,9 +67,6 @@ object StrictModeManager {
}, false)
}

private const val MANUFACTURE_HUAWEI: String = "HUAWEI"
private const val MANUFACTURE_ONE_PLUS: String = "OnePlus"

/**
* 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
2 changes: 2 additions & 0 deletions app/src/main/java/org/mozilla/fenix/components/Components.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import mozilla.components.support.migration.state.MigrationStore
import org.mozilla.fenix.BuildConfig
import org.mozilla.fenix.Config
import org.mozilla.fenix.HomeActivity
import org.mozilla.fenix.StrictModeManager
import org.mozilla.fenix.components.metrics.AppStartupTelemetry
import org.mozilla.fenix.ext.settings
import org.mozilla.fenix.utils.ClipboardHandler
Expand Down Expand Up @@ -126,6 +127,7 @@ class Components(private val context: Context) {
val performance by lazy { PerformanceComponent() }
val push by lazy { Push(context, analytics.crashReporter) }
val wifiConnectionMonitor by lazy { WifiConnectionMonitor(context as Application) }
val strictMode by lazy { StrictModeManager(Config) }

val settings by lazy { Settings(context) }

Expand Down
27 changes: 15 additions & 12 deletions app/src/test/java/org/mozilla/fenix/StrictModeManagerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@ import io.mockk.confirmVerified
import io.mockk.every
import io.mockk.impl.annotations.MockK
import io.mockk.mockk
import io.mockk.mockkObject
import io.mockk.mockkStatic
import io.mockk.slot
import io.mockk.unmockkObject
import io.mockk.unmockkStatic
import io.mockk.verify
import org.junit.After
Expand All @@ -26,44 +24,49 @@ import org.mozilla.fenix.helpers.FenixRobolectricTestRunner
@RunWith(FenixRobolectricTestRunner::class)
class StrictModeManagerTest {

private lateinit var debugManager: StrictModeManager
private lateinit var releaseManager: StrictModeManager

@MockK(relaxUnitFun = true) private lateinit var fragmentManager: FragmentManager

@Before
fun setup() {
MockKAnnotations.init(this)
mockkStatic(StrictMode::class)
mockkObject(Config)

// These tests log a warning that mockk couldn't set the backing field of Config.channel
// but it doesn't seem to impact their correctness so I'm ignoring it.
val debugConfig: Config = mockk { every { channel } returns ReleaseChannel.Debug }
debugManager = StrictModeManager(debugConfig)

val releaseConfig: Config = mockk { every { channel } returns ReleaseChannel.Release }
releaseManager = StrictModeManager(releaseConfig)
}

@After
fun teardown() {
unmockkStatic(StrictMode::class)
unmockkObject(Config)
}

@Test
fun `test enableStrictMode in release`() {
every { Config.channel } returns ReleaseChannel.Release
StrictModeManager.enableStrictMode(false)

releaseManager.enableStrictMode(false)
verify(exactly = 0) { StrictMode.setThreadPolicy(any()) }
verify(exactly = 0) { StrictMode.setVmPolicy(any()) }
}

@Test
fun `test enableStrictMode in debug`() {
every { Config.channel } returns ReleaseChannel.Debug
StrictModeManager.enableStrictMode(false)

debugManager.enableStrictMode(false)
verify { StrictMode.setThreadPolicy(any()) }
verify { StrictMode.setVmPolicy(any()) }
}

@Test
fun `test changeStrictModePolicies`() {
fun `test changeStrictModePolicies in debug`() {
val callbacks = slot<FragmentManager.FragmentLifecycleCallbacks>()

StrictModeManager.attachListenerToDisablePenaltyDeath(fragmentManager)
debugManager.attachListenerToDisablePenaltyDeath(fragmentManager)
verify { fragmentManager.registerFragmentLifecycleCallbacks(capture(callbacks), false) }
confirmVerified(fragmentManager)

Expand Down

0 comments on commit 6abeb2d

Please sign in to comment.