fix(java): Ensure potential callback exceptions are caught #2123

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ### Fixes
 
+- Ensure potential callback exceptions are caught #2123 ([#2291](https://github.com/getsentry/sentry-java/pull/2291))
 - Remove verbose FrameMetricsAggregator failure logging ([#2293](https://github.com/getsentry/sentry-java/pull/2293))
 
 ## 6.5.0

sentry/src/main/java/io/sentry/Hub.java

@@ -830,9 +830,13 @@ SpanContext getSpanContext(final @NotNull Throwable throwable) {
   private Scope buildLocalScope(
       final @NotNull Scope scope, final @Nullable ScopeCallback callback) {
     if (callback != null) {
-      final Scope localScope = new Scope(scope);
-      callback.run(localScope);
-      return localScope;
+      try {
+        final Scope localScope = new Scope(scope);
+        callback.run(localScope);
+        return localScope;
+      } catch (Throwable t) {
+        options.getLogger().log(SentryLevel.ERROR, "Error in the 'ScopeCallback' callback.", t);
+      }
     }
     return scope;
   }

sentry/src/main/java/io/sentry/Sentry.java

@@ -113,7 +113,7 @@ public static <T extends SentryOptions> void init(
       throws IllegalAccessException, InstantiationException, NoSuchMethodException,
           InvocationTargetException {
     final T options = clazz.createInstance();
-    optionsConfiguration.configure(options);
+    applyOptionsConfiguration(optionsConfiguration, options);
     init(options, globalHubMode);
   }
 
@@ -136,10 +136,21 @@ public static void init(
       final @NotNull OptionsConfiguration<SentryOptions> optionsConfiguration,
       final boolean globalHubMode) {
     final SentryOptions options = new SentryOptions();
-    optionsConfiguration.configure(options);
+    applyOptionsConfiguration(optionsConfiguration, options);
     init(options, globalHubMode);
   }
 
+  private static <T extends SentryOptions> void applyOptionsConfiguration(
+      OptionsConfiguration<T> optionsConfiguration, T options) {
+    try {
+      optionsConfiguration.configure(options);
+    } catch (Throwable t) {
+      options
+          .getLogger()
+          .log(SentryLevel.ERROR, "Error in the 'OptionsConfiguration.configure' callback.", t);
+    }
+  }
+
   /**
    * Initializes the SDK with a SentryOptions.
    *

sentry/src/main/java/io/sentry/TracesSampler.java

@@ -29,15 +29,28 @@ TracesSamplingDecision sample(final @NotNull SamplingContext samplingContext) {
 
     Double profilesSampleRate = null;
     if (options.getProfilesSampler() != null) {
-      profilesSampleRate = options.getProfilesSampler().sample(samplingContext);
+      try {
+        profilesSampleRate = options.getProfilesSampler().sample(samplingContext);
+      } catch (Throwable t) {
+        options
+            .getLogger()
+            .log(SentryLevel.ERROR, "Error in the 'ProfilesSamplerCallback' callback.", t);
+      }
     }
     if (profilesSampleRate == null) {
       profilesSampleRate = options.getProfilesSampleRate();
     }
     Boolean profilesSampled = profilesSampleRate != null && sample(profilesSampleRate);
 
     if (options.getTracesSampler() != null) {
-      final Double samplerResult = options.getTracesSampler().sample(samplingContext);
+      Double samplerResult = null;
+      try {
+        samplerResult = options.getTracesSampler().sample(samplingContext);
+      } catch (Throwable t) {
+        options
+            .getLogger()
+            .log(SentryLevel.ERROR, "Error in the 'TracesSamplerCallback' callback.", t);
+      }
       if (samplerResult != null) {
         return new TracesSamplingDecision(
             sample(samplerResult), samplerResult, profilesSampled, profilesSampleRate);

sentry/src/test/java/io/sentry/HubTest.kt

@@ -433,6 +433,24 @@ class HubTest {
         assertEquals("testValue", argumentCaptor.allValues[0].tags["test"])
         assertNull(argumentCaptor.allValues[1].tags["test"])
     }
+
+    @Test
+    fun `when captureEvent is called with a ScopeCallback that crashes then the event should still be captured`() {
+        val (sut, mockClient, logger) = getEnabledHub()
+
+        val exception = Exception("scope callback exception")
+        sut.captureEvent(SentryEvent(), null) {
+            throw exception
+        }
+
+        verify(mockClient).captureEvent(
+            any(),
+            anyOrNull(),
+            anyOrNull()
+        )
+
+        verify(logger).log(eq(SentryLevel.ERROR), any(), eq(exception))
+    }
     //endregion
 
     //region captureMessage tests
@@ -509,6 +527,24 @@ class HubTest {
         assertNull(argumentCaptor.allValues[1].tags["test"])
     }
 
+    @Test
+    fun `when captureMessage is called with a ScopeCallback that crashes then the message should still be captured`() {
+        val (sut, mockClient, logger) = getEnabledHub()
+
+        val exception = Exception("scope callback exception")
+        sut.captureMessage("Hello World") {
+            throw exception
+        }
+
+        verify(mockClient).captureMessage(
+            any(),
+            anyOrNull(),
+            anyOrNull()
+        )
+
+        verify(logger).log(eq(SentryLevel.ERROR), any(), eq(exception))
+    }
+
     //endregion
 
     //region captureException tests
@@ -621,6 +657,24 @@ class HubTest {
         assertNull(argumentCaptor.allValues[1].tags["test"])
     }
 
+    @Test
+    fun `when captureException is called with a ScopeCallback that crashes then the exception should still be captured`() {
+        val (sut, mockClient, logger) = getEnabledHub()
+
+        val exception = Exception("scope callback exception")
+        sut.captureException(Throwable()) {
+            throw exception
+        }
+
+        verify(mockClient).captureEvent(
+            any(),
+            anyOrNull(),
+            anyOrNull()
+        )
+
+        verify(logger).log(eq(SentryLevel.ERROR), any(), eq(exception))
+    }
+
     //endregion
 
     //region captureUserFeedback tests
@@ -708,6 +762,20 @@ class HubTest {
         sut.withScope(scopeCallback)
         verify(scopeCallback).run(any())
     }
+
+    @Test
+    fun `when withScope throws an exception then it should be caught`() {
+        val (hub, _, logger) = getEnabledHub()
+
+        val exception = Exception("scope callback exception")
+        val scopeCallback = ScopeCallback {
+            throw exception
+        }
+
+        hub.withScope(scopeCallback)
+
+        verify(logger).log(eq(SentryLevel.ERROR), any(), eq(exception))
+    }
     //endregion
 
     //region configureScope tests
@@ -731,6 +799,20 @@ class HubTest {
         sut.configureScope(scopeCallback)
         verify(scopeCallback).run(any())
     }
+
+    @Test
+    fun `when configureScope throws an exception then it should be caught`() {
+        val (hub, _, logger) = getEnabledHub()
+
+        val exception = Exception("scope callback exception")
+        val scopeCallback = ScopeCallback {
+            throw exception
+        }
+
+        hub.configureScope(scopeCallback)
+
+        verify(logger).log(eq(SentryLevel.ERROR), any(), eq(exception))
+    }
     //endregion
 
     @Test
@@ -1555,16 +1637,21 @@ class HubTest {
         return Hub(options)
     }
 
-    private fun getEnabledHub(): Pair<Hub, ISentryClient> {
+    private fun getEnabledHub(): Triple<Hub, ISentryClient, ILogger> {
+        val logger = mock<ILogger>()
+
         val options = SentryOptions()
         options.cacheDirPath = file.absolutePath
         options.dsn = "https://key@sentry.io/proj"
         options.setSerializer(mock())
         options.tracesSampleRate = 1.0
+        options.isDebug = true
+        options.setLogger(logger)
+
         val sut = Hub(options)
         val mockClient = mock<ISentryClient>()
         sut.bindClient(mockClient)
-        return Pair(sut, mockClient)
+        return Triple(sut, mockClient, logger)
     }
 
     private fun hashedFolder(): String {

sentry/src/test/java/io/sentry/SentryTest.kt

@@ -1,5 +1,6 @@
 package io.sentry
 
+import com.nhaarman.mockitokotlin2.any
 import com.nhaarman.mockitokotlin2.argThat
 import com.nhaarman.mockitokotlin2.eq
 import com.nhaarman.mockitokotlin2.mock
@@ -233,6 +234,38 @@ class SentryTest {
         assertFalse(hub is NoOpHub)
     }
 
+    @Test
+    fun `when init is called and configure throws an exception then an error is logged`() {
+        val logger = mock<ILogger>()
+        val initException = Exception("init")
+
+        Sentry.init({
+            it.dsn = dsn
+            it.isDebug = true
+            it.setLogger(logger)
+            throw initException
+        }, true)
+
+        verify(logger).log(eq(SentryLevel.ERROR), any(), eq(initException))
+    }
+
+    @Test
+    fun `when init with a SentryOptions Subclass is called and configure throws an exception then an error is logged`() {
+        class ExtendedSentryOptions : SentryOptions()
+
+        val logger = mock<ILogger>()
+        val initException = Exception("init")
+
+        Sentry.init(OptionsContainer.create(ExtendedSentryOptions::class.java)) { options: ExtendedSentryOptions ->
+            options.dsn = dsn
+            options.isDebug = true
+            options.setLogger(logger)
+            throw initException
+        }
+
+        verify(logger).log(eq(SentryLevel.ERROR), any(), eq(initException))
+    }
+
     private fun getTempPath(): String {
         val tempFile = Files.createTempDirectory("cache").toFile()
         tempFile.delete()