[Android Auto] Fix trip session state race condition (#6661)

* Fix trip session state race condition

* add pr to changelog

* add case where route progress is emitted while trip session state is stopped

Author: kmadsen

android-auto-app/src/main/java/com/mapbox/navigation/examples/androidauto/CarAppSyncComponent.kt

@@ -31,7 +31,7 @@ class CarAppSyncComponent private constructor() : MapboxNavigationObserver {
     private var navigationView: NavigationView? = null
     private var session: Session? = null
 
-    fun setNavigationView(navigationView: NavigationView) {
+    fun attachNavigationView(navigationView: NavigationView) {
         this.navigationView = navigationView
         navigationView.lifecycle.addObserver(object : DefaultLifecycleObserver {
             override fun onCreate(owner: LifecycleOwner) {

android-auto-app/src/main/java/com/mapbox/navigation/examples/androidauto/app/MainActivity.kt

@@ -3,11 +3,9 @@ package com.mapbox.navigation.examples.androidauto.app
 
 import android.os.Bundle
 import androidx.appcompat.app.AppCompatActivity
-import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
 import com.mapbox.navigation.examples.androidauto.CarAppSyncComponent
 import com.mapbox.navigation.examples.androidauto.databinding.MapboxActivityNavigationViewBinding
 
-@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
 class MainActivity : AppCompatActivity() {
     private lateinit var binding: MapboxActivityNavigationViewBinding
 
@@ -20,6 +18,6 @@ class MainActivity : AppCompatActivity() {
         // This allows to simulate your location
 //        binding.navigationView.api.routeReplayEnabled(true)
 
-        CarAppSyncComponent.getInstance().setNavigationView(binding.navigationView)
+        CarAppSyncComponent.getInstance().attachNavigationView(binding.navigationView)
     }
 }

libnavui-androidauto/CHANGELOG.md

@@ -15,7 +15,8 @@ Mapbox welcomes participation and contributions from everyone.
 - Renamed `RoadLabelRenderer` to `CarRoadLabelRenderer`. [#6588](https://github.com/mapbox/mapbox-navigation-android/pull/6588)
 - Made `ActiveGuidanceScreen` internal in favor of `MapboxScreen.ACTIVE_GUIDANCE`. [#6588](https://github.com/mapbox/mapbox-navigation-android/pull/6588)
 - Made `NeedsLocationPermissionsScreen` internal in favor of `MapboxScreen.NEEDS_LOCATION_PERMISSION`. [#6588](https://github.com/mapbox/mapbox-navigation-android/pull/6588)
-
+- Fixed race condition where `NavigationManager.updateTrip` can be called after `navigationEnded` is called. This is a likely cause to a crash seen. [#6661](https://github.com/mapbox/mapbox-navigation-android/pull/6661)
+ 
 ## androidauto-v0.16.0 - 04 November, 2022
 ### Changelog
 [Changes between 0.15.0 and 0.16.0](https://github.com/mapbox/mapbox-navigation-android/compare/androidauto-v0.15.0...androidauto-v0.16.0)

libnavui-androidauto/src/main/java/com/mapbox/androidauto/navigation/MapboxCarNavigationManager.kt

@@ -37,7 +37,9 @@ class MapboxCarNavigationManager internal constructor(
     private val routeProgressObserver = RouteProgressObserver { routeProgress ->
         val maneuverApi = maneuverApi ?: return@RouteProgressObserver
         val trip = CarManeuverMapper.from(routeProgress, maneuverApi)
-        navigationManager.updateTrip(trip)
+        if (mapboxNavigation?.getTripSessionState() == TripSessionState.STARTED) {
+            navigationManager.updateTrip(trip)
+        }
     }
 
     private val tripSessionStateObserver = TripSessionStateObserver { tripSessionState ->
@@ -85,15 +87,11 @@ class MapboxCarNavigationManager internal constructor(
         logAndroidAuto("MapboxNavigationManager onDetached")
         this.mapboxNavigation = null
         carTelemetry.onDetached(mapboxNavigation)
-        mapboxNavigation.unregisterTripSessionStateObserver(tripSessionStateObserver)
         mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)
+        mapboxNavigation.unregisterTripSessionStateObserver(tripSessionStateObserver)
 
-        // clearNavigationManagerCallback() can't be called during active navigation.
-        // However, this callback lets AA stop the trip session which we don't want it to do if
-        // the user simply exited AA but is still navigating via the phone app. Since
-        // there is only one instance of MapboxNavigation allowing AA to stop the trip
-        // session when the MainCarSession is inactive but possibly the phone app. is
-        // actively navigating would cause unpredictable side effects.
+        // Tell android auto navigation stopped. Navigation can continue with mapbox navigation
+        // on another device.
         navigationManager.navigationEnded()
         navigationManager.clearNavigationManagerCallback()
     }

libnavui-androidauto/src/test/java/com/mapbox/androidauto/navigation/MapboxCarNavigationManagerTest.kt

@@ -45,6 +45,14 @@ class MapboxCarNavigationManagerTest {
     private val navigationManagerCallbackSlot = slot<NavigationManagerCallback>()
     private val navigationManager: NavigationManager = mockk(relaxed = true) {
         every { setNavigationManagerCallback(capture(navigationManagerCallbackSlot)) } just Runs
+        every { navigationStarted() } answers {
+            every { clearNavigationManagerCallback() } throws IllegalStateException()
+            every { updateTrip(any()) } just Runs
+        }
+        every { navigationEnded() } answers {
+            every { clearNavigationManagerCallback() } just Runs
+            every { updateTrip(any()) } throws IllegalStateException()
+        }
     }
     private val carContext: CarContext = mockk {
         every { getCarService(NavigationManager::class.java) } returns navigationManager
@@ -138,18 +146,20 @@ class MapboxCarNavigationManagerTest {
 
     @Test
     fun `RouteProgress should trigger updateTrip`() {
-        val mapboxNavigation: MapboxNavigation = mockk(relaxed = true)
-        val progressObserverSlot = slot<RouteProgressObserver>()
-        every {
-            mapboxNavigation.registerRouteProgressObserver(capture(progressObserverSlot))
-        } just Runs
-        sut.onAttached(mapboxNavigation)
+        val tripSessionStateSlot = mutableListOf<TripSessionStateObserver>()
+        val routeProgressObserverSlot = mutableListOf<RouteProgressObserver>()
+        val mapboxNavigation: MapboxNavigation = mapboxNavigationMock(
+            tripSessionStateSlot,
+            routeProgressObserverSlot
+        )
 
+        sut.onAttached(mapboxNavigation)
+        mapboxNavigation.startTripSession()
         val routeProgress = mockk<RouteProgress> {
             every { durationRemaining } returns 100.0
             every { distanceRemaining } returns 500.0f
         }
-        progressObserverSlot.captured.onRouteProgressChanged(routeProgress)
+        routeProgressObserverSlot.forEach { it.onRouteProgressChanged(routeProgress) }
 
         verify { navigationManager.updateTrip(any()) }
     }
@@ -192,4 +202,79 @@ class MapboxCarNavigationManagerTest {
         assertEquals(1, resultsSlot.size)
         assertTrue(resultsSlot[0])
     }
+
+    @Test
+    fun `updateTrip should not be called while detaching MapboxNavigation`() = coroutineRule.runBlockingTest {
+        val tripSessionStateSlot = mutableListOf<TripSessionStateObserver>()
+        val routeProgressObserverSlot = mutableListOf<RouteProgressObserver>()
+        val mapboxNavigation: MapboxNavigation = mapboxNavigationMock(
+            tripSessionStateSlot,
+            routeProgressObserverSlot
+        )
+
+        mapboxNavigation.startTripSession()
+        sut.onAttached(mapboxNavigation)
+        routeProgressObserverSlot.forEach { it.onRouteProgressChanged(mockk()) }
+        sut.onDetached(mapboxNavigation)
+
+        verify(exactly = 1) { navigationManager.updateTrip(any()) }
+    }
+
+    @Test
+    fun `updateTrip will not happen when MapboxNavigation emits progress while stopped`() = coroutineRule.runBlockingTest {
+        val tripSessionStateSlot = mutableListOf<TripSessionStateObserver>()
+        val routeProgressObserverSlot = mutableListOf<RouteProgressObserver>()
+        val mapboxNavigation: MapboxNavigation = mapboxNavigationMock(
+            tripSessionStateSlot,
+            routeProgressObserverSlot
+        )
+
+        mapboxNavigation.startTripSession()
+        sut.onAttached(mapboxNavigation)
+        routeProgressObserverSlot.forEach { it.onRouteProgressChanged(mockk()) }
+        mapboxNavigation.stopTripSession()
+        routeProgressObserverSlot.forEach { it.onRouteProgressChanged(mockk()) }
+        sut.onDetached(mapboxNavigation)
+
+        verify(exactly = 1) { navigationManager.updateTrip(any()) }
+    }
+
+    private fun mapboxNavigationMock(
+        tripSessionStateSlot: MutableList<TripSessionStateObserver>,
+        routeProgressObserverSlot: MutableList<RouteProgressObserver>
+    ): MapboxNavigation {
+        val mapboxNavigation: MapboxNavigation = mockk(relaxed = true) {
+            every { registerTripSessionStateObserver(any()) } answers {
+                tripSessionStateSlot.add(firstArg())
+                firstArg<TripSessionStateObserver>().onSessionStateChanged(getTripSessionState())
+            }
+            every { unregisterTripSessionStateObserver(any()) } answers {
+                tripSessionStateSlot.remove(firstArg())
+            }
+            every { registerRouteProgressObserver(any()) } answers {
+                routeProgressObserverSlot.add(firstArg())
+            }
+            every { unregisterRouteProgressObserver(any()) } answers {
+                routeProgressObserverSlot.remove(firstArg())
+            }
+            every { unregisterTripSessionStateObserver(any()) } answers {
+                // Correct ordering when unregistering the observer will make it so
+                // routeProgressObserverSlot is empty.
+                routeProgressObserverSlot.forEach { it.onRouteProgressChanged(mockk()) }
+                tripSessionStateSlot.remove(firstArg())
+            }
+            every { startTripSession() } answers {
+                every { getTripSessionState() } returns TripSessionState.STARTED
+                tripSessionStateSlot.forEach { it.onSessionStateChanged(TripSessionState.STARTED) }
+            }
+            every { stopTripSession() } answers {
+                every { getTripSessionState() } returns TripSessionState.STOPPED
+                tripSessionStateSlot.forEach { it.onSessionStateChanged(TripSessionState.STOPPED) }
+            }
+        }
+        tripSessionStateSlot.add { tripSessionState ->
+            every { mapboxNavigation.getTripSessionState() } returns tripSessionState
+        }
+        return mapboxNavigation
+    }
 }