Crash with overlay shop enabled

[Jean-BaptisteC]

How to Reproduce

• Start app
• Enable shop overlay
• Press + button
• Touch a shop around you
• Switch to another app
• Go back to SC -> app crashed

Stack trace:

java.lang.NullPointerException
 at de.westnordost.streetcomplete.overlays.AbstractOverlayForm.getGeometry(Unknown Source:10)
 at de.westnordost.streetcomplete.overlays.AbstractOverlayForm.onSaveInstanceState(Unknown Source:10)
 at de.westnordost.streetcomplete.overlays.shops.ShopsOverlayForm.onSaveInstanceState(Unknown Source:5)
 at androidx.fragment.app.Fragment.performSaveInstanceState(Unknown Source:0)
 at androidx.fragment.app.FragmentStateManager.saveState(Unknown Source:44)
 at androidx.fragment.app.FragmentStore.saveActiveFragments(Unknown Source:41)
 at androidx.fragment.app.FragmentManager.saveAllStateInternal(Unknown Source:24)
 at androidx.fragment.app.FragmentStateManager.saveState(Unknown Source:93)
 at androidx.fragment.app.FragmentStore.saveActiveFragments(Unknown Source:41)
 at androidx.fragment.app.FragmentManager.saveAllStateInternal(Unknown Source:24)
 at androidx.fragment.app.FragmentManager.lambda$attachController$4(Unknown Source:0)
 at androidx.fragment.app.FragmentManager.$r8$lambda$sido8p6zuWx0PQxIkv4qM-BRiGM(SourceFile:0)
 at androidx.fragment.app.FragmentManager$$ExternalSyntheticLambda4.saveState(SourceFile:0)
 at androidx.savedstate.SavedStateRegistry.performSave(Unknown Source:52)
 at androidx.savedstate.SavedStateRegistryController.performSave(Unknown Source:7)
 at androidx.activity.ComponentActivity.onSaveInstanceState(Unknown Source:20)
 at android.app.Activity.performSaveInstanceState(activity.java:2298)
 at android.app.Instrumentation.callActivityOnSaveInstanceState(instrumentation.java:1635)
 at android.app.ActivityThread.callActivityOnSaveInstanceState(activitythread.java:6003)
 at android.app.ActivityThread.callActivityOnStop(activitythread.java:5423)
 at android.app.ActivityThread.performStopActivityInner(activitythread.java:5389)
 at android.app.ActivityThread.handleStopActivity(activitythread.java:5454)
 at android.app.servertransaction.StopActivityItem.execute(stopactivityitem.java:43)
 at android.app.servertransaction.ActivityTransactionItem.execute(activitytransactionitem.java:45)
 at android.app.servertransaction.TransactionExecutor.executeLifecycleState(transactionexecutor.java:180)
 at android.app.servertransaction.TransactionExecutor.execute(transactionexecutor.java:98)
 at android.app.ActivityThread$H.handleMessage(activitythread.java:2468)
 at android.os.Handler.dispatchMessage(handler.java:106)
 at android.os.Looper.loopOnce(looper.java:205)
 at android.os.Looper.loop(looper.java:294)
 at android.app.ActivityThread.main(activitythread.java:8248)
 at java.lang.reflect.Method.invoke(Native Method)
 at com.android.internal.os.RuntimeInit$[MethodAndArgsCaller.run](https://methodandargscaller.run/)(runtimeinit.java:552)
 at com.android.internal.os.ZygoteInit.main(zygoteinit.java:971)

Log:
2024-02-22T21:48:24.326: [QuestAutoSyncer] Checking whether to automatically download new quests at 49.1199088,6.1693713
2024-02-22T21:48:24.345: [MapDataController] Fetched 7540 elements and geometries in 17ms
2024-02-22T21:48:24.354: [QuestAutoSyncer] All downloaded in radius of 195 meters around user (4 tiles)
2024-02-22T21:48:24.356: [Upload] Starting upload
2024-02-22T21:48:24.359: [Upload] Finished upload
2024-02-22T21:48:25.422: [QuestAutoSyncer] Checking whether to automatically download new quests at 49.1198700,6.1692894
2024-02-22T21:48:25.449: [MapDataController] Fetched 7540 elements and geometries in 24ms
2024-02-22T21:48:25.466: [QuestAutoSyncer] All downloaded in radius of 195 meters around user (4 tiles)
2024-02-22T21:52:28.282: [QuestAutoSyncer] Checking whether to automatically download new quests at 49.1198700,6.1692894
2024-02-22T21:52:28.304: [MapDataController] Fetched 7540 elements and geometries in 19ms
2024-02-22T21:52:28.315: [Upload] Starting upload
2024-02-22T21:52:28.317: [QuestAutoSyncer] All downloaded in radius of 195 meters around user (4 tiles)
2024-02-22T21:52:28.317: [Upload] Finished upload
2024-02-22T21:52:33.130: [MapDataController] Fetched 7540 elements and geometries in 24ms

Expected Behavior
App not crashed

Versions affected
Android 14 - v57.0-beta2

[mnalis]

I can confirm (Also Android 14, SC v57.0-beta2)

[rhhsm]

Can't reproduce in v56.1, Android 10 (though I'm not sure what is meant by "Touch a shop around you"; in shop overlay, I tapped the plus button, then an existing shop, and then switched to another app)

[mnalis]

Yes, it means tap an existing shop. Here is how it looks to me on Android 14, SC v57.0-beta2:

shops_overlay_crush.mp4

[westnordost]

Thanks for the report!

[westnordost]

It is reproducible with the things overlay, too.

So, the old fragment for the newly to be added element is for some reason not removed when it is replaced by the fragment for the clicked element. Not sure, why. The code to show a fragment in the bottom sheet area is in MainFragment::showInBottomSheet.

[westnordost]

Last change regarding the child fragment manager and handling of back stack in that class was made by @tapetis. Pinging him because maybe he has an idea.

[tapetis]

Isn't the problem related to the following line that adds every bottom sheet fragment shown to the back stack, which in turn causes all of them to save their state when the app is moved to the background unless they were popped beforehand from the back stack?

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt

Line 955 in 4cc2e2a

addToBackStack(BOTTOM_SHEET)

It seems like closeBottomSheet, which would pop the fragment opened by pressing the + button, is not called when selecting a different element in the overlays.

[westnordost]

Ah, so this issue must have existed for a very long time already...

I wonder how the fragment transaction API is supposed to be used when one wants to add or replace a fragment which should be able to be popped.

[tapetis]

The way the addToBackStack API is currently being used for the fragment transaction seems fine, otherwise the back button would not work correctly. I think the crash is specific to the cases described in this issue, which causes two instances of the AbstractOverlayForm, one of which seems to have an already destroyed view, to be in the back stack.

[westnordost]

But shouldn't the old fragment be removed already because of the line above?

StreetComplete/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt

Line 954 in 4cc2e2a

replace(R.id.map_bottom_sheet_container, f, BOTTOM_SHEET)

[tapetis]

The fragment is removed in the sense that it is no longer displayed in its container. However, it remains in the back stack due to the separate addToBackStack call, so that the replace operation can be undone by pressing the back button.

[westnordost]

I created a pull request with a fix, but I am not sure if this is how it should be done.