Memory leak

[polivmi1]

I have mentioned this in #26 , but it is already closed, so I am opening a new issue.
I am getting a leak if the map component is cleared and then the activity is destroyed.

Previously when implementing it through AndroidView, to get rid of the maps leak, I had to use:

DisposableEffect(lifecycle) {
        lifecycle.addObserver(lifecycleObserver)
        onDispose {
            lifecycle.removeObserver(lifecycleObserver)
            mapView.onDestroy()
            mapView.removeAllViews()
        }
    }

And

remember(mapView) {
        LifecycleEventObserver { _, event ->
            when (event) {
                Lifecycle.Event.ON_CREATE -> mapView.onCreate(Bundle())
                Lifecycle.Event.ON_START -> mapView.onStart()
                Lifecycle.Event.ON_RESUME -> mapView.onResume()
                Lifecycle.Event.ON_PAUSE -> mapView.onPause()
                Lifecycle.Event.ON_STOP -> mapView.onStop()
                Lifecycle.Event.ON_DESTROY -> {
                    //handled in onDispose
                }
                else -> throw IllegalStateException()
            }
        }
    }

The reason is that Lifecycle.Event.ON_DESTROY might not be called, and this way you would always dispose of the map view in onDispose, which will always be called.
Would you be able to add this? I did fork and update it and the big memory leak disappeared.

`1 APPLICATION LEAKS

References underlined with "~~~" are likely causes.
Learn more at https://squ.re/leaks.

56446 bytes retained by leaking objects
Signature: 34a030bd011ab1ccd655bbeac3fbc8465c53ce5f
┬───
│ GC Root: Thread object
│
├─ com.google.maps.api.android.lib6.gmm6.vector.n instance
│    Leaking: UNKNOWN
│    Retaining 3.5 MB in 26933 objects
│    Thread name: 'RenderDrive'
│    ↓ n.e
│        ~
├─ com.google.maps.api.android.lib6.gmm6.vector.p instance
│    Leaking: UNKNOWN
│    Retaining 3.5 MB in 26930 objects
│    ↓ p.f
│        ~
├─ com.google.maps.api.android.lib6.gmm6.api.ac instance
│    Leaking: UNKNOWN
│    Retaining 3.5 MB in 26929 objects
│    View not part of a window view hierarchy
│    View.mAttachInfo is null (view detached)
│    View.mWindowAttachCount = 1
│    mContext instance of ExampleApplication
│    ↓ ac.N
│         ~
├─ com.google.maps.api.android.lib6.impl.ax instance
│    Leaking: UNKNOWN
│    Retaining 20 B in 1 objects
│    a instance of ExampleApplication
│    b instance of ExampleActivity with mDestroyed = true
│    ↓ ax.b
│         ~
╰→ ExampleActivity instance`

[arriolac]

Thanks for filling and opening a new issue. I'm not certain that your proposed workaround will fix this leak. MapView.onDestroy() is meant to be called on an activity/fragment onDestroy event, which as you mentioned, is not guaranteed to be called. So in theory the map should not leak memory even if onDestroy is not called.

Looking up existing issues on the Maps SDK issue tracker, it appears that this might be an issue on the Maps SDK itself and independent of Maps Compose. See https://issuetracker.google.com/issues/219068016

[polivmi1]

@arriolac it is true that it won't fix the very long-term google maps SDK issue, but that issue leaks a few kb of internal memory (can be fixed with weak references). That is a different leak.

However, this issue leaks multiple megabytes because nothing is being cleared. I have this tested in AS and also in production where this fix brought the average memory used down from 450MB to 140MB. I am using the same thing for camerax compose android view.

If you do call OnDispose and that removes the lifecycle before the activity is killed, the lifecycle onDestroy is never called even if activity onDestroy is called. There is a memory leak

android-maps-compose/maps-compose/src/main/java/com/google/maps/android/compose/GoogleMap.kt

Line 170 in e4e6796

onDispose {

That can happen often.

A similar issue that causes onDestroy not to be called because of the same lifecycle that here causes memory leaks is discussed here google/accompanist#978

Might also be similar to android/camera-samples#352 that google maps is keeping something on a separate process, which then causes a leak if stuff isn't cleared. Actually I think this is the cause (leaking the few kb of google maps memory), but in this case if we don't clear the other things in google maps it will also leak the activity and application which is few MB.

[arriolac]

Can you provide some repro steps? Would be great if we can validate this in the sample app included in the repro and verify that your proposed fix addresses it.

[polivmi1]

@arriolac this is a pretty standard project, where there is a map, marker and clicking the marker goes to other screen, clicking back button goes back. Using compose navigation. Unfortunately, I can't share the big project where the leak is big, because it is private. I can add you these logs and create it in the sample to validate based on the lifecycle + you could add leakcanary and see the difference between this (will depend on the app data size, because it leaks it all) and the small google internal leak.
I can create it if you need it, but first I wanted to sent you logs from such an app lifecycle, so you can see that in this case ON_DESTROY is never called and memory keeps leaking (the previous screen is on backstack until the back button is pressed and it is re-created):

(it would be called when we get back to the screen and everything would be cleared, but onDispose removes the listener when moving to screen B)

open screen A with map
2022-06-10 06:53:21.977 2289-2289/ D/AAA: ON_CREATE
2022-06-10 06:53:22.146 2289-2289/ D/AAA: ON_START
2022-06-10 06:53:22.146 2289-2289/ D/AAA: ON_RESUME
navigate to screen B
2022-06-10 06:53:48.368 2289-2289/ D/AAA: ON_PAUSE
2022-06-10 06:53:48.368 2289-2289/ D/AAA: ON_STOP
screen B create,start,resume
2022-06-10 06:53:49.245 2289-2289/ D/AAA: MapLifecycle: onDispose
press back to screen A (this will create a new mapview and if the old one wasn't disposed in onDispose, it leaks data)
2022-06-10 06:53:53.243 2289-2289/ D/AAA: ON_CREATE
2022-06-10 06:53:53.265 2289-2289/ D/AAA: ON_START
2022-06-10 06:53:54.004 2289-2289/ D/AAA: ON_RESUME

[polivmi1]

@arriolac does this help or would you want me to extend the sample project to show this problem?

[arriolac]

@polivmi1 yes, that would be helpful.

[polivmi1]

@arriolac I have added a sample here https://github.com/polivmi1/android-maps-compose
It contains 2 screens using compose navigation.
Steps to reproduce the problem:

1. Open in profiler and memory
2. open basic map
3. click on marker
4. click on BACK button
5. repeat steps 3 and 4 multiple times seeing that memory is never cleaned
6. minimize the app to free the leaked objects

I am also attaching screenshots of the step 5 and 6:

Please let me know if you need more information. It should be clear from this, but you can also add the lifecycle logs and try with the fix I mentioned in the first comment.

Thanks for looking into it!

[polivmi1]

@arriolac is this in your queue or is there anything else I could add to help with it?

[arriolac]

@polivmi1 you've been really helpful with the amount of information you've shared already. This is in my queue to look into.

[arriolac]

I can confirm the memory leak and your proposed workaround does indeed fix the issue, however, I think the underlying lifecycle for the NavBackStackEntry should be receiving the ON_DESTROY event. Will raise this to the navigation compose team to see where the fix should be applied.

[polivmi1]

@arriolac is there an issuetracker discussion or a github issue with the navigation compose team that I could follow?

[arriolac]

@polivmi1 supposedly adding a custom animation within a NavHost is not supported. Can you try removing the wrapping AnimatedVisibility to see if you still encounter a leak?

[polivmi1]

@arriolac what AnimatedVisibility do you mean? I don't use anything that would wrap it on that screen. I use AnimatedVisibility only for some elements on some other screens to animate a button for example. What I use for screen animation is https://google.github.io/accompanist/navigation-animation/ - AnimatedNavHost - is that the problem?
The one used in the sample and wrapping CircularProgressIndicator doesn't cause this - I don't have it in my app and after removing it, the memory is still not GC and there is no ON_DESTROY.

[arriolac]

Ok I've confirmed that it is the intended behavior that the previous destination is not getting the DESTROYED event when navigating away from it. Your proposed workaround #138 (comment) should be put in place to resolve this @polivmi1. Would you like to make this contribution to the library?

[googlemaps-bot]

🎉 This issue has been resolved in version 2.7.1 🎉

The release is available on:

• v2.7.1
• GitHub release

Your semantic-release bot 📦🚀

[googlemaps-bot]

🎉 This issue has been resolved in version 2.7.2 🎉

The release is available on:

• v2.7.2
• GitHub release

Your semantic-release bot 📦🚀