LocationsCollectorImpl crashes with `ConcurrentModificationException`

[tomaszrybakiewicz]

Description

One of our customers detect a crash caused by the LocationsCollectorImpl.accumulatePostEventLocation method throwing ConcurrentModificationException.

After investigating we discovered that the crash is caused by the OpenJDK ArrayList.Itr iterator. The iterator only handles the removal of a single element during iteration (via Itr.remove() method). When the source array is modified via ArrayList.add(), ArrayList.remove() or ArrayList.clear() operation during iteration step, the call to Itr.remove() will detect the unexpected modification (if (modCount != expectedModCount) check) and will raise ConcurrentModificationException (see superclass modCount field description for more details).

Potential solutions

• Update LocationsCollectorImpl.eventsLocationsBuffer field to use CopyOnWriteArrayList instead.
• Update LocationsCollectorImpl.eventsLocationsBuffer field to use ConcurrentLinkedQueue instead.

Both classes are Thread-safe and permit concurrent modification.

Crash details

Investigation and support ticket: https://github.com/mapbox/ask-navigation/issues/96

Configuration:

Android API: 31 (Android 12) Mapbox Navigation SDK version: 2.2

Captured crash log

Fatal Exception: java.util.ConcurrentModificationException
       at java.util.ArrayList$Itr.remove(ArrayList.java:875)
       at com.mapbox.navigation.core.telemetry.LocationsCollectorImpl.accumulatePostEventLocation(LocationsCollectorImpl.kt:30)
       at com.mapbox.navigation.core.telemetry.LocationsCollectorImpl.onNewRawLocation(LocationsCollectorImpl.kt:77)
       at com.mapbox.navigation.core.trip.session.MapboxTripSession.updateRawLocation(MapboxTripSession.kt:247)
       at com.mapbox.navigation.core.trip.session.MapboxTripSession.access$setOffRoute(MapboxTripSession.kt:59)
       at com.mapbox.navigation.core.trip.session.MapboxTripSession$start$1.invoke(MapboxTripSession.kt:240)
       at com.mapbox.navigation.core.trip.session.MapboxTripSession$start$1.invoke(MapboxTripSession.kt:239)
       at com.mapbox.navigation.core.trip.session.TripSessionLocationEngine$locationEngineCallback$1.onSuccess(TripSessionLocationEngine.kt:59)
       at com.mapbox.navigation.core.trip.session.TripSessionLocationEngine$locationEngineCallback$1.onSuccess(TripSessionLocationEngine.kt:56)
       at com.mapbox.android.core.location.GoogleLocationEngineImpl$GoogleLocationEngineCallbackTransport.onLocationResult(GoogleLocationEngineImpl.java:118)
       at com.google.android.gms.internal.location.zzap.notifyListener(com.google.android.gms:play-services-location@@18.0.0:2)
       at com.google.android.gms.common.api.internal.ListenerHolder.notifyListenerInternal(com.google.android.gms:play-services-base@@17.5.0:18)
       at com.google.android.gms.common.api.internal.ListenerHolder$zaa.handleMessage(com.google.android.gms:play-services-base@@17.5.0:6)
       at android.os.Handler.dispatchMessage(Handler.java:106)
       at android.os.Looper.loop(Looper.java:214)
       at android.app.ActivityThread.main(ActivityThread.java:7078)
       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)

[LukasPaczos]

Before we commit to changing the type of the collection, it'd be great to understand why the collection is accessed from multiple threads. Whether this is caused internally by the SDK, or possibly by developer interacting with the public APIs (like MapboxNavigation#provideFeedbackMetadataWrapper) from a worker thread which would be a mis-use since MapboxNavigation is not thread-safe.

[dzinad]

I've looked through the code to understand where the invocations from non-main thread may occur. In some of the places we guarantee that it's gonna be main by explicitly using mainJobControl, while in some places it all comes from public API. The ones that could in theory cause said ConcurrentModificationException are listed in the collab ticket.

[dzinad]

Closing in favour of https://github.com/mapbox/mapbox-navigation-android/issues/6210.