Skip to content

Commit

Permalink
Fix database access on main thread that caused a crash (#5006)
Browse files Browse the repository at this point in the history
Task/Issue URL:
https://app.asana.com/0/1202552961248957/1208294040908744/f

### Description

### Steps to test this PR

_Feature 1_
- [x] Open https://www.disneyaulani.com/ 
- [x] Check app doesn't crash and works normally
- [x] Open maps.google.com and click on the location button
- [x] Check app doesn't crash and works normally
  • Loading branch information
CrisBarreiro authored Sep 13, 2024
1 parent 715d842 commit dac3b5f
Show file tree
Hide file tree
Showing 9 changed files with 40 additions and 41 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2425,9 +2425,7 @@ class BrowserTabFragment :
id: String?,
data: JSONObject?,
) {
appCoroutineScope.launch(dispatchers.main()) {
viewModel.processJsCallbackMessage(featureName, method, id, data, it.url)
}
viewModel.processJsCallbackMessage(featureName, method, id, data, it.url)
}
},
)
Expand All @@ -2440,9 +2438,7 @@ class BrowserTabFragment :
id: String?,
data: JSONObject?,
) {
appCoroutineScope.launch(dispatchers.main()) {
viewModel.processJsCallbackMessage(featureName, method, id, data, it.url)
}
viewModel.processJsCallbackMessage(featureName, method, id, data, it.url)
}
},
)
Expand Down
22 changes: 12 additions & 10 deletions app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3167,7 +3167,7 @@ class BrowserTabViewModel @Inject constructor(
)
}

suspend fun processJsCallbackMessage(
fun processJsCallbackMessage(
featureName: String,
method: String,
id: String?,
Expand Down Expand Up @@ -3200,7 +3200,7 @@ class BrowserTabViewModel @Inject constructor(

when (featureName) {
DUCK_PLAYER_FEATURE_NAME, DUCK_PLAYER_PAGE_FEATURE_NAME -> {
withContext(dispatchers.io()) {
viewModelScope.launch(dispatchers.io()) {
val response = duckPlayerJSHelper.processJsCallbackMessage(featureName, method, id, data, url)
withContext(dispatchers.main()) {
response?.let {
Expand Down Expand Up @@ -3230,15 +3230,17 @@ class BrowserTabViewModel @Inject constructor(
id: String,
data: JSONObject,
) {
val response = if (url == null) {
getDataForPermissionState(featureName, method, id, SitePermissionQueryResponse.Denied)
} else {
val permissionState = sitePermissionsManager.getPermissionsQueryResponse(url!!, tabId, data.optString("name"))
getDataForPermissionState(featureName, method, id, permissionState)
}
viewModelScope.launch(dispatchers.io()) {
val response = if (url == null) {
getDataForPermissionState(featureName, method, id, SitePermissionQueryResponse.Denied)
} else {
val permissionState = sitePermissionsManager.getPermissionsQueryResponse(url!!, tabId, data.optString("name"))
getDataForPermissionState(featureName, method, id, permissionState)
}

viewModelScope.launch(dispatchers.main()) {
command.value = SendResponseToJs(response)
withContext(dispatchers.main()) {
command.value = SendResponseToJs(response)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ interface SitePermissionsManager {
* @param queriedPermission permission being queried (note: this is different from WebView permissions, check link above)
* @return state of the permission as expected by the API: 'granted', 'prompt', or 'denied'
*/
fun getPermissionsQueryResponse(url: String, tabId: String, queriedPermission: String): SitePermissionQueryResponse
suspend fun getPermissionsQueryResponse(url: String, tabId: String, queriedPermission: String): SitePermissionQueryResponse

data class SitePermissions(
val autoAccept: List<String>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class SitePermissionsManagerImpl @Inject constructor(
private val dispatcherProvider: DispatcherProvider,
) : SitePermissionsManager {

private fun getSitePermissionsGranted(
private suspend fun getSitePermissionsGranted(
url: String,
tabId: String,
resources: Array<String>,
Expand Down Expand Up @@ -85,7 +85,7 @@ class SitePermissionsManagerImpl @Inject constructor(
}
}

override fun getPermissionsQueryResponse(
override suspend fun getPermissionsQueryResponse(
url: String,
tabId: String,
queriedPermission: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ interface SitePermissionsRepository {
var askCameraEnabled: Boolean
var askMicEnabled: Boolean
var askDrmEnabled: Boolean
fun isDomainAllowedToAsk(url: String, permission: String): Boolean
fun isDomainGranted(url: String, tabId: String, permission: String): Boolean
suspend fun isDomainAllowedToAsk(url: String, permission: String): Boolean
suspend fun isDomainGranted(url: String, tabId: String, permission: String): Boolean
fun sitePermissionGranted(url: String, tabId: String, permission: String)
fun sitePermissionsWebsitesFlow(): Flow<List<SitePermissionsEntity>>
fun sitePermissionsForAllWebsites(): List<SitePermissionsEntity>
Expand Down Expand Up @@ -85,7 +85,7 @@ class SitePermissionsRepositoryImpl @Inject constructor(

private val drmSessions = mutableMapOf<String, Boolean>()

override fun isDomainAllowedToAsk(url: String, permission: String): Boolean {
override suspend fun isDomainAllowedToAsk(url: String, permission: String): Boolean {
val domain = url.extractDomain() ?: url
val sitePermissionsForDomain = sitePermissionsDao.getSitePermissionsByDomain(domain)
return when (permission) {
Expand All @@ -111,7 +111,7 @@ class SitePermissionsRepositoryImpl @Inject constructor(
}
}

override fun isDomainGranted(url: String, tabId: String, permission: String): Boolean {
override suspend fun isDomainGranted(url: String, tabId: String, permission: String): Boolean {
val domain = url.extractDomain() ?: url
val sitePermissionForDomain = sitePermissionsDao.getSitePermissionsByDomain(domain)
val permissionAllowedEntity = sitePermissionsAllowedDao.getSitePermissionAllowed(domain, tabId, permission)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ class SitePermissionsManagerTest {
whenever(mockSitePermissionsRepository.isDomainAllowedToAsk(url, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(true)
whenever(mockSitePermissionsRepository.isDomainAllowedToAsk(url, PermissionRequest.RESOURCE_AUDIO_CAPTURE)).thenReturn(false)
whenever(mockPackageManager.hasSystemFeature(PackageManager.FEATURE_CAMERA_ANY)).thenReturn(true)
whenever(mockSitePermissionsRepository.isDomainGranted(url, tabId, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(false)

val permissionRequest: PermissionRequest = mock()
whenever(permissionRequest.origin).thenReturn(url.toUri())
Expand Down Expand Up @@ -167,14 +168,14 @@ class SitePermissionsManagerTest {
}

@Test
fun whenDomainGrantedThenGetPermissionsQueryResponseReturnsGranted() {
fun whenDomainGrantedThenGetPermissionsQueryResponseReturnsGranted() = runTest {
whenever(mockSitePermissionsRepository.isDomainGranted(url, tabId, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(true)

assertEquals(SitePermissionQueryResponse.Granted, testee.getPermissionsQueryResponse(url, tabId, "camera"))
}

@Test
fun whenDomainAllowedToAskThenGetPermissionsQueryResponseReturnsPrompt() {
fun whenDomainAllowedToAskThenGetPermissionsQueryResponseReturnsPrompt() = runTest {
whenever(mockSitePermissionsRepository.isDomainGranted(url, tabId, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(false)
whenever(mockPackageManager.hasSystemFeature(PackageManager.FEATURE_CAMERA_ANY)).thenReturn(true)
whenever(mockSitePermissionsRepository.isDomainAllowedToAsk(url, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(true)
Expand All @@ -183,7 +184,7 @@ class SitePermissionsManagerTest {
}

@Test
fun whenDomainNotAllowedToAskThenGetPermissionsQueryResponseReturnsDenied() {
fun whenDomainNotAllowedToAskThenGetPermissionsQueryResponseReturnsDenied() = runTest {
whenever(mockSitePermissionsRepository.isDomainGranted(url, tabId, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(false)
whenever(mockPackageManager.hasSystemFeature(PackageManager.FEATURE_CAMERA_ANY)).thenReturn(true)
whenever(mockSitePermissionsRepository.isDomainAllowedToAsk(url, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(false)
Expand All @@ -192,7 +193,7 @@ class SitePermissionsManagerTest {
}

@Test
fun whenHardwareNotSupportedThenGetPermissionsQueryResponseReturnsDenied() {
fun whenHardwareNotSupportedThenGetPermissionsQueryResponseReturnsDenied() = runTest {
whenever(mockSitePermissionsRepository.isDomainGranted(url, tabId, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(false)
whenever(mockPackageManager.hasSystemFeature(PackageManager.FEATURE_CAMERA_ANY)).thenReturn(false)
whenever(mockSitePermissionsRepository.isDomainAllowedToAsk(url, PermissionRequest.RESOURCE_VIDEO_CAPTURE)).thenReturn(true)
Expand All @@ -201,7 +202,7 @@ class SitePermissionsManagerTest {
}

@Test
fun whenAndroidPermissionNotSupportedThenGetPermissionsQueryResponseReturnsDenied() {
fun whenAndroidPermissionNotSupportedThenGetPermissionsQueryResponseReturnsDenied() = runTest {
assertEquals(SitePermissionQueryResponse.Denied, testee.getPermissionsQueryResponse(url, tabId, "unsupported"))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,31 +63,31 @@ class SitePermissionsRepositoryTest {
private val domain = "domain.com"

@Test
fun givenPermissionNotSupportedThenDomainIsNotAllowedToAsk() {
fun givenPermissionNotSupportedThenDomainIsNotAllowedToAsk() = runTest {
setInitialSettings()
val permission = PermissionRequest.RESOURCE_MIDI_SYSEX

assertFalse(repository.isDomainAllowedToAsk(url, permission))
}

@Test
fun givenPermissionSupportedThenDomainIsAllowedToAsk() {
fun givenPermissionSupportedThenDomainIsAllowedToAsk() = runTest {
setInitialSettings()
val permission = PermissionRequest.RESOURCE_AUDIO_CAPTURE

assertTrue(repository.isDomainAllowedToAsk(url, permission))
}

@Test
fun whenAskForPermissionIsDisabledThenDomainIsNotAllowedToAsk() {
fun whenAskForPermissionIsDisabledThenDomainIsNotAllowedToAsk() = runTest {
setInitialSettings(cameraEnabled = false)
val permission = PermissionRequest.RESOURCE_VIDEO_CAPTURE

assertFalse(repository.isDomainAllowedToAsk(url, permission))
}

@Test
fun whenAskForPermissionDisabledButSitePermissionSettingIsAlwaysAllowThenIsAllowedToAsk() {
fun whenAskForPermissionDisabledButSitePermissionSettingIsAlwaysAllowThenIsAllowedToAsk() = runTest {
val testEntity = SitePermissionsEntity(domain, askMicSetting = SitePermissionAskSettingType.ALLOW_ALWAYS.name)
setInitialSettings(micEnabled = false, sitePermissionEntity = testEntity)
val permission = PermissionRequest.RESOURCE_AUDIO_CAPTURE
Expand All @@ -96,7 +96,7 @@ class SitePermissionsRepositoryTest {
}

@Test
fun whenSitePermissionSettingIsDenyAlwaysThenDomainIsNotAllowedToAsk() {
fun whenSitePermissionSettingIsDenyAlwaysThenDomainIsNotAllowedToAsk() = runTest {
val testEntity = SitePermissionsEntity(domain, askCameraSetting = SitePermissionAskSettingType.DENY_ALWAYS.name)
setInitialSettings(sitePermissionEntity = testEntity)
val permission = PermissionRequest.RESOURCE_VIDEO_CAPTURE
Expand All @@ -105,7 +105,7 @@ class SitePermissionsRepositoryTest {
}

@Test
fun whenNoSitePermissionSettingAndDrmBlockedThenDomainIsNotAllowedToAsk() {
fun whenNoSitePermissionSettingAndDrmBlockedThenDomainIsNotAllowedToAsk() = runTest {
val permission = PermissionRequest.RESOURCE_PROTECTED_MEDIA_ID

whenever(mockDrmBlock.isDrmBlockedForUrl(url)).thenReturn(true)
Expand All @@ -114,7 +114,7 @@ class SitePermissionsRepositoryTest {
}

@Test
fun whenSitePermissionSettingIsAskAndDrmBlockedThenDomainIsAllowedToAsk() {
fun whenSitePermissionSettingIsAskAndDrmBlockedThenDomainIsAllowedToAsk() = runTest {
val testEntity = SitePermissionsEntity(domain, askDrmSetting = SitePermissionAskSettingType.ASK_EVERY_TIME.name)
setInitialSettings(sitePermissionEntity = testEntity)
val permission = PermissionRequest.RESOURCE_PROTECTED_MEDIA_ID
Expand All @@ -125,7 +125,7 @@ class SitePermissionsRepositoryTest {
}

@Test
fun whenSitePermissionsWasGrantedWithin24hThenReturnPermissionGranted() {
fun whenSitePermissionsWasGrantedWithin24hThenReturnPermissionGranted() = runTest {
setInitialSettings()
val permission = PermissionRequest.RESOURCE_VIDEO_CAPTURE
val tabId = "tabId"
Expand All @@ -136,7 +136,7 @@ class SitePermissionsRepositoryTest {
}

@Test
fun whenSitePermissionsWasMoreThen24hAgoThenReturnPermissionNotGranted() {
fun whenSitePermissionsWasMoreThen24hAgoThenReturnPermissionNotGranted() = runTest {
setInitialSettings()
val permission = PermissionRequest.RESOURCE_VIDEO_CAPTURE
val tabId = "tabId"
Expand All @@ -147,7 +147,7 @@ class SitePermissionsRepositoryTest {
}

@Test
fun whenSitePermissionsSettingIsAllowAlwaysThenReturnPermissionGranted() {
fun whenSitePermissionsSettingIsAllowAlwaysThenReturnPermissionGranted() = runTest {
val testEntity = SitePermissionsEntity(domain, askCameraSetting = SitePermissionAskSettingType.ALLOW_ALWAYS.name)
setInitialSettings(sitePermissionEntity = testEntity)
val permission = PermissionRequest.RESOURCE_VIDEO_CAPTURE
Expand Down Expand Up @@ -264,7 +264,7 @@ class SitePermissionsRepositoryTest {
micEnabled: Boolean = true,
drmEnabled: Boolean = true,
sitePermissionEntity: SitePermissionsEntity? = null,
) {
) = runTest {
whenever(mockSitePermissionsPreferences.askCameraEnabled).thenReturn(cameraEnabled)
whenever(mockSitePermissionsPreferences.askMicEnabled).thenReturn(micEnabled)
whenever(mockSitePermissionsPreferences.askDrmEnabled).thenReturn(drmEnabled)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ interface SitePermissionsDao {
fun getAllSitesPermissionsAsFlow(): Flow<List<SitePermissionsEntity>>

@Query("select * from site_permissions where domain = :domain")
fun getSitePermissionsByDomain(domain: String): SitePermissionsEntity?
suspend fun getSitePermissionsByDomain(domain: String): SitePermissionsEntity?

@Delete
fun delete(sitePermissionsEntity: SitePermissionsEntity): Int
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ interface SitePermissionsAllowedDao {
fun getAllSitesPermissionsAllowedAsFlow(): Flow<List<SitePermissionAllowedEntity>>

@Query("select * from site_permission_allowed where domain = :domain and tabId = :tabId and permissionAllowed = :permissionAllowed")
fun getSitePermissionAllowed(domain: String, tabId: String, permissionAllowed: String): SitePermissionAllowedEntity?
suspend fun getSitePermissionAllowed(domain: String, tabId: String, permissionAllowed: String): SitePermissionAllowedEntity?

@Delete
fun delete(sitePermissionsEntity: SitePermissionAllowedEntity): Int
Expand Down

0 comments on commit dac3b5f

Please sign in to comment.