fix(anr): Always report stacktraces for ANRv1 (#4918)

* refactor(core): Decouple SentryThreadFactory from SentryOptions

* docs: Add PR #4918 to CHANGELOG

* refactor(core): Update API dump after thread factory changes

* Address PR feedback

* Update CHANGELOG.md

* Update CHANGELOG

* Update CHANGELOG.md

Author: markushi

CHANGELOG.md

@@ -7,6 +7,10 @@
 - Avoid forking `rootScopes` for Reactor if current thread has `NoOpScopes` ([#4793](https://github.com/getsentry/sentry-java/pull/4793))
   - This reduces the SDKs overhead by avoiding unnecessary scope forks
 
+### Fixes 
+
+- Fix missing thread stacks for ANRv1 events ([#4918](https://github.com/getsentry/sentry-java/pull/4918))
+
 ## 8.27.1
 
 ### Fixes

sentry/api/sentry.api

@@ -3865,7 +3865,7 @@ public final class io/sentry/SentryStackTraceFactory {
 }
 
 public final class io/sentry/SentryThreadFactory {
-	public fun <init> (Lio/sentry/SentryStackTraceFactory;Lio/sentry/SentryOptions;)V
+	public fun <init> (Lio/sentry/SentryStackTraceFactory;)V
 }
 
 public final class io/sentry/SentryTraceHeader {

sentry/src/main/java/io/sentry/MainEventProcessor.java

@@ -35,7 +35,7 @@ public MainEventProcessor(final @NotNull SentryOptions options) {
         new SentryStackTraceFactory(this.options);
 
     sentryExceptionFactory = new SentryExceptionFactory(sentryStackTraceFactory);
-    sentryThreadFactory = new SentryThreadFactory(sentryStackTraceFactory, this.options);
+    sentryThreadFactory = new SentryThreadFactory(sentryStackTraceFactory);
   }
 
   MainEventProcessor(
@@ -255,17 +255,20 @@ private void setThreads(final @NotNull SentryEvent event, final @NotNull Hint hi
       if (options.isAttachThreads() || HintUtils.hasType(hint, AbnormalExit.class)) {
         final Object sentrySdkHint = HintUtils.getSentrySdkHint(hint);
         boolean ignoreCurrentThread = false;
+        boolean attachStacktrace = options.isAttachStacktrace();
         if (sentrySdkHint instanceof AbnormalExit) {
           ignoreCurrentThread = ((AbnormalExit) sentrySdkHint).ignoreCurrentThread();
+          attachStacktrace = true;
         }
         event.setThreads(
-            sentryThreadFactory.getCurrentThreads(mechanismThreadIds, ignoreCurrentThread));
+            sentryThreadFactory.getCurrentThreads(
+                mechanismThreadIds, ignoreCurrentThread, attachStacktrace));
       } else if (options.isAttachStacktrace()
           && (eventExceptions == null || eventExceptions.isEmpty())
           && !isCachedHint(hint)) {
         // 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());
+        event.setThreads(sentryThreadFactory.getCurrentThread(options.isAttachStacktrace()));
       }
     }
   }

sentry/src/main/java/io/sentry/SentryThreadFactory.java

@@ -20,21 +20,14 @@ public final class SentryThreadFactory {
   /** the SentryStackTraceFactory */
   private final @NotNull SentryStackTraceFactory sentryStackTraceFactory;
 
-  /** the SentryOptions. */
-  private final @NotNull SentryOptions options;
-
   /**
    * ctor SentryThreadFactory that takes a SentryStackTraceFactory
    *
    * @param sentryStackTraceFactory the SentryStackTraceFactory
-   * @param options the SentryOptions
    */
-  public SentryThreadFactory(
-      final @NotNull SentryStackTraceFactory sentryStackTraceFactory,
-      final @NotNull SentryOptions options) {
+  public SentryThreadFactory(final @NotNull SentryStackTraceFactory sentryStackTraceFactory) {
     this.sentryStackTraceFactory =
         Objects.requireNonNull(sentryStackTraceFactory, "The SentryStackTraceFactory is required.");
-    this.options = Objects.requireNonNull(options, "The SentryOptions is required");
   }
 
   /**
@@ -44,12 +37,12 @@ public SentryThreadFactory(
    * @return a list of SentryThread with a single item or null
    */
   @Nullable
-  List<SentryThread> getCurrentThread() {
+  List<SentryThread> getCurrentThread(final boolean attachStackTrace) {
     final Map<Thread, StackTraceElement[]> threads = new HashMap<>();
     final Thread currentThread = Thread.currentThread();
     threads.put(currentThread, currentThread.getStackTrace());
 
-    return getCurrentThreads(threads, null, false);
+    return getCurrentThreads(threads, null, false, attachStackTrace);
   }
 
   /**
@@ -63,13 +56,18 @@ List<SentryThread> getCurrentThread() {
    */
   @Nullable
   List<SentryThread> getCurrentThreads(
-      final @Nullable List<Long> mechanismThreadIds, final boolean ignoreCurrentThread) {
-    return getCurrentThreads(Thread.getAllStackTraces(), mechanismThreadIds, ignoreCurrentThread);
+      final @Nullable List<Long> mechanismThreadIds,
+      final boolean ignoreCurrentThread,
+      final boolean attachStackTrace) {
+    return getCurrentThreads(
+        Thread.getAllStackTraces(), mechanismThreadIds, ignoreCurrentThread, attachStackTrace);
   }
 
   @Nullable
-  List<SentryThread> getCurrentThreads(final @Nullable List<Long> mechanismThreadIds) {
-    return getCurrentThreads(Thread.getAllStackTraces(), mechanismThreadIds, false);
+  List<SentryThread> getCurrentThreads(
+      final @Nullable List<Long> mechanismThreadIds, final boolean attachStackTrace) {
+    return getCurrentThreads(
+        Thread.getAllStackTraces(), mechanismThreadIds, false, attachStackTrace);
   }
 
   /**
@@ -87,7 +85,8 @@ List<SentryThread> getCurrentThreads(final @Nullable List<Long> mechanismThreadI
   List<SentryThread> getCurrentThreads(
       final @NotNull Map<Thread, StackTraceElement[]> threads,
       final @Nullable List<Long> mechanismThreadIds,
-      final boolean ignoreCurrentThread) {
+      final boolean ignoreCurrentThread,
+      final boolean attachStackTrace) {
     List<SentryThread> result = null;
 
     final Thread currentThread = Thread.currentThread();
@@ -109,7 +108,7 @@ List<SentryThread> getCurrentThreads(
                     && mechanismThreadIds.contains(thread.getId())
                     && !ignoreCurrentThread);
 
-        result.add(getSentryThread(crashed, item.getValue(), item.getKey()));
+        result.add(getSentryThread(crashed, item.getValue(), item.getKey(), attachStackTrace));
       }
     }
 
@@ -127,7 +126,8 @@ List<SentryThread> getCurrentThreads(
   private @NotNull SentryThread getSentryThread(
       final boolean crashed,
       final @NotNull StackTraceElement[] stackFramesElements,
-      final @NotNull Thread thread) {
+      final @NotNull Thread thread,
+      final boolean attachStacktrace) {
     final SentryThread sentryThread = new SentryThread();
 
     sentryThread.setName(thread.getName());
@@ -137,15 +137,17 @@ List<SentryThread> getCurrentThreads(
     sentryThread.setState(thread.getState().name());
     sentryThread.setCrashed(crashed);
 
-    final List<SentryStackFrame> frames =
-        sentryStackTraceFactory.getStackFrames(stackFramesElements, false);
+    if (attachStacktrace) {
+      final List<SentryStackFrame> frames =
+          sentryStackTraceFactory.getStackFrames(stackFramesElements, false);
 
-    if (options.isAttachStacktrace() && frames != null && !frames.isEmpty()) {
-      final SentryStackTrace sentryStackTrace = new SentryStackTrace(frames);
-      // threads are always gotten either via Thread.currentThread() or Thread.getAllStackTraces()
-      sentryStackTrace.setSnapshot(true);
+      if (frames != null && !frames.isEmpty()) {
+        final SentryStackTrace sentryStackTrace = new SentryStackTrace(frames);
+        // threads are always gotten either via Thread.currentThread() or Thread.getAllStackTraces()
+        sentryStackTrace.setSnapshot(true);
 
-      sentryThread.setStacktrace(sentryStackTrace);
+        sentryThread.setStacktrace(sentryStackTrace);
+      }
     }
 
     return sentryThread;

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

@@ -249,7 +249,7 @@ class MainEventProcessorTest {
   }
 
   @Test
-  fun `when attach threads is disabled, but the hint is Abnormal, still sets threads`() {
+  fun `when attach threads is disabled, but the hint is Abnormal, still sets threads and stacktraces`() {
     val sut = fixture.getSut(attachThreads = false, attachStackTrace = false)
 
     var event = SentryEvent(RuntimeException("error"))
@@ -258,6 +258,20 @@ class MainEventProcessorTest {
 
     assertNotNull(event.threads)
     assertEquals(1, event.threads!!.count { it.isCrashed == true })
+    assertNotNull(event.threads!!.first().stacktrace)
+  }
+
+  @Test
+  fun `when attach threads is enabled, but stacktraces is not its reflected`() {
+    val sut = fixture.getSut(attachThreads = true, attachStackTrace = false)
+
+    var event = SentryEvent(RuntimeException("error"))
+    val hint = Hint()
+    event = sut.process(event, hint)
+
+    assertNotNull(event.threads)
+    assertEquals(1, event.threads!!.count { it.isCrashed == true })
+    assertNull(event.threads!!.first().stacktrace)
   }
 
   @Test

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

@@ -10,13 +10,9 @@ import kotlin.test.assertTrue
 
 class SentryThreadFactoryTest {
   class Fixture {
-    internal fun getSut(attachStacktrace: Boolean = true) =
+    internal fun getSut() =
       SentryThreadFactory(
-        SentryStackTraceFactory(SentryOptions().apply { addInAppExclude("io.sentry") }),
-        with(SentryOptions()) {
-          isAttachStacktrace = attachStacktrace
-          this
-        },
+        SentryStackTraceFactory(SentryOptions().apply { addInAppExclude("io.sentry") })
       )
   }
 
@@ -25,34 +21,37 @@ class SentryThreadFactoryTest {
   @Test
   fun `when getCurrentThreads is called, not empty result`() {
     val sut = fixture.getSut()
-    val threads = sut.getCurrentThreads(null)
+    val threads = sut.getCurrentThreads(null, false)
     assertNotEquals(0, threads!!.count())
   }
 
   @Test
   fun `when currentThreads is called, current thread is marked crashed`() {
     val sut = fixture.getSut()
-    assertEquals(1, sut.getCurrentThreads(null)!!.filter { it.isCrashed == true }.count())
+    assertEquals(1, sut.getCurrentThreads(null, false)!!.filter { it.isCrashed == true }.count())
   }
 
   @Test
   fun `when currentThreads is called with ignoreCurrentThread, current thread is not marked crashed`() {
     val sut = fixture.getSut()
-    assertEquals(0, sut.getCurrentThreads(null, true)!!.filter { it.isCrashed == true }.count())
+    assertEquals(
+      0,
+      sut.getCurrentThreads(null, true, false)!!.filter { it.isCrashed == true }.count(),
+    )
   }
 
   @Test
   fun `when currentThreads is called, thread state is captured`() {
     val sut = fixture.getSut()
-    assertTrue(sut.getCurrentThreads(null)!!.all { it.state != null })
+    assertTrue(sut.getCurrentThreads(null, false)!!.all { it.state != null })
   }
 
   @Test
   fun `when currentThreads is called, some thread stack frames are captured`() {
     val sut = fixture.getSut()
     assertTrue(
       sut
-        .getCurrentThreads(null)!!
+        .getCurrentThreads(null, true)!!
         .filter { it.stacktrace != null }
         .any { it.stacktrace!!.frames!!.count() > 0 }
     )
@@ -63,18 +62,18 @@ class SentryThreadFactoryTest {
     val sut = fixture.getSut()
     assertTrue(
       sut
-        .getCurrentThreads(null)!!
+        .getCurrentThreads(null, true)!!
         .filter { it.stacktrace != null }
         .any { it.stacktrace!!.snapshot == true }
     )
   }
 
   @Test
   fun `when currentThreads and attachStacktrace is disabled, stack frames are not captured`() {
-    val sut = fixture.getSut(false)
+    val sut = fixture.getSut()
     assertFalse(
       sut
-        .getCurrentThreads(null)!!
+        .getCurrentThreads(null, false)!!
         .filter { it.stacktrace != null }
         .any { it.stacktrace!!.frames!!.count() > 0 }
     )
@@ -87,15 +86,15 @@ class SentryThreadFactoryTest {
     val currentThread = Thread.currentThread()
     stackTraces.remove(currentThread)
 
-    val threads = sut.getCurrentThreads(stackTraces, null, false)
+    val threads = sut.getCurrentThreads(stackTraces, null, false, false)
 
     assertNotNull(threads!!.firstOrNull { it.id == currentThread.id })
   }
 
   @Test
   fun `When passing empty param to getCurrentThreads, returns null`() {
     val sut = fixture.getSut()
-    val threads = sut.getCurrentThreads(mapOf(), null, false)
+    val threads = sut.getCurrentThreads(mapOf(), null, false, false)
 
     assertNull(threads)
   }
@@ -108,15 +107,15 @@ class SentryThreadFactoryTest {
     val stacktraces = emptyArray<StackTraceElement>()
     val threadList = mutableMapOf(thread to stacktraces)
 
-    val threads = sut.getCurrentThreads(threadList, threadIds, false)
+    val threads = sut.getCurrentThreads(threadList, threadIds, false, false)
 
     assertNotNull(threads!!.firstOrNull { it.isCrashed == true })
   }
 
   @Test
   fun `when getCurrentThread is called, returns current thread`() {
     val sut = fixture.getSut()
-    val threads = sut.currentThread
+    val threads = sut.getCurrentThread(false)
     assertEquals(1, threads!!.count())
   }
 }