Check for duplicate layer IDs

include/mbgl/map/backend.hpp

@@ -10,6 +10,8 @@ namespace gl {
 class Context;
 } // namespace gl
 
+class BackendScope;
+
 class Backend {
 public:
     Backend();
@@ -18,6 +20,14 @@ class Backend {
     // Returns the backend's context which manages OpenGL state.
     gl::Context& getContext();
 
+    // Called when the map needs to be rendered; the backend should call Map::render() at some point
+    // in the near future. (Not called for Map::renderStill() mode.)
+    virtual void invalidate() = 0;
+
+    // Notifies a watcher of map x/y/scale/rotation changes.
+    virtual void notifyMapChange(MapChange change);
+
+protected:
     // Called when the backend's GL context needs to be made active or inactive. These are called,
     // as a matched pair, in four situations:
     //
@@ -31,15 +41,24 @@ class Backend {
     virtual void activate() = 0;
     virtual void deactivate() = 0;
 
-    // Called when the map needs to be rendered; the backend should call Map::render() at some point
-    // in the near future. (Not called for Map::renderStill() mode.)
-    virtual void invalidate() = 0;
+private:
+    const std::unique_ptr<gl::Context> context;
 
-    // Notifies a watcher of map x/y/scale/rotation changes.
-    virtual void notifyMapChange(MapChange change);
+    friend class BackendScope;
+};
+
+class BackendScope {
+public:
+    BackendScope(Backend& backend_) : backend(backend_) {
+        backend.activate();
+    }
+
+    ~BackendScope() {
+        backend.deactivate();
+    }
 
 private:
-    const std::unique_ptr<gl::Context> context;
+    Backend& backend;
 };
 
 } // namespace mbgl

platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/RuntimeStyleTests.java

@@ -9,6 +9,7 @@
 import android.view.View;
 
 import com.mapbox.mapboxsdk.maps.MapboxMap;
+import com.mapbox.mapboxsdk.style.layers.CannotAddLayerException;
 import com.mapbox.mapboxsdk.style.layers.FillLayer;
 import com.mapbox.mapboxsdk.style.layers.NoSuchLayerException;
 import com.mapbox.mapboxsdk.style.layers.Property;
@@ -127,6 +128,14 @@ public void perform(UiController uiController, View view) {
 
             //Ensure it's there
             Assert.assertNotNull(mapboxMap.getLayer(layer.getId()));
+
+            //Test adding a duplicate layer
+            try {
+                mapboxMap.addLayer(new FillLayer("building", "composite"));
+                fail("Should not have been allowed to add a layer with a duplicate id");
+            } catch (CannotAddLayerException e) {
+                //OK
+            }
         }
     }
 

platform/android/src/native_map_view.cpp

@@ -175,7 +175,7 @@ void NativeMapView::invalidate() {
 }
 
 void NativeMapView::render() {
-    activate();
+    BackendScope guard(*this);
 
     if (framebufferSizeChanged) {
         getContext().viewport = { 0, 0, getFramebufferSize() };
@@ -214,8 +214,6 @@ void NativeMapView::render() {
     } else {
         mbgl::Log::Info(mbgl::Event::Android, "Not swapping as we are not ready");
     }
-
-    deactivate();
 }
 
 mbgl::Map &NativeMapView::getMap() { return *map; }
@@ -410,7 +408,7 @@ void NativeMapView::createSurface(ANativeWindow *window_) {
     if (!firstTime) {
         firstTime = true;
 
-        activate();
+        BackendScope guard(*this);
 
         if (!eglMakeCurrent(display, surface, surface, context)) {
             mbgl::Log::Error(mbgl::Event::OpenGL, "eglMakeCurrent() returned error %d",
@@ -421,8 +419,6 @@ void NativeMapView::createSurface(ANativeWindow *window_) {
         mbgl::gl::InitializeExtensions([] (const char * name) {
              return reinterpret_cast<mbgl::gl::glProc>(eglGetProcAddress(name));
         });
-
-        deactivate();
     }
 }
 

platform/android/src/native_map_view.hpp

@@ -24,8 +24,6 @@ class NativeMapView : public mbgl::View, public mbgl::Backend {
     void updateViewBinding();
     void bind() override;
 
-    void activate() override;
-    void deactivate() override;
     void invalidate() override;
 
     void notifyMapChange(mbgl::MapChange) override;
@@ -54,6 +52,10 @@ class NativeMapView : public mbgl::View, public mbgl::Backend {
 
     void scheduleTakeSnapshot();
 
+protected:
+    void activate() override;
+    void deactivate() override;
+
 private:
     EGLConfig chooseConfig(const EGLConfig configs[], EGLint numConfigs);
 

platform/darwin/src/MGLStyle.mm

@@ -271,10 +271,18 @@ - (void)insertObject:(MGLStyleLayer *)styleLayer inLayersAtIndex:(NSUInteger)ind
         [NSException raise:NSRangeException
                     format:@"Cannot insert style layer at out-of-bounds index %lu.", (unsigned long)index];
     } else if (index == 0) {
-        [styleLayer addToMapView:self.mapView];
+        try {
+            [styleLayer addToMapView:self.mapView];
+        } catch (const std::runtime_error & err) {
+            [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+        }
     } else {
-        MGLStyleLayer *sibling = [self layerFromMBGLLayer:layers.at(layers.size() - index)];
-        [styleLayer addToMapView:self.mapView belowLayer:sibling];
+        try {
+            MGLStyleLayer *sibling = [self layerFromMBGLLayer:layers.at(layers.size() - index)];
+            [styleLayer addToMapView:self.mapView belowLayer:sibling];
+        } catch (std::runtime_error & err) {
+            [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+        }
     }
 }
 
@@ -350,7 +358,11 @@ - (void)addLayer:(MGLStyleLayer *)layer
          layer];
     }
     [self willChangeValueForKey:@"layers"];
-    [layer addToMapView:self.mapView];
+    try {
+        [layer addToMapView:self.mapView];
+    } catch (std::runtime_error & err) {
+        [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+    }
     [self didChangeValueForKey:@"layers"];
 }
 
@@ -375,7 +387,11 @@ - (void)insertLayer:(MGLStyleLayer *)layer belowLayer:(MGLStyleLayer *)sibling
          sibling];
     }
     [self willChangeValueForKey:@"layers"];
-    [layer addToMapView:self.mapView belowLayer:sibling];
+    try {
+        [layer addToMapView:self.mapView belowLayer:sibling];
+    } catch (std::runtime_error & err) {
+        [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+    }
     [self didChangeValueForKey:@"layers"];
 }
 
@@ -413,10 +429,18 @@ - (void)insertLayer:(MGLStyleLayer *)layer aboveLayer:(MGLStyleLayer *)sibling {
          @"Make sure sibling was obtained using -[MGLStyle layerWithIdentifier:].",
          sibling];
     } else if (index + 1 == layers.size()) {
-        [layer addToMapView:self.mapView];
+        try {
+            [layer addToMapView:self.mapView];
+        } catch (std::runtime_error & err) {
+            [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+        }
     } else {
         MGLStyleLayer *sibling = [self layerFromMBGLLayer:layers.at(index + 1)];
-        [layer addToMapView:self.mapView belowLayer:sibling];
+        try {
+            [layer addToMapView:self.mapView belowLayer:sibling];
+        } catch (std::runtime_error & err) {
+            [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+        }
     }
     [self didChangeValueForKey:@"layers"];
 }

platform/darwin/test/MGLStyleTests.mm

@@ -1,5 +1,8 @@
 #import "MGLMapView.h"
 #import "MGLStyle_Private.h"
+#if TARGET_OS_IPHONE
+#import "MGLMapView+MGLCustomStyleLayerAdditions.h"
+#endif
 
 #import "MGLGeoJSONSource.h"
 #import "MGLRasterSource.h"
@@ -165,6 +168,30 @@ - (void)testAddingLayersTwice {
     XCTAssertThrowsSpecificNamed([self.style addLayer:symbolLayer], NSException, @"MGLRedundantLayerException");
 }
 
+- (void)testAddingLayersWithDuplicateIdentifiers {
+    //Just some source
+    MGLVectorSource *source = [[MGLVectorSource alloc] initWithIdentifier:@"my-source" URL:[NSURL URLWithString:@"mapbox://mapbox.mapbox-terrain-v2"]];
+    [self.mapView.style addSource: source];
+
+    //Add initial layer
+    MGLFillStyleLayer *initial = [[MGLFillStyleLayer alloc] initWithIdentifier:@"my-layer" source:source];
+    [self.mapView.style addLayer:initial];
+
+    //Try to add the duplicate
+    XCTAssertThrowsSpecificNamed([self.mapView.style addLayer:[[MGLFillStyleLayer alloc] initWithIdentifier:@"my-layer" source:source]], NSException, @"MGLRedundantLayerIdentifierException");
+    XCTAssertThrowsSpecificNamed([self.mapView.style insertLayer:[[MGLFillStyleLayer alloc] initWithIdentifier:@"my-layer" source:source] belowLayer:initial],NSException, @"MGLRedundantLayerIdentifierException");
+    XCTAssertThrowsSpecificNamed([self.mapView.style insertLayer:[[MGLFillStyleLayer alloc] initWithIdentifier:@"my-layer" source:source] aboveLayer:initial], NSException, @"MGLRedundantLayerIdentifierException");
+    XCTAssertThrowsSpecificNamed([self.mapView.style insertLayer:[[MGLFillStyleLayer alloc] initWithIdentifier:@"my-layer" source:source] atIndex:0], NSException, @"MGLRedundantLayerIdentifierException");
+
+#if TARGET_OS_IPHONE
+    //Try to insert a duplicate custom layer
+    MGLCustomStyleLayerDrawingHandler drawingHandler = ^(CGSize size, CLLocationCoordinate2D centerCoordinate, double zoomLevel, CLLocationDirection direction, CGFloat pitch, CGFloat perspectiveSkew) {
+    };
+
+    XCTAssertThrowsSpecific([self.mapView insertCustomStyleLayerWithIdentifier:@"my-layer" preparationHandler:^{} drawingHandler:drawingHandler completionHandler: ^{} belowStyleLayerWithIdentifier:nil], NSException);
+#endif
+}
+
 - (NSString *)stringWithContentsOfStyleHeader {
     NSURL *styleHeaderURL = [[[NSBundle mgl_frameworkBundle].bundleURL
                               URLByAppendingPathComponent:@"Headers" isDirectory:YES]

platform/ios/src/MGLMapView.mm

@@ -5353,9 +5353,13 @@ - (void)insertCustomStyleLayerWithIdentifier:(NSString *)identifier preparationH
 {
     NSAssert(identifier, @"Style layer needs an identifier");
     MGLCustomStyleLayerHandlers *context = new MGLCustomStyleLayerHandlers(preparation, drawing, completion);
-    _mbglMap->addLayer(std::make_unique<mbgl::style::CustomLayer>(identifier.UTF8String, MGLPrepareCustomStyleLayer,
-                                                                  MGLDrawCustomStyleLayer, MGLFinishCustomStyleLayer, context),
-                       otherIdentifier ? mbgl::optional<std::string>(otherIdentifier.UTF8String) : mbgl::optional<std::string>());
+    try {
+        _mbglMap->addLayer(std::make_unique<mbgl::style::CustomLayer>(identifier.UTF8String, MGLPrepareCustomStyleLayer,
+                                                                      MGLDrawCustomStyleLayer, MGLFinishCustomStyleLayer, context),
+                           otherIdentifier ? mbgl::optional<std::string>(otherIdentifier.UTF8String) : mbgl::optional<std::string>());
+    } catch (std::runtime_error & err) {
+        [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+    }
 }
 
 - (void)removeCustomStyleLayerWithIdentifier:(NSString *)identifier

src/mbgl/map/map.cpp

@@ -147,7 +147,7 @@ Map::Impl::Impl(Map& map_,
 }
 
 Map::~Map() {
-    impl->backend.activate();
+    BackendScope guard(impl->backend);
 
     impl->styleRequest = nullptr;
 
@@ -156,8 +156,6 @@ Map::~Map() {
     impl->style.reset();
     impl->annotationManager.reset();
     impl->painter.reset();
-
-    impl->backend.deactivate();
 }
 
 void Map::renderStill(View& view, StillImageCallback callback) {
@@ -282,9 +280,8 @@ void Map::Impl::update() {
         backend.invalidate();
     } else if (stillImageRequest && style->isLoaded()) {
         // TODO: determine whether we need activate/deactivate
-        backend.activate();
+        BackendScope guard(backend);
         render(stillImageRequest->view);
-        backend.deactivate();
     }
 
     updateFlags = Update::Nothing;
@@ -882,12 +879,10 @@ void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& be
     }
 
     impl->styleMutated = true;
-    impl->backend.activate();
+    BackendScope guard(impl->backend);
 
     impl->style->addLayer(std::move(layer), before);
     impl->onUpdate(Update::Classes);
-
-    impl->backend.deactivate();
 }
 
 std::unique_ptr<Layer> Map::removeLayer(const std::string& id) {
@@ -896,13 +891,11 @@ std::unique_ptr<Layer> Map::removeLayer(const std::string& id) {
     }
 
     impl->styleMutated = true;
-    impl->backend.activate();
+    BackendScope guard(impl->backend);
 
     auto removedLayer = impl->style->removeLayer(id);
     impl->onUpdate(Update::Classes);
 
-    impl->backend.deactivate();
-
     return removedLayer;
 }
 
@@ -1060,9 +1053,8 @@ void Map::setSourceTileCacheSize(size_t size) {
 
 void Map::onLowMemory() {
     if (impl->painter) {
-        impl->backend.activate();
+        BackendScope guard(impl->backend);
         impl->painter->cleanup();
-        impl->backend.deactivate();
     }
     if (impl->style) {
         impl->style->onLowMemory();

src/mbgl/style/style.cpp

@@ -198,6 +198,15 @@ Layer* Style::getLayer(const std::string& id) const {
 Layer* Style::addLayer(std::unique_ptr<Layer> layer, optional<std::string> before) {
     // TODO: verify source
 
+    // Guard against duplicate layer ids
+    auto it = std::find_if(layers.begin(), layers.end(), [&](const auto& existing) {
+        return existing->getID() == layer->getID();
+    });
+
+    if (it != layers.end()) {
+        throw std::runtime_error(std::string{"Layer "} + layer->getID() + " already exists");
+    }
+
     if (SymbolLayer* symbolLayer = layer->as<SymbolLayer>()) {
         if (!symbolLayer->impl->spriteAtlas) {
             symbolLayer->impl->spriteAtlas = spriteAtlas.get();

test/style/style_layer.test.cpp

@@ -1,5 +1,7 @@
 #include <mbgl/test/util.hpp>
 #include <mbgl/test/stub_layer_observer.hpp>
+#include <mbgl/test/stub_file_source.hpp>
+#include <mbgl/style/style.hpp>
 #include <mbgl/style/layers/background_layer.hpp>
 #include <mbgl/style/layers/background_layer_impl.hpp>
 #include <mbgl/style/layers/circle_layer.hpp>
@@ -15,6 +17,10 @@
 #include <mbgl/style/layers/symbol_layer.hpp>
 #include <mbgl/style/layers/symbol_layer_impl.hpp>
 #include <mbgl/util/color.hpp>
+#include <mbgl/util/run_loop.hpp>
+#include <mbgl/util/io.hpp>
+
+#include <memory>
 
 using namespace mbgl;
 using namespace mbgl::style;
@@ -268,3 +274,25 @@ TEST(Layer, Observer) {
     layer->setLineCap(lineCap);
     EXPECT_FALSE(layoutPropertyChanged);
 }
+
+TEST(Layer, DuplicateLayer) {
+    util::RunLoop loop;
+
+    //Setup style
+    StubFileSource fileSource;
+    Style style { fileSource, 1.0 };
+    style.setJSON(util::read_file("test/fixtures/resources/style-unused-sources.json"));
+
+    //Add initial layer
+    style.addLayer(std::make_unique<LineLayer>("line", "unusedsource"));
+
+    //Try to add duplicate
+    try {
+        style.addLayer(std::make_unique<LineLayer>("line", "unusedsource"));
+        FAIL() << "Should not have been allowed to add a duplicate layer id";
+    } catch (const std::runtime_error e) {
+        //Expected
+        ASSERT_STREQ("Layer line already exists", e.what());
+    }
+}
+