From 18fc0b76536ad9191cc5c4699097617de51555c9 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 23 Jul 2018 16:08:44 -0700 Subject: [core] Check all bucket dynamic_casts A mismatch can occur when a layer changes from one type to another. --- platform/android/CHANGELOG.md | 1 + platform/ios/CHANGELOG.md | 1 + platform/macos/CHANGELOG.md | 1 + src/mbgl/renderer/layers/render_circle_layer.cpp | 7 ++- .../layers/render_fill_extrusion_layer.cpp | 16 ++++--- src/mbgl/renderer/layers/render_fill_layer.cpp | 14 ++++-- src/mbgl/renderer/layers/render_heatmap_layer.cpp | 7 ++- .../renderer/layers/render_hillshade_layer.cpp | 8 +++- src/mbgl/renderer/layers/render_line_layer.cpp | 7 ++- src/mbgl/renderer/layers/render_raster_layer.cpp | 7 ++- src/mbgl/renderer/layers/render_symbol_layer.cpp | 7 ++- src/mbgl/text/cross_tile_symbol_index.cpp | 9 ++-- src/mbgl/text/placement.cpp | 22 +++++---- test/fixtures/map/issue12432/0-0-0.mvt | Bin 0 -> 482553 bytes test/map/map.test.cpp | 49 +++++++++++++++++++++ 15 files changed, 122 insertions(+), 34 deletions(-) create mode 100644 test/fixtures/map/issue12432/0-0-0.mvt diff --git a/platform/android/CHANGELOG.md b/platform/android/CHANGELOG.md index aaeb1f53a0..0216d91a58 100644 --- a/platform/android/CHANGELOG.md +++ b/platform/android/CHANGELOG.md @@ -4,6 +4,7 @@ Mapbox welcomes participation and contributions from everyone. If you'd like to ## master - Token string syntax (`"{token}"`) in `SymbolLayer` `textField` and `iconImage` property values is now correctly converted to the appropriate expression equivalent. ([#11659](https://github.com/mapbox/mapbox-gl-native/issues/11659)) + - Fixed a crash when switching between two styles having layers with the same identifier but different layer types. ([#12432](https://github.com/mapbox/mapbox-gl-native/issues/12432)) ## 6.3.0 - July 18, 2018 - Harden map events creation [#12406](https://github.com/mapbox/mapbox-gl-native/pull/12406) diff --git a/platform/ios/CHANGELOG.md b/platform/ios/CHANGELOG.md index 9cc953bd1c..08fc156f91 100644 --- a/platform/ios/CHANGELOG.md +++ b/platform/ios/CHANGELOG.md @@ -9,6 +9,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT * Fixed a crash that occurred when the user started a gesture before the drift animation for a previous gesture was complete. ([#12148](https://github.com/mapbox/mapbox-gl-native/pull/12148)) * Token string syntax (`"{token}"`) in `MGLSymbolStyleLayer` `text` and `iconImageName` properties is now correctly converted to the appropriate `NSExpression` equivalent. ([#11659](https://github.com/mapbox/mapbox-gl-native/issues/11659)) * Added an `MGLMapView.locationManager` property and `MGLLocationManager` protocol for tracking user location using a custom alternative to `CLLocationManager`. ([#12013](https://github.com/mapbox/mapbox-gl-native/pull/12013)) +* Fixed a crash when switching between two styles having layers with the same identifier but different layer types. ([#12432](https://github.com/mapbox/mapbox-gl-native/issues/12432)) ## 4.2.0 - July 18, 2018 diff --git a/platform/macos/CHANGELOG.md b/platform/macos/CHANGELOG.md index 0386b78db0..001b3792d5 100644 --- a/platform/macos/CHANGELOG.md +++ b/platform/macos/CHANGELOG.md @@ -5,6 +5,7 @@ ## Styles and rendering * Token string syntax (`"{token}"`) in `MGLSymbolStyleLayer` `text` and `iconImageName` properties is now correctly converted to the appropriate `NSExpression` equivalent. ([#11659](https://github.com/mapbox/mapbox-gl-native/issues/11659)) +* Fixed a crash when switching between two styles having layers with the same identifier but different layer types. ([#12432](https://github.com/mapbox/mapbox-gl-native/issues/12432)) # 0.9.0 - July 18, 2018 diff --git a/src/mbgl/renderer/layers/render_circle_layer.cpp b/src/mbgl/renderer/layers/render_circle_layer.cpp index b433a9d3fa..b1b4d22561 100644 --- a/src/mbgl/renderer/layers/render_circle_layer.cpp +++ b/src/mbgl/renderer/layers/render_circle_layer.cpp @@ -56,8 +56,11 @@ void RenderCircleLayer::render(PaintParameters& parameters, RenderSource*) { const bool pitchWithMap = evaluated.get() == AlignmentType::Map; for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - CircleBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + CircleBucket& bucket = *bucket_; const auto& paintPropertyBinders = bucket.paintPropertyBinders.at(getID()); diff --git a/src/mbgl/renderer/layers/render_fill_extrusion_layer.cpp b/src/mbgl/renderer/layers/render_fill_extrusion_layer.cpp index d5282c9b0d..d37f6f528b 100644 --- a/src/mbgl/renderer/layers/render_fill_extrusion_layer.cpp +++ b/src/mbgl/renderer/layers/render_fill_extrusion_layer.cpp @@ -100,9 +100,11 @@ void RenderFillExtrusionLayer::render(PaintParameters& parameters, RenderSource* if (evaluated.get().from.empty()) { for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - FillExtrusionBucket& bucket = - *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + FillExtrusionBucket& bucket = *bucket_; draw( parameters.programs.fillExtrusion.get(evaluated), @@ -129,9 +131,11 @@ void RenderFillExtrusionLayer::render(PaintParameters& parameters, RenderSource* parameters.imageManager.bind(parameters.context, 0); for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - FillExtrusionBucket& bucket = - *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + FillExtrusionBucket& bucket = *bucket_; draw( parameters.programs.fillExtrusionPattern.get(evaluated), diff --git a/src/mbgl/renderer/layers/render_fill_layer.cpp b/src/mbgl/renderer/layers/render_fill_layer.cpp index c59ca6f906..cce4399262 100644 --- a/src/mbgl/renderer/layers/render_fill_layer.cpp +++ b/src/mbgl/renderer/layers/render_fill_layer.cpp @@ -61,8 +61,11 @@ bool RenderFillLayer::hasTransition() const { void RenderFillLayer::render(PaintParameters& parameters, RenderSource*) { if (evaluated.get().from.empty()) { for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - FillBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + FillBucket& bucket = *bucket_; auto draw = [&] (auto& program, const auto& drawMode, @@ -146,8 +149,11 @@ void RenderFillLayer::render(PaintParameters& parameters, RenderSource*) { parameters.imageManager.bind(parameters.context, 0); for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - FillBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + FillBucket& bucket = *bucket_; auto draw = [&] (auto& program, const auto& drawMode, diff --git a/src/mbgl/renderer/layers/render_heatmap_layer.cpp b/src/mbgl/renderer/layers/render_heatmap_layer.cpp index dec9edb318..d2c6f6d970 100644 --- a/src/mbgl/renderer/layers/render_heatmap_layer.cpp +++ b/src/mbgl/renderer/layers/render_heatmap_layer.cpp @@ -83,8 +83,11 @@ void RenderHeatmapLayer::render(PaintParameters& parameters, RenderSource*) { parameters.context.clear(Color{ 0.0f, 0.0f, 0.0f, 1.0f }, {}, {}); for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - HeatmapBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + HeatmapBucket& bucket = *bucket_; const auto extrudeScale = tile.id.pixelsToTileUnits(1, parameters.state.getZoom()); diff --git a/src/mbgl/renderer/layers/render_hillshade_layer.cpp b/src/mbgl/renderer/layers/render_hillshade_layer.cpp index 411305edf4..0edc8737c1 100644 --- a/src/mbgl/renderer/layers/render_hillshade_layer.cpp +++ b/src/mbgl/renderer/layers/render_hillshade_layer.cpp @@ -114,8 +114,12 @@ void RenderHillshadeLayer::render(PaintParameters& parameters, RenderSource* src matrix::translate(mat, mat, 0, -util::EXTENT, 0); for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - HillshadeBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + HillshadeBucket& bucket = *bucket_; + if (!bucket.hasData()){ continue; } diff --git a/src/mbgl/renderer/layers/render_line_layer.cpp b/src/mbgl/renderer/layers/render_line_layer.cpp index 361ad0c76b..ea51705792 100644 --- a/src/mbgl/renderer/layers/render_line_layer.cpp +++ b/src/mbgl/renderer/layers/render_line_layer.cpp @@ -58,8 +58,11 @@ void RenderLineLayer::render(PaintParameters& parameters, RenderSource*) { } for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - LineBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + LineBucket& bucket = *bucket_; auto draw = [&] (auto& program, auto&& uniformValues) { auto& programInstance = program.get(evaluated); diff --git a/src/mbgl/renderer/layers/render_raster_layer.cpp b/src/mbgl/renderer/layers/render_raster_layer.cpp index f31f53481f..13f2f5e38c 100644 --- a/src/mbgl/renderer/layers/render_raster_layer.cpp +++ b/src/mbgl/renderer/layers/render_raster_layer.cpp @@ -142,8 +142,11 @@ void RenderRasterLayer::render(PaintParameters& parameters, RenderSource* source } } else { for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - RasterBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + RasterBucket& bucket = *bucket_; if (!bucket.hasData()) continue; diff --git a/src/mbgl/renderer/layers/render_symbol_layer.cpp b/src/mbgl/renderer/layers/render_symbol_layer.cpp index 861112526b..d397ffd724 100644 --- a/src/mbgl/renderer/layers/render_symbol_layer.cpp +++ b/src/mbgl/renderer/layers/render_symbol_layer.cpp @@ -75,8 +75,11 @@ void RenderSymbolLayer::render(PaintParameters& parameters, RenderSource*) { } for (const RenderTile& tile : renderTiles) { - assert(dynamic_cast(tile.tile.getBucket(*baseImpl))); - SymbolBucket& bucket = *reinterpret_cast(tile.tile.getBucket(*baseImpl)); + auto bucket_ = dynamic_cast(tile.tile.getBucket(*baseImpl)); + if (!bucket_) { + continue; + } + SymbolBucket& bucket = *bucket_; const auto& layout = bucket.layout; diff --git a/src/mbgl/text/cross_tile_symbol_index.cpp b/src/mbgl/text/cross_tile_symbol_index.cpp index 8df26ca117..58dcda46f5 100644 --- a/src/mbgl/text/cross_tile_symbol_index.cpp +++ b/src/mbgl/text/cross_tile_symbol_index.cpp @@ -178,9 +178,12 @@ bool CrossTileSymbolIndex::addLayer(RenderSymbolLayer& symbolLayer, float lng) { continue; } - auto bucket = renderTile.tile.getBucket(*symbolLayer.baseImpl); - assert(dynamic_cast(bucket)); - SymbolBucket& symbolBucket = *reinterpret_cast(bucket); + auto bucket = dynamic_cast(renderTile.tile.getBucket(*symbolLayer.baseImpl)); + if (!bucket) { + continue; + } + SymbolBucket& symbolBucket = *bucket; + if (symbolBucket.bucketLeaderID != symbolLayer.getID()) { // Only add this layer if it's the "group leader" for the bucket continue; diff --git a/src/mbgl/text/placement.cpp b/src/mbgl/text/placement.cpp index fd0710d959..96c1873e33 100644 --- a/src/mbgl/text/placement.cpp +++ b/src/mbgl/text/placement.cpp @@ -50,12 +50,13 @@ void Placement::placeLayer(RenderSymbolLayer& symbolLayer, const mat4& projMatri } assert(dynamic_cast(&renderTile.tile)); GeometryTile& geometryTile = static_cast(renderTile.tile); - - - auto bucket = geometryTile.getBucket(*symbolLayer.baseImpl); - assert(dynamic_cast(bucket)); - SymbolBucket& symbolBucket = *reinterpret_cast(bucket); - + + auto bucket = dynamic_cast(geometryTile.getBucket(*symbolLayer.baseImpl)); + if (!bucket) { + continue; + } + SymbolBucket& symbolBucket = *bucket; + if (symbolBucket.bucketLeaderID != symbolLayer.getID()) { // Only place this layer if it's the "group leader" for the bucket continue; @@ -231,9 +232,12 @@ void Placement::updateLayerOpacities(RenderSymbolLayer& symbolLayer) { continue; } - auto bucket = renderTile.tile.getBucket(*symbolLayer.baseImpl); - assert(dynamic_cast(bucket)); - SymbolBucket& symbolBucket = *reinterpret_cast(bucket); + auto bucket = dynamic_cast(renderTile.tile.getBucket(*symbolLayer.baseImpl)); + if (!bucket) { + continue; + } + SymbolBucket& symbolBucket = *bucket; + if (symbolBucket.bucketLeaderID != symbolLayer.getID()) { // Only update opacities this layer if it's the "group leader" for the bucket continue; diff --git a/test/fixtures/map/issue12432/0-0-0.mvt b/test/fixtures/map/issue12432/0-0-0.mvt new file mode 100644 index 0000000000..a0f049ad43 Binary files /dev/null and b/test/fixtures/map/issue12432/0-0-0.mvt differ diff --git a/test/map/map.test.cpp b/test/map/map.test.cpp index 4f7bd2df7b..8e2d9cb9cd 100644 --- a/test/map/map.test.cpp +++ b/test/map/map.test.cpp @@ -653,3 +653,52 @@ TEST(Map, NoContentTiles) { 0.0015, 0.1); } + +// https://github.com/mapbox/mapbox-gl-native/issues/12432 +TEST(Map, Issue12432) { + MapTest<> test { 1, MapMode::Continuous }; + + test.fileSource.tileResponse = [&](const Resource&) { + Response result; + result.data = std::make_shared(util::read_file("test/fixtures/map/issue12432/0-0-0.mvt")); + return result; + }; + + test.map.getStyle().loadJSON(R"STYLE({ + "version": 8, + "sources": { + "mapbox": { + "type": "vector", + "tiles": ["http://example.com/{z}-{x}-{y}.vector.pbf"] + } + }, + "layers": [{ + "id": "water", + "type": "fill", + "source": "mapbox", + "source-layer": "water" + }] + })STYLE"); + + test.observer.didFinishLoadingMapCallback = [&]() { + test.map.getStyle().loadJSON(R"STYLE({ + "version": 8, + "sources": { + "mapbox": { + "type": "vector", + "tiles": ["http://example.com/{z}-{x}-{y}.vector.pbf"] + } + }, + "layers": [{ + "id": "water", + "type": "line", + "source": "mapbox", + "source-layer": "water" + }] + })STYLE"); + + test.runLoop.stop(); + }; + + test.runLoop.run(); +} -- cgit v1.2.1