maplibre / maplibre-native

MapLibre Native - Interactive vector tile maps for iOS, Android and other platforms.
https://maplibre.org
BSD 2-Clause "Simplified" License
1.03k stars 300 forks source link

Crash on Android while in background #2432

Closed Helium314 closed 1 month ago

Helium314 commented 4 months ago

Describe the bug I used an app with MapLibre 11.0.0, then did something else, and on next time opening the app about an hour later I noticed it had crashed, probably while not using the phone. Stack Trace (update: replaced with the strack trace from 11.1.0 as this one shows line numbers):

FATAL EXCEPTION: main
Process: de.westnordost.streetcomplete.expert, PID: 677
java.lang.IllegalMonitorStateException
at java.util.concurrent.locks.ReentrantLock$Sync.tryRelease(ReentrantLock.java:156)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.release(AbstractQueuedSynchronizer.java:1291)
at java.util.concurrent.locks.ReentrantLock.unlock(ReentrantLock.java:466)
at org.maplibre.android.storage.FileSource.unlockPathLoaders(FileSource.java:367)
at org.maplibre.android.storage.FileSource.access$100(FileSource.java:31)
at org.maplibre.android.storage.FileSource$FileDirsPathsTask.onPostExecute(FileSource.java:224)
at org.maplibre.android.storage.FileSource$FileDirsPathsTask.onPostExecute(FileSource.java:204)
at android.os.AsyncTask.finish(AsyncTask.java:755)
at android.os.AsyncTask.access$900(AsyncTask.java:192)
at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:772)
at android.os.Handler.dispatchMessage(Handler.java:107)
at android.os.Looper.loop(Looper.java:214)
at android.app.ActivityThread.main(ActivityThread.java:7356)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:940)

There is no app code involved, so I don't know whether the crash might be related to mis-handling MapLibre somehow.

To Reproduce Unclear how to reproduce it. Possibly triggered by some stuff Android does with background apps. The stack trace might not be particularly useful, it doesn't even contain line numbers. Is there anything I can do to help finding out what is wrong?

Platform information (please complete the following information):

westnordost commented 4 months ago

I've had a short look at the source code here: https://github.com/maplibre/maplibre-native/blob/main/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/storage/FileSource.java#L204

Such a crash happens when the reentrant lock is attempted to be unlocked on a different thread than it was locked. Looking at the code, this should be impossible, however, given the information that the app was in the background for a while, my guess is that Android assigns a different thread for the UI thread when the application is re-launched. So, the FileDirsPathsTask was started, never finished, and then one hour later, onCancelled was called, but on a different thread.

Note also that AsyncTask is deprecated (for reasons like this). Documentation recommends to replace that with Kotlin coroutines with lifecycle components. (But, well, as long as you did not fully migrate to Kotlin yet, this makes little sense.)

For now, I'd recommend to just catch that exception with a note comment about that Android doesn't guarantee that the UI thread when the AsyncTask started has the same thread id when it ended if the app was not in foreground for a while.

westnordost commented 4 months ago

FYI in Kotlin with coroutines, if one must offload getting the cache dir to an IO thread, it would look like this for internalCachePath


private val internalCachePath: String? = null
private val internalCachePathMutex = Mutex()

suspend fun getInternalCachePath(context: Context): String {
  internalCachePathMutex.withLock {
    if (internalCachePath == null) {
      withContext(IO) {
        internalCachePath = getCachePath(context)
      }
    }
    return internalCachePath
  }
}

(To initialize, just call the getter)

If there should not be a suspending function on the interface, the above would need to be wrapped in `runBlocking { }̀ to make the call blocking.

louwers commented 4 months ago

Thanks a lot for the bug report @Helium314 and for the additional details and suggestions @westnordost 🙏

Helium314 commented 1 month ago

Updated the stack trace, the one from MapLibre 11.1.0 has line numbers.

westnordost commented 1 month ago

In our alpha test, it turns out that this happens very frequently - in the background or when the app has been in the background for a while.

Since the rework #2436 will only be included in MapLibre 12.x which might take a while until it is released/stable, I wonder if for MapLibre 11.x you'd accept a PR that just "fixes" this issue by catching and ignoring the exception?

westnordost commented 1 month ago

I've submitted a PR with what I believe to be an actual fix (not a "fix", as announced).