Merge pull request #3885 from kiwix/Fixes#3877

Fixed: Xapian crash scenarios.

app/src/androidTest/java/org/kiwix/kiwixmobile/core/di/components/TestComponent.kt

@@ -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
@@ -42,7 +43,8 @@ import javax.inject.Singleton
     JNIModule::class,
     DataModule::class,
     CoreViewModelModule::class,
-    SearchModule::class
+    SearchModule::class,
+    MutexModule::class
   ]
 )
 interface TestComponent : CoreComponent {

app/src/androidTest/java/org/kiwix/kiwixmobile/search/SearchFragmentTest.kt

@@ -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
@@ -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
@@ -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
@@ -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) {
@@ -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
   }
 }

core/src/main/java/org/kiwix/kiwixmobile/core/di/components/CoreComponent.kt

@@ -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
@@ -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
@@ -68,7 +70,8 @@ import javax.inject.Singleton
     JNIModule::class,
     DataModule::class,
     CoreViewModelModule::class,
-    SearchModule::class
+    SearchModule::class,
+    MutexModule::class
   ]
 )
 interface CoreComponent {
@@ -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)

core/src/main/java/org/kiwix/kiwixmobile/core/di/modules/MutexModule.kt

@@ -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()
+}

core/src/main/java/org/kiwix/kiwixmobile/core/search/SearchFragment.kt

@@ -37,17 +37,15 @@ import androidx.core.widget.ContentLoadingProgressBar
 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
@@ -74,6 +72,7 @@ import org.kiwix.kiwixmobile.core.search.viewmodel.SearchOrigin.FromWebView
 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"
@@ -91,11 +90,10 @@ class SearchFragment : BaseFragment() {
   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)
@@ -155,27 +153,29 @@ class SearchFragment : BaseFragment() {
   @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() {
@@ -284,7 +284,14 @@ class SearchFragment : BaseFragment() {
       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.
@@ -293,26 +300,34 @@ class SearchFragment : BaseFragment() {
     // 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}"
+        )
+      } finally {
+        fragmentSearchBinding?.searchLoadingIndicator?.isShowing(false)
       }
     }
   }