Release references to native objects when destoy() called (#7856) (#7858)

* Release references to native objects when destoy() called

* Rename changelog files

Author: DzmitryFomchyn

Makefile

@@ -52,6 +52,13 @@ check-kotlin-lint:
 	&& $(call run-gradle-tasks,$(ANDROIDAUTO_MODULES),ktlint) \
 	&& $(call run-gradle-tasks,$(APPLICATION_MODULES),ktlint)
 
+.PHONY: format-kotlin-lint
+format-kotlin-lint:
+	$(call run-gradle-tasks,$(CORE_MODULES),ktlintFormat) \
+	&& $(call run-gradle-tasks,$(UI_MODULES),ktlintFormat) \
+	&& $(call run-gradle-tasks,$(ANDROIDAUTO_MODULES),ktlintFormat) \
+	&& $(call run-gradle-tasks,$(APPLICATION_MODULES),ktlintFormat)
+
 .PHONY: check-android-lint
 check-android-lint:
 	$(call run-gradle-tasks,$(CORE_MODULES),lint) \

changelog/unreleased/bugfixes/7856.md

@@ -0,0 +1 @@
+- Fixed an issue where native memory was not being properly released after the `MapboxNavigation` object was destroyed.

changelog/unreleased/features/7856.md

@@ -0,0 +1 @@
+- The `PredictiveCacheController(PredictiveCacheOptions)` constructor is now deprecated. Use `PredictiveCacheController(MapboxNavigation, PredictiveCacheOptions)` instead.

libnavigation-core/src/androidTest/java/com/mapbox/navigation/core/trip/service/ArtificialDriverTest.kt

@@ -13,7 +13,6 @@ import com.mapbox.navigation.core.replay.history.mapToLocation
 import com.mapbox.navigation.core.replay.route.ReplayRouteMapper
 import com.mapbox.navigation.core.test.R
 import com.mapbox.navigation.navigator.internal.MapboxNativeNavigator
-import com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl
 import com.mapbox.navigator.NavigationStatus
 import com.mapbox.navigator.NavigationStatusOrigin
 import com.mapbox.navigator.NavigatorObserver
@@ -43,19 +42,20 @@ class ArtificialDriverTest {
     @Ignore("test sometimes fails because of https://mapbox.atlassian.net/browse/NN-418")
     fun nativeNavigatorFollowsArtificialDriverWithoutReroutes() =
         runBlocking<Unit>(Dispatchers.Main) {
-            withNavigators { mapboxNavigation, nativeNavigator ->
+            withNavigators { mapboxNavigation ->
                 mapboxNavigation.historyRecorder.startRecording()
                 val testRoute = getTestRoute()
                 val events = createArtificialLocationUpdates(testRoute)
-                val setRoutesResult =
-                    nativeNavigator.setRoutes(testRoute, reason = SetRoutesReason.NEW_ROUTE)
+                val setRoutesResult = mapboxNavigation.navigator
+                    .setRoutes(testRoute, reason = SetRoutesReason.NEW_ROUTE)
                 assertTrue("result is $setRoutesResult", setRoutesResult.isValue)
                 val statusesTracking = async<List<NavigationStatus>> {
-                    nativeNavigator.collectStatuses(untilRouteState = RouteState.COMPLETE)
+                    mapboxNavigation.navigator
+                        .collectStatuses(untilRouteState = RouteState.COMPLETE)
                 }
 
                 for (location in events.map { it.location.mapToLocation() }) {
-                    assertTrue(nativeNavigator.updateLocation(location.toFixLocation()))
+                    assertTrue(mapboxNavigation.navigator.updateLocation(location.toFixLocation()))
                 }
 
                 val states = statusesTracking.await()
@@ -113,7 +113,7 @@ fun MapboxNativeNavigator.statusUpdates(): Flow<OnStatusUpdateParameters> {
 }
 
 private suspend fun withNavigators(
-    block: suspend (MapboxNavigation, MapboxNativeNavigator) -> Unit
+    block: suspend (MapboxNavigation) -> Unit
 ) {
     val context = InstrumentationRegistry.getInstrumentation().targetContext
     val mapboxNavigation = MapboxNavigationProvider.create(
@@ -122,7 +122,7 @@ private suspend fun withNavigators(
             .build()
     )
     try {
-        block(mapboxNavigation, MapboxNativeNavigatorImpl)
+        block(mapboxNavigation)
     } finally {
         mapboxNavigation.onDestroy()
     }

libnavigation-core/src/androidTest/java/com/mapbox/navigation/core/trip/service/NativeNavigatorCallbackOrderTest.kt

@@ -16,7 +16,6 @@ import com.mapbox.navigation.core.MapboxNavigation
 import com.mapbox.navigation.core.MapboxNavigationProvider
 import com.mapbox.navigation.core.test.R
 import com.mapbox.navigation.core.tests.activity.TripServiceActivity
-import com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl
 import com.mapbox.navigation.testing.ui.BaseTest
 import com.mapbox.navigation.testing.ui.utils.runOnMainSync
 import com.mapbox.navigator.FixLocation
@@ -72,11 +71,7 @@ internal class NativeNavigatorCallbackOrderTest :
             )
             // starts raw location updates - otherwise we don't get onStatus calls
             mapboxNavigation.startTripSession()
-            val nativeNavigatorField = mapboxNavigation.javaClass.getDeclaredField("navigator")
-            nativeNavigatorField.isAccessible = true
-            val nativeNavigatorImpl =
-                nativeNavigatorField.get(mapboxNavigation) as MapboxNativeNavigatorImpl
-            nativeNavigatorField.isAccessible = false
+            val nativeNavigatorImpl = mapboxNavigation.navigator
             val navigatorField = nativeNavigatorImpl.javaClass.getDeclaredField("navigator")
             navigatorField.isAccessible = true
             navigator = navigatorField.get(nativeNavigatorImpl) as Navigator

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt

@@ -266,7 +266,6 @@ class MapboxNavigation @VisibleForTesting internal constructor(
 
     private val mainJobController = threadController.getMainScopeAndRootJob()
     private val directionsSession: DirectionsSession
-    private var navigator: MapboxNativeNavigator
     private var historyRecorderHandles: NavigatorLoader.HistoryRecorderHandles
     private val tripService: TripService
     private val tripSession: TripSession
@@ -348,6 +347,11 @@ class MapboxNavigation @VisibleForTesting internal constructor(
     private var rerouteController: InternalRerouteController?
     private val defaultRerouteController: InternalRerouteController
 
+    private var _navigator: MapboxNativeNavigator?
+
+    internal val navigator: MapboxNativeNavigator
+        get() = _navigator ?: error("MapboxNavigation is destroyed")
+
     /**
      * [NavigationVersionSwitchObserver] is notified when navigation switches tiles version.
      */
@@ -492,7 +496,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
             is NavigationRouter -> LegacyNavigationRouterAdapter(result)
             else -> LegacyNavigationRouterAdapter(LegacyRouterAdapter(result))
         }
-        navigator = NavigationComponentProvider.createNativeNavigator(
+        _navigator = NavigationComponentProvider.createNativeNavigator(
             cacheHandle,
             config,
             historyRecorderHandles.composite,
@@ -1299,6 +1303,8 @@ class MapboxNavigation @VisibleForTesting internal constructor(
         }
         resetAdasisMessageObserver()
 
+        _navigator = null
+
         isDestroyed = true
         hasInstance = false
     }

libnavigation-core/src/main/java/com/mapbox/navigation/core/NavigationComponentProvider.kt

@@ -43,7 +43,7 @@ internal object NavigationComponentProvider {
         historyRecorderComposite: HistoryRecorderHandle?,
         accessToken: String,
         router: RouterInterface?,
-    ): MapboxNativeNavigator = MapboxNativeNavigatorImpl.create(
+    ): MapboxNativeNavigator = MapboxNativeNavigatorImpl(
         cacheHandle,
         config,
         historyRecorderComposite,

libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/PredictiveCache.kt

@@ -3,10 +3,10 @@ package com.mapbox.navigation.core.internal
 import com.mapbox.common.TileStore
 import com.mapbox.common.TilesetDescriptor
 import com.mapbox.navigation.base.options.PredictiveCacheLocationOptions
-import com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl
+import com.mapbox.navigation.core.MapboxNavigation
 import com.mapbox.navigator.PredictiveCacheController
 
-object PredictiveCache {
+class PredictiveCache(private val mapboxNavigation: MapboxNavigation) {
 
     internal val cachedNavigationPredictiveCacheControllers =
         mutableSetOf<PredictiveCacheController>()
@@ -22,9 +22,9 @@ object PredictiveCache {
     internal val mapsPredictiveCacheLocationOptionsTileVariant =
         mutableMapOf<Any, MutableMap<String, Pair<TileStore, PredictiveCacheLocationOptions>>>()
 
-    fun init() {
+    init {
         // recreate controllers with the same options but with a new navigator instance
-        MapboxNativeNavigatorImpl.setNativeNavigatorRecreationObserver {
+        mapboxNavigation.navigator.setNativeNavigatorRecreationObserver {
             val navOptions = navPredictiveCacheLocationOptions.toSet()
             val mapsOptions = mapsPredictiveCacheLocationOptions.toMap()
             val mapsOptionsTileVariant = mapsPredictiveCacheLocationOptionsTileVariant.toMap()
@@ -61,7 +61,7 @@ object PredictiveCache {
     ) {
         navPredictiveCacheLocationOptions.add(predictiveCacheLocationOptions)
         val predictiveCacheController =
-            MapboxNativeNavigatorImpl.createNavigationPredictiveCacheController(
+            mapboxNavigation.navigator.createNavigationPredictiveCacheController(
                 predictiveCacheLocationOptions
             )
         cachedNavigationPredictiveCacheControllers.add(predictiveCacheController)
@@ -77,7 +77,7 @@ object PredictiveCache {
         predictiveCacheLocationOptions: PredictiveCacheLocationOptions
     ) {
         val predictiveCacheController =
-            MapboxNativeNavigatorImpl.createMapsPredictiveCacheControllerTileVariant(
+            mapboxNavigation.navigator.createMapsPredictiveCacheControllerTileVariant(
                 tileStore,
                 tileVariant,
                 predictiveCacheLocationOptions
@@ -100,7 +100,7 @@ object PredictiveCache {
         descriptorsAndOptions: List<Pair<TilesetDescriptor, PredictiveCacheLocationOptions>>
     ) {
         val descriptorsToPredictiveCacheControllers = descriptorsAndOptions.map {
-            it.first to MapboxNativeNavigatorImpl.createMapsPredictiveCacheController(
+            it.first to mapboxNavigation.navigator.createMapsPredictiveCacheController(
                 tileStore,
                 it.first,
                 it.second

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt

@@ -29,7 +29,6 @@ import com.mapbox.navigation.core.trip.service.TripService
 import com.mapbox.navigation.core.trip.session.eh.EHorizonObserver
 import com.mapbox.navigation.core.trip.session.eh.EHorizonSubscriptionManager
 import com.mapbox.navigation.navigator.internal.MapboxNativeNavigator
-import com.mapbox.navigation.navigator.internal.MapboxNativeNavigatorImpl
 import com.mapbox.navigation.navigator.internal.utils.calculateRemainingWaypoints
 import com.mapbox.navigation.navigator.internal.utils.getCurrentLegDestination
 import com.mapbox.navigation.utils.internal.JobControl
@@ -64,7 +63,7 @@ import java.util.concurrent.CopyOnWriteArraySet
 internal class MapboxTripSession(
     override val tripService: TripService,
     private val tripSessionLocationEngine: TripSessionLocationEngine,
-    private val navigator: MapboxNativeNavigator = MapboxNativeNavigatorImpl,
+    private val navigator: MapboxNativeNavigator,
     private val threadController: ThreadController,
     private val eHorizonSubscriptionManager: EHorizonSubscriptionManager
 ) : TripSession {

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/eh/GraphAccessor.kt

@@ -34,7 +34,7 @@ class GraphAccessor internal constructor(
      * @return list of Points representing edge shape
      */
     fun getEdgeShape(edgeId: Long): List<Point>? {
-        return navigator.graphAccessor?.getEdgeShape(edgeId)
+        return navigator.graphAccessor.getEdgeShape(edgeId)
     }
 
     /**
@@ -45,7 +45,7 @@ class GraphAccessor internal constructor(
      * @return EHorizonEdgeMetadata
      */
     fun getEdgeMetadata(edgeId: Long): EHorizonEdgeMetadata? {
-        return navigator.graphAccessor?.getEdgeMetadata(edgeId)?.let {
+        return navigator.graphAccessor.getEdgeMetadata(edgeId)?.let {
             EHorizonFactory.buildEHorizonEdgeMetadata(it)
         }
     }
@@ -55,7 +55,7 @@ class GraphAccessor internal constructor(
      * If any of path edges is not accessible, returns null.
      */
     fun getPathShape(graphPath: EHorizonGraphPath): List<Point>? {
-        return navigator.graphAccessor?.getPathShape(
+        return navigator.graphAccessor.getPathShape(
             EHorizonFactory.buildNativeGraphPath(graphPath)
         )
     }
@@ -65,7 +65,7 @@ class GraphAccessor internal constructor(
      * If position's edge is not accessible, returns null.
      */
     fun getGraphPositionCoordinate(graphPosition: EHorizonGraphPosition): Point? {
-        return navigator.graphAccessor?.getPositionCoordinate(
+        return navigator.graphAccessor.getPositionCoordinate(
             EHorizonFactory.buildNativeGraphPosition(graphPosition)
         )
     }
@@ -75,7 +75,7 @@ class GraphAccessor internal constructor(
      */
     @ExperimentalPreviewMapboxNavigationAPI
     fun getAdasisEdgeAttributes(edgeId: Long): AdasEdgeAttributes? {
-        return navigator.graphAccessor?.getAdasAttributes(edgeId)?.let {
+        return navigator.graphAccessor.getAdasAttributes(edgeId)?.let {
             AdasEdgeAttributes.createFromNativeObject(it)
         }
     }