Android: Location

[WillHick]

If You Select "Allow Always" Or "Deny Always" For Location Access, It Saves To The Location Menu Either As Allowed Or Denied But If You Press The Fire Button All Saved Location Settings And The List Resets

Im Unsure If This Meant To Happen But It Does Making It Annoying And Always Having To Come Back To Redo It.

[BennoFlo]

Seems to me like this is a race condition, so it only happens intermittently.

ClearPersonalDataAction is calling:
geoLocationPermissions.clearAllButFireproofed()
on with the Dispatchers.IO context.

In GeoLocationPermissionsManager, the implementation of clearAllButFireproofed spawns another thread by using withContext again in that function. Removing that coroutine context makes this work every time. If you leave it in, there's a chance that the browser potentially restarts before GeolocationPermissions can clear the domain.

I was able to test failure scenarios by calling GeolocationPermissions.getOrigins. If this fails due to the race condition, the domain will still sit in the browser, while not sitting in the database table, thus causing this issue.

[cmonfortep]

Thanks for reporting @WillHick, I will check with the team what's the expected behavior there.

From the code, I see that the expected behavior is to remove all the information inside Location settings except if you fireproofed that domain. Did you try fireproofing the domain you want to keep the location info?

@BennoFlo I may be missing something on your description, but when using withContext execution suspends till the value is returned:

<T> withContext(
    context: CoroutineContext,
    block: suspend CoroutineScope.() -> T
): T 

So, in our case, restarting the app will not happen till all the info has been cleared.

[BennoFlo]

@cmonfortep - Very odd. There must be something else going on under the hood. You are correct, but after removing the nested coroutine, I couldn't reproduce this at all, so I made an assumption that there was some sort of thread switching going on causing a race condition.

I'll keep digging -- but it looks like the primary issue is that the location is cleared from the database but not cleared from the GeolocationPermissions store. Issue seems intermittent.

[BennoFlo]

Will can confirm, but the issue I found was that when clicking Clear All Data - it sometimes doesn't actually clear the web browsers permissions - only in the internal database. Maybe that's a different issue.

[cmonfortep]

@malmstein could you triage if this is a bug? 🐛

[malmstein]

Hey @WillHick thanks for writing this up!

The decision of removing the location permissions after using the Fire button is indeed intentional. It's important that anything related to a site is burnt after using the Fire button.

If you want to keep the Location permission for a site, I'd recommend that you Fireproof that site. You can do that by opening the Overflow menu and tapping on Fireproof Website. You can always remove a site from the Fireproof list from the app's Settings.

@BennoFlo good job spotting the second coroutine! It's not necessary, so I've opened https://github.com/duckduckgo/Android/pull/941 to get rid of it