Fix remaining voice instructions out of bounds crash#2138
Conversation
|
|
||
| if let instruction = currentStepProgress.currentVisualInstruction { | ||
| // Announce visual instruction if it was updated or it is the first location being reported | ||
| if didUpdate || isFirstLocation { |
There was a problem hiding this comment.
If didUpdate is false, then is it safe to announce anything even if isFirstLocation is true?
There was a problem hiding this comment.
yep. This is a known anomaly in how data is received from directions. I wonder if this has been resolved.
There was a problem hiding this comment.
if isFirstLocation is true, that would mean stepIndex, visualInstructionIndex, and spokenInstructionIndex is 0. I can't think of a case where that wouldn't be safe, besides routeless navigation which currently is not supported. Also, there are no changes wrt to didUpdate and isFirst compared to before this PR which is reassuring.
There was a problem hiding this comment.
Ah, between the whitespace changes and variable renaming, I had mistakenly thought the logic had changed too. 👍
Incidentally, how well do we handle the pathological case of a route that ends at exactly or roughly the same spot as it begins, or a route with two coincident destinations? If I remember correctly, one of the features of OSRM is that it can give you a no-op route, unlike some other routing engines that make you go in a circle. Perhaps that edge case raises other issues that are out of scope for this PR, but that’s the only case that seems relevant to me.
There was a problem hiding this comment.
case of a route that ends at exactly or roughly the same spot as it begins, or a route with two coincident destinations?
Valid concerns but slightly out of scope for this PR.
I found one more place where indexes could be out of sync.
mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift
Lines 253 to 257 in b309653
Fixed in bfe5ba7
| ?? DefaultBehavior.shouldRerouteFromLocation | ||
|
|
||
| updateRouteStepProgress(status: status) | ||
| updateIndexes(status: status) |
There was a problem hiding this comment.
nit: plural of index is indices
There was a problem hiding this comment.
both are valid pluralizations, indexes is even being used in Foundation, e.g. IndexPath.init(indexes:).
There was a problem hiding this comment.
That's fine. Realized it's accepted in American and Canadian English 😸
| } | ||
|
|
||
| func updateSpokenInstructionProgress(status: MBNavigationStatus, willReRoute: Bool) { | ||
| func updateIndexes(status: MBNavigationStatus) { |
There was a problem hiding this comment.
function signature suggestion:
func updateIndices(status: MBNavigationStatus) {
...
}There was a problem hiding this comment.
nvm. it's an accepted spelling.
4 participants
This PR fixes an out of bounds array access crash in
RouteStepProgress.getter:remainingVisualInstructionswhereRouteController.updateRouteLegProgress(status:)would access it at a point in time where the spoken instruction index hadn't been updated.To keep the indexes in sync, all updating is now consolidated in
RouteController.updateIndexes(status:)besides the leg index where the receiver ofRouterDelegate.router(_:didArriveAt:)can determine if the index should be updated or not.@mapbox/navigation-ios