ArrayIndexOutOfBoundsException in MapboxRouteLineUtils

[LukasPaczos]

Caught on v2.0.0-beta.5.

Looks like

mapbox-navigation-android/libnavui-maps/src/main/java/com/mapbox/navigation/ui/maps/internal/route/line/MapboxRouteLineUtils.kt

Line 83 in a1c8f01

val firstItemIndex = routeLineExpressionData.indexOf(filteredItems.first())

didn't find an element and returned -1 which resulted in:

Fatal Exception: java.lang.ArrayIndexOutOfBoundsException: length=22; index=-2
       at java.util.ArrayList.get(ArrayList.java:439)
       at com.mapbox.navigation.ui.maps.internal.route.line.MapboxRouteLineUtils.getTrafficLineExpression$libnavui_maps_release(MapboxRouteLineUtils.java:87)
       at com.mapbox.navigation.ui.maps.route.line.api.VanishingRouteLine.getTraveledRouteLineExpressions$libnavui_maps_release(VanishingRouteLine.java:163)
       at com.mapbox.navigation.ui.maps.route.line.api.MapboxRouteLineApi.updateTraveledRouteLine(MapboxRouteLineApi.java:264)
       at com.mapbox.onetap.map.presentation.ui.RouteLineBinder$3.onIndicatorPositionChanged(RouteLineBinder.java:68)
       at com.mapbox.maps.plugin.locationcomponent.LocationComponentPluginImpl$indicatorPositionChangedListener$1.onIndicatorPositionChanged(LocationComponentPluginImpl.java:68)
       at com.mapbox.maps.plugin.locationcomponent.animators.PuckPositionAnimator.updateLayer(PuckPositionAnimator.java:10)
       at com.mapbox.maps.plugin.locationcomponent.animators.PuckPositionAnimator.updateLayer(PuckPositionAnimator.java:6)
       at com.mapbox.maps.plugin.locationcomponent.animators.PuckAnimator$1.onAnimationUpdate(PuckAnimator.java:31)
       at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1558)
       at android.animation.ValueAnimator.animateBasedOnTime(ValueAnimator.java:1349)
       at android.animation.ValueAnimator.doAnimationFrame(ValueAnimator.java:1481)
       at android.animation.AnimationHandler.doAnimationFrame(AnimationHandler.java:146)
       at android.animation.AnimationHandler.access$100(AnimationHandler.java:37)
       at android.animation.AnimationHandler$1.doFrame(AnimationHandler.java:54)
       at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1055)
       at android.view.Choreographer.doCallbacks(Choreographer.java:875)
       at android.view.Choreographer.doFrame(Choreographer.java:772)
       at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1042)
       at android.os.Handler.handleCallback(Handler.java:888)
       at android.os.Handler.dispatchMessage(Handler.java:100)
       at android.os.Looper.loop(Looper.java:213)
       at android.app.ActivityThread.main(ActivityThread.java:8178)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:513)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1101)

Can this be a regression from #4106?

routeLineExpressionData is filled up on a worker thread, while MapboxRouteLineApi#updateTraveledRouteLine -> VanishingRouteLine#getTraveledRouteLineExpressions -> MapboxRouteLineUtils#getTrafficLineExpression is all done on the synchronously.

I think even a call to MapboxRouteLineApi#clearRouteLine could trigger this race condition and clear the routeLineExpressionData while it's being processed by the vanishing route line code.

Should MapboxRouteLineApi#updateTraveledRouteLine also be guarded by the mutex since it accesses the shared state? Are there any other areas in the codebase like this?

cc @cafesilencio @Guardiola31337 @korshaknn

[cafesilencio]

@LukasPaczos

I've been experimenting with the actor strategy for the shared mutable state in the vanishing route line class rather than the mutex used in the route line api.

https://kotlinlang.org/docs/shared-mutable-state-and-concurrency.html#actors

I like the implementation however the actor class is marked with @ObsoleteCoroutinesApi.  Looking at the source of the class I could write my own version since it's not very sophisticated but I'd prefer to use the kotlin version. 

Based on this github question and answer it doesn't appear there's much risk of the class changing. 
Kotlin/kotlinx.coroutines#87 (comment)

Can I go ahead and use the kotlin actor class even though it's marked @ObsoleteCoroutinesApi?

[Guardiola31337]

I'd rather avoiding this. From @ObsoleteCoroutinesApi docs:

Marks declarations that are obsolete in coroutines API, which means that the design of the corresponding declarations has serious known flaws and they will be redesigned in the future. Roughly speaking, these declarations will be deprecated in the future but there is no replacement for them yet, so they cannot be deprecated right away.

Also it seems that there's recent movement in Kotlin/kotlinx.coroutines#87 and Kotlin/kotlinx.coroutines#776

We had a similar discussion with @ExperimentalCoroutinesApi and passed on using it, if required to mark public code with the annotation (as that'd force clients to do the same on their side). It shouldn't be a problem though, if we could workaround it and only use it internally without impacting clients.

[cafesilencio]

It would be used for the vanishing route line class which external developers don't interact with.

[Guardiola31337]

It would be used for the vanishing route line class which external developers don't interact with.

In any case, let's make sure they're not impacted at all. It may be situations in which clients don't interact directly with the APIs but they're forced to make changes on their side. If that's not the case, disregard this comment.

[LukasPaczos]

In my opinion, using a stateful actor approach (with cancellable mailbox) or any other approach would more target to resolve #4232.

To resolve this crash, we seem to be missing a synchronization step for some of the shared resources. I think we should focus on narrowing this down and resolving the crash itself, and then we can turn to something more elaborate.

[zugaldia]

Adding the release blocker label so that we prioritize a fix, it's a crash being reported by customers adopting v2.

[LukasPaczos]

Closing as we think we were able to prevent the crash in #4298. We didn't have a reproducible case to confirm the fix, so let's keep an eye out for this and reopen if necessary.

[LukasPaczos]

This happened again on v2.0.0-beta.15:

Fatal Exception: java.lang.ArrayIndexOutOfBoundsException: length=186; index=-2
       at java.util.ArrayList.get(ArrayList.java:439)
       at com.mapbox.navigation.ui.maps.internal.route.line.MapboxRouteLineUtils.getTrafficLineExpression$libnavui_maps_release(MapboxRouteLineUtils.java:89)
       at com.mapbox.navigation.ui.maps.route.line.api.VanishingRouteLine.getTraveledRouteLineExpressions$libnavui_maps_release(VanishingRouteLine.java:173)
       at com.mapbox.navigation.ui.maps.route.line.api.MapboxRouteLineApi.updateTraveledRouteLine(MapboxRouteLineApi.java:256)
       at com.mapbox.onetap.map.presentation.ui.RouteLineBinder$3.onIndicatorPositionChanged(RouteLineBinder.java:61)
       at com.mapbox.maps.plugin.locationcomponent.LocationComponentPluginImpl$indicatorPositionChangedListener$1.onIndicatorPositionChanged(LocationComponentPluginImpl.java:68)
       at com.mapbox.maps.plugin.locationcomponent.animators.PuckPositionAnimator.updateLayer(PuckPositionAnimator.java:10)
       at com.mapbox.maps.plugin.locationcomponent.animators.PuckPositionAnimator.updateLayer(PuckPositionAnimator.java:6)
       at com.mapbox.maps.plugin.locationcomponent.animators.PuckAnimator$1.onAnimationUpdate(PuckAnimator.java:31)
       at android.animation.ValueAnimator.animateValue(ValueAnimator.java:1547)
       at android.animation.ValueAnimator.animateBasedOnTime(ValueAnimator.java:1339)
       at android.animation.ValueAnimator.doAnimationFrame(ValueAnimator.java:1471)
       at android.animation.AnimationHandler.doAnimationFrame(AnimationHandler.java:146)
       at android.animation.AnimationHandler.access$100(AnimationHandler.java:37)
       at android.animation.AnimationHandler$1.doFrame(AnimationHandler.java:54)
       at android.view.Choreographer$CallbackRecord.run(Choreographer.java:984)
       at android.view.Choreographer.doCallbacks(Choreographer.java:764)
       at android.view.Choreographer.doFrame(Choreographer.java:696)
       at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:965)
       at android.os.Handler.handleCallback(Handler.java:873)
       at android.os.Handler.dispatchMessage(Handler.java:99)
       at android.os.Looper.loop(Looper.java:214)
       at android.app.ActivityThread.main(ActivityThread.java:7073)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:494)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:964)

[cafesilencio]

I made some modifications for the work in PR #4489 that I think would eliminate the possibility of this issue occurring.

[cafesilencio]

This issue seems to be the same as #4483 so closing this. If it's thought this is somehow different then reopen.