[core] Fix potential race condition crash in symbol querying..

Second half of fix for issue #11538.

If a global placement took place between the time a tile's non-symbol layout updated and the time new symbol buckets arrived, the tile's new FeatureIndex would be committed, but the global CollisionIndex would be generated against the old symbolBuckets. The mismatch could cause the CollisionIndex to contain indices that didn't exist in the new FeatureIndex, and attempting to access them would generate an out-of-bounds exception.

2 files changed, 25 insertions, 8 deletions

diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp
index 7fb0c9922d..0c8a92d8b7 100644
--- a/src/mbgl/tile/geometry_tile.cpp
+++ b/src/mbgl/tile/geometry_tile.cpp
@@ -130,8 +130,10 @@ void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelatio
     //  replacing a tile at a different zoom that _did_ have symbols.
     (void)resultCorrelationID;
     nonSymbolBuckets = std::move(result.nonSymbolBuckets);
-    pendingFeatureIndex = { std::move(result.featureIndex) };
-    pendingData = { std::move(result.tileData) };
+    // It is possible for multiple onLayouts to be called before an onPlacement is called, in which
+    // case we will discard previous pending data
+    pendingFeatureIndex = {{ false, std::move(result.featureIndex) }};
+    pendingData = {{ false, std::move(result.tileData) }};
     observer->onTileChanged(*this);
 }
 
@@ -142,6 +144,17 @@ void GeometryTile::onPlacement(PlacementResult result, const uint64_t resultCorr
         pending = false;
     }
     symbolBuckets = std::move(result.symbolBuckets);
+    // When symbolBuckets arrive, mark pendingData/FeatureIndex as "ready for commit" at the
+    // time of the next global placement. We are counting on these symbolBuckets being
+    // in sync with the data/index in the last `onLayout` call, even though the correlation IDs
+    // may not be the same (e.g. "setShowCollisionBoxes" could bump the correlation ID while
+    // waiting for glyph dependencies)
+    if (pendingData) {
+        pendingData->first = true;
+    }
+    if (pendingFeatureIndex) {
+        pendingFeatureIndex->first = true;
+    }
     if (result.glyphAtlasImage) {
         glyphAtlasImage = std::move(*result.glyphAtlasImage);
     }
@@ -214,12 +227,16 @@ Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const {
 }
 
 void GeometryTile::commitFeatureIndex() {
-    if (pendingFeatureIndex) {
-        featureIndex = std::move(*pendingFeatureIndex);
+    // We commit our pending FeatureIndex and GeometryTileData when:
+    // 1) An `onPlacement` result has delivered us updated symbolBuckets since we received the pending data
+    // 2) A global placement has run, synchronizing the global CollisionIndex with the latest
+    //    symbolBuckets (and thus with the latest FeatureIndex/GeometryTileData)
+    if (pendingFeatureIndex && pendingFeatureIndex->first) {
+        featureIndex = std::move(pendingFeatureIndex->second);
         pendingFeatureIndex = nullopt;
     }
-    if (pendingData) {
-        data = std::move(*pendingData);
+    if (pendingData && pendingData->first) {
+        data = std::move(pendingData->second);
         pendingData = nullopt;
     }
 }
diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp
index 0c6adf0337..8843d6e579 100644
--- a/src/mbgl/tile/geometry_tile.hpp
+++ b/src/mbgl/tile/geometry_tile.hpp
@@ -125,9 +125,9 @@ private:
 
     std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets;
     std::unique_ptr<FeatureIndex> featureIndex;
-    optional<std::unique_ptr<FeatureIndex>> pendingFeatureIndex;
+    optional<std::pair<bool,std::unique_ptr<FeatureIndex>>> pendingFeatureIndex;
     std::unique_ptr<const GeometryTileData> data;
-    optional<std::unique_ptr<const GeometryTileData>> pendingData;
+    optional<std::pair<bool, std::unique_ptr<const GeometryTileData>>> pendingData;
 
     optional<AlphaImage> glyphAtlasImage;
     optional<PremultipliedImage> iconAtlasImage;