Mapbox-gl-native: [ios] Crashes caused by dangling rawSource pointer after new style is parsed.

[julianrex]

If a developer retains a MGLSource, then sets a new style URL (loading a new style), then that MGLSource will end up with a dangling rawSource pointer.

In Style::Impl::parse we have sources.clear(); without a mechanism that updates the platform object. The fact this doesn't crash immediately when accessing the rawSource pointer is just pure luck.

Interestingly @asheemmamoowala and I worked on a similar issue with CustomLayer/RenderCustomLayer. I think a similar mechanism may be needed here; it's a generic enough problem.

Related: https://github.com/mapbox/mapbox-gl-native/issues/6180

/cc @samfader @tmpsantos @frederoni

[alexshalamov]

@julianrex I think this is an API usability issue, not a bug. There are many platform and library APIs that work in the same way. Objects become invalid once the owner of objects is invalidated.

without a mechanism that updates the platform object.

Style change invalidates all source / layer objects. If needed, platform can detach required objects before style is changed and re-attach if needed after new style finished loading, or on demand, when MGLSource / Layer is accessed.

Another option would be to change core API, so that we operate with shared pointers to Source / Layer (maybe even Style).

@pozdnyakov wdyt?

[pozdnyakov]

@alexshalamov I agree that this sound more like API usability issue. It would be better if we do not expose source and layer objects as their lifecycle is not quite clear, instead we could export generic key-value API for setting a style.

[julianrex]

This may be an internal API issue, i.e. between the platform and core - but we should assume that developers could hold on to the iOS objects for a long time (even if they become "invalid" for some reason).

[alexshalamov]

@julianrex I agree that API provided by core is not the most elegant or well documented, however, ownership is more or less clearly defined.

addSource (Layer) takes unique pointer, therefore, ownership is transferred to Style
removeSource (Layer) returns unique pointer, ownership is transferred from Style
getSource (Layer), allows access to an object owned by the Style, objects are valid until style is invalidated.

Platform can track MGL* objects that were given out to the client and detach core objects (removeSource / Layer) before style is changed.

[asheemmamoowala]

It would be useful for core to emit lifecycle events to the platform for the top level style objects. The SourceObserver and LayerObserver could be leveraged to provide this. Platform wrappers could register to be notified that the object will be detached or was detached.

[pozdnyakov]

The plan for fixing the issue is that MGL.. wrappers will hold weak pointers to the core objects instead of raw pointers. The iOS SDK implementation will have to check these weak pointers before using them and gracefully handle the situations when these pointers are equal to nullptr.

[chloekraw]

Does this need to be handled on the @mapbox/maps-android side as well?

[asheemmamoowala]

A similar issue to this was addressed for Android in https://github.com/mapbox/mapbox-gl-native/pull/9983, specifically these changes:

https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/src/style/sources/source.cpp#L114-L143

Maybe @LukasPaczos is aware of continued issues with stale pointers in the Android SDK.

[tobrun]

Does this need to be handled on the @mapbox/maps-android side as well?

The weak pointer setup as outlined by @pozdnyakov in https://github.com/mapbox/mapbox-gl-native/issues/15333#issuecomment-525209827 would be the ideal solution for Android as well. We currently have some workaround in place that flags a source/layer being "detached", being able to check the pointer if it's still valid would be ideal solution.