From 839deb0105c3eaba71e45ac98e94a921524ddf71 Mon Sep 17 00:00:00 2001
From: Mikhail Pozdnyakov <mikhail.pozdnyakov@mapbox.com>
Date: Thu, 9 Jan 2020 15:50:48 +0200
Subject: [core] Fix GeoJSONVTData ownership and life cycle

Before this change, the `GeoJSONVTData` instance was retained at the scheduled
lambda, which run on the worker thread represented by the `GeoJSONVTData::scheduler`
class member:

```
        std::weak_ptr<GeoJSONVTData> weak = shared_from_this();
        scheduler->scheduleAndReplyValue(
            [id, weak, this]() -> TileFeatures {
                if (auto self = weak.lock()) {
                    return impl.getTile(id.z, id.x, id.y).features;
                }
                return {};
            },
            fn);
```

It caused program termination in case `self` turned to be the last reference to `this`,
as the `std::thread` destructor was called from the thread it represented.

Now, only the `GeoJSONVTData::impl` class member is retained.
---
 src/mbgl/style/sources/geojson_source_impl.cpp | 17 ++++++-----------
 src/mbgl/util/thread_pool.hpp                  |  1 +
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/mbgl/style/sources/geojson_source_impl.cpp b/src/mbgl/style/sources/geojson_source_impl.cpp
index 9a883ba96d..e73876b429 100644
--- a/src/mbgl/style/sources/geojson_source_impl.cpp
+++ b/src/mbgl/style/sources/geojson_source_impl.cpp
@@ -13,19 +13,12 @@
 namespace mbgl {
 namespace style {
 
-class GeoJSONVTData : public GeoJSONData, public std::enable_shared_from_this<GeoJSONVTData> {
+class GeoJSONVTData : public GeoJSONData {
 public:
     void getTile(const CanonicalTileID& id, const std::function<void(TileFeatures)>& fn) final {
         assert(fn);
-        std::weak_ptr<GeoJSONVTData> weak = shared_from_this();
         scheduler->scheduleAndReplyValue(
-            [id, weak, this]() -> TileFeatures {
-                if (auto self = weak.lock()) {
-                    return impl.getTile(id.z, id.x, id.y).features;
-                }
-                return {};
-            },
-            fn);
+            [id, impl = this->impl]() -> TileFeatures { return impl->getTile(id.z, id.x, id.y).features; }, fn);
     }
 
     Features getChildren(const std::uint32_t) final { return {}; }
@@ -39,8 +32,10 @@ public:
 private:
     friend GeoJSONData;
     GeoJSONVTData(const GeoJSON& geoJSON, const mapbox::geojsonvt::Options& options)
-        : impl(geoJSON, options), scheduler(Scheduler::GetSequenced()) {}
-    mapbox::geojsonvt::GeoJSONVT impl;
+        : impl(std::make_shared<mapbox::geojsonvt::GeoJSONVT>(geoJSON, options)),
+          scheduler(Scheduler::GetSequenced()) {}
+
+    std::shared_ptr<mapbox::geojsonvt::GeoJSONVT> impl; // Accessed on worker thread.
     std::shared_ptr<Scheduler> scheduler;
 };
 
diff --git a/src/mbgl/util/thread_pool.hpp b/src/mbgl/util/thread_pool.hpp
index e39c1abc73..9791ff9460 100644
--- a/src/mbgl/util/thread_pool.hpp
+++ b/src/mbgl/util/thread_pool.hpp
@@ -48,6 +48,7 @@ public:
     ~ThreadedScheduler() override {
         terminate();
         for (auto& thread : threads) {
+            assert(std::this_thread::get_id() != thread.get_id());
             thread.join();
         }
     }
-- 
cgit v1.2.1