Add weak pointer handling (#3763)

include/mbgl/style/layer.hpp

@@ -10,9 +10,8 @@
 
 #include <cassert>
 #include <memory>
-#include <string>
-#include <stdexcept>
 #include <optional>
+#include <string>
 
 namespace mbgl {
 namespace style {
@@ -109,6 +108,8 @@ bool layerTypeInfoEquals(const mbgl::style::LayerTypeInfo* one, const mbgl::styl
  * create an instance of the desired type, calling `LayerManager`:
  *
  *     auto circleLayer = LayerManager::get()->createLayer("circle", ...);
+ *
+ * NOTE: Derived class must invalidate `weakFactory` in their destructor
  */
 class Layer {
 public:

include/mbgl/style/sources/custom_geometry_source.hpp

@@ -20,6 +20,7 @@ using TileFunction = std::function<void(const CanonicalTileID&)>;
 
 class CustomTileLoader;
 
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
 class CustomGeometrySource final : public Source {
 public:
     struct TileOptions {

include/mbgl/style/sources/geojson_source.hpp

@@ -54,6 +54,7 @@ public:
     virtual std::uint8_t getClusterExpansionZoom(std::uint32_t) = 0;
 };
 
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
 class GeoJSONSource final : public Source {
 public:
     GeoJSONSource(std::string id, Immutable<GeoJSONOptions> = GeoJSONOptions::defaultOptions());

include/mbgl/style/sources/image_source.hpp

@@ -11,6 +11,7 @@ class AsyncRequest;
 
 namespace style {
 
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
 class ImageSource final : public Source {
 public:
     ImageSource(std::string id, std::array<LatLng, 4>);

include/mbgl/style/sources/raster_dem_source.hpp

@@ -5,14 +5,14 @@
 #include <mbgl/util/variant.hpp>
 
 namespace mbgl {
-
 namespace style {
 
 struct RasterDEMOptions {
     std::optional<Tileset::DEMEncoding> encoding = std::nullopt;
 };
 
-class RasterDEMSource : public RasterSource {
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
+class RasterDEMSource final : public RasterSource {
 public:
     RasterDEMSource(std::string id,
                     variant<std::string, Tileset> urlOrTileset,

include/mbgl/style/sources/raster_source.hpp

@@ -5,7 +5,6 @@
 #include <mbgl/util/variant.hpp>
 
 namespace mbgl {
-
 namespace style {
 
 class RasterSource : public TileSource {

include/mbgl/style/sources/vector_source.hpp

@@ -10,6 +10,7 @@ class AsyncRequest;
 
 namespace style {
 
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
 class VectorSource final : public TileSource {
 public:
     VectorSource(std::string id,

include/mbgl/util/run_loop.hpp

@@ -7,18 +7,18 @@
 #include <mbgl/util/work_task.hpp>
 #include <mbgl/util/work_request.hpp>
 
-#include <atomic>
 #include <functional>
-#include <utility>
-#include <queue>
 #include <mutex>
+#include <queue>
+#include <utility>
 
 namespace mbgl {
 namespace util {
 
 using LOOP_HANDLE = void*;
 
-class RunLoop : public Scheduler, private util::noncopyable {
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
+class RunLoop final : public Scheduler, private util::noncopyable {
 public:
     enum class Type : uint8_t {
         Default,
@@ -88,6 +88,10 @@ public:
 
     class Impl;
 
+    // Allows derived classes to invalidate weak pointers in
+    // their destructor before their own members are torn down.
+    void invalidateWeakPtrsEarly() { weakFactory.invalidateWeakPtrs(); }
+
 private:
     MBGL_STORE_THREAD(tid)
 

platform/android/MapLibreAndroid/src/cpp/map_renderer.hpp

@@ -37,8 +37,10 @@ public:
  * performed on the GL Thread.
  *
  * The public methods are safe to call from the main thread, others are not.
+ *
+ * NOTE: Any derived class must invalidate `weakFactory` in the destructor
  */
-class MapRenderer : public Scheduler {
+class MapRenderer final : public Scheduler {
 public:
     static constexpr auto Name() { return "org/maplibre/android/maps/renderer/MapRenderer"; };
 
@@ -95,6 +97,10 @@ protected:
 
     void scheduleSnapshot(std::unique_ptr<SnapshotCallback>);
 
+    // Allows derived classes to invalidate weak pointers in
+    // their destructor before their own members are torn down.
+    void invalidateWeakPtrsEarly() { weakFactory.invalidateWeakPtrs(); }
+
 private:
     struct MailboxData {
         explicit MailboxData(Scheduler*);

src/mbgl/annotation/annotation_manager.hpp

@@ -5,13 +5,12 @@
 #include <mbgl/style/image.hpp>
 #include <mbgl/util/noncopyable.hpp>
 
+#include <mapbox/std/weak.hpp>
+
 #include <mutex>
 #include <string>
-#include <vector>
-#include <unordered_set>
 #include <unordered_map>
-
-#include <mapbox/std/weak.hpp>
+#include <unordered_set>
 
 namespace mbgl {
 
@@ -25,7 +24,8 @@ namespace style {
 class Style;
 } // namespace style
 
-class AnnotationManager : private util::noncopyable {
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
+class AnnotationManager final : private util::noncopyable {
 public:
     AnnotationManager(style::Style&);
     ~AnnotationManager();

src/mbgl/annotation/annotation_source.hpp

@@ -5,6 +5,7 @@
 
 namespace mbgl {
 
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
 class AnnotationSource final : public style::Source {
 public:
     AnnotationSource();

src/mbgl/renderer/render_source.hpp

@@ -11,10 +11,10 @@
 #include <mbgl/style/source_impl.hpp>
 #include <mbgl/style/layer_properties.hpp>
 
-#include <unordered_map>
-#include <vector>
 #include <map>
 #include <memory>
+#include <unordered_map>
+#include <vector>
 
 namespace mbgl {
 

src/mbgl/sprite/sprite_loader.hpp

@@ -7,9 +7,6 @@
 
 #include <string>
 #include <map>
-#include <set>
-#include <vector>
-#include <array>
 #include <memory>
 
 namespace mbgl {
@@ -17,7 +14,8 @@ namespace mbgl {
 class FileSource;
 class SpriteLoaderObserver;
 
-class SpriteLoader {
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
+class SpriteLoader final {
 public:
     SpriteLoader(float pixelRatio, const TaggedScheduler& threadPool_);
     ~SpriteLoader();

src/mbgl/style/layers/background_layer.cpp

@@ -39,7 +39,9 @@ BackgroundLayer::BackgroundLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-BackgroundLayer::~BackgroundLayer() = default;
+BackgroundLayer::~BackgroundLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const BackgroundLayer::Impl& BackgroundLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/circle_layer.cpp

@@ -39,7 +39,9 @@ CircleLayer::CircleLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-CircleLayer::~CircleLayer() = default;
+CircleLayer::~CircleLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const CircleLayer::Impl& CircleLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/fill_extrusion_layer.cpp

@@ -39,7 +39,9 @@ FillExtrusionLayer::FillExtrusionLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-FillExtrusionLayer::~FillExtrusionLayer() = default;
+FillExtrusionLayer::~FillExtrusionLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const FillExtrusionLayer::Impl& FillExtrusionLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/fill_layer.cpp

@@ -39,7 +39,9 @@ FillLayer::FillLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-FillLayer::~FillLayer() = default;
+FillLayer::~FillLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const FillLayer::Impl& FillLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/heatmap_layer.cpp

@@ -39,7 +39,9 @@ HeatmapLayer::HeatmapLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-HeatmapLayer::~HeatmapLayer() = default;
+HeatmapLayer::~HeatmapLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const HeatmapLayer::Impl& HeatmapLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/hillshade_layer.cpp

@@ -39,7 +39,9 @@ HillshadeLayer::HillshadeLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-HillshadeLayer::~HillshadeLayer() = default;
+HillshadeLayer::~HillshadeLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const HillshadeLayer::Impl& HillshadeLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/layer.cpp.ejs

@@ -129,7 +129,9 @@ const LayerTypeInfo* <%- camelize(type) %>Layer::Impl::staticTypeInfo() noexcept
     : Layer(std::move(impl_)) {
 }
 
-<%- camelize(type) %>Layer::~<%- camelize(type) %>Layer() = default;
+<%- camelize(type) %>Layer::~<%- camelize(type) %>Layer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const <%- camelize(type) %>Layer::Impl& <%- camelize(type) %>Layer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/line_layer.cpp

@@ -39,7 +39,9 @@ LineLayer::LineLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-LineLayer::~LineLayer() = default;
+LineLayer::~LineLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const LineLayer::Impl& LineLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/location_indicator_layer.cpp

@@ -39,7 +39,9 @@ LocationIndicatorLayer::LocationIndicatorLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-LocationIndicatorLayer::~LocationIndicatorLayer() = default;
+LocationIndicatorLayer::~LocationIndicatorLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const LocationIndicatorLayer::Impl& LocationIndicatorLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/raster_layer.cpp

@@ -39,7 +39,9 @@ RasterLayer::RasterLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-RasterLayer::~RasterLayer() = default;
+RasterLayer::~RasterLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const RasterLayer::Impl& RasterLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/layers/symbol_layer.cpp

@@ -39,7 +39,9 @@ SymbolLayer::SymbolLayer(Immutable<Impl> impl_)
     : Layer(std::move(impl_)) {
 }
 
-SymbolLayer::~SymbolLayer() = default;
+SymbolLayer::~SymbolLayer() {
+    weakFactory.invalidateWeakPtrs();
+}
 
 const SymbolLayer::Impl& SymbolLayer::impl() const {
     return static_cast<const Impl&>(*baseImpl);

src/mbgl/style/sources/raster_dem_source.cpp

@@ -6,6 +6,7 @@
 #include <mbgl/style/sources/tile_source_impl.hpp>
 #include <mbgl/tile/tile.hpp>
 #include <mbgl/util/mapbox.hpp>
+
 #include <utility>
 
 namespace mbgl {

src/mbgl/tile/geojson_tile.hpp

@@ -10,7 +10,8 @@ class GeoJSONData;
 
 class TileParameters;
 
-class GeoJSONTile : public GeometryTile {
+// NOTE: Any derived class must invalidate `weakFactory` in the destructor
+class GeoJSONTile final : public GeometryTile {
 public:
     GeoJSONTile(const OverscaledTileID&,
                 std::string sourceID,

src/mbgl/util/thread_pool.hpp

@@ -7,9 +7,7 @@
 #include <mbgl/util/identity.hpp>
 #include <mbgl/util/instrumentation.hpp>
 
-#include <algorithm>
 #include <condition_variable>
-#include <map>
 #include <mutex>
 #include <queue>
 #include <thread>
@@ -145,6 +143,11 @@ public:
 
     mapbox::base::WeakPtr<Scheduler> makeWeakPtr() override { return weakFactory.makeWeakPtr(); }
 
+protected:
+    // Allows derived classes to invalidate weak pointers in
+    // their destructor before their own members are torn down.
+    void invalidateWeakPtrsEarly() { weakFactory.invalidateWeakPtrs(); }
+
 private:
     std::vector<std::thread> threads;
 
@@ -159,22 +162,25 @@ private:
     // Do not add members here, see `WeakPtrFactory`
 };
 
-class SequencedScheduler : public ThreadedScheduler {
+class SequencedScheduler final : public ThreadedScheduler {
 public:
     SequencedScheduler()
         : ThreadedScheduler(1) {}
+    ~SequencedScheduler() override { invalidateWeakPtrsEarly(); }
 };
 
 class ParallelScheduler : public ThreadedScheduler {
 public:
     ParallelScheduler(std::size_t extra)
         : ThreadedScheduler(1 + extra) {}
+    ~ParallelScheduler() override { invalidateWeakPtrsEarly(); }
 };
 
-class ThreadPool : public ParallelScheduler {
+class ThreadPool final : public ParallelScheduler {
 public:
     ThreadPool()
         : ParallelScheduler(3) {}
+    ~ThreadPool() override { invalidateWeakPtrsEarly(); }
 };
 
 } // namespace mbgl

test/actor/actor.test.cpp

@@ -80,7 +80,8 @@ TEST(Actor, DestructionBlocksOnReceive) {
 TEST(Actor, DestructionBlocksOnSend) {
     // Destruction blocks until the actor is not being sent a message.
 
-    struct TestScheduler : public Scheduler {
+    // NOTE: Any derived class must invalidate `weakFactory` in the destructor
+    struct TestScheduler final : public Scheduler {
         std::promise<void> promise;
         std::future<void> future;
         std::atomic<bool> waited;