Skip to content

Commit

Permalink
Release references to native objects when destoy() called
Browse files Browse the repository at this point in the history
  • Loading branch information
DzmitryFomchyn committed Aug 23, 2024
1 parent 95377ec commit e2a3635
Show file tree
Hide file tree
Showing 21 changed files with 391 additions and 381 deletions.
7 changes: 7 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
1 change: 1 addition & 0 deletions changelog/unreleased/bugfixes/0000.md
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.
1 change: 1 addition & 0 deletions changelog/unreleased/features/0000.md
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.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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(
Expand All @@ -122,7 +122,7 @@ private suspend fun withNavigators(
.build()
)
try {
block(mapboxNavigation, MapboxNativeNavigatorImpl)
block(mapboxNavigation)
} finally {
mapboxNavigation.onDestroy()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1299,6 +1303,8 @@ class MapboxNavigation @VisibleForTesting internal constructor(
}
resetAdasisMessageObserver()

_navigator = null

isDestroyed = true
hasInstance = false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ internal object NavigationComponentProvider {
historyRecorderComposite: HistoryRecorderHandle?,
accessToken: String,
router: RouterInterface?,
): MapboxNativeNavigator = MapboxNativeNavigatorImpl.create(
): MapboxNativeNavigator = MapboxNativeNavigatorImpl(
cacheHandle,
config,
historyRecorderComposite,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>()
Expand All @@ -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()
Expand Down Expand Up @@ -61,7 +61,7 @@ object PredictiveCache {
) {
navPredictiveCacheLocationOptions.add(predictiveCacheLocationOptions)
val predictiveCacheController =
MapboxNativeNavigatorImpl.createNavigationPredictiveCacheController(
mapboxNavigation.navigator.createNavigationPredictiveCacheController(
predictiveCacheLocationOptions
)
cachedNavigationPredictiveCacheControllers.add(predictiveCacheController)
Expand All @@ -77,7 +77,7 @@ object PredictiveCache {
predictiveCacheLocationOptions: PredictiveCacheLocationOptions
) {
val predictiveCacheController =
MapboxNativeNavigatorImpl.createMapsPredictiveCacheControllerTileVariant(
mapboxNavigation.navigator.createMapsPredictiveCacheControllerTileVariant(
tileStore,
tileVariant,
predictiveCacheLocationOptions
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

/**
Expand All @@ -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)
}
}
Expand All @@ -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)
)
}
Expand All @@ -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)
)
}
Expand All @@ -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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class RoadObjectMatcher internal constructor(
init {
navigator.setNativeNavigatorRecreationObserver {
if (roadObjectMatcherObservers.isNotEmpty()) {
navigator.roadObjectMatcher?.setListener(roadObjectMatcherListener)
navigator.roadObjectMatcher.setListener(roadObjectMatcherListener)
}
}
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
Expand All @@ -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)
},
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
},
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
},
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
},
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
},
Expand All @@ -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()
}
}
Loading

0 comments on commit e2a3635

Please sign in to comment.