From 2f167b8403dc947ecb2f7b6f43385e36c781f464 Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 5 Oct 2026 16:16:07 +0530 Subject: [PATCH 1/4] Drop bookmark name resolution that lands after the bookmark is gone resolveNameIfNeeded launches a reverse geocode on Dispatchers.IO and never cancels it. If the bookmark is removed, or wiped by clearAll, while that lookup is in flight, the coroutine still writes the resolved place name into _bookmarkNames and persists it to SharedPreferences. clearAll is the panic path (ChatViewModel clears bookmarks so "panic should remove everything"), and its "clear any in-flight resolutions to avoid repopulating" comment does not hold: emptying the resolving set does not stop the coroutine that is already running. The result is a geocoded location name left on disk after a panic wipe, visible again on the next launch. Commit the resolved name through commitResolvedName, which drops it unless the geohash is still bookmarked. membership is now read from the IO dispatcher, so guard the accessors that touch it and move the resolving bookkeeping behind the same lock. --- .../android/geohash/GeohashBookmarksStore.kt | 55 ++++++++++++++++--- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt b/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt index 92e237cbd..4cc89492f 100644 --- a/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt +++ b/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt @@ -20,7 +20,10 @@ import java.util.Locale * - Persistence: SharedPreferences (JSON string array) * - Semantics: geohashes are normalized to lowercase base32 and de-duplicated */ -class GeohashBookmarksStore private constructor(private val context: Context) { +class GeohashBookmarksStore private constructor( + private val context: Context, + private val resolveNames: Boolean +) { companion object { private const val TAG = "GeohashBookmarksStore" @@ -30,10 +33,18 @@ class GeohashBookmarksStore private constructor(private val context: Context) { @Volatile private var INSTANCE: GeohashBookmarksStore? = null fun getInstance(context: Context): GeohashBookmarksStore { return INSTANCE ?: synchronized(this) { - INSTANCE ?: GeohashBookmarksStore(context.applicationContext).also { INSTANCE = it } + INSTANCE ?: GeohashBookmarksStore(context.applicationContext, resolveNames = true) + .also { INSTANCE = it } } } + /** + * Isolated store for unit tests. Reverse geocoding is disabled so a test never + * reaches the network; the persistence and bookkeeping paths are unchanged. + */ + internal fun createForTest(context: Context): GeohashBookmarksStore = + GeohashBookmarksStore(context.applicationContext, resolveNames = false) + private val allowedChars = "0123456789bcdefghjkmnpqrstuvwxyz".toSet() fun normalize(raw: String): String { return raw.trim().lowercase(Locale.US) @@ -58,13 +69,16 @@ class GeohashBookmarksStore private constructor(private val context: Context) { init { load() } + @Synchronized fun isBookmarked(geohash: String): Boolean = membership.contains(normalize(geohash)) + @Synchronized fun toggle(geohash: String) { val gh = normalize(geohash) if (membership.contains(gh)) remove(gh) else add(gh) } + @Synchronized fun add(geohash: String) { val gh = normalize(geohash) if (gh.isEmpty() || membership.contains(gh)) return @@ -76,6 +90,7 @@ class GeohashBookmarksStore private constructor(private val context: Context) { resolveNameIfNeeded(gh) } + @Synchronized fun remove(geohash: String) { val gh = normalize(geohash) if (!membership.contains(gh)) return @@ -142,6 +157,7 @@ class GeohashBookmarksStore private constructor(private val context: Context) { // MARK: - Destructive Reset + @Synchronized fun clearAll() { try { membership.clear() @@ -162,7 +178,9 @@ class GeohashBookmarksStore private constructor(private val context: Context) { // MARK: - Friendly Name Resolution + @Synchronized fun resolveNameIfNeeded(geohash: String) { + if (!resolveNames) return val gh = normalize(geohash) if (gh.isEmpty()) return if (_bookmarkNames.value?.containsKey(gh) == true) return @@ -206,20 +224,39 @@ class GeohashBookmarksStore private constructor(private val context: Context) { pickNameForLength(gh.length, a) } - if (!name.isNullOrEmpty()) { - val current = _bookmarkNames.value.toMutableMap() - current[gh] = name - _bookmarkNames.value = current - persistNames(current) - } + commitResolvedName(gh, name) } catch (e: Exception) { Log.w(TAG, "Bookmark name resolution failed") } finally { - resolving.remove(gh) + finishResolving(gh) } } } + @Synchronized + private fun finishResolving(geohash: String) { + resolving.remove(geohash) + } + + /** + * Stores a resolved friendly name, but only while the geohash is still bookmarked. + * + * A reverse geocode runs on the IO dispatcher and can outlive the bookmark it was + * started for. Without this check a lookup in flight during [remove] or [clearAll] + * writes the place name back to SharedPreferences after the wipe, so a panic clear + * leaves a resolved location name on disk for the next launch. + */ + @Synchronized + internal fun commitResolvedName(geohash: String, name: String?) { + val gh = normalize(geohash) + if (gh.isEmpty() || name.isNullOrEmpty()) return + if (!membership.contains(gh)) return + val current = _bookmarkNames.value.toMutableMap() + current[gh] = name + _bookmarkNames.value = current + persistNames(current) + } + private fun pickNameForLength(len: Int, address: android.location.Address?): String? { if (address == null) return null return when (len) { From e61113b2cb1bfd69f7379e5817123991f4c4e2b0 Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 5 Oct 2026 16:16:07 +0530 Subject: [PATCH 2/4] Cover the bookmark name commit rules Pins the three cases the commit seam has to get right: a name resolved for a live bookmark is stored, and a name that lands after remove or after a panic clearAll is dropped, including across a reload from SharedPreferences. createForTest builds an isolated store with reverse geocoding disabled so the tests never reach the network. --- .../geohash/GeohashBookmarkNameCommitTest.kt | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt diff --git a/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt b/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt new file mode 100644 index 000000000..b6c96e8bf --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt @@ -0,0 +1,82 @@ +package com.bitchat.android.geohash + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * A reverse geocode started for a bookmark runs on the IO dispatcher and can finish after + * the bookmark is gone. These tests pin the commit step: a resolved name is stored only + * while the geohash is still bookmarked. + */ +@RunWith(RobolectricTestRunner::class) +class GeohashBookmarkNameCommitTest { + + private lateinit var store: GeohashBookmarksStore + + @Before + fun setUp() { + val context = ApplicationProvider.getApplicationContext() + store = GeohashBookmarksStore.createForTest(context) + store.clearAll() + } + + @Test + fun `resolved name is stored for a bookmark that is still present`() { + store.add("u4pruy") + + store.commitResolvedName("u4pruy", "Copenhagen") + + assertEquals("Copenhagen", store.bookmarkNames.value["u4pruy"]) + } + + @Test + fun `resolved name is dropped when the bookmark was removed while in flight`() { + store.add("u4pruy") + store.remove("u4pruy") + + store.commitResolvedName("u4pruy", "Copenhagen") + + assertNull(store.bookmarkNames.value["u4pruy"]) + } + + @Test + fun `resolved name is dropped when a panic clear happened while in flight`() { + store.add("u4pruy") + store.clearAll() + + store.commitResolvedName("u4pruy", "Copenhagen") + + assertNull(store.bookmarkNames.value["u4pruy"]) + assertEquals(emptyMap(), store.bookmarkNames.value) + } + + @Test + fun `a panic clear survives a restart when a lookup lands after the wipe`() { + store.add("u4pruy") + store.clearAll() + store.commitResolvedName("u4pruy", "Copenhagen") + + // A fresh store reads back what was persisted, which is what the next launch sees. + val reloaded = GeohashBookmarksStore.createForTest( + ApplicationProvider.getApplicationContext() + ) + assertEquals(emptyList(), reloaded.bookmarks.value) + assertEquals(emptyMap(), reloaded.bookmarkNames.value) + } + + @Test + fun `an empty or blank name is never stored`() { + store.add("u4pruy") + + store.commitResolvedName("u4pruy", null) + store.commitResolvedName("u4pruy", "") + + assertNull(store.bookmarkNames.value["u4pruy"]) + } +} From 81f78fdb21e35dc4f038cc3704c7110477e794ad Mon Sep 17 00:00:00 2001 From: Taksh Date: Mon, 5 Oct 2026 16:16:07 +0530 Subject: [PATCH 3/4] Use explicitly synthetic bookmark fixtures --- .../geohash/GeohashBookmarkNameCommitTest.kt | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt b/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt index b6c96e8bf..f0ce32a9f 100644 --- a/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/geohash/GeohashBookmarkNameCommitTest.kt @@ -17,6 +17,8 @@ import org.robolectric.RobolectricTestRunner @RunWith(RobolectricTestRunner::class) class GeohashBookmarkNameCommitTest { + // Artificial all-zero geohash and label; neither comes from a device or user location. + private lateinit var store: GeohashBookmarksStore @Before @@ -28,39 +30,39 @@ class GeohashBookmarkNameCommitTest { @Test fun `resolved name is stored for a bookmark that is still present`() { - store.add("u4pruy") + store.add("000000") - store.commitResolvedName("u4pruy", "Copenhagen") + store.commitResolvedName("000000", "Synthetic bookmark A") - assertEquals("Copenhagen", store.bookmarkNames.value["u4pruy"]) + assertEquals("Synthetic bookmark A", store.bookmarkNames.value["000000"]) } @Test fun `resolved name is dropped when the bookmark was removed while in flight`() { - store.add("u4pruy") - store.remove("u4pruy") + store.add("000000") + store.remove("000000") - store.commitResolvedName("u4pruy", "Copenhagen") + store.commitResolvedName("000000", "Synthetic bookmark A") - assertNull(store.bookmarkNames.value["u4pruy"]) + assertNull(store.bookmarkNames.value["000000"]) } @Test fun `resolved name is dropped when a panic clear happened while in flight`() { - store.add("u4pruy") + store.add("000000") store.clearAll() - store.commitResolvedName("u4pruy", "Copenhagen") + store.commitResolvedName("000000", "Synthetic bookmark A") - assertNull(store.bookmarkNames.value["u4pruy"]) + assertNull(store.bookmarkNames.value["000000"]) assertEquals(emptyMap(), store.bookmarkNames.value) } @Test fun `a panic clear survives a restart when a lookup lands after the wipe`() { - store.add("u4pruy") + store.add("000000") store.clearAll() - store.commitResolvedName("u4pruy", "Copenhagen") + store.commitResolvedName("000000", "Synthetic bookmark A") // A fresh store reads back what was persisted, which is what the next launch sees. val reloaded = GeohashBookmarksStore.createForTest( @@ -72,11 +74,11 @@ class GeohashBookmarkNameCommitTest { @Test fun `an empty or blank name is never stored`() { - store.add("u4pruy") + store.add("000000") - store.commitResolvedName("u4pruy", null) - store.commitResolvedName("u4pruy", "") + store.commitResolvedName("000000", null) + store.commitResolvedName("000000", "") - assertNull(store.bookmarkNames.value["u4pruy"]) + assertNull(store.bookmarkNames.value["000000"]) } } From dc3a6c2bf3222387733456ec3fa6dde3c1cf894c Mon Sep 17 00:00:00 2001 From: Taksh Date: Sun, 25 Oct 2026 05:19:58 +0530 Subject: [PATCH 4/4] fix: cancel bookmark geocoding when the list is wiped A panic clear was still sending the bookmark coordinates to the geocoder if the lookup had already started. --- .../android/geohash/GeohashBookmarksStore.kt | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt b/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt index 4cc89492f..afeb93827 100644 --- a/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt +++ b/app/src/main/java/com/bitchat/android/geohash/GeohashBookmarksStore.kt @@ -9,6 +9,8 @@ import com.google.gson.Gson import com.google.gson.reflect.TypeToken import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -66,6 +68,7 @@ class GeohashBookmarksStore private constructor( // For throttling / preventing duplicate geocode lookups private val resolving = mutableSetOf() + private val resolveJobs = mutableMapOf() init { load() } @@ -167,7 +170,11 @@ class GeohashBookmarksStore private constructor( .remove(STORE_KEY) .remove(NAMES_STORE_KEY) .apply() - // Clear any in-flight resolutions to avoid repopulating + // Clear any in-flight resolutions to avoid repopulating, and + // cancel the geocode so a panic clear does not still send the + // bookmark's coordinates to the geocoder. + resolveJobs.values.forEach { it.cancel() } + resolveJobs.clear() resolving.clear() Log.i(TAG, "Cleared all geohash bookmarks and names") } catch (e: Exception) { @@ -187,8 +194,10 @@ class GeohashBookmarksStore private constructor( if (resolving.contains(gh)) return resolving.add(gh) - CoroutineScope(Dispatchers.IO).launch { + val job = CoroutineScope(Dispatchers.IO).launch { try { + ensureActive() + if (!isBookmarked(gh)) return@launch val geocoderProvider = GeocoderFactory.get(context) val name: String? = if (gh.length <= 2) { // Composite admin name from multiple points @@ -231,11 +240,13 @@ class GeohashBookmarksStore private constructor( finishResolving(gh) } } + resolveJobs[gh] = job } @Synchronized private fun finishResolving(geohash: String) { resolving.remove(geohash) + resolveJobs.remove(geohash) } /**