tei/MapLibreParticleLab
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix segfault this-capture GeoJsonSource which may be deleted (#3536)

Browse Source 
Co-authored-by: Tim Sylvester <tim@wetdogweather.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This commit is contained in:
Bart Louwers
2025-06-06 23:38:19 +02:00
committed by GitHub
parent f6de3bc0e9
commit b0e2e2ce85
11 changed files with 96 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
include/mbgl/style/source.hpp
View File

@@ -55,13 +55,21 @@ public:
/// Dynamically cast this source to the given subtype.
template <class T>
requires(std::is_base_of_v<Source, T>)
T* as() {
return is<T>() ? reinterpret_cast<T*>(this) : nullptr;
if constexpr (std::is_same_v<T, Source>) {
return this;
}
return is<T>() ? static_cast<T*>(this) : nullptr;
}
template <class T>
requires(std::is_base_of_v<Source, T>)
const T* as() const {
return is<T>() ? reinterpret_cast<const T*>(this) : nullptr;
if constexpr (std::is_same_v<T, Source>) {
return this;
}
return is<T>() ? static_cast<const T*>(this) : nullptr;
}
SourceType getType() const;

10
platform/android/MapLibreAndroid/src/cpp/style/sources/custom_geometry_source.cpp
View File

@@ -141,18 +141,18 @@ void CustomGeometrySource::setTileData(
// Update the core source if not cancelled
if (!isCancelled(z, x, y)) {
source.as<mbgl::style::CustomGeometrySource>()->CustomGeometrySource::setTileData(CanonicalTileID(z, x, y),
GeoJSON(geometry));
getSource<mbgl::style::CustomGeometrySource>().CustomGeometrySource::setTileData(CanonicalTileID(z, x, y),
GeoJSON(geometry));
}
}
void CustomGeometrySource::invalidateTile(jni::JNIEnv&, jni::jint z, jni::jint x, jni::jint y) {
source.as<mbgl::style::CustomGeometrySource>()->CustomGeometrySource::invalidateTile(CanonicalTileID(z, x, y));
getSource<mbgl::style::CustomGeometrySource>().CustomGeometrySource::invalidateTile(CanonicalTileID(z, x, y));
}
void CustomGeometrySource::invalidateBounds(jni::JNIEnv& env, const jni::Object<LatLngBounds>& jBounds) {
auto bounds = LatLngBounds::getLatLngBounds(env, jBounds);
source.as<mbgl::style::CustomGeometrySource>()->CustomGeometrySource::invalidateRegion(bounds);
getSource<mbgl::style::CustomGeometrySource>().CustomGeometrySource::invalidateRegion(bounds);
}
jni::Local<jni::Array<jni::Object<geojson::Feature>>> CustomGeometrySource::querySourceFeatures(
@@ -162,7 +162,7 @@ jni::Local<jni::Array<jni::Object<geojson::Feature>>> CustomGeometrySource::quer
std::vector<mbgl::Feature> features;
if (rendererFrontend) {
features = rendererFrontend->querySourceFeatures(source.getID(), {{}, toFilter(env, jfilter)});
features = rendererFrontend->querySourceFeatures(getSource().getID(), {{}, toFilter(env, jfilter)});
}
return Feature::convert(env, features);
}

28
platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.cpp
View File

@@ -50,12 +50,12 @@ GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, const jni::String& sourceId, cons
std::make_unique<mbgl::style::GeoJSONSource>(jni::Make<std::string>(env, sourceId),
convertGeoJSONOptions(env, options))),
converter(std::make_unique<Actor<FeatureConverter>>(Scheduler::GetBackground(),
source.as<style::GeoJSONSource>()->impl().getOptions())) {}
getSource<style::GeoJSONSource>().impl().getOptions())) {}
GeoJSONSource::GeoJSONSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, AndroidRendererFrontend* frontend)
: Source(env, coreSource, createJavaPeer(env), frontend),
converter(std::make_unique<Actor<FeatureConverter>>(Scheduler::GetBackground(),
source.as<style::GeoJSONSource>()->impl().getOptions())) {}
getSource<style::GeoJSONSource>().impl().getOptions())) {}
GeoJSONSource::~GeoJSONSource() = default;
@@ -83,11 +83,11 @@ void GeoJSONSource::setGeometry(jni::JNIEnv& env, const jni::Object<geojson::Geo
void GeoJSONSource::setURL(jni::JNIEnv& env, const jni::String& url) {
// Update the core source
source.as<style::GeoJSONSource>()->setURL(jni::Make<std::string>(env, url));
getSource<style::GeoJSONSource>().setURL(jni::Make<std::string>(env, url));
}
jni::Local<jni::String> GeoJSONSource::getURL(jni::JNIEnv& env) {
std::optional<std::string> url = source.as<style::GeoJSONSource>()->getURL();
std::optional<std::string> url = getSource<style::GeoJSONSource>().getURL();
return url ? jni::Make<jni::String>(env, *url) : jni::Local<jni::String>();
}
@@ -98,7 +98,7 @@ jni::Local<jni::Array<jni::Object<geojson::Feature>>> GeoJSONSource::querySource
std::vector<mbgl::Feature> features;
if (rendererFrontend) {
features = rendererFrontend->querySourceFeatures(source.getID(), {{}, toFilter(env, jfilter)});
features = rendererFrontend->querySourceFeatures(getSource().getID(), {{}, toFilter(env, jfilter)});
}
return Feature::convert(env, features);
}
@@ -112,7 +112,7 @@ jni::Local<jni::Array<jni::Object<geojson::Feature>>> GeoJSONSource::getClusterC
mbgl::Feature _feature = Feature::convert(env, feature);
_feature.properties["cluster_id"] = static_cast<uint64_t>(_feature.properties["cluster_id"].get<double>());
const auto featureExtension = rendererFrontend->queryFeatureExtensions(
source.getID(), _feature, "supercluster", "children", {});
getSource().getID(), _feature, "supercluster", "children", {});
if (featureExtension.is<mbgl::FeatureCollection>()) {
return Feature::convert(env, featureExtension.get<mbgl::FeatureCollection>());
}
@@ -131,7 +131,7 @@ jni::Local<jni::Array<jni::Object<geojson::Feature>>> GeoJSONSource::getClusterL
const std::map<std::string, mbgl::Value> options = {{"limit", static_cast<uint64_t>(limit)},
{"offset", static_cast<uint64_t>(offset)}};
auto featureExtension = rendererFrontend->queryFeatureExtensions(
source.getID(), _feature, "supercluster", "leaves", options);
getSource().getID(), _feature, "supercluster", "leaves", options);
if (featureExtension.is<mbgl::FeatureCollection>()) {
return Feature::convert(env, featureExtension.get<mbgl::FeatureCollection>());
}
@@ -148,7 +148,7 @@ jint GeoJSONSource::getClusterExpansionZoom(jni::JNIEnv& env, const jni::Object<
mbgl::Feature _feature = Feature::convert(env, feature);
_feature.properties["cluster_id"] = static_cast<uint64_t>(_feature.properties["cluster_id"].get<double>());
auto featureExtension = rendererFrontend->queryFeatureExtensions(
source.getID(), _feature, "supercluster", "expansion-zoom", {});
getSource().getID(), _feature, "supercluster", "expansion-zoom", {});
if (featureExtension.is<mbgl::Value>()) {
auto value = featureExtension.get<mbgl::Value>();
if (value.is<uint64_t>()) {
@@ -181,12 +181,20 @@ void GeoJSONSource::setAsync(Update::Converter converterFn) {
awaitingUpdate = std::make_unique<Update>(
std::move(converterFn),
std::make_unique<Actor<GeoJSONDataCallback>>(
*Scheduler::GetCurrent(), [this](std::shared_ptr<style::GeoJSONData> geoJSONData) {
*Scheduler::GetCurrent(),
[awaitingUpdateWeak = std::weak_ptr(awaitingUpdate),
sourceWeak = getWeakSource(),
updateWeak = std::weak_ptr(update)](std::shared_ptr<style::GeoJSONData> geoJSONData) {
auto awaitingUpdate = awaitingUpdateWeak.lock();
auto source = sourceWeak.lock();
auto update = updateWeak.lock();
if (!awaitingUpdate || !source || !update) return;
// conversion from Java features to core ones finished
android::UniqueEnv _env = android::AttachEnv();
// Update the core source
source.as<mbgl::style::GeoJSONSource>()->setGeoJSONData(std::move(geoJSONData));
source->get().as<style::GeoJSONSource>()->setGeoJSONData(std::move(geoJSONData));
// if there is an awaiting update, execute it, otherwise, release resources
if (awaitingUpdate) {

4
platform/android/MapLibreAndroid/src/cpp/style/sources/geojson_source.hpp
View File

@@ -70,8 +70,8 @@ private:
jni::Local<jni::String> getURL(jni::JNIEnv&);
jni::Local<jni::Object<Source>> createJavaPeer(jni::JNIEnv&);
std::unique_ptr<Update> awaitingUpdate;
std::unique_ptr<Update> update;
std::shared_ptr<Update> awaitingUpdate;
std::shared_ptr<Update> update;
std::shared_ptr<ThreadPool> threadPool;
std::unique_ptr<Actor<FeatureConverter>> converter;

8
platform/android/MapLibreAndroid/src/cpp/style/sources/image_source.cpp
View File

@@ -29,20 +29,20 @@ ImageSource::~ImageSource() = default;
void ImageSource::setURL(jni::JNIEnv& env, const jni::String& url) {
// Update the core source
source.as<mbgl::style::ImageSource>()->ImageSource::setURL(jni::Make<std::string>(env, url));
getSource<mbgl::style::ImageSource>().ImageSource::setURL(jni::Make<std::string>(env, url));
}
jni::Local<jni::String> ImageSource::getURL(jni::JNIEnv& env) {
std::optional<std::string> url = source.as<mbgl::style::ImageSource>()->ImageSource::getURL();
std::optional<std::string> url = getSource<mbgl::style::ImageSource>().ImageSource::getURL();
return url ? jni::Make<jni::String>(env, *url) : jni::Local<jni::String>();
}
void ImageSource::setImage(jni::JNIEnv& env, const jni::Object<Bitmap>& bitmap) {
source.as<mbgl::style::ImageSource>()->setImage(Bitmap::GetImage(env, bitmap));
getSource<mbgl::style::ImageSource>().setImage(Bitmap::GetImage(env, bitmap));
}
void ImageSource::setCoordinates(jni::JNIEnv& env, const jni::Object<LatLngQuad>& coordinatesObject) {
source.as<mbgl::style::ImageSource>()->setCoordinates(LatLngQuad::getLatLngArray(env, coordinatesObject));
getSource<mbgl::style::ImageSource>().setCoordinates(LatLngQuad::getLatLngArray(env, coordinatesObject));
}
jni::Local<jni::Object<Source>> ImageSource::createJavaPeer(jni::JNIEnv& env) {

2
platform/android/MapLibreAndroid/src/cpp/style/sources/raster_dem_source.cpp
View File

@@ -26,7 +26,7 @@ RasterDEMSource::RasterDEMSource(jni::JNIEnv& env, mbgl::style::Source& coreSour
RasterDEMSource::~RasterDEMSource() = default;
jni::Local<jni::String> RasterDEMSource::getURL(jni::JNIEnv& env) {
std::optional<std::string> url = source.as<mbgl::style::RasterDEMSource>()->RasterDEMSource::getURL();
auto url = getSource<mbgl::style::RasterDEMSource>().RasterDEMSource::getURL();
return url ? jni::Make<jni::String>(env, *url) : jni::Local<jni::String>();
}

2
platform/android/MapLibreAndroid/src/cpp/style/sources/raster_source.cpp
View File

@@ -25,7 +25,7 @@ RasterSource::RasterSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, An
RasterSource::~RasterSource() = default;
jni::Local<jni::String> RasterSource::getURL(jni::JNIEnv& env) {
std::optional<std::string> url = source.as<mbgl::style::RasterSource>()->RasterSource::getURL();
auto url = getSource<mbgl::style::RasterSource>().RasterSource::getURL();
return url ? jni::Make<jni::String>(env, *url) : jni::Local<jni::String>();
}

34
platform/android/MapLibreAndroid/src/cpp/style/sources/source.cpp
View File

@@ -70,13 +70,13 @@ Source::Source(jni::JNIEnv& env,
mbgl::style::Source& coreSource,
const jni::Object<Source>& obj,
AndroidRendererFrontend* frontend)
: source(coreSource),
: source(std::make_shared<std::reference_wrapper<mbgl::style::Source>>(coreSource)),
javaPeer(jni::NewGlobal(env, obj)),
rendererFrontend(frontend) {}
Source::Source(jni::JNIEnv&, std::unique_ptr<mbgl::style::Source> coreSource)
: ownedSource(std::move(coreSource)),
source(*ownedSource) {}
source(std::make_shared<std::reference_wrapper<mbgl::style::Source>>(*ownedSource)) {}
Source::~Source() {
if (ownedSource) {
@@ -102,24 +102,24 @@ Source::~Source() {
}
jni::Local<jni::String> Source::getId(jni::JNIEnv& env) {
return jni::Make<jni::String>(env, source.getID());
return jni::Make<jni::String>(env, source->get().getID());
}
jni::Local<jni::String> Source::getAttribution(jni::JNIEnv& env) {
auto attribution = source.getAttribution();
auto attribution = source->get().getAttribution();
return attribution ? jni::Make<jni::String>(env, attribution.value()) : jni::Make<jni::String>(env, "");
}
void Source::setPrefetchZoomDelta(jni::JNIEnv& env, jni::Integer& delta) {
if (!delta) {
source.setPrefetchZoomDelta(std::nullopt);
source->get().setPrefetchZoomDelta(std::nullopt);
} else {
source.setPrefetchZoomDelta(jni::Unbox(env, delta));
source->get().setPrefetchZoomDelta(jni::Unbox(env, delta));
}
}
jni::Local<jni::Integer> Source::getPrefetchZoomDelta(jni::JNIEnv& env) {
auto delta = source.getPrefetchZoomDelta();
auto delta = source->get().getPrefetchZoomDelta();
if (delta.has_value()) {
return jni::Box(env, jni::jint(delta.value()));
}
@@ -128,14 +128,14 @@ jni::Local<jni::Integer> Source::getPrefetchZoomDelta(jni::JNIEnv& env) {
void Source::setMaxOverscaleFactorForParentTiles(jni::JNIEnv& env, jni::Integer& maxOverscaleFactor) {
if (!maxOverscaleFactor) {
source.setMaxOverscaleFactorForParentTiles(std::nullopt);
source->get().setMaxOverscaleFactorForParentTiles(std::nullopt);
} else {
source.setMaxOverscaleFactorForParentTiles(jni::Unbox(env, maxOverscaleFactor));
source->get().setMaxOverscaleFactorForParentTiles(jni::Unbox(env, maxOverscaleFactor));
}
}
jni::Local<jni::Integer> Source::getMaxOverscaleFactorForParentTiles(jni::JNIEnv& env) {
auto maxOverscaleFactor = source.getMaxOverscaleFactorForParentTiles();
auto maxOverscaleFactor = source->get().getMaxOverscaleFactorForParentTiles();
if (maxOverscaleFactor) {
return jni::Box(env, jni::jint(*maxOverscaleFactor));
}
@@ -151,7 +151,7 @@ void Source::addToStyle(JNIEnv& env, const jni::Object<Source>& obj, mbgl::style
style.addSource(std::move(ownedSource));
// Add peer to core source
source.peer = std::unique_ptr<Source>(this);
source->get().peer = std::unique_ptr<Source>(this);
// Add strong reference to java source
javaPeer = jni::NewGlobal(env, obj);
@@ -167,7 +167,7 @@ void Source::addToMap(JNIEnv& env, const jni::Object<Source>& obj, mbgl::Map& ma
map.getStyle().addSource(std::move(ownedSource));
// Add peer to core source
source.peer = std::unique_ptr<Source>(this);
source->get().peer = std::unique_ptr<Source>(this);
// Add strong reference to java source
javaPeer = jni::NewGlobal(env, obj);
@@ -182,26 +182,26 @@ bool Source::removeFromMap(JNIEnv&, const jni::Object<Source>&, mbgl::Map& map)
}
// Remove the source from the map and take ownership
ownedSource = map.getStyle().removeSource(source.getID());
ownedSource = map.getStyle().removeSource(source->get().getID());
// The source may not be removed if any layers still reference it
return ownedSource != nullptr;
}
jni::Local<jni::Boolean> Source::isVolatile(jni::JNIEnv& env) {
return jni::Box(env, jni::jboolean(source.isVolatile()));
return jni::Box(env, jni::jboolean(source->get().isVolatile()));
}
void Source::setVolatile(JNIEnv& env, jni::Boolean& value) {
source.setVolatile(jni::Unbox(env, value));
source->get().setVolatile(jni::Unbox(env, value));
}
void Source::setMinimumTileUpdateInterval(JNIEnv& env, jni::Long& interval) {
source.setMinimumTileUpdateInterval(Milliseconds(jni::Unbox(env, interval)));
source->get().setMinimumTileUpdateInterval(Milliseconds(jni::Unbox(env, interval)));
}
jni::Local<jni::Long> Source::getMinimumTileUpdateInterval(JNIEnv& env) {
return jni::Box(env, jni::jlong(source.getMinimumTileUpdateInterval().count() / 1000000));
return jni::Box(env, jni::jlong(source->get().getMinimumTileUpdateInterval().count() / 1000000));
}
void Source::releaseJavaPeer() {

14
platform/android/MapLibreAndroid/src/cpp/style/sources/source.hpp
View File

@@ -8,6 +8,7 @@
#include "../../android_renderer_frontend.hpp"
#include <jni/jni.hpp>
#include <functional>
namespace mbgl {
namespace android {
@@ -63,12 +64,21 @@ public:
jni::Local<jni::Long> getMinimumTileUpdateInterval(JNIEnv&);
protected:
template <typename T = style::Source>
T& getSource() {
return *ownedSource->as<T>();
}
std::weak_ptr<std::reference_wrapper<style::Source>> getWeakSource() const { return source; }
private:
// Set on newly created sources until added to the map.
std::unique_ptr<mbgl::style::Source> ownedSource;
// Raw pointer that is valid at all times.
mbgl::style::Source& source;
// Reference that is valid at all times.
std::shared_ptr<std::reference_wrapper<mbgl::style::Source>> source;
protected:
// Set when the source is added to a map.
jni::Global<jni::Object<Source>> javaPeer;

4
platform/android/MapLibreAndroid/src/cpp/style/sources/vector_source.cpp
View File

@@ -31,7 +31,7 @@ VectorSource::VectorSource(jni::JNIEnv& env, mbgl::style::Source& coreSource, An
VectorSource::~VectorSource() = default;
jni::Local<jni::String> VectorSource::getURL(jni::JNIEnv& env) {
std::optional<std::string> url = source.as<mbgl::style::VectorSource>()->VectorSource::getURL();
std::optional<std::string> url = getSource<mbgl::style::VectorSource>().VectorSource::getURL();
return url ? jni::Make<jni::String>(env, *url) : jni::Local<jni::String>();
}
@@ -42,7 +42,7 @@ jni::Local<jni::Array<jni::Object<geojson::Feature>>> VectorSource::querySourceF
std::vector<mbgl::Feature> features;
if (rendererFrontend) {
features = rendererFrontend->querySourceFeatures(source.getID(),
features = rendererFrontend->querySourceFeatures(getSource().getID(),
{toVector(env, jSourceLayerIds), toFilter(env, jfilter)});
}
return Feature::convert(env, features);

24
platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/style/GeoJsonSourceTests.java
View File

@@ -1,5 +1,6 @@
package org.maplibre.android.testapp.style;
import org.maplibre.android.style.sources.CannotAddSourceException;
import org.maplibre.geojson.Feature;
import org.maplibre.geojson.FeatureCollection;
import org.maplibre.geojson.Point;
@@ -155,6 +156,29 @@ public class GeoJsonSourceTests extends EspressoTest {
});
}
@Test
public void testDuplicateSourceDuringAsyncSetGeoJson() {
// regression test for segfault when setting GeoJSON contents
// for a source that has been deleted
// https://github.com/maplibre/maplibre-native/issues/3493
validateTestSetup();
MapLibreMapAction.invoke(maplibreMap, (uiController, maplibreMap) -> {
String geoJsonString = "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[0,0]}}";
for (int i = 0; i < 1000; i++) {
// Create a GeoJSON source with the same ID each iteration
GeoJsonSource source = new GeoJsonSource("source");
try {
maplibreMap.getStyle().addSource(source);
} catch (CannotAddSourceException ex) {
// ignore, this is expected
}
// Schedule an async update via setGeoJson(String)
source.setGeoJson(geoJsonString);
}
});
}
public abstract class BaseViewAction implements ViewAction {
@Override