From 3489fc0a0d3f2bb5ef9cc9ac4bf0146b8e209b05 Mon Sep 17 00:00:00 2001
From: Chris Loer <chris.loer@gmail.com>
Date: Fri, 30 Mar 2018 12:01:42 -0700
Subject: [core] Fix GeometryTile race condition. Follow up fix for issue
 #11538. This closes a hole in the logic where: - Layout result 1 arrived -
 Symbols arrives for result 1, it was marked ready for commit - Layout result
 2 arrived, clearing "ready for commit" state - Global placement ran on
 symbols for result 1, but layout result 1 data wasn't committed because it
 had been replaced with layout result 2.

---
 src/mbgl/renderer/renderer_impl.cpp |  5 +++++
 src/mbgl/tile/geometry_tile.cpp     | 24 ++++++++----------------
 src/mbgl/tile/geometry_tile.hpp     |  6 +++---
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/src/mbgl/renderer/renderer_impl.cpp b/src/mbgl/renderer/renderer_impl.cpp
index 2ac714e122..b8937c4d6e 100644
--- a/src/mbgl/renderer/renderer_impl.cpp
+++ b/src/mbgl/renderer/renderer_impl.cpp
@@ -404,6 +404,11 @@ void Renderer::Impl::render(const UpdateParameters& updateParameters) {
         // commitFeatureIndexes depends on the assumption that no new FeatureIndex has been loaded since placement
         // started. If we violate this assumption, then we need to either make CollisionIndex completely independendent of
         // FeatureIndex, or find a way for its entries to point to multiple FeatureIndexes.
+        // We also depend on the assumption that anything that can change the results of CollisionIndex::queryRenderedSymbols
+        // will cause 'placementChanged' to be true (note that adding a non-placed item to the index will _not_ make
+        // placementChanged true).
+        // We should consider switching to the "always commit new placements" strategy used in
+        //   https://github.com/mapbox/mapbox-gl-js/pull/6104
         commitFeatureIndexes();
         crossTileSymbolIndex.pruneUnusedLayers(usedSymbolLayers);
         if (placementChanged || symbolBucketsChanged) {
diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp
index 0c8a92d8b7..0039fdc2be 100644
--- a/src/mbgl/tile/geometry_tile.cpp
+++ b/src/mbgl/tile/geometry_tile.cpp
@@ -20,8 +20,6 @@
 #include <mbgl/util/logging.hpp>
 #include <mbgl/actor/scheduler.hpp>
 
-#include <iostream>
-
 namespace mbgl {
 
 using namespace style;
@@ -132,8 +130,7 @@ void GeometryTile::onLayout(LayoutResult result, const uint64_t resultCorrelatio
     nonSymbolBuckets = std::move(result.nonSymbolBuckets);
     // 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) }};
+    dataPendingSymbolBuckets = {{ std::move(result.featureIndex), std::move(result.tileData) }};
     observer->onTileChanged(*this);
 }
 
@@ -149,11 +146,9 @@ void GeometryTile::onPlacement(PlacementResult result, const uint64_t resultCorr
     // 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 (dataPendingSymbolBuckets) {
+        std::swap(dataPendingSymbolBuckets, dataPendingCommit);
+        dataPendingSymbolBuckets = nullopt;
     }
     if (result.glyphAtlasImage) {
         glyphAtlasImage = std::move(*result.glyphAtlasImage);
@@ -231,13 +226,10 @@ void GeometryTile::commitFeatureIndex() {
     // 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 && pendingData->first) {
-        data = std::move(pendingData->second);
-        pendingData = nullopt;
+    if (dataPendingCommit) {
+        featureIndex = std::move(dataPendingCommit->first);
+        data = std::move(dataPendingCommit->second);
+        dataPendingCommit = nullopt;
     }
 }
 
diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp
index 8843d6e579..7423de7c74 100644
--- a/src/mbgl/tile/geometry_tile.hpp
+++ b/src/mbgl/tile/geometry_tile.hpp
@@ -124,11 +124,11 @@ private:
     uint64_t correlationID = 0;
 
     std::unordered_map<std::string, std::shared_ptr<Bucket>> nonSymbolBuckets;
+    optional<std::pair<std::unique_ptr<FeatureIndex>, std::unique_ptr<const GeometryTileData>>> dataPendingSymbolBuckets;
+    optional<std::pair<std::unique_ptr<FeatureIndex>, std::unique_ptr<const GeometryTileData>>> dataPendingCommit;
     std::unique_ptr<FeatureIndex> featureIndex;
-    optional<std::pair<bool,std::unique_ptr<FeatureIndex>>> pendingFeatureIndex;
     std::unique_ptr<const GeometryTileData> data;
-    optional<std::pair<bool, std::unique_ptr<const GeometryTileData>>> pendingData;
-
+    
     optional<AlphaImage> glyphAtlasImage;
     optional<PremultipliedImage> iconAtlasImage;
 
-- 
cgit v1.2.1