Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Fix issue #11538 (race condition crash for heavily modified annotations) #11551

Merged
merged 2 commits into from
Mar 29, 2018

Conversation

ChrisLoer
Copy link
Contributor

This PR contains two fixes for issue #11538.

The first fix closes a logic error that would prevent the GeometryTile "data" member from updating if the data went from non-null to null. This could allow the tile to have an empty FeatureIndex with non-empty data (since it would keep the previous non-null data). This could lead to an out of bounds exception being thrown here:

for (const std::string& layerID : bucketLayerIDs.at(indexedFeature.bucketName)) {

The crash that would be generated doesn't match the stack trace reported in #11538, but it would be likely to happen in the same sort of stress case (quickly adding and removing annotations).

The second fix closes a race condition in which the FeatureIndex/GeometryTileData received in GeometryTile::onLayout could be committed before the corresponding updated symbolBuckets arrived in GeometryTile::onPlacement. Because the contents of the global CollisionIndex are based on the symbolBuckets, this would allow the CollisionIndex to end up "behind" the corresponding FeatureIndex/GeometryTileData, and if an item in the CollisionIndex had been removed from the new data, it would be possible to get an out-of-bounds exception at:

geometryTileFeature = sourceLayer->getFeature(indexedFeature.index);

... which matches the stack trace in #11538.

A more satisfying fix for these issues would be to do the "one-phase" refactoring suggested in #10457 -- the split between "layout" and "placement" that allowed the race condition is mostly vestigial after that global collision detection changes.

@julianrex Since you were able to reproduce the original crash much more reliably than me, can you try to reproduce the crash off of this branch?

/cc @friedbunny @tobrun @jfirebaugh @lilykaiser

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.
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.
@friedbunny friedbunny added the Core The cross-platform C++ core, aka mbgl label Mar 28, 2018
@friedbunny friedbunny added this to the ios-v4.0.0 milestone Mar 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants