Cherry pick 7257 into iOS 3.4.0 release branch

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/src/native_map_view.cpp

@@ -190,7 +190,7 @@ void NativeMapView::invalidate() {
 }
 
 void NativeMapView::render() {
-    activate();
+    BackendScope guard(*this);
 
     updateViewBinding();
     map->render(*this);
@@ -235,8 +235,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; }
@@ -421,7 +419,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",
@@ -442,8 +440,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

@@ -23,8 +23,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;
@@ -53,6 +51,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

@@ -287,10 +287,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 belowLayer:nil];
+        try {
+            [styleLayer addToMapView:self.mapView belowLayer:nil];
+        } 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()];
+        }
     }
 }
 
@@ -374,7 +382,11 @@ - (void)addLayer:(MGLStyleLayer *)layer
          layer];
     }
     [self willChangeValueForKey:@"layers"];
-    [layer addToMapView:self.mapView belowLayer:nil];
+    try {
+        [layer addToMapView:self.mapView belowLayer:nil];
+    } catch (std::runtime_error & err) {
+        [NSException raise:@"MGLRedundantLayerIdentifierException" format:@"%s", err.what()];
+    }
     [self didChangeValueForKey:@"layers"];
 }
 
@@ -399,7 +411,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"];
 }
 
@@ -437,10 +453,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 belowLayer:nil];
+        try {
+            [layer addToMapView:self.mapView belowLayer:nil];
+        } 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,6 @@
 #import "MGLMapView.h"
 #import "MGLStyle_Private.h"
+#import "MGLOpenGLStyleLayer.h"
 
 #import "MGLShapeSource.h"
 #import "MGLRasterSource.h"
@@ -170,6 +171,23 @@ - (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");
+    XCTAssertThrowsSpecificNamed([self.mapView.style insertLayer:[[MGLOpenGLStyleLayer alloc] initWithIdentifier:@"my-layer"] atIndex:0], NSException, @"MGLRedundantLayerIdentifierException");
+}
+
 - (NSString *)stringWithContentsOfStyleHeader {
     NSURL *styleHeaderURL = [[[NSBundle mgl_frameworkBundle].bundleURL
                               URLByAppendingPathComponent:@"Headers" isDirectory:YES]

src/mbgl/map/map.cpp

@@ -140,7 +140,7 @@ Map::Impl::Impl(Backend& backend_,
 }
 
 Map::~Map() {
-    impl->backend.activate();
+    BackendScope guard(impl->backend);
 
     impl->styleRequest = nullptr;
 
@@ -149,8 +149,6 @@ Map::~Map() {
     impl->style.reset();
     impl->annotationManager.reset();
     impl->painter.reset();
-
-    impl->backend.deactivate();
 }
 
 void Map::renderStill(View& view, StillImageCallback callback) {
@@ -275,9 +273,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;
@@ -848,12 +845,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) {
@@ -862,13 +857,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;
 }
 
@@ -1033,9 +1026,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());
+    }
+}
+