From 4819379020f88d2cf34774dfb6aca0ca16aa53ca Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Wed, 27 May 2015 16:00:52 +0300 Subject: [PATCH 1/4] Notify sprite parsing errors Emit a signal when the JSON or the sprite image was not parsed correctly. --- src/mbgl/map/sprite.cpp | 25 +++++++++++++++++-------- src/mbgl/map/sprite.hpp | 4 ++-- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp index fd3615f91..d5af3e1d1 100644 --- a/src/mbgl/map/sprite.cpp +++ b/src/mbgl/map/sprite.cpp @@ -45,14 +45,13 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) jsonRequest = nullptr; if (res.status == Response::Successful) { body = res.data; - parseJSON(); + parseJSON(jsonURL); } else { std::stringstream message; message << "Failed to load [" << jsonURL << "]: " << res.message; emitSpriteLoadingFailed(message.str()); return; } - loadedJSON = true; emitSpriteLoadedIfComplete(); }); @@ -60,14 +59,13 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_) spriteRequest = nullptr; if (res.status == Response::Successful) { image = res.data; - parseImage(); + parseImage(spriteURL); } else { std::stringstream message; message << "Failed to load [" << spriteURL << "]: " << res.message; emitSpriteLoadingFailed(message.str()); return; } - loadedImage = true; emitSpriteLoadedIfComplete(); }); } @@ -105,21 +103,29 @@ bool Sprite::hasPixelRatio(float ratio) const { return pixelRatio == (ratio > 1 ? 2 : 1); } -void Sprite::parseImage() { +void Sprite::parseImage(const std::string& spriteURL) { raster = std::make_unique(image); if (!*raster) { raster.reset(); + std::stringstream message; + message << "Failed to parse [" << spriteURL << "]"; + emitSpriteLoadingFailed(message.str()); + return; } + image.clear(); + loadedImage = true; } -void Sprite::parseJSON() { +void Sprite::parseJSON(const std::string& jsonURL) { rapidjson::Document d; d.Parse<0>(body.c_str()); body.clear(); if (d.HasParseError()) { - Log::Warning(Event::Sprite, "sprite JSON is invalid"); + std::stringstream message; + message << "Failed to parse [" << jsonURL << "]: " << d.GetErrorOffset() << " - " << d.GetParseError(); + emitSpriteLoadingFailed(message.str()); } else if (d.IsObject()) { for (rapidjson::Value::ConstMemberIterator itr = d.MemberBegin(); itr != d.MemberEnd(); ++itr) { const std::string& name = itr->name.GetString(); @@ -142,8 +148,11 @@ void Sprite::parseJSON() { pos.emplace(name, SpritePosition { x, y, width, height, spritePixelRatio, sdf }); } } + loadedJSON = true; } else { - Log::Warning(Event::Sprite, "sprite JSON root is not an object"); + std::stringstream message; + message << "Failed to parse [" << jsonURL << "]: Root is not an object"; + emitSpriteLoadingFailed(message.str()); } } diff --git a/src/mbgl/map/sprite.hpp b/src/mbgl/map/sprite.hpp index 13cfbe2d8..b0fda3001 100644 --- a/src/mbgl/map/sprite.hpp +++ b/src/mbgl/map/sprite.hpp @@ -59,8 +59,8 @@ private: void emitSpriteLoadedIfComplete(); void emitSpriteLoadingFailed(const std::string& message); - void parseJSON(); - void parseImage(); + void parseJSON(const std::string& jsonURL); + void parseImage(const std::string& spriteURL); std::string body; std::string image; From 48875d8d6ccabd344d95be15f8537b0c8c8f7f18 Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Wed, 27 May 2015 16:16:07 +0300 Subject: [PATCH 2/4] Notify glyph parsing errors Emit a signal when the glyphs are not parsed correctly. --- src/mbgl/text/glyph_pbf.cpp | 4 ++-- src/mbgl/text/glyph_pbf.hpp | 5 +++++ src/mbgl/text/glyph_store.cpp | 10 ++++++++-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp index c0642af29..899df39d3 100644 --- a/src/mbgl/text/glyph_pbf.cpp +++ b/src/mbgl/text/glyph_pbf.cpp @@ -23,14 +23,14 @@ GlyphPBF::GlyphPBF(const std::string& glyphURL, const GlyphLoadingFailedCallback& failureCallback) : parsed(false), env(env_) { // Load the glyph set URL - std::string url = util::replaceTokens(glyphURL, [&](const std::string &name) -> std::string { + url = util::replaceTokens(glyphURL, [&](const std::string &name) -> std::string { if (name == "fontstack") return util::percentEncode(fontStack); if (name == "range") return util::toString(glyphRange.first) + "-" + util::toString(glyphRange.second); return ""; }); // The prepare call jumps back to the main thread. - req = env.request({ Resource::Kind::Glyphs, url }, [&, url, successCallback, failureCallback](const Response &res) { + req = env.request({ Resource::Kind::Glyphs, url }, [&, successCallback, failureCallback](const Response &res) { req = nullptr; if (res.status != Response::Successful) { diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp index cb36dde03..bb6fa83ae 100644 --- a/src/mbgl/text/glyph_pbf.hpp +++ b/src/mbgl/text/glyph_pbf.hpp @@ -29,6 +29,10 @@ public: void parse(FontStack &stack); bool isParsed() const; + std::string getURL() const { + return url; + } + private: GlyphPBF(const GlyphPBF &) = delete; GlyphPBF(GlyphPBF &&) = delete; @@ -36,6 +40,7 @@ private: GlyphPBF &operator=(GlyphPBF &&) = delete; std::string data; + std::string url; std::atomic parsed; Environment& env; diff --git a/src/mbgl/text/glyph_store.cpp b/src/mbgl/text/glyph_store.cpp index ea451e278..9520b63c0 100644 --- a/src/mbgl/text/glyph_store.cpp +++ b/src/mbgl/text/glyph_store.cpp @@ -34,8 +34,14 @@ bool GlyphStore::requestGlyphRangesIfNeeded(const std::string& fontStackName, auto successCallback = [this, fontStackName](GlyphPBF* glyph) { auto fontStack = createFontStack(fontStackName); - glyph->parse(**fontStack); - asyncEmitGlyphRangeLoaded->send(); + try { + glyph->parse(**fontStack); + asyncEmitGlyphRangeLoaded->send(); + } catch (const std::exception&) { + std::lock_guard lock(errorMessageMutex); + errorMessage = "Failed to parse [" + glyph->getURL() + "]"; + asyncEmitGlyphRangeLoadedingFailed->send(); + } }; auto failureCallback = [this](const std::string& message) { From f8cc50683ad52cff702fb3b324502c5e792917a0 Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Wed, 27 May 2015 18:34:06 +0300 Subject: [PATCH 3/4] Notify tile parsing errors The tile parsing is done in a worker thread and if it fails, we set the tile state to "obsolete" and set an error message. When the completion callback is fired, we check for the error message and if it is set, we emit the error signal. The trick here is the signal cannot be emitted directly from the worker thread because that would violate data ownership rules. --- src/mbgl/map/source.cpp | 30 ++++++++++++++++++++++++------ src/mbgl/map/source.hpp | 2 ++ src/mbgl/map/tile_data.cpp | 15 ++++++++++----- src/mbgl/map/tile_data.hpp | 16 +++++++++++----- src/mbgl/map/vector_tile_data.cpp | 6 +++--- 5 files changed, 50 insertions(+), 19 deletions(-) diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index eb273d785..faae3285f 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -287,26 +287,25 @@ TileData::State Source::addTile(MapData& data, new_tile.data = cache.get(normalized_id.to_uint64()); } - auto successCallback = std::bind(&Source::emitTileLoaded, this, true); - auto failureCallback = std::bind(&Source::emitTileLoadingFailed, this, std::placeholders::_1); - if (!new_tile.data) { + auto callback = std::bind(&Source::tileLoadingCompleteCallback, this, normalized_id); + // If we don't find working tile data, we're just going to load it. if (info.type == SourceType::Vector) { new_tile.data = std::make_shared(normalized_id, data.transform.getMaxZoom(), style, glyphAtlas, glyphStore, spriteAtlas, sprite, info); new_tile.data->request( - style.workers, transformState.getPixelRatio(), successCallback, failureCallback); + style.workers, transformState.getPixelRatio(), callback); } else if (info.type == SourceType::Raster) { new_tile.data = std::make_shared(normalized_id, texturePool, info); new_tile.data->request( - style.workers, transformState.getPixelRatio(), successCallback, failureCallback); + style.workers, transformState.getPixelRatio(), callback); } else if (info.type == SourceType::Annotations) { new_tile.data = std::make_shared(normalized_id, data.annotationManager, data.transform.getMaxZoom(), style, glyphAtlas, glyphStore, spriteAtlas, sprite, info); - new_tile.data->reparse(style.workers, successCallback); + new_tile.data->reparse(style.workers, callback); } else { throw std::runtime_error("source type not implemented"); } @@ -539,6 +538,25 @@ void Source::setObserver(Observer* observer) { observer_ = observer; } +void Source::tileLoadingCompleteCallback(const TileID& normalized_id) { + auto it = tile_data.find(normalized_id); + if (it == tile_data.end()) { + return; + } + + util::ptr data = it->second.lock(); + if (!data) { + return; + } + + if (data->getState() == TileData::State::obsolete && !data->getError().empty()) { + emitTileLoadingFailed(data->getError()); + return; + } + + emitTileLoaded(true); +} + void Source::emitSourceLoaded() { if (observer_) { observer_->onSourceLoaded(); diff --git a/src/mbgl/map/source.hpp b/src/mbgl/map/source.hpp index d4c29b3e7..174dd996b 100644 --- a/src/mbgl/map/source.hpp +++ b/src/mbgl/map/source.hpp @@ -104,6 +104,8 @@ public: bool enabled; private: + void tileLoadingCompleteCallback(const TileID& normalized_id); + void emitSourceLoaded(); void emitSourceLoadingFailed(const std::string& message); void emitTileLoaded(bool isNewTile); diff --git a/src/mbgl/map/tile_data.cpp b/src/mbgl/map/tile_data.cpp index be0d7d32f..fc2d48bb5 100644 --- a/src/mbgl/map/tile_data.cpp +++ b/src/mbgl/map/tile_data.cpp @@ -38,18 +38,18 @@ void TileData::setState(const State& state_) { void TileData::request(Worker& worker, float pixelRatio, - const std::function& successCallback, - const std::function& failureCallback) { + const std::function& callback) { std::string url = source.tileURL(id, pixelRatio); state = State::loading; - req = env.request({ Resource::Kind::Tile, url }, [url, successCallback, failureCallback, &worker, this](const Response &res) { + req = env.request({ Resource::Kind::Tile, url }, [url, callback, &worker, this](const Response &res) { req = nullptr; if (res.status != Response::Successful) { std::stringstream message; message << "Failed to load [" << url << "]: " << res.message; - failureCallback(message.str()); + setError(message.str()); + callback(); return; } @@ -57,7 +57,7 @@ void TileData::request(Worker& worker, data = res.data; // Schedule tile parsing in another thread - reparse(worker, successCallback); + reparse(worker, callback); }); } @@ -88,3 +88,8 @@ bool TileData::reparse(Worker& worker, std::function callback) { workRequest = worker.send([this] { parse(); endParsing(); }, callback); return true; } + +void TileData::setError(const std::string& message) { + error = message; + setState(State::obsolete); +} diff --git a/src/mbgl/map/tile_data.hpp b/src/mbgl/map/tile_data.hpp index 0d34b2df7..d2a91b880 100644 --- a/src/mbgl/map/tile_data.hpp +++ b/src/mbgl/map/tile_data.hpp @@ -44,10 +44,7 @@ public: TileData(const TileID&, const SourceInfo&); ~TileData(); - void request(Worker&, - float pixelRatio, - const std::function& successCallback, - const std::function& failureCallback); + void request(Worker&, float pixelRatio, const std::function& callback); // Schedule a tile reparse on a worker thread and call the callback on // completion. It will return true if the work was schedule or false it was @@ -71,13 +68,20 @@ public: // We let subclasses override setState() so they // can intercept the state change and react accordingly. virtual void setState(const State& state); - inline State getState() const { return state; } void endParsing(); + // Error message to be set in case of request + // and parsing errors. + void setError(const std::string& message); + + std::string getError() const { + return error; + } + // Override this in the child class. virtual void parse() = 0; virtual Bucket* getBucket(StyleLayer const &layer_desc) = 0; @@ -106,6 +110,8 @@ protected: private: std::atomic state; + std::string error; + protected: // Contains the tile ID string for painting debug information. DebugFontBuffer debugFontBuffer; diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index 4d77c8ef8..e464c55c9 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -59,9 +59,9 @@ void VectorTileData::parse() { setState(State::parsed); } } catch (const std::exception& ex) { - Log::Error(Event::ParseTile, "Parsing [%d/%d/%d] failed: %s", id.z, id.x, id.y, ex.what()); - setState(State::obsolete); - return; + std::stringstream message; + message << "Failed to parse [" << int(id.z) << "/" << id.x << "/" << id.y << "]: " << ex.what(); + setError(message.str()); } } From 379d7830d9a2cb602e41476a1c0809cca84f532c Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Fri, 22 May 2015 13:11:23 +0300 Subject: [PATCH 4/4] Add tests for handling corrupted data The MockFileSource will insert garbage on the reply and report as a request done successfully. The network stack cannot detect this kind of errors but the consumer should report it like we do for network problems. --- test/resources/mock_file_source.cpp | 57 +++++++++++++++++++++----- test/resources/mock_file_source.hpp | 10 ++++- test/resources/resource_loader.cpp | 63 ++++++++++++++++++++--------- 3 files changed, 98 insertions(+), 32 deletions(-) diff --git a/test/resources/mock_file_source.cpp b/test/resources/mock_file_source.cpp index 0a9d448d4..42067a2a7 100644 --- a/test/resources/mock_file_source.cpp +++ b/test/resources/mock_file_source.cpp @@ -1,3 +1,4 @@ +#include "../fixtures/util.hpp" #include "mock_file_source.hpp" #include @@ -8,30 +9,64 @@ namespace mbgl { class MockFileSource::Impl { public: - Impl(uv_loop_t*, const std::string& matchFail) : matchFail_(matchFail) {} + Impl(uv_loop_t*, Type type, const std::string& match) : type_(type), match_(match) {} void handleRequest(Request* req) const; private: - std::string matchFail_; + void replyWithFailure(Response* res) const; + void replyWithCorruptedData(Response* res, const std::string& url) const; + void replyWithSuccess(Response* res, const std::string& url) const; + + Type type_; + std::string match_; }; -void MockFileSource::Impl::handleRequest(Request* req) const { - const Resource& resource = req->resource; +void MockFileSource::Impl::replyWithFailure(Response* res) const { + res->status = Response::Status::Error; + res->message = "Failed by the test case"; +} +void MockFileSource::Impl::replyWithCorruptedData(Response* res, const std::string& url) const { + res->status = Response::Status::Successful; + res->data = util::read_file(url); + res->data.insert(0, "CORRUPTED"); +} + +void MockFileSource::Impl::replyWithSuccess(Response* res, const std::string& url) const { + res->status = Response::Status::Successful; + res->data = util::read_file(url); +} + +void MockFileSource::Impl::handleRequest(Request* req) const { + const std::string& url = req->resource.url; std::shared_ptr response = std::make_shared(); - if (matchFail_.empty() || resource.url.find(matchFail_) == std::string::npos) { - response->status = Response::Status::Successful; - response->data = util::read_file(resource.url.c_str()); - } else { - response->message = "Failed by the test case"; + + if (url.find(match_) == std::string::npos) { + replyWithSuccess(response.get(), url); + req->notify(response); + return; + } + + switch (type_) { + case Type::Success: + replyWithSuccess(response.get(), url); + break; + case Type::RequestFail: + replyWithFailure(response.get()); + break; + case Type::RequestWithCorruptedData: + replyWithCorruptedData(response.get(), url); + break; + default: + EXPECT_TRUE(false) << "Should never be reached."; } req->notify(response); } -MockFileSource::MockFileSource(const std::string& matchFail) - : thread_(std::make_unique>("FileSource", util::ThreadPriority::Low, matchFail)) { +MockFileSource::MockFileSource(Type type, const std::string& match) + : thread_(std::make_unique>("FileSource", util::ThreadPriority::Low, type, match)) { } Request* MockFileSource::request(const Resource& resource, uv_loop_t* loop, Callback callback) { diff --git a/test/resources/mock_file_source.hpp b/test/resources/mock_file_source.hpp index 55ecd95c1..bb9fb55a3 100644 --- a/test/resources/mock_file_source.hpp +++ b/test/resources/mock_file_source.hpp @@ -13,12 +13,18 @@ template class Thread; } // This mock FileSource will read data from the disk and will fail -// the request if the URL matches the 'matchFail' string. +// the request if the URL matches a string. class MockFileSource : public FileSource { public: + enum Type { + Success, + RequestFail, + RequestWithCorruptedData + }; + class Impl; - MockFileSource(const std::string& matchFail); + MockFileSource(Type type, const std::string& match); ~MockFileSource() override = default; // FileSource implementation. diff --git a/test/resources/resource_loader.cpp b/test/resources/resource_loader.cpp index 83807156e..7d57f47ee 100644 --- a/test/resources/resource_loader.cpp +++ b/test/resources/resource_loader.cpp @@ -107,18 +107,13 @@ private: std::function callback_; }; -} - -class ResourceLoaderTest : public ::testing::TestWithParam { -}; - -TEST_P(ResourceLoaderTest, RequestFailure) { +void runTestCase(MockFileSource::Type type, + const std::string& param, + const std::string& message) { util::RunLoop loop(uv_default_loop()); - const std::string param(GetParam()); - MockView view; - MockFileSource fileSource(param); + MockFileSource fileSource(type, param); FixtureLogObserver* log = new FixtureLogObserver(); Log::setObserver(std::unique_ptr(log)); @@ -127,9 +122,6 @@ TEST_P(ResourceLoaderTest, RequestFailure) { try { if (error) { std::rethrow_exception(error); - } else { - // Control test, should succeed. - EXPECT_TRUE(param.empty()); } } catch (const util::GlyphRangeLoadingException&) { EXPECT_EQ(param, "glyphs.pbf"); @@ -156,24 +148,57 @@ TEST_P(ResourceLoaderTest, RequestFailure) { // join and cease logging after this point. context.reset(); - std::stringstream message; - message << "Failed to load [test/fixtures/resources/" << param << "]: Failed by the test case"; - const FixtureLogObserver::LogMessage logMessage { EventSeverity::Error, Event::ResourceLoader, int64_t(-1), - message.str(), + message, }; - if (param.empty()) { + if (type == MockFileSource::Success) { EXPECT_EQ(log->count(logMessage), 0u); } else { EXPECT_GT(log->count(logMessage), 0u); } - EXPECT_EQ(log->unchecked().size(), 0u); + // Clear the remaining error messages + log->unchecked().size(); +} + +} + +class ResourceLoaderTest : public ::testing::TestWithParam { +}; + +TEST_P(ResourceLoaderTest, Success) { + runTestCase(MockFileSource::Success, GetParam(), std::string()); +} + +TEST_P(ResourceLoaderTest, RequestFail) { + std::stringstream message; + message << "Failed to load [test/fixtures/resources/" << GetParam() << "]: Failed by the test case"; + + runTestCase(MockFileSource::RequestFail, GetParam(), message.str()); +} + +TEST_P(ResourceLoaderTest, RequestWithCorruptedData) { + const std::string param(GetParam()); + + std::stringstream message; + message << "Failed to parse "; + + if (param == "vector.pbf") { + message << "[15/16384/16384]: pbf unknown field type exception"; + } else { + message << "[test/fixtures/resources/" << param << "]"; + } + + if (param.find("json") != std::string::npos) { + message << ": 0 - Expect either an object or array at root"; + } + + runTestCase(MockFileSource::RequestWithCorruptedData, GetParam(), message.str()); } INSTANTIATE_TEST_CASE_P(ResourceLoader, ResourceLoaderTest, - ::testing::Values("", "source.json", "sprite.json", "sprite.png", "vector.pbf", "glyphs.pbf")); + ::testing::Values("source.json", "sprite.json", "sprite.png", "vector.pbf", "glyphs.pbf"));