diff --git a/android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationComponentManager.kt b/android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationComponentManager.kt index 1711ed328..6c9053c4a 100644 --- a/android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationComponentManager.kt +++ b/android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationComponentManager.kt @@ -108,7 +108,7 @@ class LocationComponentManager(mapView: RNMBXMapView, context: Context) { private fun applyStateChanges(map: RNMBXMapView, oldState: State, newState: State, fullUpdate: Boolean) { val mapView = map.mapPlugins - if (map.getLifecycleState() != Lifecycle.State.STARTED && map.getLifecycleState() != Lifecycle.State.INITIALIZED) { + if (!isRunning(map)) { // In case lifecycle was already stopped, so we're part of shutdown, do not call updateSettings as it'll just restart // the loationComponent that will not be stopped. See https://github.com/mapbox/mapbox-maps-android/issues/2017 if (!newState.enabled) { @@ -223,6 +223,18 @@ class LocationComponentManager(mapView: RNMBXMapView, context: Context) { update { it } } + private fun isRunning(map: RNMBXMapView) = + map.getLifecycleState() == Lifecycle.State.STARTED || map.getLifecycleState() == Lifecycle.State.INITIALIZED + + // Lets RNMBXNativeUserLocation rebuild a puck the Maps SDK can no longer add. + internal val puckGuardComponent = LocationComponentReset( + isRequested = { mState.enabled }, + isRunning = { isRunning(mMapView) }, + isEnabled = { mMapView.mapPlugins.location.enabled }, + setEnabled = { mMapView.mapPlugins.location.enabled = it }, + holdProvider = ::holdLocationProviderBriefly, + ) + // All maps share one Mapbox device location provider, and restarting it before its stop completes leaks a // fusedLocationClientHandlerThread (#4225), so stay subscribed briefly in case another map takes over in the same frame. private fun holdLocationProviderBriefly() { diff --git a/android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationPuckGuard.kt b/android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationPuckGuard.kt new file mode 100644 index 000000000..e7eaf208b --- /dev/null +++ b/android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationPuckGuard.kt @@ -0,0 +1,91 @@ +package com.rnmapbox.rnmbx.components.location + +/** + * Applies location puck settings, recovering when the Maps SDK fails to re-add the puck layer. + * + * The location component keeps the last puck bearing it received, including NaN or Infinity. + * The next time the puck layer is added, such as when the puck settings change, + * `addPersistentStyleLayer` throws `IllegalArgumentException: Parse error at offset 165: Missing + * a colon after a name of object member.` See https://github.com/mapbox/mapbox-maps-android/issues/2340 + * + * Resetting the location component drops the puck state that holds the bad bearing, so the guard + * resets it and applies once more. If the map is stopped, or the retry fails because the location + * provider replays the same bearing, the component stays disabled and the next [apply] resets it. + */ +internal class LocationPuckGuard { + interface Component { + /** Whether the location component should be shown and the map is running. */ + val canReset: Boolean + + /** Disables and re-enables the location component. */ + fun reset() + + fun disable() + } + + /** True while this guard keeps the location component disabled after a failure. */ + var disabledAfterError = false + private set + + /** @return the failure that was recovered from, or null when [apply] succeeded the first time. */ + fun apply(component: Component, apply: () -> Unit): IllegalArgumentException? { + val resetting = disabledAfterError && component.canReset + try { + if (resetting) { + disabledAfterError = false + component.reset() + } + apply() + return null + } catch (error: IllegalArgumentException) { + if (!error.isAddLayerFailure()) throw error + component.disable() + disabledAfterError = true + if (!resetting && component.canReset) { + try { + component.reset() + apply() + disabledAfterError = false + } catch (retryError: IllegalArgumentException) { + if (!retryError.isAddLayerFailure()) throw retryError + component.disable() + error.addSuppressed(retryError) + } + } + return error + } + } +} + +/** + * Resets the location component for [LocationPuckGuard] without restarting the location provider. + * + * Giving up location that is on holds the provider briefly, so re-enabling does not restart it + * before its stop completes (#4225). Location that is already off, or on a stopped map, is not held: + * holding subscribes to the provider, which would start location collection. + */ +internal class LocationComponentReset( + private val isRequested: () -> Boolean, + private val isRunning: () -> Boolean, + private val isEnabled: () -> Boolean, + private val setEnabled: (Boolean) -> Unit, + private val holdProvider: () -> Unit, +) : LocationPuckGuard.Component { + override val canReset: Boolean + get() = isRequested() && isRunning() + + override fun reset() { + disable() + setEnabled(true) + } + + override fun disable() { + if (isEnabled() && isRunning()) { + holdProvider() + } + setEnabled(false) + } +} + +private fun IllegalArgumentException.isAddLayerFailure() = + stackTrace.any { it.methodName == "addPersistentStyleLayer" } diff --git a/android/src/main/java/com/rnmapbox/rnmbx/components/location/RNMBXNativeUserLocation.kt b/android/src/main/java/com/rnmapbox/rnmbx/components/location/RNMBXNativeUserLocation.kt index 38f932d71..97ae4705e 100644 --- a/android/src/main/java/com/rnmapbox/rnmbx/components/location/RNMBXNativeUserLocation.kt +++ b/android/src/main/java/com/rnmapbox/rnmbx/components/location/RNMBXNativeUserLocation.kt @@ -56,6 +56,7 @@ class RNMBXNativeUserLocation(context: Context) : AbstractMapFeature(context), O private var mContext : Context = context private var imageManager: ImageManager? = null + private val puckGuard = LocationPuckGuard() // region bearing var androidRenderMode: RenderMode? = null @@ -134,6 +135,18 @@ class RNMBXNativeUserLocation(context: Context) : AbstractMapFeature(context), O } private fun _apply(mapView: MapView) { + val locationComponent = mMapView?.locationComponentManager?.puckGuardComponent + if (locationComponent == null) { + _applySettings(mapView) + return + } + puckGuard.apply(locationComponent) { _applySettings(mapView) }?.let { error -> + val outcome = if (puckGuard.disabledAfterError) "disabled the location component" else "recreated the puck" + Logger.e(LOG_TAG, "Failed to add the location puck, $outcome: ${error.message}") + } + } + + private fun _applySettings(mapView: MapView) { val location2 = mapView.location2; val locationPuck3D = model?.let { makeLocationPuck3D(it) } diff --git a/android/src/test/kotlin/com/rnmapbox/rnmbx/components/location/LocationPuckGuardTest.kt b/android/src/test/kotlin/com/rnmapbox/rnmbx/components/location/LocationPuckGuardTest.kt new file mode 100644 index 000000000..81c92e2e7 --- /dev/null +++ b/android/src/test/kotlin/com/rnmapbox/rnmbx/components/location/LocationPuckGuardTest.kt @@ -0,0 +1,215 @@ +package com.rnmapbox.rnmbx.components.location + +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertSame +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows + +class LocationPuckGuardTest { + private class FakeComponent(override var canReset: Boolean = true) : LocationPuckGuard.Component { + val calls = mutableListOf() + override fun reset() { + calls += "reset" + } + + override fun disable() { + calls += "disable" + } + } + + // The Maps SDK's failure when a non-finite puck bearing reaches addPersistentStyleLayer. + private fun addLayerError() = IllegalArgumentException( + "Parse error at offset 165: Missing a colon after a name of object member." + ).apply { + stackTrace = arrayOf( + StackTraceElement("com.mapbox.maps.StyleManager", "addPersistentStyleLayer", "StyleManager.java", -2), + ) + } + + @Test + fun `leaves the location component alone when the puck applies`() { + val component = FakeComponent() + + val error = LocationPuckGuard().apply(component) {} + + assertNull(error) + assertEquals(emptyList(), component.calls) + } + + @Test + fun `resets the location component and applies again after the layer cannot be added`() { + val component = FakeComponent() + val guard = LocationPuckGuard() + val failure = addLayerError() + var applies = 0 + + val error = guard.apply(component) { + applies++ + if (applies == 1) throw failure + } + + assertSame(failure, error) + assertEquals(2, applies) + assertEquals(listOf("disable", "reset"), component.calls) + assertFalse(guard.disabledAfterError) + } + + @Test + fun `leaves the location component disabled when the retry also fails`() { + val component = FakeComponent() + val guard = LocationPuckGuard() + var applies = 0 + + val error = guard.apply(component) { + applies++ + throw addLayerError() + } + + assertEquals(2, applies) + assertEquals(1, error?.suppressed?.size) + assertEquals(listOf("disable", "reset", "disable"), component.calls) + assertTrue(guard.disabledAfterError) + } + + @Test + fun `resets the location component on the next apply`() { + val component = FakeComponent() + val guard = LocationPuckGuard() + guard.apply(component) { throw addLayerError() } + component.calls.clear() + + val error = guard.apply(component) {} + + assertNull(error) + assertEquals(listOf("reset"), component.calls) + assertFalse(guard.disabledAfterError) + } + + @Test + fun `disables without a second retry when the next reset fails again`() { + val component = FakeComponent() + val guard = LocationPuckGuard() + guard.apply(component) { throw addLayerError() } + component.calls.clear() + var applies = 0 + + guard.apply(component) { + applies++ + throw addLayerError() + } + + assertEquals(1, applies) + assertEquals(listOf("reset", "disable"), component.calls) + assertTrue(guard.disabledAfterError) + } + + @Test + fun `does not reset the location component while the map is stopped`() { + val component = FakeComponent(canReset = false) + val guard = LocationPuckGuard() + var applies = 0 + + guard.apply(component) { + applies++ + throw addLayerError() + } + guard.apply(component) { applies++ } + + assertEquals(2, applies) + assertEquals(listOf("disable"), component.calls) + assertTrue(guard.disabledAfterError) + + component.canReset = true + guard.apply(component) {} + + assertEquals(listOf("disable", "reset"), component.calls) + assertFalse(guard.disabledAfterError) + } + + @Test + fun `does not catch other IllegalArgumentExceptions`() { + val component = FakeComponent() + + assertThrows { + LocationPuckGuard().apply(component) { throw IllegalArgumentException("unrelated") } + } + assertEquals(emptyList(), component.calls) + } + + @Test + fun `does not catch other exceptions`() { + val component = FakeComponent() + + assertThrows { + LocationPuckGuard().apply(component) { throw IllegalStateException("unrelated") } + } + assertEquals(emptyList(), component.calls) + } +} + +class LocationComponentResetTest { + private class FakeLocation(var enabled: Boolean = true, var running: Boolean = true) { + val calls = mutableListOf() + val reset = LocationComponentReset( + isRequested = { true }, + isRunning = { running }, + isEnabled = { enabled }, + setEnabled = { + calls += "enabled=$it" + enabled = it + }, + holdProvider = { calls += "hold" }, + ) + } + + @Test + fun `holds the provider before giving up location on a running map`() { + val location = FakeLocation() + + location.reset.disable() + + assertEquals(listOf("hold", "enabled=false"), location.calls) + } + + @Test + fun `does not hold the provider when location is already off`() { + val location = FakeLocation(enabled = false) + + location.reset.disable() + + assertEquals(listOf("enabled=false"), location.calls) + } + + @Test + fun `does not hold the provider while the map is stopped`() { + val location = FakeLocation(running = false) + + location.reset.disable() + + assertFalse(location.reset.canReset) + assertEquals(listOf("enabled=false"), location.calls) + } + + @Test + fun `reset hands location over through the provider hold`() { + val location = FakeLocation() + + location.reset.reset() + + assertEquals(listOf("hold", "enabled=false", "enabled=true"), location.calls) + assertTrue(location.enabled) + } + + @Test + fun `reset of location that is already off enables it without a hold`() { + val location = FakeLocation(enabled = false) + + location.reset.reset() + + assertEquals(listOf("enabled=false", "enabled=true"), location.calls) + assertTrue(location.enabled) + } +}