Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: Xapian crash scenarios. #3885

Merged
merged 8 commits into from
Jun 20, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import org.kiwix.kiwixmobile.core.data.DataModule
import org.kiwix.kiwixmobile.core.di.modules.ApplicationModule
import org.kiwix.kiwixmobile.core.di.modules.CoreViewModelModule
import org.kiwix.kiwixmobile.core.di.modules.JNIModule
import org.kiwix.kiwixmobile.core.di.modules.MutexModule
import org.kiwix.kiwixmobile.core.di.modules.SearchModule
import org.kiwix.kiwixmobile.core.di.modules.TestNetworkModule
import javax.inject.Singleton
Expand All @@ -42,7 +43,8 @@ import javax.inject.Singleton
JNIModule::class,
DataModule::class,
CoreViewModelModule::class,
SearchModule::class
SearchModule::class,
MutexModule::class
]
)
interface TestComponent : CoreComponent {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import androidx.core.content.ContextCompat
import androidx.core.content.edit
import androidx.core.net.toUri
import androidx.lifecycle.Lifecycle
import androidx.navigation.fragment.NavHostFragment
import androidx.preference.PreferenceManager
import androidx.test.core.app.ActivityScenario
import androidx.test.espresso.accessibility.AccessibilityChecks
Expand All @@ -31,6 +32,8 @@ import androidx.test.uiautomator.UiDevice
import com.google.android.apps.common.testing.accessibility.framework.AccessibilityCheckResultUtils.matchesCheck
import com.google.android.apps.common.testing.accessibility.framework.AccessibilityCheckResultUtils.matchesViews
import com.google.android.apps.common.testing.accessibility.framework.checks.TouchTargetSizeCheck
import kotlinx.coroutines.delay
import kotlinx.coroutines.runBlocking
import leakcanary.LeakAssertions
import okhttp3.OkHttpClient
import okhttp3.Request
Expand All @@ -41,6 +44,8 @@ import org.junit.Rule
import org.junit.Test
import org.kiwix.kiwixmobile.BaseActivityTest
import org.kiwix.kiwixmobile.R
import org.kiwix.kiwixmobile.core.search.SearchFragment
import org.kiwix.kiwixmobile.core.search.viewmodel.Action
import org.kiwix.kiwixmobile.core.utils.LanguageUtils.Companion.handleLocaleChange
import org.kiwix.kiwixmobile.core.utils.SharedPreferenceUtil
import org.kiwix.kiwixmobile.main.KiwixMainActivity
Expand Down Expand Up @@ -205,9 +210,60 @@ class SearchFragmentTest : BaseActivityTest() {
LeakAssertions.assertNoLeaks()
}

@Test
fun testConcurrencyOfSearch() = runBlocking {
val searchTerms = listOf(
"A Song",
"The Ra",
"The Ge",
"Wish",
"WIFI",
"Woman",
"Big Ba",
"My Wor",
"100"
)
activityScenario.onActivity {
kiwixMainActivity = it
kiwixMainActivity.navigate(R.id.libraryFragment)
}
downloadingZimFile = getDownloadingZimFile(false)
openKiwixReaderFragmentWithFile(downloadingZimFile)
search { checkZimFileSearchSuccessful(R.id.readerFragment) }
openSearchWithQuery(searchTerms[0], downloadingZimFile)
// wait for searchFragment become visible on screen.
delay(2000)
val navHostFragment: NavHostFragment =
kiwixMainActivity.supportFragmentManager
.findFragmentById(R.id.nav_host_fragment) as NavHostFragment
val searchFragment = navHostFragment.childFragmentManager.fragments[0] as SearchFragment
for (i in 1..100) {
// This will execute the render method 100 times frequently.
val searchTerm = searchTerms[i % searchTerms.size]
searchFragment.searchViewModel.actions.trySend(Action.Filter(searchTerm)).isSuccess
}
for (i in 1..100) {
// this will execute the render method 100 times with 100MS delay.
delay(100)
val searchTerm = searchTerms[i % searchTerms.size]
searchFragment.searchViewModel.actions.trySend(Action.Filter(searchTerm)).isSuccess
}
for (i in 1..100) {
// this will execute the render method 100 times with 200MS delay.
delay(200)
val searchTerm = searchTerms[i % searchTerms.size]
searchFragment.searchViewModel.actions.trySend(Action.Filter(searchTerm)).isSuccess
}
for (i in 1..100) {
// this will execute the render method 100 times with 200MS delay.
delay(300)
val searchTerm = searchTerms[i % searchTerms.size]
searchFragment.searchViewModel.actions.trySend(Action.Filter(searchTerm)).isSuccess
}
}

private fun removeTemporaryZimFilesToFreeUpDeviceStorage() {
testZimFile.delete()
downloadingZimFile.delete()
}

private fun openKiwixReaderFragmentWithFile(zimFile: File) {
Expand Down Expand Up @@ -273,10 +329,12 @@ class SearchFragmentTest : BaseActivityTest() {
return zimFile
}

private fun getDownloadingZimFile(): File {
private fun getDownloadingZimFile(isDeletePreviousZimFile: Boolean = true): File {
val zimFile = File(context.cacheDir, "ray_charles.zim")
if (zimFile.exists()) zimFile.delete()
zimFile.createNewFile()
if (isDeletePreviousZimFile) {
if (zimFile.exists()) zimFile.delete()
zimFile.createNewFile()
}
return zimFile
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import android.net.wifi.WifiManager
import dagger.BindsInstance
import dagger.Component
import eu.mhutti1.utils.storage.StorageSelectDialog
import kotlinx.coroutines.sync.Mutex
import org.kiwix.kiwixmobile.core.CoreApp
import org.kiwix.kiwixmobile.core.StorageObserver
import org.kiwix.kiwixmobile.core.dao.FetchDownloadDao
Expand All @@ -46,6 +47,7 @@ import org.kiwix.kiwixmobile.core.data.remote.ObjectBoxToRoomMigrator
import org.kiwix.kiwixmobile.core.di.modules.ApplicationModule
import org.kiwix.kiwixmobile.core.di.modules.CoreViewModelModule
import org.kiwix.kiwixmobile.core.di.modules.JNIModule
import org.kiwix.kiwixmobile.core.di.modules.MutexModule
import org.kiwix.kiwixmobile.core.di.modules.NetworkModule
import org.kiwix.kiwixmobile.core.di.modules.SearchModule
import org.kiwix.kiwixmobile.core.downloader.Downloader
Expand All @@ -68,7 +70,8 @@ import javax.inject.Singleton
JNIModule::class,
DataModule::class,
CoreViewModelModule::class,
SearchModule::class
SearchModule::class,
MutexModule::class
]
)
interface CoreComponent {
Expand Down Expand Up @@ -109,6 +112,7 @@ interface CoreComponent {
fun downloader(): Downloader
fun notificationManager(): NotificationManager
fun searchResultGenerator(): SearchResultGenerator
fun mutex(): Mutex

fun inject(application: CoreApp)
fun inject(kiwixWebView: KiwixWebView)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Kiwix Android
* Copyright (c) 2024 Kiwix <android.kiwix.org>
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package org.kiwix.kiwixmobile.core.di.modules

import dagger.Module
import dagger.Provides
import kotlinx.coroutines.sync.Mutex
import javax.inject.Singleton

@Module
class MutexModule {

@Provides
@Singleton
fun provideMutex(): Mutex = Mutex()
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,15 @@
import androidx.lifecycle.Lifecycle
import androidx.lifecycle.ViewModelProvider
import androidx.lifecycle.lifecycleScope
import androidx.lifecycle.viewModelScope
import androidx.navigation.fragment.findNavController
import androidx.recyclerview.widget.LinearLayoutManager
import androidx.recyclerview.widget.RecyclerView
import io.reactivex.android.schedulers.AndroidSchedulers
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.cancelAndJoin
import kotlinx.coroutines.cancelChildren
import kotlinx.coroutines.launch
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import kotlinx.coroutines.withContext
import org.kiwix.kiwixmobile.core.R
import org.kiwix.kiwixmobile.core.base.BaseActivity
Expand All @@ -74,6 +72,7 @@
import org.kiwix.kiwixmobile.core.search.viewmodel.SearchState
import org.kiwix.kiwixmobile.core.search.viewmodel.SearchViewModel
import org.kiwix.kiwixmobile.core.utils.SimpleTextListener
import org.kiwix.kiwixmobile.core.utils.files.Log
import javax.inject.Inject

const val NAV_ARG_SEARCH_STRING = "searchString"
Expand All @@ -91,11 +90,10 @@
private var findInPageTextView: TextView? = null
private var fragmentSearchBinding: FragmentSearchBinding? = null

private val searchViewModel by lazy { viewModel<SearchViewModel>(viewModelFactory) }
val searchViewModel by lazy { viewModel<SearchViewModel>(viewModelFactory) }
private var searchAdapter: SearchAdapter? = null
private var isDataLoading = false
private var renderingJob: Job? = null
private val searchMutex = Mutex()

override fun inject(baseActivity: BaseActivity) {
baseActivity.cachedComponent.inject(this)
Expand Down Expand Up @@ -155,27 +153,29 @@
@SuppressLint("CheckResult")
private fun loadMoreSearchResult() {
if (isDataLoading) return
val safeStartIndex = searchAdapter?.itemCount ?: 0
isDataLoading = true
val safeStartIndex = searchAdapter?.itemCount ?: 0
// Show a loading indicator while data is being loaded
fragmentSearchBinding?.loadingMoreDataIndicator?.isShowing(true)
// Request more search results from the ViewModel, providing the start index and existing results
searchViewModel.loadMoreSearchResults(safeStartIndex, searchAdapter?.items)
.observeOn(AndroidSchedulers.mainThread())
.subscribe { searchResults ->
// Hide the loading indicator when data loading is complete
fragmentSearchBinding?.loadingMoreDataIndicator?.isShowing(false)
// Update data loading status based on the received search results
isDataLoading = when {
searchResults == null -> true
searchResults.isEmpty() -> false
else -> {
// Append the new search results to the existing list
searchAdapter?.addData(searchResults)
false
lifecycleScope.launch {
// Request more search results from the ViewModel, providing the start
// index and existing results
searchViewModel.loadMoreSearchResults(safeStartIndex, searchAdapter?.items)
.let { searchResults ->
// Hide the loading indicator when data loading is complete
fragmentSearchBinding?.loadingMoreDataIndicator?.isShowing(false)
// Update data loading status based on the received search results
isDataLoading = when {
searchResults == null -> true
searchResults.isEmpty() -> false
else -> {
// Append the new search results to the existing list
searchAdapter?.addData(searchResults)
false
}
}
}
}
}
}

private fun handleBackPress() {
Expand Down Expand Up @@ -284,7 +284,14 @@
it.value.equals(query, ignoreCase = true)
}

private suspend fun render(state: SearchState) {
suspend fun render(state: SearchState) {
renderingJob?.apply {
// cancel the children job. Since we are getting the result on IO thread
// with `withContext` that is child for this job
cancelChildren()
// `cancelAndJoin` cancels the previous running job and waits for it to completely cancel.
cancelAndJoin()
}
// Check if the fragment is visible on the screen. This method called multiple times
// (7-14 times) when an item in the search list is clicked, which leads to unnecessary
// data loading and also causes a crash.
Expand All @@ -293,26 +300,34 @@
// To avoid unnecessary data loading and prevent crashes, we check if the search screen is
// visible to the user before proceeding. If the screen is not visible,
// we skip the data loading process.
if (!isVisible) return
searchMutex.withLock {
// `cancelAndJoin` cancels the previous running job and waits for it to completely cancel.
renderingJob?.cancelAndJoin()
isDataLoading = false
searchInTextMenuItem?.actionView?.isVisible = state.searchOrigin == FromWebView
setIsPageSearchEnabled(state.searchTerm.isNotBlank())
if (!isVisible) {
return
}
isDataLoading = false
searchInTextMenuItem?.actionView?.isVisible = state.searchOrigin == FromWebView
setIsPageSearchEnabled(state.searchTerm.isNotBlank())

fragmentSearchBinding?.searchLoadingIndicator?.isShowing(true)
renderingJob = searchViewModel.viewModelScope.launch(Dispatchers.Main) {
fragmentSearchBinding?.searchLoadingIndicator?.isShowing(true)
renderingJob = lifecycleScope.launch {
try {
val searchResult = withContext(Dispatchers.IO) {
state.getVisibleResults(0, renderingJob)
state.getVisibleResults(0, coroutineContext[Job])
}

fragmentSearchBinding?.searchLoadingIndicator?.isShowing(false)

searchResult?.let {
fragmentSearchBinding?.searchNoResults?.isVisible = it.isEmpty()
searchAdapter?.items = it
}
} catch (ignore: CancellationException) {
Log.e("SEARCH_RESULT", "Cancelled the previous job ${ignore.message}")
} catch (ignore: Exception) {
Log.e(
"SEARCH_RESULT",
"Error in getting searched result\nOriginal exception ${ignore.message}"

Check warning on line 327 in core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt#L324-L327

Added lines #L324 - L327 were not covered by tests
)
} finally {
fragmentSearchBinding?.searchLoadingIndicator?.isShowing(false)
}
}
}
Expand Down
Loading
Loading