Check source usage before removal

include/mbgl/style/layer.hpp

@@ -6,8 +6,10 @@
 #include <mbgl/style/layer_type.hpp>
 #include <mbgl/style/types.hpp>
 
+#include <cassert>
 #include <memory>
 #include <string>
+#include <stdexcept>
 
 namespace mbgl {
 namespace style {
@@ -89,6 +91,11 @@ class Layer : public mbgl::util::noncopyable {
         case LayerType::FillExtrusion:
             return std::forward<V>(visitor)(*as<FillExtrusionLayer>());
         }
+
+
+        // Not reachable, but placate GCC.
+        assert(false);
+        throw new std::runtime_error("unknown layer type");
     }
 
     LayerType getType() const;

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

@@ -12,6 +12,7 @@
 import com.mapbox.mapboxsdk.style.layers.CircleLayer;
 import com.mapbox.mapboxsdk.style.layers.FillLayer;
 import com.mapbox.mapboxsdk.style.layers.Layer;
+import com.mapbox.mapboxsdk.style.layers.LineLayer;
 import com.mapbox.mapboxsdk.style.layers.Property;
 import com.mapbox.mapboxsdk.style.layers.PropertyFactory;
 import com.mapbox.mapboxsdk.style.sources.CannotAddSourceException;
@@ -223,6 +224,23 @@ public void testGeoJsonSourceUrlGetter() throws MalformedURLException {
     assertEquals("http://mapbox.com/my-file.json", source.getUrl());
   }
 
+  @Test
+  public void testRemoveSourceInUse() {
+    validateTestSetup();
+
+    onView(withId(R.id.mapView)).perform(new BaseViewAction() {
+
+      @Override
+      public void perform(UiController uiController, View view) {
+        mapboxMap.addSource(new VectorSource("my-source", "mapbox://mapbox.mapbox-terrain-v2"));
+        mapboxMap.addLayer(new LineLayer("my-layer", "my-source"));
+        mapboxMap.removeSource("my-source");
+        assertNotNull(mapboxMap.getSource("my-source"));
+      }
+
+    });
+  }
+
   /**
    * https://github.com/mapbox/mapbox-gl-native/issues/7973
    */

platform/darwin/test/MGLStyleTests.mm

@@ -240,6 +240,22 @@ - (void)testAddingSourceOfTypeABeforeSourceOfTypeBWithSameIdentifier {
     XCTAssertTrue([[self.style sourceWithIdentifier:shapeSource.identifier] isMemberOfClass:[MGLVectorSource class]]);
 }
 
+- (void)testRemovingSourceInUse {
+    // Add a raster source
+    MGLRasterSource *rasterSource = [[MGLRasterSource alloc] initWithIdentifier:@"some-identifier" tileURLTemplates:@[] options:nil];
+    [self.style addSource:rasterSource];
+
+    // Add a layer using it
+    MGLFillStyleLayer *fillLayer = [[MGLFillStyleLayer alloc] initWithIdentifier:@"fillLayer" source:rasterSource];
+    [self.style addLayer:fillLayer];
+
+    // Attempt to remove the raster source
+    [self.style removeSource:rasterSource];
+
+    // Ensure it is still there
+    XCTAssertTrue([[self.style sourceWithIdentifier:rasterSource.identifier] isMemberOfClass:[MGLRasterSource class]]);
+}
+
 - (void)testLayers {
     NSArray<MGLStyleLayer *> *initialLayers = self.style.layers;
     if ([initialLayers.firstObject.identifier isEqualToString:@"com.mapbox.annotations.points"]) {

src/mbgl/style/style.cpp

@@ -158,7 +158,30 @@ void Style::addSource(std::unique_ptr<Source> source) {
     sources.emplace_back(std::move(source));
 }
 
+struct SourceIdUsageEvaluator {
+    const std::string& sourceId;
+
+    bool operator()(BackgroundLayer&) { return false; }
+    bool operator()(CustomLayer&) { return false; }
+
+    template <class LayerType>
+    bool operator()(LayerType& layer) {
+        return layer.getSourceID() == sourceId;
+    }
+};
+
 std::unique_ptr<Source> Style::removeSource(const std::string& id) {
+    // Check if source is in use
+    SourceIdUsageEvaluator sourceIdEvaluator {id};
+    auto layerIt = std::find_if(layers.begin(), layers.end(), [&](const auto& layer) {
+        return layer->accept(sourceIdEvaluator);
+    });
+
+    if (layerIt != layers.end()) {
+        Log::Warning(Event::General, "Source '%s' is in use, cannot remove", id.c_str());
+        return nullptr;
+    }
+
     auto it = std::find_if(sources.begin(), sources.end(), [&](const auto& source) {
         return source->getID() == id;
     });

test/style/style.test.cpp

@@ -1,10 +1,12 @@
 #include <mbgl/test/util.hpp>
 #include <mbgl/test/stub_file_source.hpp>
+#include <mbgl/test/fixture_log_observer.hpp>
 
 #include <mbgl/style/style.hpp>
 #include <mbgl/style/source_impl.hpp>
 #include <mbgl/style/sources/vector_source.hpp>
 #include <mbgl/style/layer.hpp>
+#include <mbgl/style/layers/line_layer.hpp>
 #include <mbgl/util/io.hpp>
 #include <mbgl/util/run_loop.hpp>
 #include <mbgl/util/default_thread_pool.hpp>
@@ -67,3 +69,33 @@ TEST(Style, DuplicateSource) {
         // Expected
     }
 }
+
+TEST(Style, RemoveSourceInUse) {
+    util::RunLoop loop;
+
+    auto log = new FixtureLogObserver();
+    Log::setObserver(std::unique_ptr<Log::Observer>(log));
+
+    ThreadPool threadPool{ 1 };
+    StubFileSource fileSource;
+    Style style { threadPool, fileSource, 1.0 };
+
+    style.setJSON(util::read_file("test/fixtures/resources/style-unused-sources.json"));
+
+    style.addSource(std::make_unique<VectorSource>("sourceId", "mapbox://mapbox.mapbox-terrain-v2"));
+    style.addLayer(std::make_unique<LineLayer>("layerId", "sourceId"));
+
+    // Should not remove the source
+    auto removed = style.removeSource("sourceId");
+    ASSERT_EQ(nullptr, removed);
+    ASSERT_NE(nullptr, style.getSource("sourceId"));
+
+    const FixtureLogObserver::LogMessage logMessage {
+            EventSeverity::Warning,
+            Event::General,
+            int64_t(-1),
+            "Source 'sourceId' is in use, cannot remove",
+    };
+
+    EXPECT_EQ(log->count(logMessage), 1u);
+}