Crash with overlay active and downloading after reentering the app

[westnordost]

I frequently get reported this crash report now.

I am not completely sure how to reproduce it, but it is relatively recent and it seems to only happen when an overlay is active but the user left the app for a while, then returned to it, then downloaded something.

My first suspicion would be that this StyleableOverlayManager is still lingering from before the app was sent to background and while its parent MainFragment had been destroyed and recreated already, along with the tangram MapController, it is still around because it still listens to the MapDataWithEditsSource. I.e. for some reason, it does not unsubscribe on updates from MapDataWithEditsSource when its parent MainFragments view is destroyed.

Since it is relatively recent that this occurs, (can't say exactly when - at least 47.1), could this have something to do with updateJob, @Helium314 ? Though, I don't see anything wrong with the code... maybe I am missing something...

If anybody can find an exact reproduction, it would be appreciated too. Since this issue seems to be happening so often, I would really like to get this fixed before releasing v48.0 (which I planned to release today 🤷 ).

java.lang.NullPointerException: Attempt to invoke virtual method 'void com.mapzen.tangram.NativeMap.getCameraPosition(com.mapzen.tangram.CameraPosition)' on a null object reference
    at com.mapzen.tangram.MapController.getCameraPosition(MapController.java:496)
    at de.westnordost.streetcomplete.screens.main.map.tangram.CameraManager.pullCameraPositionFromController(CameraManager.kt:169)
    at de.westnordost.streetcomplete.screens.main.map.tangram.CameraManager.getCamera(CameraManager.kt:63)
    at de.westnordost.streetcomplete.screens.main.map.tangram.KtMapController.getCameraPosition(KtMapController.kt:179)
    at de.westnordost.streetcomplete.screens.main.map.StyleableOverlayManager.onNewScreenPosition(StyleableOverlayManager.kt:110)
    at de.westnordost.streetcomplete.screens.main.map.StyleableOverlayManager$mapDataListener$1.onReplacedForBBox(StyleableOverlayManager.kt:72)
    at de.westnordost.streetcomplete.data.osm.edits.MapDataWithEditsSource.callOnReplacedForBBox(MapDataWithEditsSource.kt:462)
    at de.westnordost.streetcomplete.data.osm.edits.MapDataWithEditsSource.access$callOnReplacedForBBox(MapDataWithEditsSource.kt:32)
    at de.westnordost.streetcomplete.data.osm.edits.MapDataWithEditsSource$mapDataListener$1.onReplacedForBBox(MapDataWithEditsSource.kt:137)
    at de.westnordost.streetcomplete.data.osm.mapdata.MapDataController.onReplacedForBBox(MapDataController.kt:271)
    at de.westnordost.streetcomplete.data.osm.mapdata.MapDataController.putAllForBBox(MapDataController.kt:91)
    at de.westnordost.streetcomplete.data.osm.mapdata.MapDataDownloader$download$2.invokeSuspend(MapDataDownloader.kt:33)
    at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
    at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
    at kotlinx.coroutines.internal.LimitedDispatcher.run(LimitedDispatcher.kt:42)
    at kotlinx.coroutines.scheduling.TaskImpl.run(Tasks.kt:95)
    at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)

[westnordost]

So far, it seems to only occur for the StyleableOverlayManager and not for its "brother" the QuestPinManager even though the code is very similar. But this could be just because it is called earlier (it is called right after the map data has been updated, while the quests are only updated after the quests for the updated map data has been updated). So, the bug may not be in StyleableOverlayManager alone.

[tapetis]

I cannot reproduce this crash. But maybe it is related to the mapDataListener in the StyleableOverlayManager only being removed when the overlay is either hidden or onDestroy is called. As explained in the Android documentation, there is no guarantee that onDestroy is called when the app moves to the background. If it is not called, the mapDataListener leaks and then causes the crash when onReplacedForBBox is executed after the next download. At least that's what I could imagine.

[tapetis]

My intuition in the previous comment was probably correct. If I comment out the code in the onDestroy method in the StyleableOverlayManager and turn on "Don't keep activities" in the Android developer options, I can reproduce the crash after the app is brought back from the background and then manually search for quests.

[westnordost]

But this is about onDestroyView, not onDestroy, isn't it? The StyleableOverlayManager is added to the MainFragments view lifecycle in

https://github.com/streetcomplete/StreetComplete/blob/c48a9656c1231480e7e63c95b08e5347659d4310/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/MainMapFragment.kt#L120

[tapetis]

As far as I understand lifecycles in Android, both onDestroy and onDestroyView in a Fragment are linked to the onDestroy method of the containing Activity. But I'm not 100% sure about that.

[westnordost]

Hm wow, that would make onDestroyView and onDestroy pretty useless. And this would also mean that there are many more places where leaks happen because of this. Some classes have a coroutine viewLifecycleScope which is cancelled on onDestroy.

[westnordost]

Regarding

My intuition in the previous comment was probably correct. If I comment out the code in the onDestroy method in the StyleableOverlayManager and turn on "Don't keep activities" in the Android developer options, I can reproduce the crash after the app is brought back from the background and then manually search for quests.

What code did you comment out? Why would you need to comment out code to reproduce the crash?

[tapetis]

Yes, that's why listeners are usually removed in onStop. Alternatively, the View can observe the changes in the ViewModel or Controller, instead of the latter "pushing" the updates to the former, which can result in the type of crash we see here.

To reproduce the crash, I commented out both code lines in onDestroy in the StyleableOverlayManager to simulate it not being called. I did this because I do not know the exact conditions when Android omits a call to onDestroy and enabling "Don't keep activities" ensures that this method is always called when the app goes to the background (at least on my phone). Otherwise, you probably have to hope that Android kills the app in the background after some time or when too many other apps are opened.

[westnordost]

Alternatively, the View can observe the changes in the [...] Controller, instead of the latter "pushing" the updates to the former

Why, isn't that the same thing?

[tapetis]

No, it is not the same thing. The crash most likely happened because there is a mismatch between the lifecycle state of the Tangram map and the StyleableOverlayManager. Due to the way the current implementation works, the StyleableOverlayManager has to keep track of the lifecycle state of the Views in the MainMapFragment. If the map is already destroyed and the StyleableOverlayManager "pushes" a new camera position to the map controller, we get a crash.

However, if we were to use LiveData or a Kotlin flow to transmit new camera positions to the map instead, the map could observe this data holder / stream using its own lifecycle. So if the map is no longer there, there is no observer and no crash.