From 421043da768d3ca9d5d09e55e95c52881ff91198 Mon Sep 17 00:00:00 2001 From: Anthony Trasatti Date: Sun, 8 Nov 2026 18:17:56 +0000 Subject: [PATCH] fix(android): recover the location puck from a non-finite bearing The Maps SDK location component keeps the last puck bearing it receives, including NaN or Infinity. The next time the puck layer is added, for example when LocationPuck settings change or its images load, addPersistentStyleLayer throws "IllegalArgumentException: Parse error at offset 165: Missing a colon after a name of object member." and the app crashes (mapbox/mapbox-maps-android#2340). RNMBXNativeUserLocation now applies its settings through LocationPuckGuard. When adding the puck layer fails, it resets the location component through LocationComponentManager, which drops the puck state holding the bad bearing, and applies once more. The reset holds the location provider while giving up location that is on (#4225) and is skipped while the map is stopped (mapbox/mapbox-maps-android#2017). If the reset is skipped or the retry fails too, the component stays disabled and the next puck apply resets it. Co-Authored-By: Claude Opus 5.5 --- .../location/LocationComponentManager.kt | 14 +- .../components/location/LocationPuckGuard.kt | 91 ++++++++ .../location/RNMBXNativeUserLocation.kt | 13 ++ .../location/LocationPuckGuardTest.kt | 215 ++++++++++++++++++ 4 files changed, 332 insertions(+), 1 deletion(-) create mode 100644 android/src/main/java/com/rnmapbox/rnmbx/components/location/LocationPuckGuard.kt create mode 100644 android/src/test/kotlin/com/rnmapbox/rnmbx/components/location/LocationPuckGuardTest.kt 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) + } +}