From 26edac0f66b2f49f059ceec22bf5fb6f66bf9ead Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Wed, 28 Mar 2018 14:11:55 -0700 Subject: [PATCH 1/2] [core] Fix potential race condition crash in symbol querying.. First half of fix for issue #11538. Testing `if (pendingData)` didn't work if there _was_ a data update, but the update was "the data for this tile is now null". In that case, the tile's FeatureIndex would be updated, but the tile's data member would remain unchanged. In the case of a tile's data being deleted, the matching FeatureIndex would have an empty set of bucketLayerIDs, but the _old_ data would still be in place for querying, and the symbolBuckets might not be updated yet (pending onPlacement). In this case `bucketLayerIDs.at(indexedFeature.bucketName)` could throw an out-of-range exception. --- src/mbgl/tile/geometry_tile.cpp | 10 ++++++---- src/mbgl/tile/geometry_tile.hpp | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 82d0c918061..7fb0c9922d6 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -130,8 +130,8 @@ 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); + pendingFeatureIndex = { std::move(result.featureIndex) }; + pendingData = { std::move(result.tileData) }; observer->onTileChanged(*this); } @@ -215,10 +215,12 @@ Bucket* GeometryTile::getBucket(const Layer::Impl& layer) const { void GeometryTile::commitFeatureIndex() { if (pendingFeatureIndex) { - featureIndex = std::move(pendingFeatureIndex); + featureIndex = std::move(*pendingFeatureIndex); + pendingFeatureIndex = nullopt; } if (pendingData) { - data = std::move(pendingData); + data = std::move(*pendingData); + pendingData = nullopt; } } diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 00a4aafadf6..0c6adf0337e 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -125,9 +125,9 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { std::unordered_map> nonSymbolBuckets; std::unique_ptr featureIndex; - std::unique_ptr pendingFeatureIndex; + optional> pendingFeatureIndex; std::unique_ptr data; - std::unique_ptr pendingData; + optional> pendingData; optional glyphAtlasImage; optional iconAtlasImage; From 1717e11eb571345c23ecaa3befa3aa1e6cecd440 Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Wed, 28 Mar 2018 15:40:57 -0700 Subject: [PATCH 2/2] [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. --- src/mbgl/tile/geometry_tile.cpp | 29 +++++++++++++++++++++++------ src/mbgl/tile/geometry_tile.hpp | 4 ++-- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 7fb0c9922d6..0c8a92d8b7b 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 0c6adf0337e..8843d6e5791 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -125,9 +125,9 @@ class GeometryTile : public Tile, public GlyphRequestor, ImageRequestor { std::unordered_map> nonSymbolBuckets; std::unique_ptr featureIndex; - optional> pendingFeatureIndex; + optional>> pendingFeatureIndex; std::unique_ptr data; - optional> pendingData; + optional>> pendingData; optional glyphAtlasImage; optional iconAtlasImage;