Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Copy features array before passing them to core #14804

Merged
merged 1 commit into from
Jun 4, 2019

Conversation

LukasPaczos
Copy link
Member

Closes #14565.

I have noticed, that the only possible scenario when the crash from #14565 can occur is when the collection is cleared while it's being converted to an array on our worker thread:

auto jFeatures = java::util::List::toArray<Feature>(env, jFeatureList);

ArrayList#toArray builds an array with a fixed size, adding a null padding if necessary, therefore I think, that the collection has to be cleared on one thread, right when the ArrayList#toArray is copying the list on the other (after setting the resulting array's size and before finishing copying all of the elements, which creates the null padding).

This is a race condition, so the only test I was able to come up with is a brute force. The test has been reliably reproducing the crash for me on every single run, either on a physical device or an emulator.

The solution is to copy the features array to a new array so that it cannot be modified by the provider. I was considering copying each Feature object as well, but it's not critical as modifying the feature should be safe and it introduced additional overhead.

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label May 30, 2019
@LukasPaczos LukasPaczos added this to the release-oolong milestone May 30, 2019
@LukasPaczos LukasPaczos requested a review from tobrun May 30, 2019 12:06
@LukasPaczos
Copy link
Member Author

Ready for another round.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: JNI error: can't call java.lang.String com.mapbox.geojson.Feature.id() on null object
3 participants