Fix crash on double SDK init (#2679)

* all calls to ISentryExecutorService are now wrapped in a try catch block
* closing the hub will close the profiler and the performance collector
* added ui test for double init
* added ISentryExecutorService.close and isClosed internal methods
* profiling is now stopped on executorService exceptions
* ISentryExecutorService is re-init in Sentry.init if it was shutdown

CHANGELOG.md

@@ -1,9 +1,10 @@
 # Changelog
 
-## Unreleased
+### Unreleased
 
-### Fixes
+## Fixes
 
+- Fix crash on double SDK init ([#2679](https://github.com/getsentry/sentry-java/pull/2679))
 - Track a ttfd span per Activity ([#2673](https://github.com/getsentry/sentry-java/pull/2673))
 
 ## 6.18.0

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

@@ -37,6 +37,7 @@
 import java.util.Map;
 import java.util.WeakHashMap;
 import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.TimeUnit;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -250,11 +251,20 @@ private void startTracing(final @NotNull Activity activity) {
         final @NotNull ISpan ttfdSpan =
             transaction.startChild(
                 TTFD_OP, getTtfdDesc(activityName), ttidStartTime, Instrumenter.SENTRY);
-        ttfdSpanMap.put(activity, ttfdSpan);
-        ttfdAutoCloseFuture =
-            options
-                .getExecutorService()
-                .schedule(() -> finishExceededTtfdSpan(ttfdSpan, ttidSpan), TTFD_TIMEOUT_MILLIS);
+        try {
+          ttfdSpanMap.put(activity, ttfdSpan);
+          ttfdAutoCloseFuture =
+              options
+                  .getExecutorService()
+                  .schedule(() -> finishExceededTtfdSpan(ttfdSpan, ttidSpan), TTFD_TIMEOUT_MILLIS);
+        } catch (RejectedExecutionException e) {
+          options
+              .getLogger()
+              .log(
+                  SentryLevel.ERROR,
+                  "Failed to call the executor. Time to full display span will not be finished automatically. Did you call Sentry.close()?",
+                  e);
+        }
       }
 
       // lets bind to the scope so other integrations can pick it up

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

@@ -34,9 +34,11 @@
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.TimeUnit;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.TestOnly;
 
 final class AndroidTransactionProfiler implements ITransactionProfiler {
 
@@ -77,6 +79,8 @@ final class AndroidTransactionProfiler implements ITransactionProfiler {
       new ArrayDeque<>();
   private final @NotNull Map<String, ProfileMeasurement> measurementsMap = new HashMap<>();
 
+  private @Nullable ITransaction currentTransaction = null;
+
   public AndroidTransactionProfiler(
       final @NotNull Context context,
       final @NotNull SentryAndroidOptions sentryAndroidOptions,
@@ -140,7 +144,16 @@ private void init() {
 
   @Override
   public synchronized void onTransactionStart(final @NotNull ITransaction transaction) {
-    options.getExecutorService().submit(() -> onTransactionStartSafe(transaction));
+    try {
+      options.getExecutorService().submit(() -> onTransactionStartSafe(transaction));
+    } catch (RejectedExecutionException e) {
+      options
+          .getLogger()
+          .log(
+              SentryLevel.ERROR,
+              "Failed to call the executor. Profiling will not be started. Did you call Sentry.close()?",
+              e);
+    }
   }
 
   @SuppressLint("NewApi")
@@ -235,13 +248,24 @@ public void onFrameMetricCollected(
               }
             });
 
+    currentTransaction = transaction;
+
     // We stop profiling after a timeout to avoid huge profiles to be sent
-    scheduledFinish =
-        options
-            .getExecutorService()
-            .schedule(
-                () -> timedOutProfilingData = onTransactionFinish(transaction, true, null),
-                PROFILING_TIMEOUT_MILLIS);
+    try {
+      scheduledFinish =
+          options
+              .getExecutorService()
+              .schedule(
+                  () -> timedOutProfilingData = onTransactionFinish(transaction, true, null),
+                  PROFILING_TIMEOUT_MILLIS);
+    } catch (RejectedExecutionException e) {
+      options
+          .getLogger()
+          .log(
+              SentryLevel.ERROR,
+              "Failed to call the executor. Profiling will not be automatically finished. Did you call Sentry.close()?",
+              e);
+    }
 
     transactionStartNanos = SystemClock.elapsedRealtimeNanos();
     profileStartCpuMillis = Process.getElapsedCpuTime();
@@ -261,10 +285,19 @@ public void onFrameMetricCollected(
           .getExecutorService()
           .submit(() -> onTransactionFinish(transaction, false, performanceCollectionData))
           .get();
-    } catch (ExecutionException e) {
-      options.getLogger().log(SentryLevel.ERROR, "Error finishing profiling: ", e);
-    } catch (InterruptedException e) {
+    } catch (ExecutionException | InterruptedException e) {
       options.getLogger().log(SentryLevel.ERROR, "Error finishing profiling: ", e);
+      // We stop profiling even on exceptions, so profiles don't run indefinitely
+      onTransactionFinish(transaction, false, null);
+    } catch (RejectedExecutionException e) {
+      options
+          .getLogger()
+          .log(
+              SentryLevel.ERROR,
+              "Failed to call the executor. Profiling could not be finished. Did you call Sentry.close()?",
+              e);
+      // We stop profiling even on exceptions, so profiles don't run indefinitely
+      onTransactionFinish(transaction, false, null);
     }
     return null;
   }
@@ -349,6 +382,7 @@ public void onFrameMetricCollected(
     currentProfilingTransactionData = null;
     // We clear the counter in case of a timeout
     transactionsCounter = 0;
+    currentTransaction = null;
 
     if (scheduledFinish != null) {
       scheduledFinish.cancel(true);
@@ -483,6 +517,19 @@ private void putPerformanceCollectionDataInMeasurements(
     }
   }
 
+  @Override
+  public void close() {
+    // we cancel any scheduled work
+    if (scheduledFinish != null) {
+      scheduledFinish.cancel(true);
+      scheduledFinish = null;
+    }
+    // we stop profiling
+    if (currentTransaction != null) {
+      onTransactionFinish(currentTransaction, true, null);
+    }
+  }
+
   /**
    * Get MemoryInfo object representing the memory state of the application.
    *
@@ -503,4 +550,10 @@ private void putPerformanceCollectionDataInMeasurements(
       return null;
     }
   }
+
+  @TestOnly
+  @Nullable
+  ITransaction getCurrentTransaction() {
+    return currentTransaction;
+  }
 }

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

@@ -7,6 +7,7 @@
 import io.sentry.SentryOptions;
 import io.sentry.util.Objects;
 import java.util.concurrent.Future;
+import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import org.jetbrains.annotations.NotNull;
@@ -74,6 +75,13 @@ public void register(@NotNull IHub hub, @NotNull SentryOptions options) {
         }
       }
       androidOptions.getLogger().log(SentryLevel.DEBUG, "SendCachedEnvelopeIntegration installed.");
+    } catch (RejectedExecutionException e) {
+      androidOptions
+          .getLogger()
+          .log(
+              SentryLevel.ERROR,
+              "Failed to call the executor. Cached events will not be sent. Did you call Sentry.close()?",
+              e);
     } catch (Throwable e) {
       androidOptions
           .getLogger()

sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt

@@ -1100,6 +1100,7 @@ class ActivityLifecycleIntegrationTest {
                 return FutureTask {}
             }
             override fun close(timeoutMillis: Long) {}
+            override fun isClosed() = false
         }
         fixture.options.tracesSampleRate = 1.0
         fixture.options.isEnableTimeToFullDisplayTracing = true
@@ -1326,6 +1327,7 @@ class ActivityLifecycleIntegrationTest {
                 return FutureTask {}
             }
             override fun close(timeoutMillis: Long) {}
+            override fun isClosed() = false
         }
         fixture.options.tracesSampleRate = 1.0
         fixture.options.isEnableTimeToFullDisplayTracing = true

sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt

@@ -17,6 +17,7 @@ import io.sentry.TransactionContext
 import io.sentry.android.core.internal.util.SentryFrameMetricsCollector
 import io.sentry.profilemeasurements.ProfileMeasurement
 import io.sentry.test.getCtor
+import io.sentry.test.getProperty
 import org.junit.runner.RunWith
 import org.mockito.kotlin.any
 import org.mockito.kotlin.mock
@@ -75,6 +76,7 @@ class AndroidTransactionProfilerTest {
                 return FutureTask {}
             }
             override fun close(timeoutMillis: Long) {}
+            override fun isClosed() = false
         }
 
         val options = spy(SentryAndroidOptions()).apply {
@@ -150,8 +152,11 @@ class AndroidTransactionProfilerTest {
     @Test
     fun `profiler profiles current transaction`() {
         val profiler = fixture.getSut(context)
+        assertNull(profiler.currentTransaction)
         profiler.onTransactionStart(fixture.transaction1)
+        assertNotNull(profiler.currentTransaction)
         val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null)
+        assertNull(profiler.currentTransaction)
 
         assertNotNull(profilingTraceData)
         assertEquals(profilingTraceData.transactionId, fixture.transaction1.eventId.toString())
@@ -401,4 +406,22 @@ class AndroidTransactionProfilerTest {
             data.measurementsMap[ProfileMeasurement.ID_MEMORY_NATIVE_FOOTPRINT]!!.values.map { it.value }
         )
     }
+
+    @Test
+    fun `profiler stops profiling, clear current transaction and scheduled job on close`() {
+        val profiler = fixture.getSut(context)
+        profiler.onTransactionStart(fixture.transaction1)
+        assertNotNull(profiler.currentTransaction)
+
+        profiler.close()
+        assertNull(profiler.currentTransaction)
+
+        // The timeout scheduled job should be cleared
+        val scheduledJob = profiler.getProperty<Future<*>?>("scheduledFinish")
+        assertNull(scheduledJob)
+
+        // Calling transaction finish returns null, as the profiler was stopped
+        val profilingTraceData = profiler.onTransactionFinish(fixture.transaction1, null)
+        assertNull(profilingTraceData)
+    }
 }

sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt

@@ -115,5 +115,7 @@ class SentryAndroidOptionsTest {
             transaction: ITransaction,
             performanceCollectionData: List<PerformanceCollectionData>?
         ): ProfilingTraceData? = null
+
+        override fun close() {}
     }
 }

sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/SdkInitTests.kt

@@ -0,0 +1,77 @@
+package io.sentry.uitest.android
+
+import androidx.lifecycle.Lifecycle
+import androidx.test.core.app.launchActivity
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import io.sentry.ProfilingTraceData
+import io.sentry.Sentry
+import io.sentry.android.core.SentryAndroidOptions
+import io.sentry.assertEnvelopeItem
+import io.sentry.protocol.SentryTransaction
+import org.junit.runner.RunWith
+import kotlin.test.Test
+import kotlin.test.assertEquals
+
+@RunWith(AndroidJUnit4::class)
+class SdkInitTests : BaseUiTest() {
+
+    @Test
+    fun doubleInitDoesNotThrow() {
+        initSentry(false) { options: SentryAndroidOptions ->
+            options.tracesSampleRate = 1.0
+            options.profilesSampleRate = 1.0
+        }
+        val transaction = Sentry.startTransaction("e2etests", "testInit")
+        val sampleScenario = launchActivity<EmptyActivity>()
+        initSentry(false) { options: SentryAndroidOptions ->
+            options.tracesSampleRate = 1.0
+            options.profilesSampleRate = 1.0
+        }
+        transaction.finish()
+        sampleScenario.moveToState(Lifecycle.State.DESTROYED)
+        val transaction2 = Sentry.startTransaction("e2etests", "testInit")
+        transaction2.finish()
+    }
+
+    @Test
+    fun doubleInitWithSameOptionsDoesNotThrow() {
+        val options = SentryAndroidOptions()
+
+        initSentry(true) {
+            it.tracesSampleRate = 1.0
+            it.profilesSampleRate = 1.0
+            // We use the same executorService before and after closing the SDK
+            it.executorService = options.executorService
+            it.isDebug = true
+        }
+        val transaction = Sentry.startTransaction("e2etests", "testInit")
+        val sampleScenario = launchActivity<EmptyActivity>()
+        initSentry(true) {
+            it.tracesSampleRate = 1.0
+            it.profilesSampleRate = 1.0
+            // We use the same executorService before and after closing the SDK
+            it.executorService = options.executorService
+            it.isDebug = true
+        }
+        relayIdlingResource.increment()
+        transaction.finish()
+        sampleScenario.moveToState(Lifecycle.State.DESTROYED)
+        val transaction2 = Sentry.startTransaction("e2etests2", "testInit")
+        transaction2.finish()
+
+        relay.assert {
+            findEnvelope {
+                assertEnvelopeItem<SentryTransaction>(it.items.toList()).transaction == "e2etests2"
+            }.assert {
+                val transactionItem: SentryTransaction = it.assertItem()
+                // Profiling uses executorService, so if the executorService is shutdown it would fail
+                val profilingTraceData: ProfilingTraceData = it.assertItem()
+                it.assertNoOtherItems()
+                assertEquals("e2etests2", transactionItem.transaction)
+                assertEquals("e2etests2", profilingTraceData.transactionName)
+            }
+            assertNoOtherEnvelopes()
+            assertNoOtherRequests()
+        }
+    }
+}