fix: skip bulk markers with an invalid coordinate instead of crashing the map

README.md

@@ -576,11 +576,11 @@ A coordinate that arrives as `NaN` or out of range is dropped instead of being f
 - An invalid `region` is ignored, and the map keeps the region it already had.
 - An invalid `camera` is ignored the same way, and a pitch past the range the SDKs draw is pulled back to it rather than rejected.
 - An overlay whose coordinates, ring length or radius cannot be drawn is skipped; its neighbours still render.
-- Anything supplied through `region`, `camera`, or a `<Marker>` / `<Polyline>` / `<Polygon>` / `<Circle>` child is reported through `console.warn` in development.
+- Anything supplied through `region`, `camera`, the bulk `markers` prop, or a `<Marker>` / `<Polyline>` / `<Polygon>` / `<Circle>` child is reported through `console.warn` in development.
 
-Where the check runs depends on the entry point. `region`, `camera` and `fitToCoordinates` are guarded natively on both platforms, so a `hybridRef` call - `setCamera` and `animateCamera` included - cannot reach the SDKs either. Overlay descriptors are additionally filtered natively on Android, where an undrawable overlay throws inside the Fabric mount transaction and would otherwise take the whole screen down; those skips are reported to logcat rather than `console.warn`.
+Where the check runs depends on the entry point. `region`, `camera`, `fitToCoordinates` and marker descriptors are guarded natively on both platforms, so a `hybridRef` call - `setCamera` and `animateCamera` included - cannot reach the SDKs either. Polyline, polygon and circle descriptors are additionally filtered natively on Android, where an undrawable overlay throws inside the Fabric mount transaction and would otherwise take the whole screen down. Native skips are reported to logcat on Android rather than through `console.warn`.
 
-One gap is worth knowing about: descriptors passed through the bulk `markers` prop are checked on neither side - only the `<Marker>` child is.
+One gap is worth knowing about: descriptors passed through the bulk `polylines` / `polygons` / `circles` props are checked only natively on Android - on iOS they reach MapKit and the Google Maps SDK unchecked, and neither platform warns about them in development.
 
 Valid means: latitude and longitude finite and within ±90 / ±180, region deltas finite and greater than 0, camera `zoom` / `heading` / `pitch` / `altitude` finite when supplied (with `zoom` and `heading` also small enough for the 32-bit float the SDKs keep them in), two coordinates for a polyline, three per polygon ring, and a finite radius of at least 0 for a circle. A region whose span would run past a pole is pulled back to what the map can show rather than rejected.
 

package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt

@@ -33,7 +33,10 @@ internal class MapOverlayController(
   private val polygons = LinkedHashMap<String, Polygon>()
   private val circles = LinkedHashMap<String, Circle>()
   private val markerEnterAnimators = HashMap<String, Animator>()
-  private val renderState = MarkerRenderState()
+  private val renderState =
+    MarkerRenderState { descriptor ->
+      Log.w(NITRO_MAPS_LOG_TAG, "Skipped marker \"${descriptor.id}\": it cannot be drawn.")
+    }
   private var onMarkerPress: ((String) -> Unit)? = null
   private var onClusterPress: ((List<String>, Coordinate) -> Unit)? = null
   private var spatialIndex: MarkerSpatialIndex? = null
@@ -112,7 +115,8 @@ internal class MapOverlayController(
     renderState.reset()
     spatialIndex = null
     refreshGeneration += 1
-    computeExecutor.shutdown()
+    // Queued work would only be discarded by the generation check, so drop it.
+    computeExecutor.shutdownNow()
     computeExecutor = Executors.newSingleThreadExecutor()
   }
 
@@ -156,7 +160,7 @@ internal class MapOverlayController(
     refreshGeneration += 1
     val generation = refreshGeneration
 
-    computeExecutor.execute {
+    executeCompute {
       val candidates = index.candidates(bounds)
       val elements: List<ClusterElement> =
         if (clustering) {
@@ -183,7 +187,7 @@ internal class MapOverlayController(
     refreshGeneration += 1
     val generation = refreshGeneration
 
-    computeExecutor.execute {
+    executeCompute {
       val index = MarkerSpatialIndex(descriptors)
       mainHandler.post {
         if (generation != refreshGeneration) {
@@ -195,6 +199,20 @@ internal class MapOverlayController(
     }
   }
 
+  /**
+   * An exception escaping a [computeExecutor] task would reach the thread's uncaught exception
+   * handler and kill the app; a failed refresh just leaves the markers on screen as they are.
+   */
+  private fun executeCompute(task: () -> Unit) {
+    computeExecutor.execute {
+      try {
+        task()
+      } catch (error: Exception) {
+        Log.e(NITRO_MAPS_LOG_TAG, "Marker computation failed; the markers on screen were left as they are.", error)
+      }
+    }
+  }
+
   private fun applyDiff(
     diff: MarkerRenderDiff,
     animateEntering: Boolean = true,

package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerClusterEngine.kt

@@ -49,6 +49,9 @@ internal sealed interface ClusterElement {
  * Pure function over descriptor data (no map projection), so it is safe to call
  * from a background thread. Output is bounded by the number of grid cells that
  * fit on screen, keeping per-frame Google Maps work small and constant.
+ *
+ * Every coordinate must be placeable: a `NaN` one joins a cluster whose
+ * `LatLngBounds` throws. [MarkerRenderState] drops those on the way in.
  */
 internal object MarkerClusterEngine {
   private const val CELL_DP = 64.0

package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerRenderState.kt

@@ -10,8 +10,14 @@ package com.margelo.nitro.nitromaps
  *
  * [setMarkers], [setClusteringEnabled] and [attachMap] return whether the caller
  * has to redraw the markers.
+ *
+ * Markers whose coordinate cannot be placed are dropped on the way in and reported
+ * to [onSkippedMarker], so neither the synchronous path nor the viewport pipeline
+ * ever sees one. JS drops them too, but `hybridRef` reaches [setMarkers] directly.
  */
-internal class MarkerRenderState {
+internal class MarkerRenderState(
+  private val onSkippedMarker: (MarkerDescriptor) -> Unit = {},
+) {
   var descriptors: Array<MarkerDescriptor> = emptyArray()
     private set
 
@@ -33,12 +39,13 @@ internal class MarkerRenderState {
 
   fun setMarkers(next: Array<MarkerDescriptor>?): Boolean {
     val nextDescriptors = next ?: emptyArray()
+    // Fingerprinted as delivered, so redelivering drawn markers skips the filter and its reports.
     val nextFingerprint = nextDescriptors.markersFingerprint()
     if (nextFingerprint == fingerprint && isDrawn) {
       return false
     }
 
-    descriptors = nextDescriptors
+    descriptors = placeable(nextDescriptors)
     fingerprint = nextFingerprint
     if (!isMapAttached) {
       // Nothing reached the map, so nothing may be remembered as drawn.
@@ -81,6 +88,12 @@ internal class MarkerRenderState {
     isDrawn = false
   }
 
+  private fun placeable(delivered: Array<MarkerDescriptor>): Array<MarkerDescriptor> {
+    val (placeable, skipped) = delivered.partition { it.isValid() }
+    skipped.forEach(onSkippedMarker)
+    return placeable.toTypedArray()
+  }
+
   private companion object {
     /** Non-clustered datasets at or below this size reconcile synchronously. */
     const val ASYNC_THRESHOLD = 500

package/android/src/main/java/com/margelo/nitro/nitromaps/OverlayDescriptor+Validity.kt

@@ -1,5 +1,8 @@
 package com.margelo.nitro.nitromaps
 
+/** A marker is placed by its coordinate alone; clustering one that cannot be placed throws in `LatLngBounds`. */
+internal fun MarkerDescriptor.isValid(): Boolean = coordinate.isValid()
+
 /** A polyline needs two placeable points before `GoogleMap.addPolyline` accepts it. */
 internal fun PolylineDescriptor.isValid(): Boolean = coordinates.isValidPath(MINIMUM_POLYLINE_SIZE)
 

package/android/src/test/java/com/margelo/nitro/nitromaps/MarkerDescriptorFixture.kt

@@ -2,6 +2,7 @@ package com.margelo.nitro.nitromaps
 
 internal fun marker(
   id: String = "marker-1",
+  coordinate: Coordinate = Coordinate(37.77, -122.41),
   image: MarkerImage? = null,
   markerColor: String? = null,
   anchor: MarkerAnchor? = null,
@@ -17,7 +18,7 @@ internal fun marker(
   // shifts every positional argument after it.
   return MarkerDescriptor(
     id = id,
-    coordinate = Coordinate(37.77, -122.41),
+    coordinate = coordinate,
     title = "Title",
     subtitle = "Subtitle",
     draggable = false,

package/android/src/test/java/com/margelo/nitro/nitromaps/MarkerRenderStateTest.kt

@@ -150,4 +150,56 @@ class MarkerRenderStateTest {
 
     assertTrue(state.usesViewportPipeline)
   }
+
+  @Test
+  fun `markers that cannot be placed are dropped and reported`() {
+    val skipped = ArrayList<String>()
+    val state = MarkerRenderState { skipped.add(it.id) }
+    state.attachMap()
+
+    val redraw =
+      state.setMarkers(
+        arrayOf(
+          marker(id = "first"),
+          marker(id = "nan", coordinate = Coordinate(Double.NaN, Double.NaN)),
+          marker(id = "infinite", coordinate = Coordinate(Double.POSITIVE_INFINITY, 0.0)),
+          marker(id = "past-the-pole", coordinate = Coordinate(90.0001, 0.0)),
+          marker(id = "last"),
+        ),
+      )
+
+    assertTrue(redraw)
+    assertEquals(listOf("first", "last"), state.descriptors.map { it.id })
+    assertEquals(listOf("nan", "infinite", "past-the-pole"), skipped)
+  }
+
+  @Test
+  fun `redelivering markers that cannot be placed does not report them again`() {
+    val skipped = ArrayList<String>()
+    val state = MarkerRenderState { skipped.add(it.id) }
+    val markers = arrayOf(marker(id = "a"), marker(id = "nan", coordinate = Coordinate(Double.NaN, 0.0)))
+    state.attachMap()
+    state.setMarkers(markers)
+
+    assertFalse(state.setMarkers(markers))
+    assertEquals(listOf("nan"), skipped)
+  }
+
+  @Test
+  fun `markers that cannot be placed leave nothing to draw on an attaching map`() {
+    val state = MarkerRenderState()
+    state.setMarkers(arrayOf(marker(id = "nan", coordinate = Coordinate(Double.NaN, Double.NaN))))
+
+    assertFalse(state.attachMap())
+    assertTrue(state.descriptors.isEmpty())
+  }
+
+  @Test
+  fun `only markers that can be placed count towards the viewport pipeline`() {
+    val state = MarkerRenderState()
+
+    state.setMarkers(Array(500) { marker(id = "m$it") } + marker(id = "nan", coordinate = Coordinate(Double.NaN, 0.0)))
+
+    assertFalse(state.usesViewportPipeline)
+  }
 }

package/android/src/test/java/com/margelo/nitro/nitromaps/MarkerViewportPipelineTest.kt

@@ -0,0 +1,67 @@
+package com.margelo.nitro.nitromaps
+
+import com.google.android.gms.maps.model.LatLng
+import com.google.android.gms.maps.model.LatLngBounds
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertTrue
+import org.junit.Test
+
+/**
+ * [MarkerRenderState], [MarkerSpatialIndex] and [MarkerClusterEngine] in the order
+ * [MapOverlayController] runs them with clustering on; `LatLngBounds` is plain Java, so no map
+ * is needed.
+ *
+ * The viewport is 2 degrees square at 1080 x 1920 px and density 2.75, which makes the grid cells
+ * 0.25 degrees on a side: everything up to 0.25 degrees north-east of (0, 0) shares one.
+ */
+class MarkerViewportPipelineTest {
+  private val viewport = LatLngBounds(LatLng(-1.0, -1.0), LatLng(1.0, 1.0))
+  private val unplaceable = Coordinate(latitude = Double.NaN, longitude = Double.NaN)
+
+  @Test
+  fun `markers that share a grid cell form one cluster`() {
+    val cluster = clusters(marker(id = "a", coordinate = point(0.01)), marker(id = "b", coordinate = point(0.02)))
+
+    assertEquals(listOf("a", "b"), (cluster.single() as ClusterElement.Cluster).memberIds)
+  }
+
+  @Test
+  fun `markers that cannot be placed never reach a cluster`() {
+    // `floor(NaN).toInt()` is 0, so unfiltered they share the (0, 0) cell with the real markers
+    // and turn the cluster's bounds into NaN, which `LatLngBounds` throws on.
+    val cluster =
+      clusters(
+        marker(id = "a", coordinate = point(0.01)),
+        marker(id = "bad-1", coordinate = unplaceable),
+        marker(id = "b", coordinate = point(0.02)),
+        marker(id = "bad-2", coordinate = unplaceable),
+      )
+
+    assertEquals(listOf("a", "b"), (cluster.single() as ClusterElement.Cluster).memberIds)
+  }
+
+  @Test
+  fun `a dataset with nothing that can be placed clusters to nothing`() {
+    val elements = clusters(marker(id = "bad-1", coordinate = unplaceable), marker(id = "bad-2", coordinate = unplaceable))
+
+    assertTrue(elements.isEmpty())
+  }
+
+  private fun clusters(vararg markers: MarkerDescriptor): List<ClusterElement> {
+    val state = MarkerRenderState()
+    state.attachMap()
+    state.setClusteringEnabled(true)
+    state.setMarkers(arrayOf(*markers))
+
+    val candidates = MarkerSpatialIndex(state.descriptors).candidates(viewport)
+    return MarkerClusterEngine.clusters(candidates, viewport, VIEW_WIDTH_PX, VIEW_HEIGHT_PX, DENSITY)
+  }
+
+  private fun point(degrees: Double): Coordinate = Coordinate(latitude = degrees, longitude = degrees)
+
+  private companion object {
+    const val VIEW_WIDTH_PX = 1080
+    const val VIEW_HEIGHT_PX = 1920
+    const val DENSITY = 2.75f
+  }
+}

package/android/src/test/java/com/margelo/nitro/nitromaps/OverlayDescriptorValidityTest.kt

@@ -5,6 +5,14 @@ import org.junit.Assert.assertTrue
 import org.junit.Test
 
 class OverlayDescriptorValidityTest {
+  @Test
+  fun requiresAPlaceableCoordinateForAMarker() {
+    assertTrue(marker(coordinate = point()).isValid())
+    assertFalse(marker(coordinate = point(latitude = Double.NaN)).isValid())
+    assertFalse(marker(coordinate = point(longitude = Double.NEGATIVE_INFINITY)).isValid())
+    assertFalse(marker(coordinate = point(latitude = 90.0001)).isValid())
+  }
+
   @Test
   fun requiresTwoPlaceablePointsForAPolyline() {
     assertFalse(polyline().isValid())

package/ios/MarkerClusterEngine.swift

@@ -5,6 +5,9 @@ import MapKit
 /// Runs entirely off descriptor data (no `MKMapView` projection), so it is safe
 /// to call from a background queue. Output is bounded by the number of grid
 /// cells that fit on screen, keeping per-frame MapKit work small and constant.
+///
+/// Every coordinate must be placeable: the grid cell comes from `Int(_:)`, which
+/// traps on NaN and infinity. `MarkerRenderPipeline.setMarkers` drops those.
 enum MarkerClusterEngine {
   /// A single display element: an individual marker or a cluster badge.
   enum Element {
@@ -407,11 +410,20 @@ final class MarkerRenderPipeline {
     }
 
     markersFingerprint = fingerprint
-    allMarkerDescriptors = next
+    allMarkerDescriptors = Self.placeable(next)
     spatialIndex = nil
     return true
   }
 
+  /// JS drops these already, but `hybridRef` reaches the native setter directly.
+  private static func placeable(_ descriptors: [MarkerDescriptor]) -> [MarkerDescriptor] {
+    // Filtering copies every C++-backed descriptor, so skip it when nothing is dropped.
+    guard !descriptors.allSatisfy({ $0.coordinate.isValid }) else {
+      return descriptors
+    }
+    return descriptors.filter { $0.coordinate.isValid }
+  }
+
   func reapply(
     displayedVersions: [String: Int],
     region: MKCoordinateRegion,

package/ios/MarkerSpatialIndex.swift

@@ -5,6 +5,9 @@ import MapKit
 /// Built once per dataset so viewport queries cost O(cells in view + markers in
 /// those cells) instead of O(all markers). Immutable after init, so instances
 /// are safe to query from a background queue.
+///
+/// Every coordinate must be placeable: cells come from `Int(_:)`, which traps on
+/// NaN and infinity. `MarkerRenderPipeline.setMarkers` drops those.
 final class MarkerSpatialIndex {
   let count: Int
   private let cellsPerSide: Int