Address PR feedback

Author: markushi

sentry-android-core/api/sentry-android-core.api

@@ -505,10 +505,11 @@ public class io/sentry/android/core/anr/AnrProfile {
 	public fun <init> (Ljava/util/List;)V
 }
 
-public class io/sentry/android/core/anr/AnrProfileManager {
+public class io/sentry/android/core/anr/AnrProfileManager : java/io/Closeable {
 	public fun <init> (Lio/sentry/SentryOptions;)V
 	public fun add (Lio/sentry/android/core/anr/AnrStackTrace;)V
 	public fun clear ()V
+	public fun close ()V
 	public fun load ()Lio/sentry/android/core/anr/AnrProfile;
 }
 

sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java

@@ -322,16 +322,14 @@ private void applyAnrProfile(
       }
 
       @Nullable AnrProfile anrProfile = null;
-      try {
-        final AnrProfileManager provider = new AnrProfileManager(options);
+      try (final AnrProfileManager provider = new AnrProfileManager(options)) {
         anrProfile = provider.load();
       } catch (Throwable t) {
         options.getLogger().log(SentryLevel.INFO, "Could not retrieve ANR profile");
       }
 
       if (anrProfile != null) {
         options.getLogger().log(SentryLevel.INFO, "ANR profile found");
-        // TODO maybe be less strict around the end timestamp
         if (anrTimestamp >= anrProfile.startTimeMs && anrTimestamp <= anrProfile.endtimeMs) {
           final SentryProfile profile = StackTraceConverter.convert(anrProfile);
           final ProfileChunk chunk =
@@ -351,8 +349,8 @@ private void applyAnrProfile(
           final @Nullable AggregatedStackTrace culprit =
               AnrCulpritIdentifier.identify(anrProfile.stacks);
           if (culprit != null) {
-            // TODO if quality is low (e.g. when culprit is pollNative())
-            // consider throwing the ANR using a static fingerprint to reduce noise
+            // TODO Consider setting a static fingerprint to reduce noise
+            // if culprit quality is low (e.g. when culprit frame is pollNative())
             final @NotNull StackTraceElement[] stack = culprit.getStack();
             if (stack.length > 0) {
               final StackTraceElement stackTraceElement = culprit.getStack()[0];

sentry-android-core/src/main/java/io/sentry/android/core/anr/AggregatedStackTrace.java

@@ -35,7 +35,7 @@ public AggregatedStackTrace(
     this.stack = stack;
     this.stackStartIdx = stackStartIdx;
     this.stackEndIdx = stackEndIdx;
-    this.depth = stackEndIdx - stackStartIdx;
+    this.depth = stackEndIdx - stackStartIdx + 1;
     this.startTimeMs = timestampMs;
     this.endTimeMs = timestampMs;
     this.count = 1;

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrCulpritIdentifier.java

@@ -15,7 +15,7 @@ public class AnrCulpritIdentifier {
   // common Java and Android packages who are less relevant for being the actual culprit
   private static final List<String> lowQualityPackages = new ArrayList<>(9);
 
-  {
+  static {
     lowQualityPackages.add("java.lang");
     lowQualityPackages.add("java.util");
     lowQualityPackages.add("android.app");
@@ -28,25 +28,30 @@ public class AnrCulpritIdentifier {
   }
 
   /**
-   * @param dumps
-   * @return
+   * @param stacks the captured stacktraces
+   * @return the most common occurring stacktrace identified as the culprit
    */
   @Nullable
-  public static AggregatedStackTrace identify(final @NotNull List<AnrStackTrace> dumps) {
-    if (dumps.isEmpty()) {
+  public static AggregatedStackTrace identify(final @NotNull List<AnrStackTrace> stacks) {
+    if (stacks.isEmpty()) {
       return null;
     }
 
     // fold all stacktraces and count their occurrences
-    final Map<Integer, AggregatedStackTrace> stackTraceMap = new HashMap<>();
-    for (final AnrStackTrace dump : dumps) {
+    final @NotNull Map<Integer, AggregatedStackTrace> stackTraceMap = new HashMap<>();
+    for (final AnrStackTrace stackTrace : stacks) {
+
+      if (stackTrace.stack.length < 2) {
+        continue;
+      }
 
       // entry 0 is the most detailed element in the stacktrace
       // so create sub-stacks (1..n, 2..n, ...) to capture the most common root cause of an ANR
-      for (int i = 0; i < dump.stack.length - 1; i++) {
-        final int key = subArrayHashCode(dump.stack, i, dump.stack.length - 1);
+      for (int i = 0; i < stackTrace.stack.length - 1; i++) {
+        // TODO using hashcode is actually a bad key
+        final int key = subArrayHashCode(stackTrace.stack, i, stackTrace.stack.length - 1);
         int quality = 10;
-        final String clazz = dump.stack[i].getClassName();
+        final String clazz = stackTrace.stack[i].getClassName();
         for (String ignoredPackage : lowQualityPackages) {
           if (clazz.startsWith(ignoredPackage)) {
             quality = 1;
@@ -58,14 +63,22 @@ public static AggregatedStackTrace identify(final @NotNull List<AnrStackTrace> d
         if (aggregatedStackTrace == null) {
           aggregatedStackTrace =
               new AggregatedStackTrace(
-                  dump.stack, i, dump.stack.length - 1, dump.timestampMs, quality);
+                  stackTrace.stack,
+                  i,
+                  stackTrace.stack.length - 1,
+                  stackTrace.timestampMs,
+                  quality);
           stackTraceMap.put(key, aggregatedStackTrace);
         } else {
-          aggregatedStackTrace.add(dump.timestampMs);
+          aggregatedStackTrace.add(stackTrace.timestampMs);
         }
       }
     }
 
+    if (stackTraceMap.isEmpty()) {
+      return null;
+    }
+
     // the deepest stacktrace with most count wins
     return Collections.max(
         stackTraceMap.values(),

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfile.java

@@ -23,6 +23,8 @@ public AnrProfile(List<AnrStackTrace> stacks) {
 
     if (!this.stacks.isEmpty()) {
       startTimeMs = this.stacks.get(0).timestampMs;
+
+      // adding 10s to be less strict around end time
       endtimeMs = this.stacks.get(this.stacks.size() - 1).timestampMs + 10_000L;
     } else {
       startTimeMs = 0L;

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfileManager.java

@@ -9,6 +9,7 @@
 import io.sentry.cache.tape.ObjectQueue;
 import io.sentry.cache.tape.QueueFile;
 import java.io.ByteArrayInputStream;
+import java.io.Closeable;
 import java.io.DataInputStream;
 import java.io.DataOutputStream;
 import java.io.File;
@@ -19,7 +20,7 @@
 import org.jetbrains.annotations.Nullable;
 
 @ApiStatus.Internal
-public class AnrProfileManager {
+public class AnrProfileManager implements Closeable {
 
   private static final int MAX_NUM_STACKTRACES =
       (int) ((THRESHOLD_ANR_MS / POLLING_INTERVAL_MS) * 2);
@@ -85,4 +86,9 @@ public void add(AnrStackTrace trace) throws IOException {
   public AnrProfile load() throws IOException {
     return new AnrProfile(queue.asList());
   }
+
+  @Override
+  public void close() throws IOException {
+    queue.close();
+  }
 }

sentry-android-core/src/main/java/io/sentry/android/core/anr/AnrProfilingIntegration.java

@@ -31,7 +31,9 @@ public class AnrProfilingIntegration
 
   private final AtomicBoolean enabled = new AtomicBoolean(true);
   private final Runnable updater = () -> lastMainThreadExecutionTime = SystemClock.uptimeMillis();
-  private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock();
+  private final @NotNull AutoClosableReentrantLock lifecycleLock = new AutoClosableReentrantLock();
+  private final @NotNull AutoClosableReentrantLock profileManagerLock =
+      new AutoClosableReentrantLock();
 
   private volatile long lastMainThreadExecutionTime = SystemClock.uptimeMillis();
   private volatile MainThreadState mainThreadState = MainThreadState.IDLE;
@@ -52,14 +54,21 @@ public void register(@NotNull IScopes scopes, @NotNull SentryOptions options) {
   public void close() throws IOException {
     onBackground();
     enabled.set(false);
+
+    try (final @NotNull ISentryLifecycleToken ignored = profileManagerLock.acquire()) {
+      final @Nullable AnrProfileManager p = profileManager;
+      if (p != null) {
+        p.close();
+      }
+    }
   }
 
   @Override
   public void onForeground() {
     if (!enabled.get()) {
       return;
     }
-    try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
+    try (final @NotNull ISentryLifecycleToken ignored = lifecycleLock.acquire()) {
       if (inForeground) {
         return;
       }
@@ -81,7 +90,7 @@ public void onBackground() {
     if (!enabled.get()) {
       return;
     }
-    try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
+    try (final @NotNull ISentryLifecycleToken ignored = lifecycleLock.acquire()) {
       if (!inForeground) {
         return;
       }
@@ -168,14 +177,17 @@ protected MainThreadState getState() {
   @TestOnly
   @NonNull
   protected AnrProfileManager getProfileManager() {
-    final @Nullable AnrProfileManager r = profileManager;
-    if (r != null) {
-      return r;
-    } else {
-      final AnrProfileManager newManager =
-          new AnrProfileManager(Objects.requireNonNull(options, "Options can't be null"));
-      profileManager = newManager;
-      return newManager;
+    try (final @NotNull ISentryLifecycleToken ignored = profileManagerLock.acquire()) {
+      final @Nullable AnrProfileManager r = profileManager;
+      if (r != null) {
+        return r;
+      } else {
+
+        final AnrProfileManager newManager =
+            new AnrProfileManager(Objects.requireNonNull(options, "Options can't be null"));
+        profileManager = newManager;
+        return newManager;
+      }
     }
   }