Parallel copies of route in Router can get out of sync

[1ec5]

Router implementations store both an IndexedRouteResponse (that is, a RouteResponse plus a route index) and a RouteProgress (that is, a Route plus a leg index etc.) side by side. As a result, they effectively hold two parallel copies of the current Route that can get out of sync. For example, after a reroute, RouteController sets the routeProgress property but neglects to update the indexedRouteResponse property:

https://github.com/mapbox/mapbox-navigation-ios/blob/3c81bdb6df768707073f9d96188ffd92d6a79985/Sources/MapboxCoreNavigation/RouteController.swift#L587-L589

This contrasts to LegacyRouteController, which does happen to set both properties:

https://github.com/mapbox/mapbox-navigation-ios/blob/5e9954da1a5b4523dfe032a491d1beb6020abd80/Sources/MapboxCoreNavigation/LegacyRouteController.swift#L62-L64

In both RouteController and LegacyRouteController, routeProgress is publicly settable but indexedRouteResponse is not. This makes it entirely possible for client code to have inconsistent behavior depending on how it gets the route from the Router.

/cc @mapbox/navigation-ios

[1ec5]

For example, after a reroute, RouteController sets the routeProgress property but neglects to update the indexedRouteResponse property

@OttyLab found that additional internal inconsistency can result if RouteController subsequently refreshes the route using the route identifier from a previous route. Refreshing the route would change the per-segment speed limit attributes based on the previous route, but the route progress would remain consistent with the newer route. This can sometimes result in SpeedLimitView showing speed limits that have nothing to do with the current road or any nearby roads. Route congestion segments would be affected in the same way.

The inconsistency could also be the root cause of a cluster of crashes related to out-of-bounds route progress indices: #3290 #3234 #3298.

/cc @dmiluski

[1ec5]

@OttyLab found that additional internal inconsistency can result if RouteController subsequently refreshes the route using the route identifier from a previous route. Refreshing the route would change the per-segment speed limit attributes based on the previous route, but the route progress would remain consistent with the newer route. This can sometimes result in SpeedLimitView showing speed limits that have nothing to do with the current road or any nearby roads.

The screen recordings in #3344 demonstrate this issue.

[1ec5]

3344 is a targeted fix for this issue. #3345 will fix it more thoroughly by making the RouteController.routeProgress property read-only, among other things.

[1ec5]

We’ve received a report that this issue can still reproduce nondeterministically in v2.0.0-rc2.

The fixes above attempted to ensure that the two parallel copies of the route are in sync, but they don’t address the fact that there could still be two parallel copies of the route. Some Route properties, such as the attributes, are mutable for convenience when refreshing a route.

When Router/InternalRouter receives a refreshed route, it tells the RouteProgress to update its route:

https://github.com/mapbox/mapbox-navigation-ios/blob/6b7b142c9fe142eb6889db7c4d6f46a7abd3b1b9/Sources/MapboxCoreNavigation/Router.swift#L214

but it never explicitly updates the route inside Router.indexedRouteResponse in the same manner. For the most part, this is OK, because Route is a class that has reference semantics. But if for some reason Router.indexedRouteResponse and Router.routeProgress end up pointing to different Route objects, then one would get refreshed with attributes that the other doesn’t get. That would cause inconsistency between code that uses Router.indexedRouteResponse and code that uses Router.routeProgress, intending to get the same object, such as Router.route:

https://github.com/mapbox/mapbox-navigation-ios/blob/6b7b142c9fe142eb6889db7c4d6f46a7abd3b1b9/Sources/MapboxCoreNavigation/RouteController.swift#L43-L45

I can’t say for sure that this is specifically what’s contributing to the inconsistency that we’re getting reports about, but it illustrates the architectural problem.

[1ec5]

The reports we’re getting may be due to bad input, a GIGO situation from Core Navigation’s perspective: #3403.

[S2Ler]

@1ec5 Shall we make RouteProgress.legIndex property internal(set)? We already have RouteController.advanceLegIndex(completionHandler:) method that is a correct way of advancing a leg index.

[1ec5]

No, the progress classes are supposed to be value classes. The long-range vision has actually been to turn them into turn them into structs. We’d like to stop implicitly assuming a one-to-one correspondence between trips and RouteProgress objects, as though it’s a singleton.