[core] Don't allow style mutations to be overwritten by revalidation

cmake/test-files.cmake

@@ -13,7 +13,6 @@ set(MBGL_TEST_FILES
     test/api/custom_layer.cpp
     test/api/render_missing.cpp
     test/api/repeated_render.cpp
-    test/api/set_style.cpp
 
     # geometry
     test/geometry/binpack.cpp

include/mbgl/storage/response.hpp

@@ -32,6 +32,10 @@ class Response {
     optional<Timestamp> modified;
     optional<Timestamp> expires;
     optional<std::string> etag;
+
+    bool isFresh() const {
+        return !expires || *expires > util::now();
+    }
 };
 
 class Response::Error {

src/mbgl/map/map.cpp

@@ -67,6 +67,7 @@ class Map::Impl : public style::Observer {
 
     std::string styleURL;
     std::string styleJSON;
+    bool styleMutated = false;
 
     std::unique_ptr<AsyncRequest> styleRequest;
 
@@ -293,10 +294,21 @@ void Map::setStyleURL(const std::string& url) {
     impl->styleRequest = nullptr;
     impl->styleURL = url;
     impl->styleJSON.clear();
+    impl->styleMutated = false;
 
     impl->style = std::make_unique<Style>(impl->fileSource, impl->pixelRatio);
 
     impl->styleRequest = impl->fileSource.request(Resource::style(impl->styleURL), [this](Response res) {
+        // Once we get a fresh style, or the style is mutated, stop revalidating.
+        if (res.isFresh() || impl->styleMutated) {
+            impl->styleRequest.reset();
+        }
+
+        // Don't allow a loaded, mutated style to be overwritten with a new version.
+        if (impl->styleMutated && impl->style->loaded) {
+            return;
+        }
+
         if (res.error) {
             if (res.error->reason == Response::Error::Reason::NotFound &&
                 util::mapbox::isMapboxURL(impl->styleURL)) {
@@ -323,6 +335,8 @@ void Map::setStyleJSON(const std::string& json) {
 
     impl->styleURL.clear();
     impl->styleJSON.clear();
+    impl->styleMutated = false;
+
     impl->style = std::make_unique<Style>(impl->fileSource, impl->pixelRatio);
 
     impl->loadStyleJSON(json);
@@ -746,22 +760,27 @@ AnnotationIDs Map::queryPointAnnotations(const ScreenBox& box) {
 #pragma mark - Style API
 
 style::Source* Map::getSource(const std::string& sourceID) {
+    impl->styleMutated = true;
     return impl->style ? impl->style->getSource(sourceID) : nullptr;
 }
 
 void Map::addSource(std::unique_ptr<style::Source> source) {
+    impl->styleMutated = true;
     impl->style->addSource(std::move(source));
 }
 
 void Map::removeSource(const std::string& sourceID) {
+    impl->styleMutated = true;
     impl->style->removeSource(sourceID);
 }
 
 style::Layer* Map::getLayer(const std::string& layerID) {
+    impl->styleMutated = true;
     return impl->style ? impl->style->getLayer(layerID) : nullptr;
 }
 
 void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& before) {
+    impl->styleMutated = true;
     impl->view.activate();
 
     impl->style->addLayer(std::move(layer), before);
@@ -772,6 +791,7 @@ void Map::addLayer(std::unique_ptr<Layer> layer, const optional<std::string>& be
 }
 
 void Map::removeLayer(const std::string& id) {
+    impl->styleMutated = true;
     impl->view.activate();
 
     impl->style->removeLayer(id);

test/map/map.cpp

@@ -1,5 +1,7 @@
 #include <mbgl/test/util.hpp>
 #include <mbgl/test/stub_file_source.hpp>
+#include <mbgl/test/fake_file_source.hpp>
+#include <mbgl/test/fixture_log_observer.hpp>
 
 #include <mbgl/map/map.hpp>
 #include <mbgl/platform/default/headless_view.hpp>
@@ -53,6 +55,24 @@ TEST(Map, Offline) {
     NetworkStatus::Set(NetworkStatus::Status::Online);
 }
 
+TEST(Map, SetStyleInvalidJSON) {
+    MapTest test;
+
+    Log::setObserver(std::make_unique<FixtureLogObserver>());
+
+    {
+        Map map(test.view, test.fileSource, MapMode::Still);
+        map.setStyleJSON("invalid");
+    }
+
+    auto observer = Log::removeObserver();
+    auto flo = dynamic_cast<FixtureLogObserver*>(observer.get());
+    EXPECT_EQ(1u, flo->count({ EventSeverity::Error, Event::ParseStyle, -1,
+        "Error parsing style JSON at 0: Invalid value." }));
+    auto unchecked = flo->unchecked();
+    EXPECT_TRUE(unchecked.empty()) << unchecked;
+}
+
 TEST(Map, DoubleStyleLoad) {
     MapTest test;
 
@@ -61,6 +81,95 @@ TEST(Map, DoubleStyleLoad) {
     map.setStyleJSON("");
 }
 
+TEST(Map, StyleFresh) {
+    // The map should not revalidate fresh styles.
+
+    MapTest test;
+    FakeFileSource fileSource;
+
+    Map map(test.view, fileSource, MapMode::Still);
+    map.setStyleURL("mapbox://styles/test");
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    Response response;
+    response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/empty.json"));
+    response.expires = Timestamp::max();
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(0u, fileSource.requests.size());
+}
+
+TEST(Map, StyleExpired) {
+    // The map should allow expired styles to be revalidated, so long as no mutations are made.
+
+    using namespace std::chrono_literals;
+
+    MapTest test;
+    FakeFileSource fileSource;
+
+    Map map(test.view, fileSource, MapMode::Still);
+    map.setStyleURL("mapbox://styles/test");
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    Response response;
+    response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/empty.json"));
+    response.expires = util::now() - 1h;
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    map.addLayer(std::make_unique<style::BackgroundLayer>("bg"));
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(0u, fileSource.requests.size());
+    EXPECT_NE(nullptr, map.getLayer("bg"));
+}
+
+TEST(Map, StyleExpiredWithAnnotations) {
+    // Adding an annotation should not prevent revalidation of an expired style.
+
+    using namespace std::chrono_literals;
+
+    MapTest test;
+    FakeFileSource fileSource;
+
+    Map map(test.view, fileSource, MapMode::Still);
+    map.setStyleURL("mapbox://styles/test");
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    Response response;
+    response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/empty.json"));
+    response.expires = util::now() - 1h;
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    map.addAnnotation(LineAnnotation { LineString<double> {{ { 0, 0 }, { 10, 10 } }} });
+    EXPECT_EQ(1u, fileSource.requests.size());
+
+    fileSource.respond(Resource::Style, response);
+    EXPECT_EQ(1u, fileSource.requests.size());
+}
+
+TEST(Map, StyleEarlyMutation) {
+    // An early mutation should not prevent the initial style load.
+
+    MapTest test;
+    FakeFileSource fileSource;
+
+    Map map(test.view, fileSource, MapMode::Still);
+    map.setStyleURL("mapbox://styles/test");
+    map.addLayer(std::make_unique<style::BackgroundLayer>("bg"));
+
+    Response response;
+    response.data = std::make_shared<std::string>(util::read_file("test/fixtures/api/water.json"));
+    fileSource.respond(Resource::Style, response);
+
+    EXPECT_EQ(0u, fileSource.requests.size());
+    EXPECT_NE(nullptr, map.getLayer("water"));
+}
+
 TEST(Map, AddLayer) {
     MapTest test;
 

test/src/mbgl/test/fake_file_source.hpp

@@ -0,0 +1,60 @@
+#pragma once
+
+#include <mbgl/storage/file_source.hpp>
+
+#include <list>
+
+namespace mbgl {
+
+/*
+   FakeFileSource is similar to StubFileSource, but it follows a post hoc, "push" model rather than
+   a pre hoc, "pull" model. You pass it to code that makes requests, it records what requests are
+   made, then you can examine them, make assertions about them, and respond as desired.
+
+   This is particularly useful if you want to simulate multiple responses, e.g. as part of a resource
+   revalidation flow. StubFileSource allows only a single response.
+*/
+class FakeFileSource : public FileSource {
+public:
+    class FakeFileRequest : public AsyncRequest {
+    public:
+        Resource resource;
+        Callback callback;
+
+        std::list<FakeFileRequest*>& list;
+        std::list<FakeFileRequest*>::iterator link;
+
+        FakeFileRequest(Resource resource_, Callback callback_, std::list<FakeFileRequest*>& list_)
+            : resource(std::move(resource_)),
+              callback(std::move(callback_)),
+              list(list_),
+              link((list.push_back(this), std::prev(list.end()))) {
+        }
+
+        ~FakeFileRequest() override {
+            list.erase(link);
+        }
+    };
+
+    std::unique_ptr<AsyncRequest> request(const Resource& resource, Callback callback) override {
+        return std::make_unique<FakeFileRequest>(resource, callback, requests);
+    }
+
+    bool respond(Resource::Kind kind, const Response& response) {
+        auto it = std::find_if(requests.begin(), requests.end(), [&] (FakeFileRequest* request) {
+            return request->resource.kind == kind;
+        });
+
+        if (it != requests.end()) {
+            // Copy the callback, in case calling it deallocates the AsyncRequest.
+            Callback callback_ = (*it)->callback;
+            callback_(response);
+        }
+
+        return it != requests.end();
+    }
+
+    std::list<FakeFileRequest*> requests;
+};
+
+} // namespace mbgl

test/src/mbgl/test/stub_file_source.cpp

@@ -24,11 +24,19 @@ StubFileSource::StubFileSource() {
         for (auto& pair : pending_) {
             optional<Response> res = std::get<1>(pair.second)(std::get<0>(pair.second));
             if (res) {
-                std::get<2>(pair.second)(*res);
-
+                // This must be before calling the callback, because it's possible that the callback
+                // could:
+                //
+                //   1. Deallocate the AsyncRequest itself, thus removing it from pending
+                //   2. Allocate a new AsyncRequest at the same memory location
+                //
+                // If remove(pair.first) was called after both those things happened, it would
+                // remove the newly allocated request rather than the intended request.
                 if (!res->error) {
                     remove(pair.first);
                 }
+
+                std::get<2>(pair.second)(*res);
             }
         }
     });