[core] Share GeoJSONData pointer as weak_ptr instead of raw ptr

GeoJSONSource::Impl shares its private data as a raw pointer, this may lead to unwanted side-effects. Moreover GeoJSONSource, used to access its Impl data without checking whether data is valid, this PR fixes both issues.
mapbox · Oct 26, 2018 · 34c3373 · 34c3373
1 parent a7c217e
commit 34c3373
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 17 deletions.
diff --git a/src/mbgl/renderer/sources/render_geojson_source.cpp b/src/mbgl/renderer/sources/render_geojson_source.cpp
@@ -33,23 +33,23 @@ void RenderGeoJSONSource::update(Immutable<style::Source::Impl> baseImpl_,
 
     enabled = needsRendering;
 
-    GeoJSONData* data_ = impl().getData();
+    auto data_ = impl().getData().lock();
 
-    if (data_ != data) {
+    if (data.lock() != data_) {
         data = data_;
         tilePyramid.cache.clear();
 
-        if (data) {
+        if (data_) {
             const uint8_t maxZ = impl().getZoomRange().max;
             for (const auto& pair : tilePyramid.tiles) {
                 if (pair.first.canonical.z <= maxZ) {
-                    static_cast<GeoJSONTile*>(pair.second.get())->updateData(data->getTile(pair.first.canonical));
+                    static_cast<GeoJSONTile*>(pair.second.get())->updateData(data_->getTile(pair.first.canonical));
                 }
             }
         }
     }
 
-    if (!data) {
+    if (!data_) {
         tilePyramid.tiles.clear();
         tilePyramid.renderTiles.clear();
         return;
@@ -63,7 +63,7 @@ void RenderGeoJSONSource::update(Immutable<style::Source::Impl> baseImpl_,
                        util::tileSize,
                        impl().getZoomRange(),
                        optional<LatLngBounds>{},
-                       [&] (const OverscaledTileID& tileID) {
+                       [&, data = data_] (const OverscaledTileID& tileID) {
                            return std::make_unique<GeoJSONTile>(tileID, impl().id, parameters, data->getTile(tileID.canonical));
                        });
 }

diff --git a/src/mbgl/renderer/sources/render_geojson_source.hpp b/src/mbgl/renderer/sources/render_geojson_source.hpp
@@ -44,7 +44,7 @@ class RenderGeoJSONSource : public RenderSource {
     const style::GeoJSONSource::Impl& impl() const;
 
     TilePyramid tilePyramid;
-    style::GeoJSONData* data = nullptr;
+    std::weak_ptr<style::GeoJSONData> data;
 };
 
 template <>

diff --git a/src/mbgl/style/sources/geojson_source.cpp b/src/mbgl/style/sources/geojson_source.cpp
@@ -41,17 +41,29 @@ optional<std::string> GeoJSONSource::getURL() const {
 }
 
 mapbox::geometry::feature_collection<double> GeoJSONSource::getClusterChildren(const std::uint32_t cluster_id) const {
-    return impl().getData()->getClusterChildren(cluster_id);
+    if (auto data = impl().getData().lock()) {
+        return data->getClusterChildren(cluster_id);
+    }
+
+    return {};
 }
 
 mapbox::geometry::feature_collection<double> GeoJSONSource::getClusterLeaves(const std::uint32_t cluster_id,
                                                            const std::uint32_t limit,
                                                            const std::uint32_t offset) const {
-    return impl().getData()->getClusterLeaves(cluster_id, limit, offset);
+    if (auto data = impl().getData().lock()) {
+        return data->getClusterLeaves(cluster_id, limit, offset);
+    }
+
+    return {};
 }
 
 std::uint8_t GeoJSONSource::getClusterExpansionZoom(std::uint32_t cluster_id) const {
-    return impl().getData()->getClusterExpansionZoom(cluster_id);
+    if (auto data = impl().getData().lock()) {
+        return data->getClusterExpansionZoom(cluster_id);
+    }
+
+    return 0;
 }
 
 void GeoJSONSource::loadDescription(FileSource& fileSource) {

diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp
@@ -76,7 +76,7 @@ GeoJSONSource::Impl::Impl(std::string id_, GeoJSONOptions options_)
 GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON)
     : Source::Impl(other),
       options(other.options) {
-    double scale = util::EXTENT / util::tileSize;
+    constexpr double scale = util::EXTENT / util::tileSize;
 
     if (options.cluster
         && geoJSON.is<mapbox::geometry::feature_collection<double>>()
@@ -85,7 +85,7 @@ GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON)
         clusterOptions.maxZoom = options.clusterMaxZoom;
         clusterOptions.extent = util::EXTENT;
         clusterOptions.radius = ::round(scale * options.clusterRadius);
-        data = std::make_unique<SuperclusterData>(
+        data = std::make_shared<SuperclusterData>(
             geoJSON.get<mapbox::geometry::feature_collection<double>>(), clusterOptions);
     } else {
         mapbox::geojsonvt::Options vtOptions;
@@ -94,7 +94,7 @@ GeoJSONSource::Impl::Impl(const Impl& other, const GeoJSON& geoJSON)
         vtOptions.buffer = ::round(scale * options.buffer);
         vtOptions.tolerance = scale * options.tolerance;
         vtOptions.lineMetrics = options.lineMetrics;
-        data = std::make_unique<GeoJSONVTData>(geoJSON, vtOptions);
+        data = std::make_shared<GeoJSONVTData>(geoJSON, vtOptions);
     }
 }
 
@@ -104,8 +104,8 @@ Range<uint8_t> GeoJSONSource::Impl::getZoomRange() const {
     return { options.minzoom, options.maxzoom };
 }
 
-GeoJSONData* GeoJSONSource::Impl::getData() const {
-    return data.get();
+std::weak_ptr<GeoJSONData> GeoJSONSource::Impl::getData() const {
+    return data;
 }
 
 optional<std::string> GeoJSONSource::Impl::getAttribution() const {

diff --git a/src/mbgl/style/sources/geojson_source_impl.hpp b/src/mbgl/style/sources/geojson_source_impl.hpp
@@ -30,13 +30,13 @@ class GeoJSONSource::Impl : public Source::Impl {
     ~Impl() final;
 
     Range<uint8_t> getZoomRange() const;
-    GeoJSONData* getData() const;
+    std::weak_ptr<GeoJSONData> getData() const;
 
     optional<std::string> getAttribution() const final;
 
 private:
     GeoJSONOptions options;
-    std::unique_ptr<GeoJSONData> data;
+    std::shared_ptr<GeoJSONData> data;
 };
 
 } // namespace style