From f76d8e2190872733d4403bc2ff30ea1063b4e97f Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 30 Apr 2021 15:35:10 +0100 Subject: [PATCH] feat: support enabling/disabling automatic error detection for unity --- .../main/java/com/bugsnag/android/Client.java | 42 ++++++++-- .../com/bugsnag/android/ExceptionHandler.java | 7 ++ .../com/bugsnag/android/LibraryLoader.java | 8 +- .../com/bugsnag/android/NativeInterface.java | 16 +++- .../java/com/bugsnag/android/PluginClient.kt | 84 ++++++++++++++++--- .../com/bugsnag/android/ClientFacadeTest.java | 50 ++++++++++- .../bugsnag/android/ExceptionHandlerTest.kt | 6 ++ .../com/bugsnag/android/LibraryLoaderTest.kt | 3 + .../bugsnag/android/NativeInterfaceApiTest.kt | 12 +++ .../java/com/bugsnag/android/AnrPlugin.kt | 58 +++++++------ .../java/com/bugsnag/android/NdkPlugin.kt | 33 ++++++-- 11 files changed, 259 insertions(+), 60 deletions(-) diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java index 8a836bec33..9e637ba7e3 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/Client.java @@ -78,7 +78,7 @@ public class Client implements MetadataAware, CallbackAware, UserAware { final Logger logger; final DeliveryDelegate deliveryDelegate; - final ClientObservable clientObservable = new ClientObservable(); + final ClientObservable clientObservable; private PluginClient pluginClient; final Notifier notifier = new Notifier(); @@ -88,6 +88,7 @@ public class Client implements MetadataAware, CallbackAware, UserAware { final LastRunInfoStore lastRunInfoStore; final LaunchCrashTracker launchCrashTracker; final BackgroundTaskService bgTaskService = new BackgroundTaskService(); + private ExceptionHandler exceptionHandler; /** * Initialize a Bugsnag client @@ -137,6 +138,7 @@ public Unit invoke(Boolean hasConnection, String networkState) { immutableConfig = sanitiseConfiguration(appContext, configuration, connectivity); logger = immutableConfig.getLogger(); warnIfNotAppContext(androidContext); + clientObservable = new ClientObservable(); // Set up breadcrumbs callbackState = configuration.impl.callbackState.copy(); @@ -209,8 +211,9 @@ public Unit invoke(String activity, Map metadata) { immutableConfig, breadcrumbState, notifier, bgTaskService); // Install a default exception handler with this client + exceptionHandler = new ExceptionHandler(this, logger); if (immutableConfig.getEnabledErrorTypes().getUnhandledExceptions()) { - new ExceptionHandler(this, logger); + exceptionHandler.install(); } // register a receiver for automatic breadcrumbs @@ -245,6 +248,7 @@ public Unit invoke(String activity, Map metadata) { ContextState contextState, CallbackState callbackState, UserState userState, + ClientObservable clientObservable, Context appContext, @NonNull DeviceDataCollector deviceDataCollector, @NonNull AppDataCollector appDataCollector, @@ -267,6 +271,7 @@ public Unit invoke(String activity, Map metadata) { this.contextState = contextState; this.callbackState = callbackState; this.userState = userState; + this.clientObservable = clientObservable; this.appContext = appContext; this.deviceDataCollector = deviceDataCollector; this.appDataCollector = appDataCollector; @@ -365,6 +370,17 @@ void registerObserver(Observer observer) { launchCrashTracker.addObserver(observer); } + void unregisterObserver(Observer observer) { + metadataState.deleteObserver(observer); + breadcrumbState.deleteObserver(observer); + sessionTracker.deleteObserver(observer); + clientObservable.deleteObserver(observer); + userState.deleteObserver(observer); + contextState.deleteObserver(observer); + deliveryDelegate.deleteObserver(observer); + launchCrashTracker.deleteObserver(observer); + } + /** * Sends initial state values for Metadata/User/Context to any registered observers. */ @@ -985,13 +1001,7 @@ Logger getLogger() { @SuppressWarnings("rawtypes") @Nullable Plugin getPlugin(@NonNull Class clz) { - Set plugins = pluginClient.getPlugins(); - for (Plugin plugin : plugins) { - if (plugin.getClass().equals(clz)) { - return plugin; - } - } - return null; + return pluginClient.findPlugin(clz); } Notifier getNotifier() { @@ -1001,4 +1011,18 @@ Notifier getNotifier() { MetadataState getMetadataState() { return metadataState; } + + void setAutoNotify(boolean autoNotify) { + pluginClient.setAutoNotify(this, autoNotify); + + if (autoNotify) { + exceptionHandler.install(); + } else { + exceptionHandler.uninstall(); + } + } + + void setAutoDetectAnrs(boolean autoDetectAnrs) { + pluginClient.setAutoDetectAnrs(this, autoDetectAnrs); + } } diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/ExceptionHandler.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/ExceptionHandler.java index a10ee3f263..4e5f9beab9 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/ExceptionHandler.java +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/ExceptionHandler.java @@ -23,9 +23,16 @@ class ExceptionHandler implements UncaughtExceptionHandler { this.client = client; this.logger = logger; this.originalHandler = Thread.getDefaultUncaughtExceptionHandler(); + } + + void install() { Thread.setDefaultUncaughtExceptionHandler(this); } + void uninstall() { + Thread.setDefaultUncaughtExceptionHandler(originalHandler); + } + @Override public void uncaughtException(@NonNull Thread thread, @NonNull Throwable throwable) { boolean strictModeThrowable = strictModeHandler.isStrictModeThrowable(throwable); diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/LibraryLoader.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/LibraryLoader.java index f8dcb51a71..850a2f82c8 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/LibraryLoader.java +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/LibraryLoader.java @@ -4,7 +4,8 @@ class LibraryLoader { - private AtomicBoolean attemptedLoad = new AtomicBoolean(); + private final AtomicBoolean attemptedLoad = new AtomicBoolean(); + private boolean loaded = false; /** * Attempts to load a native library, returning false if the load was unsuccessful. @@ -21,6 +22,7 @@ boolean loadLibrary(String name, Client client, OnErrorCallback callback) { if (!attemptedLoad.getAndSet(true)) { try { System.loadLibrary(name); + loaded = true; return true; } catch (UnsatisfiedLinkError error) { client.notify(error, callback); @@ -28,4 +30,8 @@ boolean loadLibrary(String name, Client client, OnErrorCallback callback) { } return false; } + + boolean isLoaded() { + return loaded; + } } diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/NativeInterface.java b/bugsnag-android-core/src/main/java/com/bugsnag/android/NativeInterface.java index ffd2f30221..30e857b601 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/NativeInterface.java +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/NativeInterface.java @@ -405,11 +405,23 @@ public static Logger getLogger() { return getClient().getConfig().getLogger(); } + /** + * Switches automatic error detection on/off after Bugsnag has initialized. + * This is required to support legacy functionality in Unity. + * + * @param autoNotify whether errors should be automatically detected. + */ public static void setAutoNotify(boolean autoNotify) { - // TODO implement me + getClient().setAutoNotify(autoNotify); } + /** + * Switches automatic ANR detection on/off after Bugsnag has initialized. + * This is required to support legacy functionality in Unity. + * + * @param autoDetectAnrs whether ANRs should be automatically detected. + */ public static void setAutoDetectAnrs(boolean autoDetectAnrs) { - // TODO implement me + getClient().setAutoDetectAnrs(autoDetectAnrs); } } diff --git a/bugsnag-android-core/src/main/java/com/bugsnag/android/PluginClient.kt b/bugsnag-android-core/src/main/java/com/bugsnag/android/PluginClient.kt index 8d3671ee42..d5cf160493 100644 --- a/bugsnag-android-core/src/main/java/com/bugsnag/android/PluginClient.kt +++ b/bugsnag-android-core/src/main/java/com/bugsnag/android/PluginClient.kt @@ -1,12 +1,21 @@ package com.bugsnag.android +// TODO add e2e tests +// TODO add unit tests + internal class PluginClient( userPlugins: Set, - immutableConfig: ImmutableConfig, + private val immutableConfig: ImmutableConfig, private val logger: Logger ) { - protected val plugins: Set + companion object { + private const val NDK_PLUGIN = "com.bugsnag.android.NdkPlugin" + private const val ANR_PLUGIN = "com.bugsnag.android.AnrPlugin" + private const val RN_PLUGIN = "com.bugsnag.android.BugsnagReactNativePlugin" + } + + private val plugins: Set init { val set = mutableSetOf() @@ -14,13 +23,9 @@ internal class PluginClient( // instantiate ANR + NDK plugins by reflection as bugsnag-android-core has no // direct dependency on the artefacts - if (immutableConfig.enabledErrorTypes.ndkCrashes) { - instantiatePlugin("com.bugsnag.android.NdkPlugin")?.let { set.add(it) } - } - if (immutableConfig.enabledErrorTypes.anrs) { - instantiatePlugin("com.bugsnag.android.AnrPlugin")?.let { set.add(it) } - } - instantiatePlugin("com.bugsnag.android.BugsnagReactNativePlugin")?.let { set.add(it) } + instantiatePlugin(NDK_PLUGIN)?.let(set::add) + instantiatePlugin(ANR_PLUGIN)?.let(set::add) + instantiatePlugin(RN_PLUGIN)?.let(set::add) plugins = set.toSet() } @@ -37,11 +42,64 @@ internal class PluginClient( } } - fun loadPlugins(client: Client) = plugins.forEach { + fun loadPlugins(client: Client) { + plugins.forEach { plugin -> + try { + loadPluginInternal(plugin, client) + } catch (exc: Throwable) { + logger.e("Failed to load plugin $plugin, continuing with initialisation.", exc) + } + } + } + + private fun loadPluginInternal(plugin: Plugin, client: Client) { + val name = plugin.javaClass.name + val errorTypes = immutableConfig.enabledErrorTypes + + // only initialize NDK/ANR plugins if automatic detection enabled + if (name == NDK_PLUGIN) { + if (errorTypes.ndkCrashes) { + plugin.load(client) + } + } else if (name == ANR_PLUGIN) { + if (errorTypes.anrs) { + plugin.load(client) + } + } else { + plugin.load(client) + } + } + + fun setAutoNotify(client: Client, autoNotify: Boolean) { + alterPluginLoadState(client, ANR_PLUGIN, autoNotify) + alterPluginLoadState(client, NDK_PLUGIN, autoNotify) + } + + fun setAutoDetectAnrs(client: Client, autoDetectAnrs: Boolean) { + alterPluginLoadState(client, ANR_PLUGIN, autoDetectAnrs) + } + + fun findPlugin(clz: Class<*>): Plugin? = plugins.find { it.javaClass == clz } + + private fun alterPluginLoadState( + client: Client, + className: String, + enabled: Boolean + ) { try { - it.load(client) - } catch (exc: Throwable) { - logger.e("Failed to load plugin $it, continuing with initialisation.", exc) + val clz = Class.forName(className) + val plugin = findPlugin(clz) + + if (plugin != null) { + // TODO handle duplicate load/unload calls by tracking with Map + if (enabled) { + plugin.load(client) + } else { + plugin.unload() + } + } + } catch (ignore: ClassNotFoundException) { + // no action required } } } diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/ClientFacadeTest.java b/bugsnag-android-core/src/test/java/com/bugsnag/android/ClientFacadeTest.java index b4e54fc241..e9d4bed457 100644 --- a/bugsnag-android-core/src/test/java/com/bugsnag/android/ClientFacadeTest.java +++ b/bugsnag-android-core/src/test/java/com/bugsnag/android/ClientFacadeTest.java @@ -8,7 +8,6 @@ import static org.mockito.Mockito.when; import android.content.Context; -import android.content.SharedPreferences; import android.os.storage.StorageManager; import androidx.annotation.NonNull; @@ -23,6 +22,8 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.Observable; +import java.util.Observer; @SuppressWarnings("ConstantConditions") @RunWith(MockitoJUnitRunner.class) @@ -43,6 +44,9 @@ public class ClientFacadeTest { @Mock UserState userState; + @Mock + ClientObservable clientObservable; + @Mock Context appContext; @@ -73,9 +77,6 @@ public class ClientFacadeTest { @Mock SessionLifecycleCallback sessionLifecycleCallback; - @Mock - SharedPreferences sharedPrefs; - @Mock Connectivity connectivity; @@ -112,6 +113,7 @@ public void setUp() { contextState, callbackState, userState, + clientObservable, appContext, deviceDataCollector, appDataCollector, @@ -482,4 +484,44 @@ public void markLaunchCompleted() { client.markLaunchCompleted(); verify(launchCrashTracker, times(1)).markLaunchCompleted(); } + + + @Test + public void registerObserver() { + Observer observer = new Observer() { + @Override + public void update(Observable observable, Object arg) { + } + }; + client.registerObserver(observer); + + verify(metadataState, times(1)).addObserver(observer); + verify(breadcrumbState, times(1)).addObserver(observer); + verify(sessionTracker, times(1)).addObserver(observer); + verify(clientObservable, times(1)).addObserver(observer); + verify(userState, times(1)).addObserver(observer); + verify(contextState, times(1)).addObserver(observer); + verify(deliveryDelegate, times(1)).addObserver(observer); + verify(launchCrashTracker, times(1)).addObserver(observer); + } + + @Test + public void unregisterObserver() { + Observer observer = new Observer() { + @Override + public void update(Observable observable, Object arg) { + } + }; + client.unregisterObserver(observer); + + verify(metadataState, times(1)).deleteObserver(observer); + verify(breadcrumbState, times(1)).deleteObserver(observer); + verify(sessionTracker, times(1)).deleteObserver(observer); + verify(clientObservable, times(1)).deleteObserver(observer); + verify(userState, times(1)).deleteObserver(observer); + verify(contextState, times(1)).deleteObserver(observer); + verify(deliveryDelegate, times(1)).deleteObserver(observer); + verify(launchCrashTracker, times(1)).deleteObserver(observer); + } + } diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/ExceptionHandlerTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/ExceptionHandlerTest.kt index db41e4cced..1ba84aa619 100644 --- a/bugsnag-android-core/src/test/java/com/bugsnag/android/ExceptionHandlerTest.kt +++ b/bugsnag-android-core/src/test/java/com/bugsnag/android/ExceptionHandlerTest.kt @@ -35,7 +35,13 @@ internal class ExceptionHandlerTest { @Test fun handlerInstalled() { val exceptionHandler = ExceptionHandler(client, NoopLogger) + assertSame(originalHandler, Thread.getDefaultUncaughtExceptionHandler()) + + exceptionHandler.install() assertSame(exceptionHandler, Thread.getDefaultUncaughtExceptionHandler()) + + exceptionHandler.uninstall() + assertSame(originalHandler, Thread.getDefaultUncaughtExceptionHandler()) } @Test diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/LibraryLoaderTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/LibraryLoaderTest.kt index 970f4d60f4..8b3d8dbb97 100644 --- a/bugsnag-android-core/src/test/java/com/bugsnag/android/LibraryLoaderTest.kt +++ b/bugsnag-android-core/src/test/java/com/bugsnag/android/LibraryLoaderTest.kt @@ -20,6 +20,7 @@ class LibraryLoaderTest { val libraryLoader = LibraryLoader() val loaded = libraryLoader.loadLibrary("foo", client) { true } assertFalse(loaded) + assertFalse(libraryLoader.isLoaded) verify(client, times(1)).notify(any(), any()) } @@ -28,10 +29,12 @@ class LibraryLoaderTest { val libraryLoader = LibraryLoader() var loaded = libraryLoader.loadLibrary("foo", client) { true } assertFalse(loaded) + assertFalse(libraryLoader.isLoaded) // duplicate calls only invoke System.loadLibrary once loaded = libraryLoader.loadLibrary("foo", client) { true } assertFalse(loaded) + assertFalse(libraryLoader.isLoaded) verify(client, times(1)).notify(any(), any()) } } diff --git a/bugsnag-android-core/src/test/java/com/bugsnag/android/NativeInterfaceApiTest.kt b/bugsnag-android-core/src/test/java/com/bugsnag/android/NativeInterfaceApiTest.kt index 68d736e535..8a63a710d8 100644 --- a/bugsnag-android-core/src/test/java/com/bugsnag/android/NativeInterfaceApiTest.kt +++ b/bugsnag-android-core/src/test/java/com/bugsnag/android/NativeInterfaceApiTest.kt @@ -223,4 +223,16 @@ internal class NativeInterfaceApiTest { NativeInterface.notify("SIGPIPE", "SIGSEGV 11", Severity.ERROR, arrayOf()) verify(client, times(1)).notify(any(), any()) } + + @Test + fun autoDetectAnrs() { + NativeInterface.setAutoDetectAnrs(true) + verify(client, times(1)).setAutoDetectAnrs(true) + } + + @Test + fun autoNotify() { + NativeInterface.setAutoNotify(true) + verify(client, times(1)).setAutoNotify(true) + } } diff --git a/bugsnag-plugin-android-anr/src/main/java/com/bugsnag/android/AnrPlugin.kt b/bugsnag-plugin-android-anr/src/main/java/com/bugsnag/android/AnrPlugin.kt index 9858689943..2091b2ba49 100644 --- a/bugsnag-plugin-android-anr/src/main/java/com/bugsnag/android/AnrPlugin.kt +++ b/bugsnag-plugin-android-anr/src/main/java/com/bugsnag/android/AnrPlugin.kt @@ -2,6 +2,7 @@ package com.bugsnag.android import android.os.Handler import android.os.Looper +import java.util.concurrent.atomic.AtomicBoolean internal class AnrPlugin : Plugin { @@ -20,7 +21,8 @@ internal class AnrPlugin : Plugin { } } - private val loader = LibraryLoader() + private val libraryLoader = LibraryLoader() + private val oneTimeSetupPerformed = AtomicBoolean(false) private lateinit var client: Client private val collector = AnrDetailsCollector() @@ -37,31 +39,14 @@ internal class AnrPlugin : Plugin { } override fun load(client: Client) { - val loaded = loader.loadLibrary("bugsnag-plugin-android-anr", client) { - val error = it.errors[0] - error.errorClass = "AnrLinkError" - error.errorMessage = LOAD_ERR_MSG - true - } + this.client = client - if (loaded) { - @Suppress("UNCHECKED_CAST") - val clz = loadClass("com.bugsnag.android.NdkPlugin") as Class? - if (clz != null) { - val ndkPlugin = client.getPlugin(clz) - if (ndkPlugin != null) { - val method = ndkPlugin.javaClass.getMethod("getUnwindStackFunction") - @Suppress("UNCHECKED_CAST") - val function = method.invoke(ndkPlugin) as Long - setUnwindFunction(function) - } - } - - // this must be run from the main thread as the SIGQUIT is sent to the main thread, - // and if the handler is installed on a background thread instead we receive no signal + if (!oneTimeSetupPerformed.getAndSet(true)) { + performOneTimeSetup(client) + } + if (libraryLoader.isLoaded) { Handler(Looper.getMainLooper()).post( Runnable { - this.client = client enableAnrReporting() client.logger.i("Initialised ANR Plugin") } @@ -71,7 +56,32 @@ internal class AnrPlugin : Plugin { } } - override fun unload() = disableAnrReporting() + private fun performOneTimeSetup(client: Client) { + libraryLoader.loadLibrary("bugsnag-plugin-android-anr", client) { + val error = it.errors[0] + error.errorClass = "AnrLinkError" + error.errorMessage = LOAD_ERR_MSG + true + } + @Suppress("UNCHECKED_CAST") + val clz = loadClass("com.bugsnag.android.NdkPlugin") as Class? + if (clz != null) { + val ndkPlugin = client.getPlugin(clz) + if (ndkPlugin != null) { + val method = ndkPlugin.javaClass.getMethod("getUnwindStackFunction") + + @Suppress("UNCHECKED_CAST") + val function = method.invoke(ndkPlugin) as Long + setUnwindFunction(function) + } + } + } + + override fun unload() { + if (libraryLoader.isLoaded) { + disableAnrReporting() + } + } /** * Notifies bugsnag that an ANR has occurred, by generating an Error report and populating it diff --git a/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/NdkPlugin.kt b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/NdkPlugin.kt index 4c8434db40..918ddacc54 100644 --- a/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/NdkPlugin.kt +++ b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/NdkPlugin.kt @@ -1,6 +1,7 @@ package com.bugsnag.android import com.bugsnag.android.ndk.NativeBridge +import java.util.concurrent.atomic.AtomicBoolean internal class NdkPlugin : Plugin { @@ -9,12 +10,14 @@ internal class NdkPlugin : Plugin { "not report NDK errors. See https://docs.bugsnag.com/platforms/android/ndk-link-errors" } - private val loader = LibraryLoader() + private val libraryLoader = LibraryLoader() + private val oneTimeSetupPerformed = AtomicBoolean(false) private external fun enableCrashReporting() private external fun disableCrashReporting() private var nativeBridge: NativeBridge? = null + private var client: Client? = null private fun initNativeBridge(client: Client): NativeBridge { val nativeBridge = NativeBridge() @@ -24,23 +27,39 @@ internal class NdkPlugin : Plugin { } override fun load(client: Client) { - val loaded = loader.loadLibrary("bugsnag-ndk", client) { + this.client = client + + if (!oneTimeSetupPerformed.getAndSet(true)) { + performOneTimeSetup(client) + } + if (libraryLoader.isLoaded) { + enableCrashReporting() + client.logger.i("Initialised NDK Plugin") + } + } + + private fun performOneTimeSetup(client: Client) { + libraryLoader.loadLibrary("bugsnag-ndk", client) { val error = it.errors[0] error.errorClass = "NdkLinkError" error.errorMessage = LOAD_ERR_MSG true } - - if (loaded) { + if (libraryLoader.isLoaded) { nativeBridge = initNativeBridge(client) - enableCrashReporting() - client.logger.i("Initialised NDK Plugin") } else { client.logger.e(LOAD_ERR_MSG) } } - override fun unload() = disableCrashReporting() + override fun unload() { + if (libraryLoader.isLoaded) { + disableCrashReporting() + nativeBridge?.let { bridge -> + client?.unregisterObserver(bridge) + } + } + } fun getUnwindStackFunction(): Long { val bridge = nativeBridge