blallo
/
fenix
1
0
Fork 0
Code Releases Activity

For #747. Fix coroutines leaks in `HistoryFragment`.

master
Denys M 2019-05-13 23:50:42 +03:00 committed by Jeff Boek
parent b05e9ab14b
commit 13f144f212
3 changed files with 46 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
app/src/main/java/org/mozilla/fenix/ext/String.kt
View File

@ -18,13 +18,14 @@ fun String.replace(pairs: Map<String, String>): String {
return result
}
fun String?.urlToHost(): String {
return try {
val url = URL(this)
url.host
} catch (e: MalformedURLException) {
""
}
/**
* Try to parse and get host part if this [String] is valid URL.
* Returns **null** otherwise.
*/
fun String?.getHostFromUrl(): String? = try {
URL(this).host
} catch (e: MalformedURLException) {
null
}
fun String?.urlToTrimmedHost(): String {
@ -40,8 +41,8 @@ fun String?.urlToTrimmedHost(): String {
else -> url.host
}
} catch (e: MalformedURLException) {
this.urlToHost()
this.getHostFromUrl() ?: ""
} catch (e: StringIndexOutOfBoundsException) {
this.urlToHost()
this.getHostFromUrl() ?: ""
}
}

73
app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt
View File

@ -8,7 +8,6 @@ import android.content.DialogInterface
import android.graphics.PorterDuff
import android.graphics.PorterDuffColorFilter
import android.os.Bundle
import android.text.TextUtils
import android.view.LayoutInflater
import android.view.Menu
import android.view.MenuInflater
@ -23,11 +22,12 @@ import androidx.fragment.app.Fragment
import androidx.navigation.Navigation
import kotlinx.android.synthetic.main.fragment_history.view.*
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Job
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Dispatchers.Main
import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.MainScope
import kotlinx.coroutines.cancel
import kotlinx.coroutines.launch
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.withContext
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.base.feature.BackHandler
import org.mozilla.fenix.BrowserDirection
@ -37,6 +37,7 @@ import org.mozilla.fenix.HomeActivity
import org.mozilla.fenix.R
import org.mozilla.fenix.components.Components
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.ext.getHostFromUrl
import org.mozilla.fenix.ext.requireComponents
import org.mozilla.fenix.mvi.ActionBusFactory
import org.mozilla.fenix.mvi.getAutoDisposeObservable
@ -45,17 +46,14 @@ import org.mozilla.fenix.share.ShareTab
import java.net.MalformedURLException
import java.net.URL
import kotlin.coroutines.CoroutineContext
import java.util.concurrent.TimeUnit
@SuppressWarnings("TooManyFunctions")
class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
class HistoryFragment : Fragment(), CoroutineScope by MainScope(), BackHandler {
private lateinit var job: Job
private lateinit var historyComponent: HistoryComponent
private val navigation by lazy { Navigation.findNavController(requireView()) }
override val coroutineContext: CoroutineContext
get() = Dispatchers.Main + job
override fun onCreateView(
inflater: LayoutInflater,
container: ViewGroup?,
@ -76,7 +74,6 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
job = Job()
setHasOptionsMenu(true)
}
@ -95,8 +92,8 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
}
override fun onDestroy() {
coroutineContext.cancel()
super.onDestroy()
job.cancel()
}
override fun onCreateOptionsMenu(menu: Menu, inflater: MenuInflater) {
@ -117,9 +114,8 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
launch(Dispatchers.IO) {
reloadData()
}
launch { reloadData() }
}
// This method triggers the complexity warning. However it's actually not that hard to understand.
@ -139,10 +135,10 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
is HistoryAction.BackPressed -> getManagedEmitter<HistoryChange>()
.onNext(HistoryChange.ExitEditMode)
is HistoryAction.Delete.All -> {
activity?.let {
activity?.let { activity ->
AlertDialog.Builder(
ContextThemeWrapper(
it,
activity,
R.style.DeleteDialogStyle
)
).apply {
@ -151,7 +147,7 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
dialog.cancel()
}
setPositiveButton(R.string.history_clear_dialog) { dialog: DialogInterface, _ ->
launch(Dispatchers.IO) {
launch {
requireComponents.core.historyStorage.deleteEverything()
reloadData()
}
@ -161,13 +157,14 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
}.show()
}
}
is HistoryAction.Delete.One -> launch(Dispatchers.IO) {
is HistoryAction.Delete.One -> launch {
requireComponents.core.historyStorage.deleteVisit(it.item.url, it.item.visitedAt)
reloadData()
}
is HistoryAction.Delete.Some -> launch(Dispatchers.IO) {
it.items.forEach { item ->
requireComponents.core.historyStorage.deleteVisit(item.url, item.visitedAt)
is HistoryAction.Delete.Some -> launch {
val storage = requireComponents.core.historyStorage
for (item in it.items) {
storage.deleteVisit(item.url, item.visitedAt)
}
reloadData()
}
@ -199,7 +196,7 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
val components = context?.applicationContext?.components!!
val selectedHistory = (historyComponent.uiView as HistoryUIView).getSelected()
CoroutineScope(Main).launch {
GlobalScope.launch(Main) {
deleteSelectedHistory(selectedHistory, components)
reloadData()
}
@ -243,33 +240,29 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
VisitType.FRAMED_LINK,
VisitType.REDIRECT_PERMANENT
)
// Until we have proper pagination, only display a limited set of history to avoid blowing up the UI.
// See https://github.com/mozilla-mobile/fenix/issues/1393
@SuppressWarnings("MagicNumber")
val historyCutoffMs = 1000L * 60 * 60 * 24 * 3 // past few days
val items = requireComponents.core.historyStorage.getDetailedVisits(
System.currentTimeMillis() - historyCutoffMs, excludeTypes = excludeTypes
)
val sinceTimeMs = System.currentTimeMillis() - TimeUnit.DAYS.toMillis(HISTORY_TIME_DAYS)
val items = requireComponents.core.historyStorage
.getDetailedVisits(sinceTimeMs, excludeTypes = excludeTypes)
// We potentially have a large amount of visits, and multiple processing steps.
// Wrapping iterator in a sequence should make this a little more efficient.
.asSequence()
.sortedByDescending { it.visitTime }
.mapIndexed { id, item ->
HistoryItem(
id, if (TextUtils.isEmpty(item.title!!)) try {
URL(item.url).host
} catch (e: MalformedURLException) {
item.url
} else item.title!!, item.url, item.visitTime
)
val title = item.title
?.takeIf(String::isNotEmpty)
?: item.url.getHostFromUrl()
?: item.url
HistoryItem(id, title, item.url, item.visitTime)
}
.toList()
coroutineScope {
launch(Dispatchers.Main) {
getManagedEmitter<HistoryChange>().onNext(HistoryChange.Change(items))
}
withContext(Main) {
getManagedEmitter<HistoryChange>()
.onNext(HistoryChange.Change(items))
}
}
@ -288,3 +281,5 @@ class HistoryFragment : Fragment(), CoroutineScope, BackHandler {
Navigation.findNavController(view!!).navigate(directions)
}
}
private const val HISTORY_TIME_DAYS = 3L

3
buildSrc/src/main/java/Dependencies.kt
View File

@ -4,7 +4,7 @@
private object Versions {
const val kotlin = "1.3.30"
const val coroutines = "1.2.0-alpha-2"
const val coroutines = "1.2.1"
const val android_gradle_plugin = "3.3.2"
const val rxAndroid = "2.1.0"
const val rxKotlin = "2.3.0"
@ -65,6 +65,7 @@ object Deps {
const val tools_kotlingradle = "org.jetbrains.kotlin:kotlin-gradle-plugin:${Versions.kotlin}"
const val kotlin_stdlib = "org.jetbrains.kotlin:kotlin-stdlib-jdk7:${Versions.kotlin}"
const val kotlin_coroutines = "org.jetbrains.kotlinx:kotlinx-coroutines-core:${Versions.coroutines}"
const val kotlin_coroutines_android = "org.jetbrains.kotlinx:kotlinx-coroutines-android:${Versions.coroutines}"
const val allopen = "org.jetbrains.kotlin:kotlin-allopen:${Versions.kotlin}"