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

* Release references to native objects when destoy() called * Rename changelog files
mapbox · Aug 28, 2024 · 2dd4529 · 2dd4529
1 parent 459a17e
commit 2dd4529
Show file tree

Hide file tree

Showing 21 changed files with 391 additions and 381 deletions.
diff --git a/Makefile b/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) \

diff --git a/changelog/unreleased/bugfixes/7856.md b/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.
diff --git a/changelog/unreleased/features/7856.md b/changelog/unreleased/features/7856.md
@@ -0,0 +1 @@
+- The `PredictiveCacheController(PredictiveCacheOptions)` constructor is now deprecated. Use `PredictiveCacheController(MapboxNavigation, PredictiveCacheOptions)` instead.
diff --git a/...core/src/androidTest/java/com/mapbox/navigation/core/trip/service/ArtificialDriverTest.kt b/...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()
     }

diff --git a/...roidTest/java/com/mapbox/navigation/core/trip/service/NativeNavigatorCallbackOrderTest.kt b/...roidTest/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

diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt b/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
     }

diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/NavigationComponentProvider.kt b/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,

diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/PredictiveCache.kt b/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

diff --git a/...avigation-core/src/main/java/com/mapbox/navigation/core/trip/session/MapboxTripSession.kt b/...avigation-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 {

diff --git a/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/eh/GraphAccessor.kt b/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)
         }
     }

diff --git a/...gation-core/src/main/java/com/mapbox/navigation/core/trip/session/eh/RoadObjectMatcher.kt b/...gation-core/src/main/java/com/mapbox/navigation/core/trip/session/eh/RoadObjectMatcher.kt
@@ -36,7 +36,7 @@ class RoadObjectMatcher internal constructor(
     init {
         navigator.setNativeNavigatorRecreationObserver {
             if (roadObjectMatcherObservers.isNotEmpty()) {
-                navigator.roadObjectMatcher?.setListener(roadObjectMatcherListener)
+                navigator.roadObjectMatcher.setListener(roadObjectMatcherListener)
             }
         }
     }
@@ -47,7 +47,7 @@ class RoadObjectMatcher internal constructor(
      */
     fun registerRoadObjectMatcherObserver(roadObjectMatcherObserver: RoadObjectMatcherObserver) {
         if (roadObjectMatcherObservers.isEmpty()) {
-            navigator.roadObjectMatcher?.setListener(roadObjectMatcherListener)
+            navigator.roadObjectMatcher.setListener(roadObjectMatcherListener)
         }
         roadObjectMatcherObservers.add(roadObjectMatcherObserver)
     }
@@ -110,7 +110,7 @@ class RoadObjectMatcher internal constructor(
         openLRLocation: String,
         @OpenLRStandard.Type openLRStandard: String
     ) {
-        navigator.roadObjectMatcher?.matchOpenLRs(
+        navigator.roadObjectMatcher.matchOpenLRs(
             listOf(
                 EHorizonFactory.buildNativeMatchableOpenLr(
                     MatchableOpenLr(roadObjectId, openLRLocation, openLRStandard)
@@ -137,7 +137,7 @@ class RoadObjectMatcher internal constructor(
         matchableOpenLrs: List<MatchableOpenLr>,
         useOnlyPreloadedTiles: Boolean = false
     ) {
-        navigator.roadObjectMatcher?.matchOpenLRs(
+        navigator.roadObjectMatcher.matchOpenLRs(
             matchableOpenLrs.map {
                 EHorizonFactory.buildNativeMatchableOpenLr(it)
             },
@@ -167,7 +167,7 @@ class RoadObjectMatcher internal constructor(
         )
     )
     fun matchPolylineObject(roadObjectId: String, polyline: List<Point>) {
-        navigator.roadObjectMatcher?.matchPolylines(
+        navigator.roadObjectMatcher.matchPolylines(
             listOf(
                 EHorizonFactory.buildNativeMatchableGeometry(
                     MatchableGeometry(roadObjectId, polyline)
@@ -199,7 +199,7 @@ class RoadObjectMatcher internal constructor(
         matchableGeometries: List<MatchableGeometry>,
         useOnlyPreloadedTiles: Boolean = false
     ) {
-        navigator.roadObjectMatcher?.matchPolylines(
+        navigator.roadObjectMatcher.matchPolylines(
             matchableGeometries.map {
                 EHorizonFactory.buildNativeMatchableGeometry(it)
             },
@@ -229,7 +229,7 @@ class RoadObjectMatcher internal constructor(
         )
     )
     fun matchPolygonObject(roadObjectId: String, polygon: List<Point>) {
-        navigator.roadObjectMatcher?.matchPolygons(
+        navigator.roadObjectMatcher.matchPolygons(
             listOf(
                 EHorizonFactory.buildNativeMatchableGeometry(
                     MatchableGeometry(roadObjectId, polygon)
@@ -261,7 +261,7 @@ class RoadObjectMatcher internal constructor(
         matchableGeometries: List<MatchableGeometry>,
         useOnlyPreloadedTiles: Boolean = false
     ) {
-        navigator.roadObjectMatcher?.matchPolygons(
+        navigator.roadObjectMatcher.matchPolygons(
             matchableGeometries.map {
                 EHorizonFactory.buildNativeMatchableGeometry(it)
             },
@@ -291,7 +291,7 @@ class RoadObjectMatcher internal constructor(
         )
     )
     fun matchGantryObject(roadObjectId: String, gantry: List<Point>) {
-        navigator.roadObjectMatcher?.matchGantries(
+        navigator.roadObjectMatcher.matchGantries(
             listOf(
                 EHorizonFactory.buildNativeMatchableGeometry(
                     MatchableGeometry(roadObjectId, gantry)
@@ -323,7 +323,7 @@ class RoadObjectMatcher internal constructor(
         matchableGeometries: List<MatchableGeometry>,
         useOnlyPreloadedTiles: Boolean = false
     ) {
-        navigator.roadObjectMatcher?.matchGantries(
+        navigator.roadObjectMatcher.matchGantries(
             matchableGeometries.map {
                 EHorizonFactory.buildNativeMatchableGeometry(it)
             },
@@ -351,7 +351,7 @@ class RoadObjectMatcher internal constructor(
         )
     )
     fun matchPointObject(roadObjectId: String, point: Point) {
-        navigator.roadObjectMatcher?.matchPoints(
+        navigator.roadObjectMatcher.matchPoints(
             listOf(
                 EHorizonFactory.buildNativeMatchablePoint(
                     MatchablePoint(roadObjectId, point)
@@ -381,7 +381,7 @@ class RoadObjectMatcher internal constructor(
         matchablePoints: List<MatchablePoint>,
         useOnlyPreloadedTiles: Boolean = false
     ) {
-        navigator.roadObjectMatcher?.matchPoints(
+        navigator.roadObjectMatcher.matchPoints(
             matchablePoints.map {
                 EHorizonFactory.buildNativeMatchablePoint(it)
             },
@@ -399,13 +399,13 @@ class RoadObjectMatcher internal constructor(
      * @param roadObjectIds list of object ids to cancel matching
      */
     fun cancel(roadObjectIds: List<String>) {
-        navigator.roadObjectMatcher?.cancel(roadObjectIds)
+        navigator.roadObjectMatcher.cancel(roadObjectIds)
     }
 
     /**
      * Cancel all road objects matching
      */
     fun cancelAll() {
-        navigator.roadObjectMatcher?.cancelAll()
+        navigator.roadObjectMatcher.cancelAll()
     }
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		- Fixed an issue where native memory was not being properly released after the `MapboxNavigation` object was destroyed.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		- The `PredictiveCacheController(PredictiveCacheOptions)` constructor is now deprecated. Use `PredictiveCacheController(MapboxNavigation, PredictiveCacheOptions)` instead.