Merge branch 'main' into dependabot/github_actions/gradle/actions-bb0…

…c460cbf5354b0cddd15bacdf0d6aaa3e5a32b
getsentry · Oct 15, 2024 · 9dccc0b · 9dccc0b
2 parents f7064cf + 274c295
commit 9dccc0b
Show file tree

Hide file tree

Showing 25 changed files with 744 additions and 69 deletions.
diff --git a/.github/file-filters.yml b/.github/file-filters.yml
@@ -0,0 +1,12 @@
+# This is used by the action https://github.com/dorny/paths-filter
+
+high_risk_code: &high_risk_code
+  # Transport classes
+  - "sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java"
+  - "sentry/src/main/java/io/sentry/transport/HttpConnection.java"
+  - "sentry/src/main/java/io/sentry/transport/QueuedThreadPoolExecutor.java"
+  - "sentry/src/main/java/io/sentry/transport/RateLimiter.java"
+  - "sentry-apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java"
+
+  # Class used by hybrid SDKs
+  - "sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java"
diff --git a/.github/workflows/changes-in-high-risk-code.yml b/.github/workflows/changes-in-high-risk-code.yml
@@ -0,0 +1,49 @@
+name: Changes In High Risk Code
+on:
+  pull_request:
+
+# https://docs.github.com/en/actions/using-jobs/using-concurrency#example-using-a-fallback-value
+concurrency:
+  group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
+  cancel-in-progress: true
+
+jobs:
+  files-changed:
+    name: Detect changed files
+    runs-on: ubuntu-latest
+    # Map a step output to a job output
+    outputs:
+      high_risk_code: ${{ steps.changes.outputs.high_risk_code }}
+      high_risk_code_files: ${{ steps.changes.outputs.high_risk_code_files }}
+    steps:
+      - uses: actions/checkout@v4
+      - name: Get changed files
+        id: changes
+        uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
+        with:
+          token: ${{ github.token }}
+          filters: .github/file-filters.yml
+
+          # Enable listing of files matching each filter.
+          # Paths to files will be available in `${FILTER_NAME}_files` output variable.
+          list-files: csv
+
+  validate-high-risk-code:
+    if: needs.files-changed.outputs.high_risk_code == 'true'
+    needs: files-changed
+    runs-on: ubuntu-latest
+    steps:
+      - name: Comment on PR to notify of changes in high risk files
+        uses: actions/github-script@v7
+        env:
+          high_risk_code: ${{ needs.files-changed.outputs.high_risk_code_files }}
+        with:
+          script: |
+            const highRiskFiles = process.env.high_risk_code;
+            const fileList = highRiskFiles.split(',').map(file => `- [ ] ${file}`).join('\n');
+            github.rest.issues.createComment({
+              issue_number: context.issue.number,
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              body: `### 🚨 Detected changes in high risk code 🚨 \n High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:\n ${fileList}`
+            })
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,9 +2,17 @@
 
 ## Unreleased
 
+### Features
+
+- Add meta option to attach ANR thread dumps ([#3791](https://github.com/getsentry/sentry-java/pull/3791))
+
 ### Fixes
 
+- fix invalid profiles when the transaction name is empty ([#3747](https://github.com/getsentry/sentry-java/pull/3747))
 - Deprecate `enableTracing` option ([#3777](https://github.com/getsentry/sentry-java/pull/3777))
+- Vendor `java.util.Random` and replace `java.security.SecureRandom` usages ([#3783](https://github.com/getsentry/sentry-java/pull/3783))
+- Fix potential ANRs due to NDK scope sync ([#3754](https://github.com/getsentry/sentry-java/pull/3754))
+- Fix potential ANRs due to NDK System.loadLibrary calls ([#3670](https://github.com/getsentry/sentry-java/pull/3670))
 
 ## 7.15.0
 

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2EventProcessor.java
@@ -54,8 +54,8 @@
 import io.sentry.protocol.SentryTransaction;
 import io.sentry.protocol.User;
 import io.sentry.util.HintUtils;
+import io.sentry.util.Random;
 import java.io.File;
-import java.security.SecureRandom;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -83,7 +83,7 @@ public final class AnrV2EventProcessor implements BackfillingEventProcessor {
 
   private final @NotNull SentryExceptionFactory sentryExceptionFactory;
 
-  private final @Nullable SecureRandom random;
+  private final @Nullable Random random;
 
   public AnrV2EventProcessor(
       final @NotNull Context context,
@@ -96,7 +96,7 @@ public AnrV2EventProcessor(
       final @NotNull Context context,
       final @NotNull SentryAndroidOptions options,
       final @NotNull BuildInfoProvider buildInfoProvider,
-      final @Nullable SecureRandom random) {
+      final @Nullable Random random) {
     this.context = ContextUtils.getApplicationContext(context);
     this.options = options;
     this.buildInfoProvider = buildInfoProvider;
@@ -180,7 +180,7 @@ private boolean sampleReplay(final @NotNull SentryEvent event) {
 
     try {
       // we have to sample here with the old sample rate, because it may change between app launches
-      final @NotNull SecureRandom random = this.random != null ? this.random : new SecureRandom();
+      final @NotNull Random random = this.random != null ? this.random : new Random();
       final double replayErrorSampleRateDouble = Double.parseDouble(replayErrorSampleRate);
       if (replayErrorSampleRateDouble < random.nextDouble()) {
         options

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
@@ -26,8 +26,8 @@ final class ManifestMetadataReader {
   static final String SAMPLE_RATE = "io.sentry.sample-rate";
   static final String ANR_ENABLE = "io.sentry.anr.enable";
   static final String ANR_REPORT_DEBUG = "io.sentry.anr.report-debug";
-
   static final String ANR_TIMEOUT_INTERVAL_MILLIS = "io.sentry.anr.timeout-interval-millis";
+  static final String ANR_ATTACH_THREAD_DUMPS = "io.sentry.anr.attach-thread-dumps";
 
   static final String AUTO_INIT = "io.sentry.auto-init";
   static final String NDK_ENABLE = "io.sentry.ndk.enable";
@@ -176,6 +176,9 @@ static void applyMetadata(
                 ANR_TIMEOUT_INTERVAL_MILLIS,
                 options.getAnrTimeoutIntervalMillis()));
 
+        options.setAttachAnrThreadDump(
+            readBool(metadata, logger, ANR_ATTACH_THREAD_DUMPS, options.isAttachAnrThreadDump()));
+
         final String dsn = readString(metadata, logger, DSN, options.getDsn());
         final boolean enabled = readBool(metadata, logger, ENABLE_SENTRY, options.isEnabled());
 

diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt
@@ -360,6 +360,17 @@ class AndroidTransactionProfilerTest {
         verify(mockExecutorService, never()).submit(any<Callable<*>>())
     }
 
+    @Test
+    fun `profiling transaction with empty name fallbacks to unknown`() {
+        val profiler = fixture.getSut(context)
+        profiler.start()
+        profiler.bindTransaction(fixture.transaction1)
+        val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null, fixture.options)
+        assertNotNull(profilingTraceData)
+        assertEquals("unknown", profilingTraceData.transactionName)
+        assertEquals("unknown", profilingTraceData.transactions.first().name)
+    }
+
     @Test
     fun `profiler does not throw if traces cannot be written to disk`() {
         File(fixture.options.profilingTracesDirPath!!).setWritable(false)

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
@@ -246,6 +246,31 @@ class ManifestMetadataReaderTest {
         assertEquals(5000.toLong(), fixture.options.anrTimeoutIntervalMillis)
     }
 
+    @Test
+    fun `applyMetadata reads anr attach thread dump to options`() {
+        // Arrange
+        val bundle = bundleOf(ManifestMetadataReader.ANR_ATTACH_THREAD_DUMPS to true)
+        val context = fixture.getContext(metaData = bundle)
+
+        // Act
+        ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)
+
+        // Assert
+        assertEquals(true, fixture.options.isAttachAnrThreadDump)
+    }
+
+    @Test
+    fun `applyMetadata reads anr attach thread dump to options and keeps default`() {
+        // Arrange
+        val context = fixture.getContext()
+
+        // Act
+        ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)
+
+        // Assert
+        assertEquals(false, fixture.options.isAttachAnrThreadDump)
+    }
+
     @Test
     fun `applyMetadata reads activity breadcrumbs to options`() {
         // Arrange

diff --git a/sentry-android-ndk/build.gradle.kts b/sentry-android-ndk/build.gradle.kts
@@ -105,6 +105,6 @@ dependencies {
 
     testImplementation(kotlin(Config.kotlinStdLib, KotlinCompilerVersion.VERSION))
     testImplementation(Config.TestLibs.kotlinTestJunit)
-
     testImplementation(Config.TestLibs.mockitoKotlin)
+    testImplementation(projects.sentryTestSupport)
 }
diff --git a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java
@@ -31,12 +31,18 @@ public NdkScopeObserver(final @NotNull SentryOptions options) {
   @Override
   public void setUser(final @Nullable User user) {
     try {
-      if (user == null) {
-        // remove user if its null
-        nativeScope.removeUser();
-      } else {
-        nativeScope.setUser(user.getId(), user.getEmail(), user.getIpAddress(), user.getUsername());
-      }
+      options
+          .getExecutorService()
+          .submit(
+              () -> {
+                if (user == null) {
+                  // remove user if its null
+                  nativeScope.removeUser();
+                } else {
+                  nativeScope.setUser(
+                      user.getId(), user.getEmail(), user.getIpAddress(), user.getUsername());
+                }
+              });
     } catch (Throwable e) {
       options.getLogger().log(SentryLevel.ERROR, e, "Scope sync setUser has an error.");
     }
@@ -45,24 +51,36 @@ public void setUser(final @Nullable User user) {
   @Override
   public void addBreadcrumb(final @NotNull Breadcrumb crumb) {
     try {
-      String level = null;
-      if (crumb.getLevel() != null) {
-        level = crumb.getLevel().name().toLowerCase(Locale.ROOT);
-      }
-      final String timestamp = DateUtils.getTimestamp(crumb.getTimestamp());
+      options
+          .getExecutorService()
+          .submit(
+              () -> {
+                String level = null;
+                if (crumb.getLevel() != null) {
+                  level = crumb.getLevel().name().toLowerCase(Locale.ROOT);
+                }
+                final String timestamp = DateUtils.getTimestamp(crumb.getTimestamp());
 
-      String data = null;
-      try {
-        final Map<String, Object> dataRef = crumb.getData();
-        if (!dataRef.isEmpty()) {
-          data = options.getSerializer().serialize(dataRef);
-        }
-      } catch (Throwable e) {
-        options.getLogger().log(SentryLevel.ERROR, e, "Breadcrumb data is not serializable.");
-      }
+                String data = null;
+                try {
+                  final Map<String, Object> dataRef = crumb.getData();
+                  if (!dataRef.isEmpty()) {
+                    data = options.getSerializer().serialize(dataRef);
+                  }
+                } catch (Throwable e) {
+                  options
+                      .getLogger()
+                      .log(SentryLevel.ERROR, e, "Breadcrumb data is not serializable.");
+                }
 
-      nativeScope.addBreadcrumb(
-          level, crumb.getMessage(), crumb.getCategory(), crumb.getType(), timestamp, data);
+                nativeScope.addBreadcrumb(
+                    level,
+                    crumb.getMessage(),
+                    crumb.getCategory(),
+                    crumb.getType(),
+                    timestamp,
+                    data);
+              });
     } catch (Throwable e) {
       options.getLogger().log(SentryLevel.ERROR, e, "Scope sync addBreadcrumb has an error.");
     }
@@ -71,7 +89,7 @@ public void addBreadcrumb(final @NotNull Breadcrumb crumb) {
   @Override
   public void setTag(final @NotNull String key, final @NotNull String value) {
     try {
-      nativeScope.setTag(key, value);
+      options.getExecutorService().submit(() -> nativeScope.setTag(key, value));
     } catch (Throwable e) {
       options.getLogger().log(SentryLevel.ERROR, e, "Scope sync setTag(%s) has an error.", key);
     }
@@ -80,7 +98,7 @@ public void setTag(final @NotNull String key, final @NotNull String value) {
   @Override
   public void removeTag(final @NotNull String key) {
     try {
-      nativeScope.removeTag(key);
+      options.getExecutorService().submit(() -> nativeScope.removeTag(key));
     } catch (Throwable e) {
       options.getLogger().log(SentryLevel.ERROR, e, "Scope sync removeTag(%s) has an error.", key);
     }
@@ -89,7 +107,7 @@ public void removeTag(final @NotNull String key) {
   @Override
   public void setExtra(final @NotNull String key, final @NotNull String value) {
     try {
-      nativeScope.setExtra(key, value);
+      options.getExecutorService().submit(() -> nativeScope.setExtra(key, value));
     } catch (Throwable e) {
       options.getLogger().log(SentryLevel.ERROR, e, "Scope sync setExtra(%s) has an error.", key);
     }
@@ -98,7 +116,7 @@ public void setExtra(final @NotNull String key, final @NotNull String value) {
   @Override
   public void removeExtra(final @NotNull String key) {
     try {
-      nativeScope.removeExtra(key);
+      options.getExecutorService().submit(() -> nativeScope.removeExtra(key));
     } catch (Throwable e) {
       options
           .getLogger()

diff --git a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/SentryNdk.java b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/SentryNdk.java
@@ -1,23 +1,40 @@
 package io.sentry.android.ndk;
 
 import io.sentry.android.core.SentryAndroidOptions;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 
 @ApiStatus.Internal
 public final class SentryNdk {
 
+  private static final @NotNull CountDownLatch loadLibraryLatch = new CountDownLatch(1);
+
   private SentryNdk() {}
 
   static {
-    // On older Android versions, it was necessary to manually call "`System.loadLibrary` on all
-    // transitive dependencies before loading [the] main library."
-    // The dependencies of `libsentry.so` are currently `lib{c,m,dl,log}.so`.
-    // See
-    // https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md#changes-to-library-dependency-resolution
-    System.loadLibrary("log");
-    System.loadLibrary("sentry");
-    System.loadLibrary("sentry-android");
+    new Thread(
+            () -> {
+              // On older Android versions, it was necessary to manually call "`System.loadLibrary`
+              // on all
+              // transitive dependencies before loading [the] main library."
+              // The dependencies of `libsentry.so` are currently `lib{c,m,dl,log}.so`.
+              // See
+              // https://android.googlesource.com/platform/bionic/+/master/android-changes-for-ndk-developers.md#changes-to-library-dependency-resolution
+              try {
+                System.loadLibrary("log");
+                System.loadLibrary("sentry");
+                System.loadLibrary("sentry-android");
+              } catch (Throwable t) {
+                // ignored
+                // if loadLibrary() fails, the later init() will throw an exception anyway
+              } finally {
+                loadLibraryLatch.countDown();
+              }
+            },
+            "SentryNdkLoadLibs")
+        .start();
   }
 
   private static native void initSentryNative(@NotNull final SentryAndroidOptions options);
@@ -31,14 +48,23 @@ private SentryNdk() {}
    */
   public static void init(@NotNull final SentryAndroidOptions options) {
     SentryNdkUtil.addPackage(options.getSdkVersion());
-    initSentryNative(options);
+    try {
+      if (loadLibraryLatch.await(2000, TimeUnit.MILLISECONDS)) {
+        initSentryNative(options);
 
-    // only add scope sync observer if the scope sync is enabled.
-    if (options.isEnableScopeSync()) {
-      options.addScopeObserver(new NdkScopeObserver(options));
-    }
+        // only add scope sync observer if the scope sync is enabled.
+        if (options.isEnableScopeSync()) {
+          options.addScopeObserver(new NdkScopeObserver(options));
+        }
 
-    options.setDebugImagesLoader(new DebugImagesLoader(options, new NativeModuleListLoader()));
+        options.setDebugImagesLoader(new DebugImagesLoader(options, new NativeModuleListLoader()));
+      } else {
+        throw new IllegalStateException("Timeout waiting for Sentry NDK library to load");
+      }
+    } catch (InterruptedException e) {
+      throw new IllegalStateException(
+          "Thread interrupted while waiting for NDK libs to be loaded", e);
+    }
   }
 
   /** Closes the NDK integration */