diff --git a/CMakeLists.txt b/CMakeLists.txt index 743ca6458..74781bd83 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -441,6 +441,7 @@ list(APPEND INCLUDE_FILES ${PROJECT_SOURCE_DIR}/include/mbgl/util/size.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/string_indexer.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/string.hpp + ${PROJECT_SOURCE_DIR}/include/mbgl/util/symbol_error_observer.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/thread.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/tile_server_options.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/tileset.hpp @@ -929,6 +930,8 @@ list(APPEND SRC_FILES ${PROJECT_SOURCE_DIR}/src/mbgl/util/color$,.rs.cpp,.cpp> ${PROJECT_SOURCE_DIR}/src/mbgl/util/constants.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/convert.cpp + ${PROJECT_SOURCE_DIR}/src/mbgl/util/error_sink.cpp + ${PROJECT_SOURCE_DIR}/src/mbgl/util/error_sink.hpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/event.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/filesystem.hpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/font_stack.cpp diff --git a/bazel/core.bzl b/bazel/core.bzl index 744c50c1a..f4fff2a02 100644 --- a/bazel/core.bzl +++ b/bazel/core.bzl @@ -557,6 +557,8 @@ MLN_CORE_SOURCE = [ "src/mbgl/util/client_options.cpp", "src/mbgl/util/constants.cpp", "src/mbgl/util/convert.cpp", + "src/mbgl/util/error_sink.hpp", + "src/mbgl/util/error_sink.cpp", "src/mbgl/util/event.cpp", "src/mbgl/util/filesystem.hpp", "src/mbgl/util/font_stack.cpp", @@ -855,6 +857,7 @@ MLN_CORE_HEADERS = [ "include/mbgl/util/scoped.hpp", "include/mbgl/util/size.hpp", "include/mbgl/util/string.hpp", + "include/mbgl/util/symbol_error_observer.hpp", "include/mbgl/util/string_indexer.hpp", "include/mbgl/util/tile_server_options.hpp", "include/mbgl/util/thread.hpp", diff --git a/include/mbgl/map/map_observer.hpp b/include/mbgl/map/map_observer.hpp index c55b465f5..e039020ea 100644 --- a/include/mbgl/map/map_observer.hpp +++ b/include/mbgl/map/map_observer.hpp @@ -92,6 +92,10 @@ public: // Renderer virtual void onRenderError(std::exception_ptr) {} + + /// Called with a description of a corrupted symbol detected by the symbol guard checks + /// (see `MLN_SYMBOL_GUARDS`), on the thread which detected it. + virtual void onSymbolError(const std::string&) {} }; } // namespace mbgl diff --git a/include/mbgl/renderer/renderer_observer.hpp b/include/mbgl/renderer/renderer_observer.hpp index fad2a7bf5..9d99c3287 100644 --- a/include/mbgl/renderer/renderer_observer.hpp +++ b/include/mbgl/renderer/renderer_observer.hpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -19,9 +20,9 @@ namespace gfx { class ShaderRegistry; } -class RendererObserver { +class RendererObserver : public SymbolErrorObserver { public: - virtual ~RendererObserver() = default; + ~RendererObserver() override = default; enum class RenderMode : uint32_t { Partial, diff --git a/include/mbgl/util/symbol_error_observer.hpp b/include/mbgl/util/symbol_error_observer.hpp new file mode 100644 index 000000000..d6268c41f --- /dev/null +++ b/include/mbgl/util/symbol_error_observer.hpp @@ -0,0 +1,19 @@ +#pragma once + +#include + +namespace mbgl { + +/// Receives errors detected by the symbol guard checks (see `MLN_SYMBOL_GUARDS`). +/// Both `TileObserver` and `RendererObserver` derive from this, so that whichever of them owns +/// the thread a check ran on can be installed as that thread's error destination, see `ErrorSink`. +class SymbolErrorObserver { +public: + virtual ~SymbolErrorObserver() = default; + + /// Called with a description of a single corrupted item, on the thread which detected it. + /// Several calls may be made for the same bucket if it holds more than one corrupted item. + virtual void onSymbolError(const std::string&) {} +}; + +} // namespace mbgl diff --git a/platform/android/MapLibreAndroid/src/cpp/android_renderer_frontend.cpp b/platform/android/MapLibreAndroid/src/cpp/android_renderer_frontend.cpp index 9f1bea299..58f4e1319 100644 --- a/platform/android/MapLibreAndroid/src/cpp/android_renderer_frontend.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/android_renderer_frontend.cpp @@ -54,6 +54,10 @@ public: delegate.invoke(&RendererObserver::onRemoveUnusedStyleImages, ids); } + void onSymbolError(const std::string& message) override { + delegate.invoke(&RendererObserver::onSymbolError, message); + } + void onPreCompileShader(mbgl::shaders::BuiltIn id, mbgl::gfx::Backend::Type type, const std::string& additionalDefines) override { diff --git a/platform/android/MapLibreAndroid/src/cpp/native_map_view.cpp b/platform/android/MapLibreAndroid/src/cpp/native_map_view.cpp index f24ee5e0c..fed861621 100644 --- a/platform/android/MapLibreAndroid/src/cpp/native_map_view.cpp +++ b/platform/android/MapLibreAndroid/src/cpp/native_map_view.cpp @@ -1737,5 +1737,17 @@ void NativeMapView::onRenderError(std::exception_ptr) { } } +void NativeMapView::onSymbolError(const std::string& message) { + assert(vm != nullptr); + + android::UniqueEnv _env = android::AttachEnv(); + static auto& javaClass = jni::Class::Singleton(*_env); + static auto onSymbolError = javaClass.GetMethod(*_env, "onSymbolError"); + auto weakReference = javaPeer.get(*_env); + if (weakReference) { + weakReference.Call(*_env, onSymbolError, jni::Make(*_env, message)); + } +} + } // namespace android } // namespace mbgl diff --git a/platform/android/MapLibreAndroid/src/cpp/native_map_view.hpp b/platform/android/MapLibreAndroid/src/cpp/native_map_view.hpp index 652589304..33a332869 100644 --- a/platform/android/MapLibreAndroid/src/cpp/native_map_view.hpp +++ b/platform/android/MapLibreAndroid/src/cpp/native_map_view.hpp @@ -367,6 +367,8 @@ public: // Renderer void onRenderError(std::exception_ptr) override; + void onSymbolError(const std::string&) override; + private: std::shared_ptr rendererFrontend; diff --git a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapChangeReceiver.java b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapChangeReceiver.java index 15d667a7e..62c4b0174 100644 --- a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapChangeReceiver.java +++ b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapChangeReceiver.java @@ -60,6 +60,8 @@ class MapChangeReceiver implements NativeMapView.StateCallback { = new CopyOnWriteArrayList<>(); private final List onRenderErrorList = new CopyOnWriteArrayList<>(); + private final List onSymbolErrorList + = new CopyOnWriteArrayList<>(); @Override public void onCameraWillChange(boolean animated) { @@ -440,6 +442,17 @@ class MapChangeReceiver implements NativeMapView.StateCallback { } } + @Override + public void onSymbolError(String message) { + try { + for (MapView.OnSymbolErrorListener listener : onSymbolErrorList) { + listener.onSymbolError(message); + } + } catch (Throwable err) { + Logger.e(TAG, "Exception in onSymbolError", err); + } + } + void addOnCameraWillChangeListener(MapView.OnCameraWillChangeListener listener) { onCameraWillChangeListenerList.add(listener); } @@ -612,6 +625,10 @@ class MapChangeReceiver implements NativeMapView.StateCallback { onRenderErrorList.add(callback); } + public void addOnSymbolErrorListener(MapView.OnSymbolErrorListener callback) { + onSymbolErrorList.add(callback); + } + public void removeOnPreCompileShaderListener(MapView.OnPreCompileShaderListener callback) { onPreCompileShaderList.remove(callback); } @@ -656,6 +673,10 @@ class MapChangeReceiver implements NativeMapView.StateCallback { onRenderErrorList.remove(callback); } + public void removeOnSymbolErrorListener(MapView.OnSymbolErrorListener callback) { + onSymbolErrorList.remove(callback); + } + void clear() { onCameraWillChangeListenerList.clear(); onCameraIsChangingListenerList.clear(); @@ -683,5 +704,6 @@ class MapChangeReceiver implements NativeMapView.StateCallback { onSpriteErrorList.clear(); onSpriteRequestedList.clear(); onRenderErrorList.clear(); + onSymbolErrorList.clear(); } } diff --git a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapView.java b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapView.java index 77f959f6a..283428d90 100644 --- a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapView.java +++ b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/MapView.java @@ -1121,6 +1121,19 @@ public class MapView extends FrameLayout implements NativeMapView.ViewCallback { mapChangeReceiver.removeOnRenderErrorListener(callback); } + /** + * Set a callback that's invoked after a corrupted symbol is detected + * + * @param listener The callback that's invoked + */ + public void addOnSymbolErrorListener(MapView.OnSymbolErrorListener callback) { + mapChangeReceiver.addOnSymbolErrorListener(callback); + } + + public void removeOnSymbolErrorListener(MapView.OnSymbolErrorListener callback) { + mapChangeReceiver.removeOnSymbolErrorListener(callback); + } + /** * Interface definition for a callback to be invoked when the camera will change. *

@@ -1549,6 +1562,19 @@ public class MapView extends FrameLayout implements NativeMapView.ViewCallback { void onRenderError(); } + /** + * Interface definition for a callback to be invoked after a corrupted symbol is detected + *

+ * {@link MapView#addOnSymbolErrorListener(OnSymbolErrorListener)} + *

+ */ + public interface OnSymbolErrorListener { + /** + * Called when a corrupted symbol is detected. + */ + void onSymbolError(@NonNull String message); + } + /** * Sets a callback object which will be triggered when the {@link MapLibreMap} instance is ready to be used. * diff --git a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/NativeMapView.java b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/NativeMapView.java index 735f475be..55d3228e8 100755 --- a/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/NativeMapView.java +++ b/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/NativeMapView.java @@ -1430,6 +1430,13 @@ final class NativeMapView implements NativeMap { } } + @Keep + private void onSymbolError(String message) { + if (stateCallback != null) { + stateCallback.onSymbolError(message); + } + } + // // JNI methods // @@ -1938,5 +1945,7 @@ final class NativeMapView implements NativeMap { void onSpriteRequested(String id, String url); void onRenderError(); + + void onSymbolError(String message); } } diff --git a/platform/android/MapLibreAndroidTestApp/build.gradle.kts b/platform/android/MapLibreAndroidTestApp/build.gradle.kts index 402ed28e7..309536196 100644 --- a/platform/android/MapLibreAndroidTestApp/build.gradle.kts +++ b/platform/android/MapLibreAndroidTestApp/build.gradle.kts @@ -19,12 +19,12 @@ fun obtainTestBuildType(): String { android { ndkVersion = Versions.ndkVersion - compileSdk = 34 + compileSdk = 35 defaultConfig { applicationId = "org.maplibre.android.testapp" minSdk = 23 - targetSdk = 33 + targetSdk = 35 versionCode = 14 testInstrumentationRunner = "org.maplibre.android.InstrumentationRunner" multiDexEnabled = true @@ -155,6 +155,8 @@ dependencies { implementation(libs.maplibreJavaTurf) + implementation(libs.sentryAndroid) + implementation(libs.supportRecyclerView) implementation(libs.supportPrint) implementation(libs.supportDesign) diff --git a/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/activity/SequentialActivityTest.kt b/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/activity/SequentialActivityTest.kt index b0bef694d..f34236f93 100644 --- a/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/activity/SequentialActivityTest.kt +++ b/platform/android/MapLibreAndroidTestApp/src/androidTest/java/org/maplibre/android/testapp/activity/SequentialActivityTest.kt @@ -62,7 +62,7 @@ class SequentialActivityTest(private val activity: Class) { PackageManager.GET_ACTIVITIES ) - val activities = packageInfo.activities + val activities = packageInfo.activities!! .filter { info -> info.name.startsWith("org.maplibre.android.testapp.activity") } diff --git a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/FeatureOverviewActivity.kt b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/FeatureOverviewActivity.kt index 5241b024c..cd02b40ae 100644 --- a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/FeatureOverviewActivity.kt +++ b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/FeatureOverviewActivity.kt @@ -131,7 +131,7 @@ class FeatureOverviewActivity : AppCompatActivity() { val packageName = FeatureOverviewActivity::class.java.`package`!!.name val metaDataKey = getString(R.string.category) if (app != null) { - for (info in app.activities) { + for (info in app.activities!!) { if (info.labelRes != 0 && info.name.startsWith(packageName) && info.name != FeatureOverviewActivity::class.java.name ) { diff --git a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/events/ObserverActivity.kt b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/events/ObserverActivity.kt index 720c05cc3..750ed34eb 100644 --- a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/events/ObserverActivity.kt +++ b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/events/ObserverActivity.kt @@ -27,7 +27,8 @@ class ObserverActivity : AppCompatActivity(), MapView.OnGlyphsRequestedListener, MapView.OnSpriteLoadedListener, MapView.OnSpriteRequestedListener, - MapView.OnDidFinishRenderingFrameWithStatsListener { + MapView.OnDidFinishRenderingFrameWithStatsListener, + MapView.OnSymbolErrorListener { // # --8<-- [end:ObserverActivity] private lateinit var mapView: MapView @@ -52,6 +53,7 @@ class ObserverActivity : AppCompatActivity(), mapView.addOnSpriteLoadedListener(this) mapView.addOnSpriteRequestedListener(this) mapView.addOnDidFinishRenderingFrameListener(this) + mapView.addOnSymbolErrorListener(this) // # --8<-- [end:addListeners] // # --8<-- [start:renderStatsTracker] @@ -158,6 +160,10 @@ class ObserverActivity : AppCompatActivity(), override fun onDidFinishRenderingFrame(fully: Boolean, stats: RenderingStats) { renderStatsTracker.addFrame(stats) } + + override fun onSymbolError(message: String) { + Logger.i(TAG, "onSymbolError: $message") + } // # --8<-- [end:mapEvents] override fun onStart() { diff --git a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/NavigationMap.kt b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/NavigationMap.kt index 577e68c59..c83c548fa 100644 --- a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/NavigationMap.kt +++ b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/NavigationMap.kt @@ -3,7 +3,6 @@ package org.maplibre.android.testapp.activity.stability import android.annotation.SuppressLint import android.location.Location import android.os.Bundle -import android.os.PersistableBundle import androidx.appcompat.app.AppCompatActivity import androidx.lifecycle.lifecycleScope import com.google.gson.Gson @@ -45,8 +44,9 @@ import org.maplibre.navigation.android.navigation.v5.routeprogress.RouteProgress import java.util.Locale import java.util.logging.Logger import kotlin.random.Random +import io.sentry.Sentry -class NavigationMap : SupportMapFragment(), ProgressChangeListener, MilestoneEventListener { +class NavigationMap : SupportMapFragment(), ProgressChangeListener, MilestoneEventListener, MapView.OnSymbolErrorListener { private lateinit var map: MapLibreMap private lateinit var mapView: MapView @@ -105,6 +105,7 @@ class NavigationMap : SupportMapFragment(), ProgressChangeListener, MilestoneEve this.map = maplibreMap this.mapView = view as MapView + this.mapView.addOnSymbolErrorListener(this) LOG.info("NavigationMap seed $RANDOM_SEED") run() @@ -357,6 +358,11 @@ class NavigationMap : SupportMapFragment(), ProgressChangeListener, MilestoneEve startNewRoute() } } + + override fun onSymbolError(message: String) { + Sentry.captureMessage("NavigationMap: $message") + //Bugsnag.notify(RuntimeException(message)) + } } class NavigationMapActivity : AppCompatActivity() { diff --git a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/UserMap.kt b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/UserMap.kt index 25886a02a..211dd7ec3 100644 --- a/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/UserMap.kt +++ b/platform/android/MapLibreAndroidTestApp/src/main/java/org/maplibre/android/testapp/activity/stability/UserMap.kt @@ -27,8 +27,9 @@ import java.lang.reflect.Field import java.util.logging.Logger import kotlin.math.min import kotlin.random.Random +import io.sentry.Sentry -class UserMap : SupportMapFragment() { +class UserMap : SupportMapFragment(), MapView.OnSymbolErrorListener { private lateinit var map: MapLibreMap private lateinit var mapView: MapView private lateinit var bitmapDrawables: List @@ -140,6 +141,7 @@ class UserMap : SupportMapFragment() { this.map = maplibreMap this.mapView = view as MapView + this.mapView.addOnSymbolErrorListener(this) LOG.info("UserMap seed $RANDOM_SEED") run() @@ -267,6 +269,11 @@ class UserMap : SupportMapFragment() { ) } } + + override fun onSymbolError(message: String) { + Sentry.captureMessage("UserMap: $message") + //Bugsnag.notify(RuntimeException(message)) + } } class UserMapActivity : AppCompatActivity() { diff --git a/platform/android/gradle/libs.versions.toml b/platform/android/gradle/libs.versions.toml index 45a919c7c..016308907 100644 --- a/platform/android/gradle/libs.versions.toml +++ b/platform/android/gradle/libs.versions.toml @@ -4,6 +4,7 @@ constraintLayout = "2.2.1" interpolator = "1.0.0" recyclerView = "1.3.0" material = "1.13.0" +sentryAndroid = "8.51.0" uiAutomator = "2.3.0" espresso = "3.7.0" testRunner = "1.3.0" @@ -41,6 +42,7 @@ mockk = { group = "io.mockk", name = "mockk", version.ref = "mockk" } robolectric = { group = "org.robolectric", name = "robolectric", version.ref = "robolectric" } assertjcore = { group = "org.assertj", name = "assertj-core", version.ref = "assertj" } +sentryAndroid = { module = "io.sentry:sentry-android", version.ref = "sentryAndroid" } testRunner = { group = "androidx.test.ext", name = "junit", version.ref = "testRunner" } testRules = { group = "androidx.test", name = "rules", version.ref = "testRules" } testEspressoCore = { group = "androidx.test.espresso", name = "espresso-core", version.ref = "espresso" } diff --git a/platform/default/src/mbgl/map/map_snapshotter.cpp b/platform/default/src/mbgl/map/map_snapshotter.cpp index a87c60080..74130bccc 100644 --- a/platform/default/src/mbgl/map/map_snapshotter.cpp +++ b/platform/default/src/mbgl/map/map_snapshotter.cpp @@ -49,6 +49,10 @@ public: delegate.invoke(&RendererObserver::onStyleImageMissing, image, cb); } + void onSymbolError(const std::string& message) override { + delegate.invoke(&RendererObserver::onSymbolError, message); + } + private: std::shared_ptr mailbox; ActorRef delegate; @@ -108,6 +112,8 @@ public: rendererObserver->onStyleImageMissing(id, done); } + void onSymbolError(const std::string& message) override { rendererObserver->onSymbolError(message); } + void setObserver(std::shared_ptr observer) { assert(observer); rendererObserver = std::move(observer); diff --git a/src/mbgl/layout/symbol_instance.cpp b/src/mbgl/layout/symbol_instance.cpp index 98ae74def..8b3bec891 100644 --- a/src/mbgl/layout/symbol_instance.cpp +++ b/src/mbgl/layout/symbol_instance.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -265,21 +266,29 @@ inline std::string locationSuffix(const source_location& source) { util::toString(source.line()) + ")"; } } // namespace + +void SymbolInstance::fail(const std::string& message) const { + isFailed = true; + Log::Error(Event::Crash, message); + + // Report as soon as the failure is detected, on the thread which detected it. The destination + // is the observer owning that thread, installed with an `ErrorScope`, see `SymbolErrorObserver`. + ErrorSink::report(message); + + assert(false); +} + bool SymbolInstance::check(std::uint64_t v, int n, const source_location& source) const { if (!isFailed && v != checkVal) { - isFailed = true; - Log::Error(Event::Crash, - "SymbolInstance corrupted at " + util::toString(n) + " with value " + util::toString(v) + - locationSuffix(source)); + fail("SymbolInstance corrupted at " + util::toString(n) + " with value " + util::toString(v) + + locationSuffix(source)); } return !isFailed; } bool SymbolInstance::checkKey(const source_location& source) const { if (!isFailed && key.size() > 10000) { // largest observed value=62 - isFailed = true; - Log::Error(Event::Crash, - "SymbolInstance key corrupted with size=" + util::toString(key.size()) + locationSuffix(source)); + fail("SymbolInstance key corrupted with size=" + util::toString(key.size()) + locationSuffix(source)); } return !isFailed; } @@ -288,16 +297,16 @@ bool SymbolInstance::checkIndex(const std::optional& index, std::size_t size, const source_location& source) const { if (index.has_value() && *index >= size) { - isFailed = true; - Log::Error(Event::Crash, - "SymbolInstance index corrupted with value=" + util::toString(*index) + - " size=" + util::toString(size) + locationSuffix(source)); + fail("SymbolInstance index corrupted with value=" + util::toString(*index) + " size=" + util::toString(size) + + locationSuffix(source)); } return !isFailed; } void SymbolInstance::forceFail() const { - isFailed = true; + if (!isFailed) { + fail("SymbolInstance marked as failed by an external check"); + } } // this is just to avoid warnings about the values never being set diff --git a/src/mbgl/layout/symbol_instance.hpp b/src/mbgl/layout/symbol_instance.hpp index eadf3b15e..d697f6fbc 100644 --- a/src/mbgl/layout/symbol_instance.hpp +++ b/src/mbgl/layout/symbol_instance.hpp @@ -183,8 +183,10 @@ protected: bool check(std::uint64_t v, int n, const source_location&) const; bool checkKey(const source_location&) const; void forceFailInternal(); // this is just to avoid warnings about the values never being set + void fail(const std::string&) const; #else bool checkKey(std::string_view) const { return true; } + void fail(const std::string&) const {} #endif private: diff --git a/src/mbgl/map/map_impl.cpp b/src/mbgl/map/map_impl.cpp index e842aa142..007949ad0 100644 --- a/src/mbgl/map/map_impl.cpp +++ b/src/mbgl/map/map_impl.cpp @@ -432,6 +432,18 @@ void Map::Impl::onTileAction(TileOperation op, const OverscaledTileID& id, const void Map::Impl::onRenderError(std::exception_ptr error) { observer.onRenderError(error); + + if (actionJournal) { + actionJournal->impl->onRenderError(error); + } +} + +void Map::Impl::onSymbolError(const std::string& message) { + observer.onSymbolError(message); + + if (actionJournal) { + actionJournal->impl->onSymbolError(message); + } } } // namespace mbgl diff --git a/src/mbgl/map/map_impl.hpp b/src/mbgl/map/map_impl.hpp index fa25df9f0..d85950a64 100644 --- a/src/mbgl/map/map_impl.hpp +++ b/src/mbgl/map/map_impl.hpp @@ -75,6 +75,7 @@ public: void onGlyphsRequested(const FontStack&, const GlyphRange&) final; void onTileAction(TileOperation op, const OverscaledTileID&, const std::string&) final; void onRenderError(std::exception_ptr) final; + void onSymbolError(const std::string&) final; // Map void jumpTo(const CameraOptions&); diff --git a/src/mbgl/renderer/render_orchestrator.cpp b/src/mbgl/renderer/render_orchestrator.cpp index 8b5d45a28..3368043b8 100644 --- a/src/mbgl/renderer/render_orchestrator.cpp +++ b/src/mbgl/renderer/render_orchestrator.cpp @@ -1068,6 +1068,10 @@ void RenderOrchestrator::onTileAction(RenderSource&, observer->onTileAction(op, id, sourceID); } +void RenderOrchestrator::onSymbolError(RenderSource&, const std::string& message) { + observer->onSymbolError(message); +} + void RenderOrchestrator::onStyleImageMissing(const std::string& id, const std::function& done) { MLN_TRACE_FUNC(); diff --git a/src/mbgl/renderer/render_orchestrator.hpp b/src/mbgl/renderer/render_orchestrator.hpp index c439204cf..8b0263972 100644 --- a/src/mbgl/renderer/render_orchestrator.hpp +++ b/src/mbgl/renderer/render_orchestrator.hpp @@ -191,6 +191,7 @@ private: void onTileChanged(RenderSource&, const OverscaledTileID&) override; void onTileError(RenderSource&, const OverscaledTileID&, std::exception_ptr) override; void onTileAction(RenderSource&, TileOperation, const OverscaledTileID&, const std::string&) override; + void onSymbolError(RenderSource&, const std::string&) override; // ImageManagerObserver implementation void onStyleImageMissing(const std::string&, const std::function&) override; diff --git a/src/mbgl/renderer/render_source.cpp b/src/mbgl/renderer/render_source.cpp index 6f0872f82..e70e0ab13 100644 --- a/src/mbgl/renderer/render_source.cpp +++ b/src/mbgl/renderer/render_source.cpp @@ -86,6 +86,10 @@ void RenderSource::onTileAction(OverscaledTileID id, std::string sourceID, TileO observer->onTileAction(*this, op, id, sourceID); } +void RenderSource::onSymbolError(const std::string& message) { + observer->onSymbolError(*this, message); +} + bool RenderSource::isEnabled() const { return enabled; } diff --git a/src/mbgl/renderer/render_source.hpp b/src/mbgl/renderer/render_source.hpp index 424161a43..3d239e478 100644 --- a/src/mbgl/renderer/render_source.hpp +++ b/src/mbgl/renderer/render_source.hpp @@ -127,6 +127,7 @@ protected: void onTileChanged(Tile&) override; void onTileError(Tile&, std::exception_ptr) final; void onTileAction(OverscaledTileID, std::string, TileOperation) override; + void onSymbolError(const std::string&) override; }; } // namespace mbgl diff --git a/src/mbgl/renderer/render_source_observer.hpp b/src/mbgl/renderer/render_source_observer.hpp index 6fafac31f..cb54e200c 100644 --- a/src/mbgl/renderer/render_source_observer.hpp +++ b/src/mbgl/renderer/render_source_observer.hpp @@ -16,6 +16,8 @@ public: virtual void onTileChanged(RenderSource&, const OverscaledTileID&) {} virtual void onTileError(RenderSource&, const OverscaledTileID&, std::exception_ptr) {} virtual void onTileAction(RenderSource&, TileOperation, const OverscaledTileID&, const std::string&) {} + + virtual void onSymbolError(RenderSource&, const std::string&) {} }; } // namespace mbgl diff --git a/src/mbgl/renderer/renderer.cpp b/src/mbgl/renderer/renderer.cpp index a3bae6a5a..a24120207 100644 --- a/src/mbgl/renderer/renderer.cpp +++ b/src/mbgl/renderer/renderer.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -35,6 +36,11 @@ void Renderer::setObserver(RendererObserver* observer) { void Renderer::render(const std::shared_ptr& updateParameters) { MLN_TRACE_FUNC(); assert(updateParameters); + + // Route the errors detected on the render thread to the renderer observer for the duration of the frame (see + // `SymbolErrorObserver`). + const ErrorScope errorScope{impl->observer}; + const bool styleChanged = impl->styleLoaded && !updateParameters->styleLoaded; impl->styleLoaded = updateParameters->styleLoaded; if (!impl->dynamicTextureAtlas || styleChanged) { diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 35d629709..9f3b24867 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -204,7 +205,8 @@ GeometryTile::GeometryTile(const OverscaledTileID& id_, parameters.pixelRatio, parameters.debugOptions & MapDebugOptions::Collision, parameters.dynamicTextureAtlas, - parameters.glyphManager->getFontFaces()), + parameters.glyphManager->getFontFaces(), + observer), fileSource(parameters.fileSource), glyphManager(parameters.glyphManager), imageManager(parameters.imageManager), @@ -344,6 +346,8 @@ void GeometryTile::onLayout(std::shared_ptr&& result, const uint64 observer->onTileAction(id, sourceID, TileOperation::EndParse); } + const ErrorScope errorScope{observer}; + layoutResult = std::move(result); if (!atlasTextures) { atlasTextures = std::make_shared(); @@ -368,6 +372,12 @@ void GeometryTile::onLayout(std::shared_ptr&& result, const uint64 } } +void GeometryTile::setObserver(TileObserver* observer_) { + Tile::setObserver(observer_); + + worker.self().invoke(&GeometryTileWorker::setObserver, observer); +} + void GeometryTile::onError(std::exception_ptr err, const uint64_t resultCorrelationID) { loaded = true; if (resultCorrelationID == correlationID) { diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 1c55f84f5..51a44bb44 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -93,6 +93,8 @@ public: void onError(std::exception_ptr, uint64_t correlationID); + void setObserver(TileObserver*) override; + bool holdForFade() const override; void markRenderedIdeal() override; void markRenderedPreviously() override; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 76fb2bcf1..a4cd52ad1 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -14,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -39,7 +41,8 @@ GeometryTileWorker::GeometryTileWorker(OptionalActorRef self const float pixelRatio_, const bool showCollisionBoxes_, gfx::DynamicTextureAtlasPtr dynamicTextureAtlas_, - std::shared_ptr fontFaces_) + std::shared_ptr fontFaces_, + TileObserver* observer_) : self(std::move(self_)), parent(std::move(parent_)), scheduler(scheduler_), @@ -50,7 +53,8 @@ GeometryTileWorker::GeometryTileWorker(OptionalActorRef self pixelRatio(pixelRatio_), showCollisionBoxes(showCollisionBoxes_), dynamicTextureAtlas(dynamicTextureAtlas_), - fontFaces(fontFaces_) {} + fontFaces(fontFaces_), + observer(observer_) {} GeometryTileWorker::~GeometryTileWorker() { MLN_TRACE_FUNC(); @@ -406,6 +410,10 @@ void GeometryTileWorker::requestNewImages(const ImageDependencies& imageDependen } } +void GeometryTileWorker::setObserver(TileObserver* observer_) { + observer = observer_; +} + void GeometryTileWorker::parse() { MLN_TRACE_FUNC(); @@ -413,6 +421,10 @@ void GeometryTileWorker::parse() { return; } + // The layouts created below check the symbols they build; report through the tile's observer, + // called on this worker thread. + const ErrorScope errorScope{observer}; + MBGL_TIMING_START(watch) std::unordered_map> symbolLayoutMap; @@ -537,6 +549,10 @@ void GeometryTileWorker::finalizeLayout() { return; } + // The layouts check the symbols they build below; report through the tile's observer, + // called on this worker thread. + const ErrorScope errorScope{observer}; + MBGL_TIMING_START(watch); gfx::ImageAtlas imageAtlas; gfx::GlyphAtlas glyphAtlas; diff --git a/src/mbgl/tile/geometry_tile_worker.hpp b/src/mbgl/tile/geometry_tile_worker.hpp index 53378aca1..7663cec22 100644 --- a/src/mbgl/tile/geometry_tile_worker.hpp +++ b/src/mbgl/tile/geometry_tile_worker.hpp @@ -44,9 +44,12 @@ public: float pixelRatio, bool showCollisionBoxes_, gfx::DynamicTextureAtlasPtr, - std::shared_ptr fontFaces); + std::shared_ptr fontFaces, + TileObserver* observer); ~GeometryTileWorker(); + void setObserver(TileObserver* observer); + void setLayers(std::vector>, std::set availableImages, uint64_t correlationID); @@ -123,6 +126,8 @@ private: gfx::DynamicTextureAtlasPtr dynamicTextureAtlas; std::shared_ptr fontFaces; + + TileObserver* observer; }; } // namespace mbgl diff --git a/src/mbgl/tile/tile.hpp b/src/mbgl/tile/tile.hpp index a3e9853ed..c65804df4 100644 --- a/src/mbgl/tile/tile.hpp +++ b/src/mbgl/tile/tile.hpp @@ -62,7 +62,7 @@ public: virtual std::unique_ptr createRenderData() = 0; - void setObserver(TileObserver* observer); + virtual void setObserver(TileObserver* observer); virtual void setNecessity(TileNecessity) {} diff --git a/src/mbgl/tile/tile_observer.hpp b/src/mbgl/tile/tile_observer.hpp index 4e0a11f64..aa7b91af9 100644 --- a/src/mbgl/tile/tile_observer.hpp +++ b/src/mbgl/tile/tile_observer.hpp @@ -2,15 +2,17 @@ #include #include +#include + #include namespace mbgl { class Tile; -class TileObserver { +class TileObserver : public SymbolErrorObserver { public: - virtual ~TileObserver() = default; + ~TileObserver() override = default; virtual void onTileChanged(Tile&) {} virtual void onTileError(Tile&, std::exception_ptr) {} diff --git a/src/mbgl/util/action_journal_impl.cpp b/src/mbgl/util/action_journal_impl.cpp index b66de4fcc..a49fae3ce 100644 --- a/src/mbgl/util/action_journal_impl.cpp +++ b/src/mbgl/util/action_journal_impl.cpp @@ -487,6 +487,26 @@ void ActionJournal::Impl::onSpriteRequested(const std::optional& }); } +void ActionJournal::Impl::onRenderError(std::exception_ptr e) { + scheduler->schedule([=, this, env = MapEnvironmentSnapshot(*this)]() { + ActionJournalEvent event("onRenderError", env); + + event.addEvent("exception", toString(e)); + + log(event); + }); +} + +void ActionJournal::Impl::onSymbolError(const std::string& message) { + scheduler->schedule([=, this, env = MapEnvironmentSnapshot(*this)]() { + ActionJournalEvent event("onSymbolError", env); + + event.addEvent("message", message); + + log(event); + }); +} + void ActionJournal::Impl::onMapCreate() { scheduler->schedule( [=, this, env = MapEnvironmentSnapshot(*this)]() { log(ActionJournalEvent("onMapCreate", env)); }); diff --git a/src/mbgl/util/action_journal_impl.hpp b/src/mbgl/util/action_journal_impl.hpp index 44e0d06ef..23366afe6 100644 --- a/src/mbgl/util/action_journal_impl.hpp +++ b/src/mbgl/util/action_journal_impl.hpp @@ -55,6 +55,8 @@ public: void onSpriteLoaded(const std::optional&) override; void onSpriteError(const std::optional&, std::exception_ptr) override; void onSpriteRequested(const std::optional&) override; + void onRenderError(std::exception_ptr) override; + void onSymbolError(const std::string&) override; void onMapCreate(); void onMapDestroy(); diff --git a/src/mbgl/util/error_sink.cpp b/src/mbgl/util/error_sink.cpp new file mode 100644 index 000000000..ece02386e --- /dev/null +++ b/src/mbgl/util/error_sink.cpp @@ -0,0 +1,26 @@ +#include +#include + +namespace mbgl { + +SymbolErrorObserver*& ErrorSink::current() { + static thread_local SymbolErrorObserver* observer = nullptr; + return observer; +} + +void ErrorSink::report(const std::string& message) { + if (auto* observer = current()) { + observer->onSymbolError(message); + } +} + +ErrorScope::ErrorScope(SymbolErrorObserver* observer) + : previous(ErrorSink::current()) { + ErrorSink::current() = observer; +} + +ErrorScope::~ErrorScope() { + ErrorSink::current() = previous; +} + +} // namespace mbgl diff --git a/src/mbgl/util/error_sink.hpp b/src/mbgl/util/error_sink.hpp new file mode 100644 index 000000000..7bad3d341 --- /dev/null +++ b/src/mbgl/util/error_sink.hpp @@ -0,0 +1,43 @@ +#pragma once + +#include + +namespace mbgl { + +class SymbolErrorObserver; + +/// Thread-local destination for errors detected by code too deep to reach an observer on its own, +/// e.g. the symbol guard checks in `SymbolInstance` and `SymbolBucket` (see `MLN_SYMBOL_GUARDS`). +/// +/// Each thread which owns an observer installs it with an `ErrorScope` around the work it drives, +/// so that a failure is reported as soon as it's detected, through the observer owning the thread +/// the check ran on: `TileObserver` for tile parsing/layout, `RendererObserver` for the render loop. +class ErrorSink { +public: + /// Report an error to the observer installed on the current thread, if any. Does nothing + /// otherwise, so check sites don't have to care whether an observer is in reach. + static void report(const std::string& message); + +private: + friend class ErrorScope; + + /// The observer installed on the calling thread, `nullptr` when there's none. + static SymbolErrorObserver*& current(); +}; + +/// Installs an observer as the current thread's error destination for the lifetime of the scope, +/// restoring the previous one on destruction. +class ErrorScope { +public: + /// @param observer May be null, which mutes reporting for the duration of the scope. + explicit ErrorScope(SymbolErrorObserver* observer); + ~ErrorScope(); + + ErrorScope(const ErrorScope&) = delete; + ErrorScope& operator=(const ErrorScope&) = delete; + +private: + SymbolErrorObserver* previous; +}; + +} // namespace mbgl