Skip to content

Commit 79e9fc3

Browse files
aitorvsaitorvs
authored
Avoid race condition when pre-populating VPN db (#6040)
Task/Issue URL: https://app.asana.com/1/137249556945/project/1202279501986195/task/1210212546082973?focus=true ### Description Fix a race condition that can happen upon first install. Both the vpn db callbacks (onCreate) and the AppTP blocklist worker race to update the db. It can happen that worker wins, updates db, then callback overrides it. Leaving an inconsistent state ### Steps to test this PR _Test #1_ - [x] smoke test AppTP _Test #2_ - [x] fresh install internal build - [x] filter logcat by `VPN-db` and `tag:AppTrackerListUpdateWorker` - [x] launch the app - [x] verify `Updating the app tracker blocklist, previous/new eTag: null / 096e201c66270d520e1e186073c41a64` appears - [x] verify `VPN-db onCreate: pre-populating db` appears OR `VPN-db onCreate: SKIP pre-populating db` if the worker updates the blocklist before - [x] verify the `vpn.db` has the correct blocklist in `vpn_app_tracker_block_list` and `vpn_app_tracker_block_list_metadata` - [x] force closed the app and relaunch - [x] verify `Downloaded blocklist has same eTag, noop` logcat shows - [x] verify `VPN-db onCreate: pre-populating db` does NOT show - [x] using the device inspector, delete the all `vpn.db*` files (this simulates a first launch as vpn db is not created) - [x] force closed the app and relaunch - [x] verify `VPN-db onCreate: pre-populating db` appears - [x] verify `Updating the app tracker blocklist, previous/new eTag: null / 096e201c66270d520e1e186073c41a64` appears _Test #3_ - [x] fresh install from commit 0f6a176 - [x] launch app and enable AppTP (you can disable afterwards) - [x] force close app and re-launch - [x] verify VPN-db or `tag:AppTrackerListUpdateWorker` logs appear - [x] checkout commit e3818ad - [x] Change to `ExistingPeriodicWorkPolicy.REPLACE` in `AppTrackerListUpdateWorkerScheduler` - [x] build and update the app, relaunch - [x] verify `Updating the app tracker blocklist, previous/new eTag: null / 096e201c66270d520e1e186073c41a64` log appears
1 parent bcedc8f commit 79e9fc3

File tree

7 files changed

+172
-25
lines changed

7 files changed

+172
-25
lines changed

‎app-tracking-protection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/blocklist/AppTrackerListUpdateWorker.kt

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,14 @@ import com.duckduckgo.anvil.annotations.ContributesWorker
2828
import com.duckduckgo.app.lifecycle.MainProcessLifecycleObserver
2929
import com.duckduckgo.common.utils.DispatcherProvider
3030
import com.duckduckgo.di.scopes.AppScope
31+
import com.duckduckgo.mobile.android.vpn.di.AppTpBlocklistUpdateMutex
3132
import com.duckduckgo.mobile.android.vpn.store.VpnDatabase
3233
import com.duckduckgo.mobile.android.vpn.trackers.AppTrackerMetadata
3334
import com.squareup.anvil.annotations.ContributesMultibinding
3435
import java.util.concurrent.TimeUnit
3536
import javax.inject.Inject
37+
import kotlinx.coroutines.sync.Mutex
38+
import kotlinx.coroutines.sync.withLock
3639
import kotlinx.coroutines.withContext
3740
import logcat.LogPriority
3841
import logcat.logcat
@@ -49,9 +52,15 @@ class AppTrackerListUpdateWorker(context: Context, workerParameters: WorkerParam
4952
@Inject
5053
lateinit var dispatchers: DispatcherProvider
5154

55+
@Inject
56+
@AppTpBlocklistUpdateMutex
57+
lateinit var mutex: Mutex
58+
5259
override suspend fun doWork(): Result {
5360
return withContext(dispatchers.io()) {
54-
val updateBlocklistResult = updateTrackerBlocklist()
61+
val updateBlocklistResult = mutex.withLock {
62+
updateTrackerBlocklist()
63+
}
5564

5665
val success = Result.success()
5766
if (updateBlocklistResult != success) {
@@ -73,6 +82,11 @@ class AppTrackerListUpdateWorker(context: Context, workerParameters: WorkerParam
7382
vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBlocklistMetadata()?.eTag
7483
val updatedEtag = blocklist.etag.value
7584

85+
if (updatedEtag == currentEtag) {
86+
logcat { "Downloaded blocklist has same eTag, noop" }
87+
return Result.success()
88+
}
89+
7690
logcat { "Updating the app tracker blocklist, previous/new eTag: $currentEtag / $updatedEtag" }
7791

7892
vpnDatabase

‎app-tracking-protection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/di/VpnAppModule.kt

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import android.content.Context
2020
import android.content.res.Resources
2121
import android.net.ConnectivityManager
2222
import androidx.room.Room
23+
import com.duckduckgo.app.di.AppCoroutineScope
2324
import com.duckduckgo.common.utils.DispatcherProvider
2425
import com.duckduckgo.di.scopes.AppScope
2526
import com.duckduckgo.mobile.android.vpn.stats.AppTrackerBlockingStatsRepository
@@ -32,6 +33,9 @@ import dagger.Module
3233
import dagger.Provides
3334
import dagger.SingleInstanceIn
3435
import javax.inject.Provider
36+
import javax.inject.Qualifier
37+
import kotlinx.coroutines.CoroutineScope
38+
import kotlinx.coroutines.sync.Mutex
3539

3640
@Module
3741
@ContributesTo(AppScope::class)
@@ -47,8 +51,18 @@ object VpnAppModule {
4751
fun provideVpnDatabaseCallbackProvider(
4852
context: Context,
4953
vpnDatabase: Provider<VpnDatabase>,
54+
dispatcherProvider: DispatcherProvider,
55+
@AppCoroutineScope coroutineScope: CoroutineScope,
56+
@AppTpBlocklistUpdateMutex mutex: Mutex,
5057
): VpnDatabaseCallbackProvider {
51-
return VpnDatabaseCallbackProvider(context, vpnDatabase)
58+
return VpnDatabaseCallbackProvider(context, vpnDatabase, dispatcherProvider, coroutineScope, mutex)
59+
}
60+
61+
@Provides
62+
@SingleInstanceIn(AppScope::class)
63+
@AppTpBlocklistUpdateMutex
64+
fun providesAppTpBlocklistUpdateMutex(): Mutex {
65+
return Mutex()
5266
}
5367

5468
/**
@@ -91,3 +105,7 @@ object VpnAppModule {
91105
return RealAppTrackerBlockingStatsRepository(vpnDatabase, dispatchers)
92106
}
93107
}
108+
109+
@Retention(AnnotationRetention.BINARY)
110+
@Qualifier
111+
internal annotation class AppTpBlocklistUpdateMutex

‎app-tracking-protection/vpn-store/src/main/java/com/duckduckgo/mobile/android/vpn/store/VpnDatabase.kt

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import java.time.format.DateTimeFormatter
3030

3131
@Database(
3232
exportSchema = true,
33-
version = 33,
33+
version = 34,
3434
entities = [
3535
VpnTracker::class,
3636
VpnServiceStateStats::class,
@@ -214,6 +214,12 @@ abstract class VpnDatabase : RoomDatabase() {
214214
}
215215
}
216216

217+
private val MIGRATION_33_TO_34: Migration = object : Migration(33, 34) {
218+
override fun migrate(database: SupportSQLiteDatabase) {
219+
database.execSQL("DELETE FROM `vpn_app_tracker_blocking_list_metadata`")
220+
}
221+
}
222+
217223
val ALL_MIGRATIONS: List<Migration>
218224
get() = listOf(
219225
MIGRATION_18_TO_19,
@@ -231,6 +237,7 @@ abstract class VpnDatabase : RoomDatabase() {
231237
MIGRATION_30_TO_31,
232238
MIGRATION_31_TO_32,
233239
MIGRATION_32_TO_33,
240+
MIGRATION_33_TO_34,
234241
)
235242
}
236243
}

‎app-tracking-protection/vpn-store/src/main/java/com/duckduckgo/mobile/android/vpn/store/VpnDatabaseCallback.kt

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,44 +17,61 @@
1717
package com.duckduckgo.mobile.android.vpn.store
1818

1919
import android.content.Context
20-
import androidx.annotation.VisibleForTesting
2120
import androidx.room.RoomDatabase
2221
import androidx.sqlite.db.SupportSQLiteDatabase
2322
import com.duckduckgo.common.utils.DispatcherProvider
2423
import com.duckduckgo.mobile.android.vpn.trackers.*
2524
import com.squareup.moshi.JsonAdapter
2625
import com.squareup.moshi.Moshi
27-
import java.util.*
2826
import javax.inject.Provider
27+
import kotlinx.coroutines.CoroutineScope
2928
import kotlinx.coroutines.ExperimentalCoroutinesApi
30-
import kotlinx.coroutines.asExecutor
29+
import kotlinx.coroutines.launch
30+
import kotlinx.coroutines.sync.Mutex
31+
import kotlinx.coroutines.sync.withLock
32+
import logcat.logcat
3133

3234
@OptIn(ExperimentalCoroutinesApi::class)
3335
internal class VpnDatabaseCallback(
3436
private val context: Context,
3537
private val vpnDatabase: Provider<VpnDatabase>,
3638
private val dispatcherProvider: DispatcherProvider,
39+
private val coroutineScope: CoroutineScope,
40+
private val mutex: Mutex,
3741
) : RoomDatabase.Callback() {
3842

3943
override fun onCreate(db: SupportSQLiteDatabase) {
4044
super.onCreate(db)
41-
ioThread {
42-
prepopulateAppTrackerBlockingList()
43-
prepopulateAppTrackerExclusionList()
44-
prepopulateAppTrackerExceptionRules()
45+
coroutineScope.launch(dispatcherProvider.io()) {
46+
mutex.withLock {
47+
if (vpnDatabase.get().vpnAppTrackerBlockingDao().getTrackerBlocklistMetadata()?.eTag == null) {
48+
logcat { "VPN-db onCreate: pre-populating db" }
49+
// only pre-populate when there's no blocklist
50+
prepopulateAppTrackerBlockingList()
51+
prepopulateAppTrackerExclusionList()
52+
prepopulateAppTrackerExceptionRules()
53+
} else {
54+
logcat { "VPN-db onCreate: SKIP pre-populating db" }
55+
}
56+
}
4557
}
4658
}
4759

4860
override fun onDestructiveMigration(db: SupportSQLiteDatabase) {
49-
ioThread {
50-
prepopulateAppTrackerBlockingList()
51-
prepopulateAppTrackerExclusionList()
52-
prepopulateAppTrackerExceptionRules()
61+
coroutineScope.launch(dispatcherProvider.io()) {
62+
mutex.withLock {
63+
if (vpnDatabase.get().vpnAppTrackerBlockingDao().getTrackerBlocklistMetadata()?.eTag == null) {
64+
logcat { "VPN-db onDestructiveMigration: pre-populating db" }
65+
// only pre-populate when there's no blocklist
66+
prepopulateAppTrackerBlockingList()
67+
prepopulateAppTrackerExclusionList()
68+
prepopulateAppTrackerExceptionRules()
69+
}
70+
}
5371
}
5472
}
5573

56-
@VisibleForTesting
57-
internal fun prepopulateAppTrackerBlockingList() {
74+
private fun prepopulateAppTrackerBlockingList() {
5875
context.resources.openRawResource(R.raw.full_app_trackers_blocklist).bufferedReader()
5976
.use { it.readText() }
6077
.also {
@@ -100,10 +117,4 @@ internal class VpnDatabaseCallback(
100117
val adapter: JsonAdapter<JsonAppTrackerExceptionRules> = moshi.adapter(JsonAppTrackerExceptionRules::class.java)
101118
return adapter.fromJson(json)?.rules.orEmpty()
102119
}
103-
104-
@OptIn(ExperimentalCoroutinesApi::class)
105-
private fun ioThread(f: () -> Unit) {
106-
// At most 1 thread will be doing IO
107-
dispatcherProvider.io().limitedParallelism(1).asExecutor().execute(f)
108-
}
109120
}

‎app-tracking-protection/vpn-store/src/main/java/com/duckduckgo/mobile/android/vpn/store/VpnDatabaseCallbackProvider.kt

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,19 @@ package com.duckduckgo.mobile.android.vpn.store
1818

1919
import android.content.Context
2020
import androidx.room.RoomDatabase
21-
import com.duckduckgo.common.utils.DefaultDispatcherProvider
21+
import com.duckduckgo.common.utils.DispatcherProvider
2222
import javax.inject.Provider
23+
import kotlinx.coroutines.CoroutineScope
24+
import kotlinx.coroutines.sync.Mutex
2325

2426
class VpnDatabaseCallbackProvider constructor(
2527
private val context: Context,
2628
private val vpnDatabaseProvider: Provider<VpnDatabase>,
29+
private val dispatcherProvider: DispatcherProvider,
30+
private val coroutineScope: CoroutineScope,
31+
private val mutex: Mutex,
2732
) {
2833
fun provideCallbacks(): RoomDatabase.Callback {
29-
return VpnDatabaseCallback(context, vpnDatabaseProvider, DefaultDispatcherProvider())
34+
return VpnDatabaseCallback(context, vpnDatabaseProvider, dispatcherProvider, coroutineScope, mutex)
3035
}
3136
}

‎app-tracking-protection/vpn-store/src/test/java/com/duckduckgo/mobile/android/vpn/store/VpnDatabaseCallbackTest.kt

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/*
2+
* Copyright (c) 2025 DuckDuckGo
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.duckduckgo.mobile.android.vpn.store
18+
19+
import androidx.room.Room
20+
import androidx.test.ext.junit.runners.AndroidJUnit4
21+
import androidx.test.platform.app.InstrumentationRegistry
22+
import com.duckduckgo.common.test.CoroutineTestRule
23+
import com.duckduckgo.mobile.android.vpn.trackers.AppTrackerMetadata
24+
import kotlinx.coroutines.sync.Mutex
25+
import kotlinx.coroutines.test.runTest
26+
import org.junit.Assert.assertEquals
27+
import org.junit.Assert.assertNotNull
28+
import org.junit.Assert.assertNull
29+
import org.junit.Rule
30+
import org.junit.Test
31+
import org.junit.runner.RunWith
32+
33+
@RunWith(AndroidJUnit4::class)
34+
class VpnDatabaseCallbackTest {
35+
36+
@get:Rule
37+
var coroutineRule = CoroutineTestRule()
38+
39+
private val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext
40+
41+
private val vpnDatabase: VpnDatabase = Room.inMemoryDatabaseBuilder(
42+
context,
43+
VpnDatabase::class.java,
44+
).allowMainThreadQueries().build()
45+
46+
private val vpnDatabaseCallback = VpnDatabaseCallback(
47+
context,
48+
{ vpnDatabase },
49+
coroutineRule.testDispatcherProvider,
50+
coroutineRule.testScope,
51+
Mutex(),
52+
)
53+
54+
@Test
55+
fun whenOnCreateAndNullETagThenPrePopulate() = runTest {
56+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBySubdomain("15.taboola.com"))
57+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBlocklistMetadata())
58+
59+
vpnDatabaseCallback.onCreate(vpnDatabase.openHelper.writableDatabase)
60+
61+
assertNotNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBySubdomain("15.taboola.com"))
62+
assertEquals(110, vpnDatabase.vpnAppTrackerBlockingDao().getAppExclusionList().size)
63+
assertEquals(21, vpnDatabase.vpnAppTrackerBlockingDao().getTrackerExceptionRules().size)
64+
// pre-population doesn't set metadata
65+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBlocklistMetadata())
66+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getExclusionListMetadata())
67+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerExceptionRulesMetadata())
68+
}
69+
70+
@Test
71+
fun whenOnCreateAndETagNotNullThenSkipPrePopulate() = runTest {
72+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBySubdomain("15.taboola.com"))
73+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBlocklistMetadata())
74+
75+
vpnDatabase.vpnAppTrackerBlockingDao().setTrackerBlocklistMetadata(AppTrackerMetadata(0, "1"))
76+
77+
vpnDatabaseCallback.onCreate(vpnDatabase.openHelper.writableDatabase)
78+
79+
// tracker list continues to be empty
80+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBySubdomain("15.taboola.com"))
81+
assertEquals(0, vpnDatabase.vpnAppTrackerBlockingDao().getAppExclusionList().size)
82+
assertEquals(0, vpnDatabase.vpnAppTrackerBlockingDao().getTrackerExceptionRules().size)
83+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getExclusionListMetadata())
84+
assertNull(vpnDatabase.vpnAppTrackerBlockingDao().getTrackerExceptionRulesMetadata())
85+
assertEquals(
86+
"1",
87+
vpnDatabase.vpnAppTrackerBlockingDao().getTrackerBlocklistMetadata()?.eTag,
88+
)
89+
}
90+
}

‎app-tracking-protection/vpn-store/src/test/java/com/duckduckgo/mobile/android/vpn/trackers/AppTrackerRepositoryTest.kt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import androidx.test.platform.app.InstrumentationRegistry
2222
import com.duckduckgo.common.test.CoroutineTestRule
2323
import com.duckduckgo.mobile.android.vpn.store.VpnDatabase
2424
import com.duckduckgo.mobile.android.vpn.store.VpnDatabaseCallback
25+
import kotlinx.coroutines.sync.Mutex
2526
import org.junit.Assert.*
2627
import org.junit.Before
2728
import org.junit.Rule
@@ -45,7 +46,8 @@ class AppTrackerRepositoryTest {
4546
context,
4647
VpnDatabase::class.java,
4748
).allowMainThreadQueries().build().apply {
48-
VpnDatabaseCallback(context, { this }, coroutineRule.testDispatcherProvider).prepopulateAppTrackerBlockingList()
49+
VpnDatabaseCallback(context, { this }, coroutineRule.testDispatcherProvider, coroutineRule.testScope, Mutex())
50+
.onCreate(this.openHelper.writableDatabase)
4951
}
5052
appTrackerRepository = RealAppTrackerRepository(vpnDatabase.vpnAppTrackerBlockingDao(), vpnDatabase.vpnSystemAppsOverridesDao())
5153
}

0 commit comments

Comments
 (0)