diff --git a/CHANGELOG.md b/CHANGELOG.md index 39c9d0d06e9..f9f54939727 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ - Skip replay capture when `beforeErrorSampling` throws while retaining the error event ([#6165](https://github.com/getsentry/sentry-java/pull/6165)). - Skip Android screenshots or view hierarchies when their capture callbacks throw while retaining the error event ([#6167](https://github.com/getsentry/sentry-java/pull/6167)). - Keep the Android feedback form available when its options configurator throws ([#6239](https://github.com/getsentry/sentry-java/pull/6239)). + - Isolate nonfatal callback throwables while continuing to propagate fatal JVM errors ([#6251](https://github.com/getsentry/sentry-java/pull/6251)). ### Dependencies diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java index 6932b6be3f6..6283b47f95d 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java @@ -19,6 +19,7 @@ import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode; import io.sentry.internal.eventprocessor.SentryEventProcessor; import io.sentry.protocol.SentryTransaction; +import io.sentry.util.ExceptionUtils; import io.sentry.util.HintUtils; import io.sentry.util.Objects; import java.util.concurrent.CountDownLatch; @@ -116,7 +117,8 @@ private boolean isMaskingEnabled() { if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { return event; } - } catch (Exception e) { + } catch (Throwable e) { + ExceptionUtils.rethrowIfFatal(e); options .getLogger() .log( diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java index 00753ee4c90..ffd3323e601 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java @@ -22,6 +22,7 @@ import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.ViewHierarchy; import io.sentry.protocol.ViewHierarchyNode; +import io.sentry.util.ExceptionUtils; import io.sentry.util.HintUtils; import io.sentry.util.JsonSerializationUtils; import io.sentry.util.Objects; @@ -95,7 +96,8 @@ public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) { return event; } - } catch (Exception e) { + } catch (Throwable e) { + ExceptionUtils.rethrowIfFatal(e); options .getLogger() .log( diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt index 635d7a308c5..0abb232b807 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt @@ -44,6 +44,7 @@ import java.io.File import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull @@ -315,12 +316,12 @@ class ScreenshotEventProcessorTest { } @Test - fun `when capture callback throws, skips screenshot and retains event`() { + fun `when capture callback throws a nonfatal throwable, skips screenshot and retains event`() { CurrentActivityHolder.getInstance().setActivity(fixture.activity) val logger = mock() fixture.options.isDebug = true fixture.options.setLogger(logger) - val failure = IllegalStateException("callback failed") + val failure = AssertionError("callback failed") fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> throw failure } val processor = fixture.getSut(true) val event = SentryEvent().apply { exceptions = listOf(SentryException()) } @@ -341,6 +342,19 @@ class ScreenshotEventProcessorTest { assertThat(nextHint.screenshot).isNotNull() } + @Test + fun `when capture callback throws a fatal error, it is not swallowed`() { + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + val failure = OutOfMemoryError("callback failed") + fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> throw failure } + val processor = fixture.getSut(true) + val event = SentryEvent().apply { exceptions = listOf(SentryException()) } + + val thrown = assertFailsWith { processor.process(event, Hint()) } + + assertThat(thrown).isSameInstanceAs(failure) + } + @Test fun `when capture callback returns true, a screenshot should be captured`() { CurrentActivityHolder.getInstance().setActivity(fixture.activity) diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt index 6f19035a8d9..1396ae09e3c 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt @@ -19,6 +19,7 @@ import java.io.Writer import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull @@ -345,10 +346,10 @@ class ViewHierarchyEventProcessorTest { } @Test - fun `when capture callback throws, skips view hierarchy and retains event`() { + fun `when capture callback throws a nonfatal throwable, skips view hierarchy and retains event`() { fixture.options.isDebug = true fixture.options.setLogger(fixture.logger) - val failure = IllegalStateException("callback failed") + val failure = AssertionError("callback failed") fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> throw failure } val processor = fixture.getSut(true) val event = SentryEvent().apply { exceptions = listOf(SentryException()) } @@ -369,6 +370,18 @@ class ViewHierarchyEventProcessorTest { assertThat(nextHint.viewHierarchy).isNotNull() } + @Test + fun `when capture callback throws a fatal error, it is not swallowed`() { + val failure = OutOfMemoryError("callback failed") + fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> throw failure } + val processor = fixture.getSut(true) + val event = SentryEvent().apply { exceptions = listOf(SentryException()) } + + val thrown = assertFailsWith { processor.process(event, Hint()) } + + assertThat(thrown).isSameInstanceAs(failure) + } + @Test fun `when capture callback returns true, a view hierarchy should be captured`() { fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> true } diff --git a/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt b/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt index 2331ab17aaf..a28ff2af582 100644 --- a/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt +++ b/sentry-graphql-22/src/test/kotlin/io/sentry/graphql22/SentryInstrumentationTest.kt @@ -35,9 +35,11 @@ import io.sentry.graphql.SentryGraphqlInstrumentation import io.sentry.graphql.SentrySubscriptionHandler import java.lang.RuntimeException import java.util.concurrent.CompletableFuture +import java.util.concurrent.CompletionException import kotlin.random.Random import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertTrue @@ -210,8 +212,8 @@ class SentryInstrumentationTest { } @Test - fun `when beforeSpan throws, drops span and preserves result`() { - val failure = IllegalStateException("callback failed") + fun `when beforeSpan throws a nonfatal throwable, drops span and preserves result`() { + val failure = AssertionError("callback failed") val logger = mock() val sut = fixture.getSut( @@ -239,6 +241,18 @@ class SentryInstrumentationTest { } } + @Test + fun `fatal error from beforeSpan is not swallowed`() { + val failure = OutOfMemoryError("callback failed") + val sut = fixture.getSut(beforeSpan = { _, _, _ -> throw failure }) + + withMockScopes { + val thrown = assertFailsWith { sut.execute("{ shows { id } }") } + + assertThat(thrown.cause).isSameInstanceAs(failure) + } + } + @Test fun `when beforeSpan throws, drops async span and preserves result`() { val sut = diff --git a/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java b/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java index 34044aea435..b1f2892500a 100644 --- a/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java +++ b/sentry-graphql-core/src/main/java/io/sentry/graphql/SentryGraphqlInstrumentation.java @@ -28,6 +28,7 @@ import io.sentry.SpanStatus; import io.sentry.TypeCheckHint; import io.sentry.clientreport.DiscardReason; +import io.sentry.util.ExceptionUtils; import io.sentry.util.StringUtils; import java.util.Arrays; import java.util.List; @@ -290,7 +291,8 @@ private void finish( ISpan newSpan = span; try { newSpan = beforeSpan.execute(span, environment, result); - } catch (Exception e) { + } catch (Throwable e) { + ExceptionUtils.rethrowIfFatal(e); span.getSpanContext().setSampled(false); if (wasSampled) { scopesFromContext(environment.getGraphQlContext()) diff --git a/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt b/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt index 2652694b8f9..5f61ccfe912 100644 --- a/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt +++ b/sentry-graphql/src/test/kotlin/io/sentry/graphql/SentryInstrumentationTest.kt @@ -31,9 +31,11 @@ import io.sentry.TransactionContext import io.sentry.clientreport.DiscardReason import java.lang.RuntimeException import java.util.concurrent.CompletableFuture +import java.util.concurrent.CompletionException import kotlin.random.Random import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertTrue @@ -206,8 +208,8 @@ class SentryInstrumentationTest { } @Test - fun `when beforeSpan throws, drops span and preserves result`() { - val failure = IllegalStateException("callback failed") + fun `when beforeSpan throws a nonfatal throwable, drops span and preserves result`() { + val failure = AssertionError("callback failed") val logger = mock() val sut = fixture.getSut( @@ -235,6 +237,18 @@ class SentryInstrumentationTest { } } + @Test + fun `fatal error from beforeSpan is not swallowed`() { + val failure = OutOfMemoryError("callback failed") + val sut = fixture.getSut(beforeSpan = { _, _, _ -> throw failure }) + + withMockScopes { + val thrown = assertFailsWith { sut.execute("{ shows { id } }") } + + assertThat(thrown.cause).isSameInstanceAs(failure) + } + } + @Test fun `when beforeSpan throws, drops async span and preserves result`() { val sut = diff --git a/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt b/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt index 609ed239961..0e4538fb418 100644 --- a/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt +++ b/sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt @@ -22,6 +22,7 @@ import io.sentry.SentryOptions import io.sentry.SpanStatus import io.sentry.clientreport.DiscardReason import io.sentry.kotlin.SentryContext +import io.sentry.util.ExceptionUtils import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion import io.sentry.util.Platform import io.sentry.util.PropagationTargetsUtils @@ -193,7 +194,8 @@ public val SentryKtorClientPlugin: ClientPlugin = result = try { beforeSpan.execute(span, request) - } catch (e: Exception) { + } catch (@Suppress("TooGenericExceptionCaught") e: Throwable) { + ExceptionUtils.rethrowIfFatal(e) span.spanContext.sampled = false if (wasSampled) { (if (forceScopes) scopes else Sentry.getCurrentScopes()) diff --git a/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt b/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt index 4aadfe823a0..87d607f2553 100644 --- a/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt +++ b/sentry-ktor-client/src/test/java/io/sentry/ktorClient/SentryKtorClientPluginTest.kt @@ -38,6 +38,7 @@ import java.util.concurrent.TimeUnit import kotlin.Unit import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue @@ -451,7 +452,7 @@ class SentryKtorClientPluginTest { fixture.getSut( beforeSpan = { span, _ -> span.spanContext.sampled = false - throw IllegalStateException("callback failed") + error("callback failed") } ) fixture.sentryTracer.spanContext.sampled = sampled @@ -469,7 +470,7 @@ class SentryKtorClientPluginTest { @Test fun `when beforeSpan throws, drops span and preserves response`(): Unit = runBlocking { - val failure = IllegalStateException("callback failed") + val failure = AssertionError("callback failed") val logger = mock() val sut = fixture.getSut( @@ -498,6 +499,19 @@ class SentryKtorClientPluginTest { ) } + @Test + fun `fatal error from beforeSpan is not swallowed`(): Unit = runBlocking { + val failure = OutOfMemoryError("callback failed") + val sut = fixture.getSut(beforeSpan = { _, _ -> throw failure }) + + val thrown = + assertFailsWith { + sut.use { sut.get(fixture.server.url("/hello").toString()) } + } + + assertThat(generateSequence(thrown) { it.cause }.last()).isSameInstanceAs(failure) + } + @Test fun `beforeSpan can drop span`(): Unit = runBlocking { val sut = fixture.getSut(beforeSpan = { _, _ -> null }) diff --git a/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt b/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt index 2f07a1de240..f29396d1068 100644 --- a/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt +++ b/sentry-okhttp/src/main/java/io/sentry/okhttp/SentryOkHttpInterceptor.kt @@ -22,6 +22,7 @@ import io.sentry.TypeCheckHint.SENTRY_REPLAY_NETWORK_DETAILS import io.sentry.clientreport.DiscardReason import io.sentry.okhttp.SentryOkHttpInterceptor.BeforeSpanCallback import io.sentry.transport.CurrentDateProvider +import io.sentry.util.ExceptionUtils import io.sentry.util.IntegrationUtils.addIntegrationToSdkVersion import io.sentry.util.Platform import io.sentry.util.PropagationTargetsUtils @@ -375,7 +376,8 @@ public open class SentryOkHttpInterceptor( val result = try { beforeSpan.execute(span, request, response) - } catch (e: Exception) { + } catch (@Suppress("TooGenericExceptionCaught") e: Throwable) { + ExceptionUtils.rethrowIfFatal(e) span.spanContext.sampled = false if (wasSampled) { scopes.options.clientReportRecorder.recordLostEvent( diff --git a/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt b/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt index 59a8f121f59..7d099f8a971 100644 --- a/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt +++ b/sentry-okhttp/src/test/java/io/sentry/okhttp/SentryOkHttpInterceptorTest.kt @@ -62,6 +62,7 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoMoreInteractions import org.mockito.kotlin.whenever +@Suppress("LargeClass") class SentryOkHttpInterceptorTest { class Fixture { val scopes = mock() @@ -463,7 +464,7 @@ class SentryOkHttpInterceptorTest { fixture.getSut( beforeSpan = { span, _, _ -> span.spanContext.sampled = false - throw IllegalStateException("callback failed") + error("callback failed") } ) fixture.sentryTracer.spanContext.sampled = sampled @@ -480,8 +481,8 @@ class SentryOkHttpInterceptorTest { } @Test - fun `when beforeSpan throws, drops span and preserves response`() { - val failure = IllegalStateException("callback failed") + fun `when beforeSpan throws a nonfatal throwable, drops span and preserves response`() { + val failure = AssertionError("callback failed") val logger = mock() val sut = fixture.getSut( @@ -509,6 +510,16 @@ class SentryOkHttpInterceptorTest { ) } + @Test + fun `fatal error from beforeSpan is not swallowed`() { + val failure = OutOfMemoryError("callback failed") + val sut = fixture.getSut(beforeSpan = { _, _, _ -> throw failure }) + + val thrown = assertFailsWith { sut.newCall(getRequest()).execute() } + + assertThat(thrown).isSameInstanceAs(failure) + } + @Test fun `when beforeSpan throws, event listener still finishes span`() { val sut = diff --git a/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java b/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java index 7ed48d0dc10..25c93a15640 100644 --- a/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java +++ b/sentry-openfeign/src/main/java/io/sentry/openfeign/SentryFeignClient.java @@ -21,6 +21,7 @@ import io.sentry.SpanStatus; import io.sentry.W3CTraceparentHeader; import io.sentry.clientreport.DiscardReason; +import io.sentry.util.ExceptionUtils; import io.sentry.util.Objects; import io.sentry.util.SpanUtils; import io.sentry.util.TracingUtils; @@ -102,7 +103,8 @@ public Response execute(final @NotNull Request request, final @NotNull Request.O ISpan result = span; try { result = beforeSpan.execute(span, request, response); - } catch (Exception e) { + } catch (Throwable e) { + ExceptionUtils.rethrowIfFatal(e); span.getSpanContext().setSampled(false); if (wasSampled) { scopes diff --git a/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt b/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt index 9a2bd98d365..5771802c5b8 100644 --- a/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt +++ b/sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt @@ -319,8 +319,8 @@ class SentryFeignClientTest { } @Test - fun `when beforeSpan throws, drops span and preserves response`() { - val failure = IllegalStateException("callback failed") + fun `when beforeSpan throws a nonfatal throwable, drops span and preserves response`() { + val failure = AssertionError("callback failed") val logger = mock() fixture.sentryOptions.isDebug = true fixture.sentryOptions.setLogger(logger) @@ -345,6 +345,16 @@ class SentryFeignClientTest { ) } + @Test + fun `fatal error from beforeSpan is not swallowed`() { + val failure = OutOfMemoryError("callback failed") + val sut = fixture.getSut(beforeSpan = { _, _, _ -> throw failure }) + + val thrown = assertFailsWith { sut.getOk() } + + assertThat(thrown).isSameInstanceAs(failure) + } + @Test fun `when beforeSpan throws, preserves original request exception`() { val requestFailure = IOException("request failed")