From 8f9b3923f6ae79bdbaedfbce6a8f4df5e9158150 Mon Sep 17 00:00:00 2001 From: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> Date: Tue, 24 Nov 2020 19:32:01 +0100 Subject: [PATCH] Fix: Set current thread only if theres no exceptions (#1064) --- CHANGELOG.md | 2 + .../android/core/ManifestMetadataReader.java | 10 ++++- .../core/ManifestMetadataReaderTest.kt | 29 +++++++++++++ .../java/io/sentry/MainEventProcessor.java | 41 +++++++++++-------- .../java/io/sentry/SentryThreadFactory.java | 5 +-- .../java/io/sentry/MainEventProcessorTest.kt | 17 ++++++-- .../java/io/sentry/SentryThreadFactoryTest.kt | 2 +- 7 files changed, 81 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bcd24f46fe..1c123a107c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # vNext +* Fix: Set current thread only if theres no exceptions + # 4.0.0-alpha.1 * Enhancement: Load `sentry.properties` from the application's current working directory (#1046) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java b/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java index 80e2e7eb1f..244e4bb495 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java @@ -10,6 +10,7 @@ import java.util.Locale; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; /** Class responsible for reading values from manifest and setting them to the options */ final class ManifestMetadataReader { @@ -44,6 +45,8 @@ final class ManifestMetadataReader { static final String UNCAUGHT_EXCEPTION_HANDLER_ENABLE = "io.sentry.uncaught-exception-handler.enable"; + static final String ATTACH_THREADS = "io.sentry.attach-threads"; + /** ManifestMetadataReader ctor */ private ManifestMetadataReader() {} @@ -185,6 +188,11 @@ static void applyMetadata( UNCAUGHT_EXCEPTION_HANDLER_ENABLE, options.isEnableUncaughtExceptionHandler()); options.getLogger().log(SentryLevel.DEBUG, "enableUncaughtExceptionHandler read: %s", ndk); options.setEnableUncaughtExceptionHandler(enableUncaughtExceptionHandler); + + final boolean attachThreads = + metadata.getBoolean(ATTACH_THREADS, options.isAttachThreads()); + options.getLogger().log(SentryLevel.DEBUG, "attachThreads read: %s", attachThreads); + options.setAttachThreads(attachThreads); } options .getLogger() @@ -228,7 +236,7 @@ static boolean isAutoInit(final @NotNull Context context, final @NotNull ILogger * @return the Bundle attached to the PackageManager * @throws PackageManager.NameNotFoundException if the package name is non-existent */ - private static Bundle getMetadata(final @NotNull Context context) + private static @Nullable Bundle getMetadata(final @NotNull Context context) throws PackageManager.NameNotFoundException { final ApplicationInfo app = context diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt index 72b723836e..8b4e44cce4 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt @@ -381,4 +381,33 @@ class ManifestMetadataReaderTest { // Assert assertTrue(options.isEnableUncaughtExceptionHandler) } + + @Test + fun `applyMetadata reads attachThreads to options`() { + // Arrange + val options = SentryAndroidOptions() + val bundle = Bundle() + val mockContext = ContextUtilsTest.mockMetaData(metaData = bundle) + bundle.putBoolean(ManifestMetadataReader.ATTACH_THREADS, true) + + // Act + ManifestMetadataReader.applyMetadata(mockContext, options) + + // Assert + assertTrue(options.isAttachThreads) + } + + @Test + fun `applyMetadata reads attachThreads and keep default value if not found`() { + // Arrange + val options = SentryAndroidOptions() + val bundle = Bundle() + val mockContext = ContextUtilsTest.mockMetaData(metaData = bundle) + + // Act + ManifestMetadataReader.applyMetadata(mockContext, options) + + // Assert + assertFalse(options.isAttachThreads) + } } diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index fa1226c7a5..0a8fd749c5 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -18,24 +18,25 @@ public final class MainEventProcessor implements EventProcessor { */ private static final String DEFAULT_ENVIRONMENT = "production"; - private final SentryOptions options; - private final SentryThreadFactory sentryThreadFactory; - private final SentryExceptionFactory sentryExceptionFactory; + private final @NotNull SentryOptions options; + private final @NotNull SentryThreadFactory sentryThreadFactory; + private final @NotNull SentryExceptionFactory sentryExceptionFactory; - MainEventProcessor(final SentryOptions options) { + MainEventProcessor(final @NotNull SentryOptions options) { this.options = Objects.requireNonNull(options, "The SentryOptions is required."); - SentryStackTraceFactory sentryStackTraceFactory = - new SentryStackTraceFactory(options.getInAppExcludes(), options.getInAppIncludes()); + final SentryStackTraceFactory sentryStackTraceFactory = + new SentryStackTraceFactory( + this.options.getInAppExcludes(), this.options.getInAppIncludes()); sentryExceptionFactory = new SentryExceptionFactory(sentryStackTraceFactory); sentryThreadFactory = new SentryThreadFactory(sentryStackTraceFactory, this.options); } MainEventProcessor( - final SentryOptions options, - final SentryThreadFactory sentryThreadFactory, - final SentryExceptionFactory sentryExceptionFactory) { + final @NotNull SentryOptions options, + final @NotNull SentryThreadFactory sentryThreadFactory, + final @NotNull SentryExceptionFactory sentryExceptionFactory) { this.options = Objects.requireNonNull(options, "The SentryOptions is required."); this.sentryThreadFactory = Objects.requireNonNull(sentryThreadFactory, "The SentryThreadFactory is required."); @@ -44,13 +45,14 @@ public final class MainEventProcessor implements EventProcessor { } @Override - public @NotNull SentryEvent process(SentryEvent event, @Nullable Object hint) { + public @NotNull SentryEvent process( + final @NotNull SentryEvent event, final @Nullable Object hint) { if (event.getPlatform() == null) { // this actually means JVM related. event.setPlatform("java"); } - Throwable throwable = event.getThrowable(); + final Throwable throwable = event.getThrowable(); if (throwable != null) { event.setExceptions(sentryExceptionFactory.getSentryExceptions(throwable)); } @@ -69,7 +71,7 @@ public final class MainEventProcessor implements EventProcessor { return event; } - private void processNonCachedEvent(SentryEvent event) { + private void processNonCachedEvent(final @NotNull SentryEvent event) { if (event.getRelease() == null) { event.setRelease(options.getRelease()); } @@ -91,8 +93,12 @@ private void processNonCachedEvent(SentryEvent event) { // collecting threadIds that came from the exception mechanism, so we can mark threads as // crashed properly List mechanismThreadIds = null; - if (event.getExceptions() != null) { - for (SentryException item : event.getExceptions()) { + + final boolean hasExceptions = + event.getExceptions() != null && !event.getExceptions().isEmpty(); + + if (hasExceptions) { + for (final SentryException item : event.getExceptions()) { if (item.getMechanism() != null && item.getThreadId() != null) { if (mechanismThreadIds == null) { mechanismThreadIds = new ArrayList<>(); @@ -104,9 +110,10 @@ private void processNonCachedEvent(SentryEvent event) { if (options.isAttachThreads()) { event.setThreads(sentryThreadFactory.getCurrentThreads(mechanismThreadIds)); - } else if (options.isAttachStacktrace()) { - // when attachStacktrace is enabled, we attach only the current thread and its stack traces - event.setThreads(sentryThreadFactory.getCurrentThread(mechanismThreadIds)); + } else if (options.isAttachStacktrace() && !hasExceptions) { + // when attachStacktrace is enabled, we attach only the current thread and its stack traces, + // if there are no exceptions, exceptions have its own stack traces. + event.setThreads(sentryThreadFactory.getCurrentThread()); } } } diff --git a/sentry/src/main/java/io/sentry/SentryThreadFactory.java b/sentry/src/main/java/io/sentry/SentryThreadFactory.java index ddf8794fc0..f5a75be407 100644 --- a/sentry/src/main/java/io/sentry/SentryThreadFactory.java +++ b/sentry/src/main/java/io/sentry/SentryThreadFactory.java @@ -39,16 +39,15 @@ public SentryThreadFactory( * Converts the current thread to a SentryThread, it assumes its being called from the captured * thread. * - * @param mechanismThreadIds list of threadIds that came from exception mechanism * @return a list of SentryThread with a single item or null */ @Nullable - List getCurrentThread(final @Nullable List mechanismThreadIds) { + List getCurrentThread() { final Map threads = new HashMap<>(); final Thread currentThread = Thread.currentThread(); threads.put(currentThread, currentThread.getStackTrace()); - return getCurrentThreads(threads, mechanismThreadIds); + return getCurrentThreads(threads, null); } /** diff --git a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt index 046852fc21..c5ce69c82c 100644 --- a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt +++ b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt @@ -4,6 +4,7 @@ import com.nhaarman.mockitokotlin2.mock import io.sentry.hints.ApplyScopeData import io.sentry.hints.Cached import io.sentry.protocol.SdkVersion +import java.lang.RuntimeException import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -132,7 +133,7 @@ class MainEventProcessorTest { } @Test - fun `when processing an event and attach threads is disabled, threads should not be set`() { + fun `when attach threads is disabled, threads should not be set`() { val sut = fixture.getSut(attachThreads = false, attachStackTrace = false) var event = SentryEvent() @@ -142,7 +143,7 @@ class MainEventProcessorTest { } @Test - fun `when processing an event and attach threads is enabled, threads should be set`() { + fun `when attach threads is enabled, threads should be set`() { val sut = fixture.getSut() var event = SentryEvent() @@ -152,7 +153,7 @@ class MainEventProcessorTest { } @Test - fun `when processing an event and attach threads is disabled, but attach stacktrace is enabled, current thread should be set`() { + fun `when attach threads is disabled, but attach stacktrace is enabled, current thread should be set`() { val sut = fixture.getSut(attachThreads = false, attachStackTrace = true) var event = SentryEvent() @@ -161,6 +162,16 @@ class MainEventProcessorTest { assertEquals(1, event.threads.count()) } + @Test + fun `when attach threads is disabled, but attach stacktrace is enabled and has exceptions, threads should not be set`() { + val sut = fixture.getSut(attachThreads = false, attachStackTrace = true) + + var event = SentryEvent(RuntimeException("error")) + event = sut.process(event, null) + + assertNull(event.threads) + } + @Test fun `sets sdkVersion in the event`() { val sut = fixture.getSut() diff --git a/sentry/src/test/java/io/sentry/SentryThreadFactoryTest.kt b/sentry/src/test/java/io/sentry/SentryThreadFactoryTest.kt index 88726bf9ed..1730fa4c7d 100644 --- a/sentry/src/test/java/io/sentry/SentryThreadFactoryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryThreadFactoryTest.kt @@ -86,7 +86,7 @@ class SentryThreadFactoryTest { @Test fun `when getCurrentThread is called, returns current thread`() { val sut = fixture.getSut() - val threads = sut.getCurrentThread(null) + val threads = sut.currentThread assertEquals(1, threads!!.count()) } }