From 04f4862f0864dc00d904a30c735ea11f2fa64a88 Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Mon, 13 May 2019 16:37:40 +0300 Subject: [core] GeometryTile and TileLoader keep strong reference to FileSource Thus we fix a potential bug: if `Renderer` outlives the `Map` it will hold a stale reference to the `FileSource` instance. --- src/mbgl/map/map_impl.cpp | 2 +- src/mbgl/renderer/tile_parameters.hpp | 4 +- src/mbgl/renderer/update_parameters.hpp | 2 +- src/mbgl/tile/geometry_tile.cpp | 2 +- src/mbgl/tile/geometry_tile.hpp | 2 +- src/mbgl/tile/tile_loader.hpp | 7 +-- src/mbgl/tile/tile_loader_impl.hpp | 6 +-- test/style/source.test.cpp | 82 ++++++++++++++++----------------- test/tile/custom_geometry_tile.test.cpp | 4 +- test/tile/geojson_tile.test.cpp | 4 +- test/tile/raster_dem_tile.test.cpp | 4 +- test/tile/raster_tile.test.cpp | 4 +- test/tile/vector_tile.test.cpp | 4 +- 13 files changed, 65 insertions(+), 62 deletions(-) diff --git a/src/mbgl/map/map_impl.cpp b/src/mbgl/map/map_impl.cpp index fa15461ff2..d7c7c00d0a 100644 --- a/src/mbgl/map/map_impl.cpp +++ b/src/mbgl/map/map_impl.cpp @@ -63,7 +63,7 @@ void Map::Impl::onUpdate() { style->impl->getSourceImpls(), style->impl->getLayerImpls(), annotationManager, - *fileSource, + fileSource, prefetchZoomDelta, bool(stillImageRequest), crossSourceCollisions diff --git a/src/mbgl/renderer/tile_parameters.hpp b/src/mbgl/renderer/tile_parameters.hpp index cbe8dfb3ba..6a2deee35c 100644 --- a/src/mbgl/renderer/tile_parameters.hpp +++ b/src/mbgl/renderer/tile_parameters.hpp @@ -2,6 +2,8 @@ #include +#include + namespace mbgl { class TransformState; @@ -15,7 +17,7 @@ public: const float pixelRatio; const MapDebugOptions debugOptions; const TransformState& transformState; - FileSource& fileSource; + std::shared_ptr fileSource; const MapMode mode; AnnotationManager& annotationManager; ImageManager& imageManager; diff --git a/src/mbgl/renderer/update_parameters.hpp b/src/mbgl/renderer/update_parameters.hpp index da6ed0801e..b7aee9b572 100644 --- a/src/mbgl/renderer/update_parameters.hpp +++ b/src/mbgl/renderer/update_parameters.hpp @@ -34,7 +34,7 @@ public: const Immutable>> layers; AnnotationManager& annotationManager; - FileSource& fileSource; + std::shared_ptr fileSource; const uint8_t prefetchZoomDelta; diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 3057712176..5469fa0e6f 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -157,7 +157,7 @@ void GeometryTile::onGlyphsAvailable(GlyphMap glyphs) { } void GeometryTile::getGlyphs(GlyphDependencies glyphDependencies) { - glyphManager.getGlyphs(*this, std::move(glyphDependencies), fileSource); + glyphManager.getGlyphs(*this, std::move(glyphDependencies), *fileSource); } void GeometryTile::onImagesAvailable(ImageMap images, ImageMap patterns, ImageVersionMap versionMap, uint64_t imageCorrelationID) { diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index fadc0aab41..5c2e8245ee 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -108,7 +108,7 @@ private: std::shared_ptr mailbox; Actor worker; - FileSource& fileSource; + std::shared_ptr fileSource; GlyphManager& glyphManager; ImageManager& imageManager; diff --git a/src/mbgl/tile/tile_loader.hpp b/src/mbgl/tile/tile_loader.hpp index 92ca74330f..65f4ceb118 100644 --- a/src/mbgl/tile/tile_loader.hpp +++ b/src/mbgl/tile/tile_loader.hpp @@ -1,6 +1,5 @@ #pragma once -#include #include #include @@ -13,8 +12,10 @@ class Tileset; class TileParameters; template -class TileLoader : private util::noncopyable { +class TileLoader { public: + TileLoader(const TileLoader&) = delete; + TileLoader& operator=(const TileLoader&) = delete; TileLoader(T&, const OverscaledTileID&, const TileParameters&, @@ -50,7 +51,7 @@ private: T& tile; TileNecessity necessity; Resource resource; - FileSource& fileSource; + std::shared_ptr fileSource; std::unique_ptr request; }; diff --git a/src/mbgl/tile/tile_loader_impl.hpp b/src/mbgl/tile/tile_loader_impl.hpp index 5835858d1a..bbe579ed00 100644 --- a/src/mbgl/tile/tile_loader_impl.hpp +++ b/src/mbgl/tile/tile_loader_impl.hpp @@ -27,7 +27,7 @@ TileLoader::TileLoader(T& tile_, Resource::LoadingMethod::CacheOnly)), fileSource(parameters.fileSource) { assert(!request); - if (fileSource.supportsCacheOnlyRequests()) { + if (fileSource->supportsCacheOnlyRequests()) { // When supported, the first request is always optional, even if the TileLoader // is marked as required. That way, we can let the first optional request continue // to load when the TileLoader is later changed from required to optional. If we @@ -52,7 +52,7 @@ void TileLoader::loadFromCache() { assert(!request); resource.loadingMethod = Resource::LoadingMethod::CacheOnly; - request = fileSource.request(resource, [this](Response res) { + request = fileSource->request(resource, [this](Response res) { request.reset(); tile.setTriedCache(); @@ -118,7 +118,7 @@ void TileLoader::loadFromNetwork() { // Instead of using Resource::LoadingMethod::All, we're first doing a CacheOnly, and then a // NetworkOnly request. resource.loadingMethod = Resource::LoadingMethod::NetworkOnly; - request = fileSource.request(resource, [this](Response res) { loadedData(res); }); + request = fileSource->request(resource, [this](Response res) { loadedData(res); }); } } // namespace mbgl diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp index 77a9440490..09c433b6a4 100644 --- a/test/style/source.test.cpp +++ b/test/style/source.test.cpp @@ -49,12 +49,12 @@ using SourceType = mbgl::style::SourceType; class SourceTest { public: util::RunLoop loop; - StubFileSource fileSource; + std::shared_ptr fileSource = std::make_shared(); StubStyleObserver styleObserver; StubRenderSourceObserver renderSourceObserver; Transform transform; TransformState transformState; - Style style { fileSource, 1 }; + Style style { *fileSource, 1 }; AnnotationManager annotationManager { style }; ImageManager imageManager; GlyphManager glyphManager; @@ -93,7 +93,7 @@ public: TEST(Source, LoadingFail) { SourceTest test; - test.fileSource.sourceResponse = [&] (const Resource& resource) { + test.fileSource->sourceResponse = [&] (const Resource& resource) { EXPECT_EQ("url", resource.url); Response response; response.error = std::make_unique( @@ -110,7 +110,7 @@ TEST(Source, LoadingFail) { VectorSource source("source", "url"); source.setObserver(&test.styleObserver); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.run(); } @@ -118,7 +118,7 @@ TEST(Source, LoadingFail) { TEST(Source, LoadingCorrupt) { SourceTest test; - test.fileSource.sourceResponse = [&] (const Resource& resource) { + test.fileSource->sourceResponse = [&] (const Resource& resource) { EXPECT_EQ("url", resource.url); Response response; response.data = std::make_unique("CORRUPTED"); @@ -133,7 +133,7 @@ TEST(Source, LoadingCorrupt) { VectorSource source("source", "url"); source.setObserver(&test.styleObserver); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.run(); } @@ -141,7 +141,7 @@ TEST(Source, LoadingCorrupt) { TEST(Source, RasterTileEmpty) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.noContent = true; return response; @@ -155,7 +155,7 @@ TEST(Source, RasterTileEmpty) { tileset.tiles = { "tiles" }; RasterSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileChanged = [&] (RenderSource& source_, const OverscaledTileID&) { EXPECT_EQ("source", source_.baseImpl->id); @@ -180,7 +180,7 @@ TEST(Source, RasterTileEmpty) { TEST(Source, RasterDEMTileEmpty) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.noContent = true; return response; @@ -194,7 +194,7 @@ TEST(Source, RasterDEMTileEmpty) { tileset.tiles = { "tiles" }; RasterDEMSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileChanged = [&] (RenderSource& source_, const OverscaledTileID&) { EXPECT_EQ("source", source_.baseImpl->id); @@ -219,7 +219,7 @@ TEST(Source, RasterDEMTileEmpty) { TEST(Source, VectorTileEmpty) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.noContent = true; return response; @@ -235,7 +235,7 @@ TEST(Source, VectorTileEmpty) { tileset.tiles = { "tiles" }; VectorSource source("source", tileset); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileChanged = [&] (RenderSource& source_, const OverscaledTileID&) { EXPECT_EQ("source", source_.baseImpl->id); @@ -260,7 +260,7 @@ TEST(Source, VectorTileEmpty) { TEST(Source, RasterTileFail) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.error = std::make_unique( Response::Error::Reason::Other, @@ -276,7 +276,7 @@ TEST(Source, RasterTileFail) { tileset.tiles = { "tiles" }; RasterSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileError = [&] (RenderSource& source_, const OverscaledTileID& tileID, std::exception_ptr error) { EXPECT_EQ(SourceType::Raster, source_.baseImpl->type); @@ -299,7 +299,7 @@ TEST(Source, RasterTileFail) { TEST(Source, RasterDEMTileFail) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.error = std::make_unique( Response::Error::Reason::Other, @@ -315,7 +315,7 @@ TEST(Source, RasterDEMTileFail) { tileset.tiles = { "tiles" }; RasterDEMSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileError = [&] (RenderSource& source_, const OverscaledTileID& tileID, std::exception_ptr error) { EXPECT_EQ(SourceType::RasterDEM, source_.baseImpl->type); @@ -338,7 +338,7 @@ TEST(Source, RasterDEMTileFail) { TEST(Source, VectorTileFail) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.error = std::make_unique( Response::Error::Reason::Other, @@ -356,7 +356,7 @@ TEST(Source, VectorTileFail) { tileset.tiles = { "tiles" }; VectorSource source("source", tileset); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileError = [&] (RenderSource& source_, const OverscaledTileID& tileID, std::exception_ptr error) { EXPECT_EQ(SourceType::Vector, source_.baseImpl->type); @@ -379,7 +379,7 @@ TEST(Source, VectorTileFail) { TEST(Source, RasterTileCorrupt) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.data = std::make_unique("CORRUPTED"); return response; @@ -393,7 +393,7 @@ TEST(Source, RasterTileCorrupt) { tileset.tiles = { "tiles" }; RasterSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileError = [&] (RenderSource& source_, const OverscaledTileID& tileID, std::exception_ptr error) { EXPECT_EQ(source_.baseImpl->type, SourceType::Raster); @@ -417,7 +417,7 @@ TEST(Source, RasterTileCorrupt) { TEST(Source, RasterDEMTileCorrupt) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.data = std::make_unique("CORRUPTED"); return response; @@ -431,7 +431,7 @@ TEST(Source, RasterDEMTileCorrupt) { tileset.tiles = { "tiles" }; RasterDEMSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileError = [&] (RenderSource& source_, const OverscaledTileID& tileID, std::exception_ptr error) { EXPECT_EQ(source_.baseImpl->type, SourceType::RasterDEM); @@ -455,7 +455,7 @@ TEST(Source, RasterDEMTileCorrupt) { TEST(Source, VectorTileCorrupt) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.data = std::make_unique("CORRUPTED"); return response; @@ -471,7 +471,7 @@ TEST(Source, VectorTileCorrupt) { tileset.tiles = { "tiles" }; VectorSource source("source", tileset); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileError = [&] (RenderSource& source_, const OverscaledTileID& tileID, std::exception_ptr error) { EXPECT_EQ(source_.baseImpl->type, SourceType::Vector); @@ -494,7 +494,7 @@ TEST(Source, VectorTileCorrupt) { TEST(Source, RasterTileCancel) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { test.end(); return optional(); }; @@ -507,7 +507,7 @@ TEST(Source, RasterTileCancel) { tileset.tiles = { "tiles" }; RasterSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileChanged = [&] (RenderSource&, const OverscaledTileID&) { FAIL() << "Should never be called"; @@ -531,7 +531,7 @@ TEST(Source, RasterTileCancel) { TEST(Source, RasterDEMTileCancel) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { test.end(); return optional(); }; @@ -544,7 +544,7 @@ TEST(Source, RasterDEMTileCancel) { tileset.tiles = { "tiles" }; RasterDEMSource source("source", tileset, 512); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileChanged = [&] (RenderSource&, const OverscaledTileID&) { FAIL() << "Should never be called"; @@ -568,7 +568,7 @@ TEST(Source, RasterDEMTileCancel) { TEST(Source, VectorTileCancel) { SourceTest test; - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { test.end(); return optional(); }; @@ -583,7 +583,7 @@ TEST(Source, VectorTileCancel) { tileset.tiles = { "tiles" }; VectorSource source("source", tileset); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); test.renderSourceObserver.tileChanged = [&] (RenderSource&, const OverscaledTileID&) { FAIL() << "Should never be called"; @@ -614,13 +614,13 @@ TEST(Source, RasterTileAttribution) { std::string mapboxOSM = ("© Mapbox " "©️ OpenStreetMap"); - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.noContent = true; return response; }; - test.fileSource.sourceResponse = [&] (const Resource& resource) { + test.fileSource->sourceResponse = [&] (const Resource& resource) { EXPECT_EQ("url", resource.url); Response response; response.data = std::make_unique(R"TILEJSON({ "tilejson": "2.1.0", "attribution": ")TILEJSON" + @@ -637,7 +637,7 @@ TEST(Source, RasterTileAttribution) { RasterSource source("source", "url", 512); source.setObserver(&test.styleObserver); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); auto renderSource = RenderSource::create(source.baseImpl); renderSource->update(source.baseImpl, @@ -658,13 +658,13 @@ TEST(Source, RasterDEMTileAttribution) { std::string mapbox = ("© Mapbox "); - test.fileSource.tileResponse = [&] (const Resource&) { + test.fileSource->tileResponse = [&] (const Resource&) { Response response; response.noContent = true; return response; }; - test.fileSource.sourceResponse = [&] (const Resource& resource) { + test.fileSource->sourceResponse = [&] (const Resource& resource) { EXPECT_EQ("url", resource.url); Response response; response.data = std::make_unique(R"TILEJSON({ "tilejson": "2.1.0", "attribution": ")TILEJSON" + @@ -680,7 +680,7 @@ TEST(Source, RasterDEMTileAttribution) { RasterDEMSource source("source", "url", 512); source.setObserver(&test.styleObserver); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); auto renderSource = RenderSource::create(source.baseImpl); renderSource->update(source.baseImpl, @@ -695,7 +695,7 @@ TEST(Source, RasterDEMTileAttribution) { TEST(Source, GeoJSonSourceUrlUpdate) { SourceTest test; - test.fileSource.sourceResponse = [&] (const Resource& resource) { + test.fileSource->sourceResponse = [&] (const Resource& resource) { EXPECT_EQ("url", resource.url); Response response; response.data = std::make_unique(R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})"); @@ -711,7 +711,7 @@ TEST(Source, GeoJSonSourceUrlUpdate) { source.setObserver(&test.styleObserver); // Load initial, so the source state will be loaded=true - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); // Schedule an update test.loop.invoke([&] () { @@ -725,7 +725,7 @@ TEST(Source, GeoJSonSourceUrlUpdate) { TEST(Source, ImageSourceImageUpdate) { SourceTest test; - test.fileSource.response = [&] (const Resource& resource) { + test.fileSource->response = [&] (const Resource& resource) { EXPECT_EQ("http://url", resource.url); Response response; response.data = std::make_unique(util::read_file("test/fixtures/image/no_profile.png")); @@ -742,7 +742,7 @@ TEST(Source, ImageSourceImageUpdate) { source.setObserver(&test.styleObserver); // Load initial, so the source state will be loaded=true - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); PremultipliedImage rgba({ 1, 1 }); rgba.data[0] = 255; rgba.data[1] = 254; @@ -761,7 +761,7 @@ TEST(Source, ImageSourceImageUpdate) { TEST(Source, CustomGeometrySourceSetTileData) { SourceTest test; CustomGeometrySource source("source", CustomGeometrySource::Options()); - source.loadDescription(test.fileSource); + source.loadDescription(*test.fileSource); LineLayer layer("id", "source"); layer.setSourceLayer("water"); diff --git a/test/tile/custom_geometry_tile.test.cpp b/test/tile/custom_geometry_tile.test.cpp index 39f6e33caf..1aed821cd6 100644 --- a/test/tile/custom_geometry_tile.test.cpp +++ b/test/tile/custom_geometry_tile.test.cpp @@ -22,10 +22,10 @@ using namespace mbgl::style; class CustomTileTest { public: - FakeFileSource fileSource; + std::shared_ptr fileSource = std::make_shared(); TransformState transformState; util::RunLoop loop; - style::Style style { fileSource, 1 }; + style::Style style { *fileSource, 1 }; AnnotationManager annotationManager { style }; ImageManager imageManager; GlyphManager glyphManager; diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp index 75e9ccbc7a..bf42278e4c 100644 --- a/test/tile/geojson_tile.test.cpp +++ b/test/tile/geojson_tile.test.cpp @@ -21,10 +21,10 @@ using namespace mbgl::style; class GeoJSONTileTest { public: - FakeFileSource fileSource; + std::shared_ptr fileSource = std::make_shared(); TransformState transformState; util::RunLoop loop; - style::Style style { fileSource, 1 }; + style::Style style { *fileSource, 1 }; AnnotationManager annotationManager { style }; ImageManager imageManager; GlyphManager glyphManager; diff --git a/test/tile/raster_dem_tile.test.cpp b/test/tile/raster_dem_tile.test.cpp index 8abd6efe12..42e7594720 100644 --- a/test/tile/raster_dem_tile.test.cpp +++ b/test/tile/raster_dem_tile.test.cpp @@ -16,10 +16,10 @@ using namespace mbgl; class RasterDEMTileTest { public: - FakeFileSource fileSource; + std::shared_ptr fileSource = std::make_shared(); TransformState transformState; util::RunLoop loop; - style::Style style { fileSource, 1 }; + style::Style style { *fileSource, 1 }; AnnotationManager annotationManager { style }; ImageManager imageManager; GlyphManager glyphManager; diff --git a/test/tile/raster_tile.test.cpp b/test/tile/raster_tile.test.cpp index 3437061fa5..f19bd26260 100644 --- a/test/tile/raster_tile.test.cpp +++ b/test/tile/raster_tile.test.cpp @@ -16,10 +16,10 @@ using namespace mbgl; class RasterTileTest { public: - FakeFileSource fileSource; + std::shared_ptr fileSource = std::make_shared(); TransformState transformState; util::RunLoop loop; - style::Style style { fileSource, 1 }; + style::Style style { *fileSource, 1 }; AnnotationManager annotationManager { style }; ImageManager imageManager; GlyphManager glyphManager; diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp index 05c415fb22..b912af1f15 100644 --- a/test/tile/vector_tile.test.cpp +++ b/test/tile/vector_tile.test.cpp @@ -22,10 +22,10 @@ using namespace mbgl; class VectorTileTest { public: - FakeFileSource fileSource; + std::shared_ptr fileSource = std::make_shared(); TransformState transformState; util::RunLoop loop; - style::Style style { fileSource, 1 }; + style::Style style { *fileSource, 1 }; AnnotationManager annotationManager { style }; ImageManager imageManager; GlyphManager glyphManager; -- cgit v1.2.1