Repository navigation
fix: release GoogleMap listeners and dispose subcomposition when showing/hiding map #1039
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import androidx.compose.foundation.layout.Box | |
| import androidx.compose.foundation.layout.fillMaxSize | ||
| import androidx.compose.material3.Text | ||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.runtime.ReusableContentHost | ||
| import androidx.compose.runtime.mutableStateOf | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.test.assertIsDisplayed | ||
|
|
@@ -39,6 +40,7 @@ import kotlinx.coroutines.runBlocking | |
| import org.junit.Before | ||
| import org.junit.Rule | ||
| import org.junit.Test | ||
| import java.lang.ref.WeakReference | ||
| import java.util.concurrent.CountDownLatch | ||
| import java.util.concurrent.TimeUnit | ||
| import kotlin.math.abs | ||
|
|
@@ -501,6 +503,103 @@ class GoogleMapViewTests { | |
| .performClick() | ||
| } | ||
|
|
||
| @Test | ||
| fun testShowingHidingMapRepeatedlyReleasesMapViews() { | ||
| // Regression test for https://github.com/googlemaps/android-maps-compose/issues/581 | ||
| val showMapState = mutableStateOf(true) | ||
| val mapViews = mutableListOf<WeakReference<MapView>>() | ||
| lateinit var markerState: MarkerState | ||
|
|
||
| composeTestRule.setContent { | ||
| markerState = rememberUpdatedMarkerState(position = startingPosition) | ||
| if (showMapState.value) { | ||
| GoogleMap( | ||
| modifier = Modifier.fillMaxSize(), | ||
| cameraPositionState = cameraPositionState, | ||
| mapViewFactory = { context, options -> | ||
| MapView(context, options).also { | ||
| mapViews.add(WeakReference(it)) | ||
| } | ||
| }, | ||
| ) { | ||
| MarkerInfoWindowContent( | ||
| state = markerState, | ||
| title = "Test", | ||
| ) { | ||
| Text("Test Info") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Toggle visibility repeatedly (including rapid cycles before and after map initialization) | ||
| repeat(15) { | ||
| showMapState.value = true | ||
| composeTestRule.waitForIdle() | ||
| Thread.sleep(50) | ||
| showMapState.value = false | ||
| composeTestRule.waitForIdle() | ||
| assertThat(cameraPositionState.projection).isNull() | ||
| } | ||
|
|
||
| // Allow any pending main-thread callbacks (e.g., Compose's accessibility delegate | ||
| // periodic prune) to complete and trigger GC | ||
| Thread.sleep(250) | ||
| composeTestRule.waitForIdle() | ||
| Runtime.getRuntime().gc() | ||
| System.runFinalization() | ||
| Runtime.getRuntime().gc() | ||
|
|
||
| assertThat(mapViews.size).isAtLeast(15) | ||
| // Every released MapView should have cleared its tag and child views | ||
| for (ref in mapViews) { | ||
| val mapView = ref.get() | ||
| if (mapView != null) { | ||
| assertThat(mapView.tag).isNull() | ||
| assertThat(mapView.childCount).isEqualTo(0) | ||
| } | ||
| } | ||
| // Verify MapView instances do not accumulate across show/hide cycles | ||
| // (Compose's accessibility delegate or the JIT register map may briefly hold 1-3 recent refs) | ||
| val liveCount = mapViews.count { it.get() != null } | ||
| assertThat(liveCount).isAtMost(3) | ||
| } | ||
|
|
||
| @Test | ||
| fun testReusableContentHostResetAndReuse() { | ||
| val activeState = mutableStateOf(true) | ||
| lateinit var markerState: MarkerState | ||
|
|
||
| composeTestRule.setContent { | ||
| markerState = rememberUpdatedMarkerState(position = startingPosition) | ||
| ReusableContentHost(active = activeState.value) { | ||
| GoogleMap( | ||
| modifier = Modifier.fillMaxSize(), | ||
| cameraPositionState = cameraPositionState, | ||
| ) { | ||
| Marker( | ||
| state = markerState, | ||
| title = "Reusable Marker", | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| composeTestRule.waitForIdle() | ||
| Thread.sleep(100) | ||
|
|
||
| // Deactivate and reactivate multiple times to exercise AndroidView.onReset -> update | ||
| repeat(3) { | ||
| activeState.value = false | ||
| composeTestRule.waitForIdle() | ||
| assertThat(cameraPositionState.projection).isNull() | ||
|
|
||
| activeState.value = true | ||
| composeTestRule.waitForIdle() | ||
| Thread.sleep(100) | ||
|
Collaborator
There was a problem hiding this comment. Could we assert that the map comes back after each reactivation? Right now the test passes as long as nothing crashes. Something like
Collaborator
Author
There was a problem hiding this comment. Done in 82565df — replaced both |
||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun testRememberUpdatedMarkerStateBeUpdate() { | ||
| val testPoint0 = LatLng(0.0, 0.0) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,6 @@ | |||||
|
|
||||||
| package com.google.maps.android.compose | ||||||
|
|
||||||
| import android.content.ComponentCallbacks2 | ||||||
| import android.content.Context | ||||||
| import android.content.res.Configuration | ||||||
|
|
@@ -60,6 +59,7 @@ import kotlinx.coroutines.CoroutineStart | |||||
| import kotlinx.coroutines.Dispatchers | ||||||
| import kotlinx.coroutines.Job | ||||||
| import kotlinx.coroutines.awaitCancellation | ||||||
| import kotlinx.coroutines.isActive | ||||||
| import kotlinx.coroutines.launch | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -168,7 +168,6 @@ public fun GoogleMap( | |||||
|
|
||||||
| val parentComposition = rememberCompositionContext() | ||||||
| val currentContent by rememberUpdatedState(content) | ||||||
| val parentCompositionScope = rememberCoroutineScope() | ||||||
|
|
||||||
| AndroidView( | ||||||
|
|
@@ -216,48 +215,55 @@ public fun GoogleMap( | |||||
|
|
||||||
| val lifecycleObserver = MapLifecycleEventObserver(mapView) | ||||||
|
|
||||||
| // Only register for [lifecycleOwner]'s lifecycle events while MapView is attached | ||||||
| val onAttachStateListener = object : View.OnAttachStateChangeListener { | ||||||
| private var lifecycle: Lifecycle? = null | ||||||
|
|
||||||
| override fun onViewAttachedToWindow(mapView: View) { | ||||||
| lifecycle = mapView.findViewTreeLifecycleOwner()?.lifecycle?.also { | ||||||
|
Collaborator
There was a problem hiding this comment. Just to understand it: in which case do we get here without a lifecycle owner? Before, the
Collaborator
Author
There was a problem hiding this comment. Good catch! Since In 82565df, replaced the silent |
||||||
| it.addObserver(lifecycleObserver) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| override fun onViewDetachedFromWindow(v: View) { | ||||||
| cleanup() | ||||||
| } | ||||||
|
|
||||||
| fun cleanup() { | ||||||
| lifecycle?.removeObserver(lifecycleObserver) | ||||||
| lifecycle = null | ||||||
| lifecycleObserver.moveToBaseState() | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| mapView.tag = MapTagData( | ||||||
| componentCallbacks = componentCallbacks, | ||||||
| lifecycleObserver = lifecycleObserver, | ||||||
| onAttachStateListener = onAttachStateListener::cleanup, | ||||||
| onAttachStateChangeListener = onAttachStateListener, | ||||||
| isLiteMode = isLiteMode, | ||||||
| ) | ||||||
|
|
||||||
| mapView.addOnAttachStateChangeListener(onAttachStateListener) | ||||||
| } | ||||||
| }, | ||||||
| onReset = { mapView -> | ||||||
| mapView.onReset() | ||||||
| }, | ||||||
| onRelease = { mapView -> | ||||||
| mapView.onRelease() | ||||||
| }, | ||||||
| update = { mapView -> | ||||||
| mapView.applyFocusability(focusable) | ||||||
| mapView.setViewTreeLifecycleOwner(lifecycleOwner) | ||||||
| mapView.setViewTreeSavedStateRegistryOwner(savedStateRegistryOwner) | ||||||
| val tagData = mapView.tagDataOrNull | ||||||
| if (tagData != null && tagData.subcompositionJob == null) { | ||||||
| tagData.subcompositionJob = parentCompositionScope.launchSubcomposition( | ||||||
| mapUpdaterState, | ||||||
| parentComposition, | ||||||
| mapView, | ||||||
| tagData, | ||||||
| mapClickListeners, | ||||||
| currentContent, | ||||||
| ) | ||||||
|
|
@@ -287,30 +293,35 @@ private fun MapView.applyFocusability(focusable: Boolean) { | |||||
| * Create and apply the [content] compositions to the map + | ||||||
| * dispose the [Composition] when the parent composable is disposed. | ||||||
| * */ | ||||||
| internal fun CoroutineScope.launchSubcomposition( | ||||||
| mapUpdaterState: MapUpdaterState, | ||||||
| parentComposition: CompositionContext, | ||||||
| mapView: MapView, | ||||||
| tagData: MapTagData, | ||||||
| mapClickListeners: MapClickListeners, | ||||||
| content: | ||||||
| @Composable @GoogleMapComposable | ||||||
| () -> Unit, | ||||||
| ): Job { | ||||||
| // Use [CoroutineStart.UNDISPATCHED] to kick off GoogleMap loading immediately | ||||||
| return launch( | ||||||
| context = Dispatchers.Main.immediate, | ||||||
| start = CoroutineStart.UNDISPATCHED, | ||||||
| ) { | ||||||
| val map = mapView.awaitMap() | ||||||
| if (!isActive || mapView.tagDataOrNull !== tagData) { | ||||||
| return@launch | ||||||
| } | ||||||
| val composition = Composition( | ||||||
| applier = MapApplier( | ||||||
| map, | ||||||
| mapView, | ||||||
| mapClickListeners, | ||||||
| isLiteMode = tagData.isLiteMode, | ||||||
| ), | ||||||
| parent = parentComposition, | ||||||
| ) | ||||||
| tagData.subcomposition = composition | ||||||
|
|
||||||
| try { | ||||||
| composition.setContent { | ||||||
|
|
@@ -325,6 +336,9 @@ private fun CoroutineScope.launchSubcomposition( | |||||
| } | ||||||
| awaitCancellation() | ||||||
| } finally { | ||||||
| if (tagData.subcomposition === composition) { | ||||||
| tagData.subcomposition = null | ||||||
| } | ||||||
| composition.dispose() | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -352,14 +366,45 @@ internal class MapUpdaterState( | |||||
| } | ||||||
|
|
||||||
| /** Used to store things in the tag which must be retrievable across recompositions */ | ||||||
| internal class MapTagData( | ||||||
| val componentCallbacks: ComponentCallbacks2, | ||||||
| val lifecycleObserver: MapLifecycleEventObserver, | ||||||
| val onAttachStateListener: () -> Unit, | ||||||
|
Collaborator
There was a problem hiding this comment. nit: this sits right next to
Suggested change
Collaborator
Author
There was a problem hiding this comment. Done in 82565df — renamed |
||||||
| val onAttachStateChangeListener: View.OnAttachStateChangeListener, | ||||||
| val isLiteMode: Boolean, | ||||||
| ) { | ||||||
| var subcompositionJob: Job? = null | ||||||
| var subcomposition: Composition? = null | ||||||
|
|
||||||
| fun disposeSubcomposition() { | ||||||
| subcompositionJob?.cancel() | ||||||
| subcompositionJob = null | ||||||
| subcomposition?.dispose() | ||||||
| subcomposition = null | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| internal val MapView.tagDataOrNull: MapTagData? | ||||||
| get() = tag as? MapTagData | ||||||
|
|
||||||
| internal fun MapView.onReset() { | ||||||
| tagDataOrNull?.disposeSubcomposition() | ||||||
| } | ||||||
|
|
||||||
| internal fun MapView.onRelease() { | ||||||
| val tagData = tagDataOrNull | ||||||
| tagData?.disposeSubcomposition() | ||||||
| if (tagData != null) { | ||||||
| context.unregisterComponentCallbacks(tagData.componentCallbacks) | ||||||
| removeOnAttachStateChangeListener(tagData.onAttachStateChangeListener) | ||||||
| tagData.onAttachStateListener() | ||||||
| tagData.lifecycleObserver.moveToDestroyedState() | ||||||
| } | ||||||
| setViewTreeLifecycleOwner(null) | ||||||
| setViewTreeSavedStateRegistryOwner(null) | ||||||
| removeAllViews() | ||||||
| tag = null | ||||||
| } | ||||||
|
|
||||||
| public typealias GoogleMapFactory = @Composable () -> Unit | ||||||
|
|
||||||
|
|
@@ -398,7 +443,7 @@ public fun googleMapFactory( | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| internal class MapLifecycleEventObserver(private val mapView: MapView) : LifecycleEventObserver { | ||||||
| private var currentLifecycleState: Lifecycle.State = Lifecycle.State.INITIALIZED | ||||||
|
|
||||||
| override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) { | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this can be flaky on CI, since it depends on the GC actually collecting the views. Did it pass consistently over several runs on the emulator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point! While it passed consistently across multiple local emulator runs (and 30-cycle HPROF heap dumps confirmed 0
GoogleMap/MapApplierinstances retained), relying onRuntime.getRuntime().gc()in an instrumentation test can be non-deterministic on CI emulators.In 82565df, updated
testShowingHidingMapRepeatedlyReleasesMapViewsto assert the deterministic post-onRelease()state on every createdMapView(mapView.tag == null,mapView.childCount == 0,mapView.findViewTreeLifecycleOwner() == null,mapView.findViewTreeSavedStateRegistryOwner() == null, andcameraPositionState.projection == null) without depending on GC timing.