diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ff6b1f542e..d1a686ad4a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ - Session Replay: Fix `SentryOkHttpInterceptor` hanging forever on responses whose body has no known length, such as Server-Sent Events or a gzipped response, when network details are enabled for the URL ([#6231](https://github.com/getsentry/sentry-java/pull/6231)) - Session Replay: `SentryOkHttpInterceptor` now captures network request and response details only while Session Replay is recording or buffering ([#6486](https://github.com/getsentry/sentry-java/pull/6486)) - Stop marking an OkHttp call span as failed when a connection attempt to one of the host's addresses fails but OkHttp connects on another ([#6472](https://github.com/getsentry/sentry-java/pull/6472)) +- Fix an ANR in `SentryAndroid.init` while the connectivity and telephony callbacks are being registered ([#6511](https://github.com/getsentry/sentry-java/pull/6511)) ### Dependencies diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java index 62a212e6dcc..4853605b6e8 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java @@ -48,6 +48,10 @@ public final class AndroidConnectionStatusProvider private final @NotNull CellularNetworkTechnologyProvider cellularNetworkTechnologyProvider; private final @Nullable Handler handler; private final @NotNull AutoClosableReentrantLock lock = new AutoClosableReentrantLock(); + // Guards registering and unregistering the callbacks, so `lock` is never held across those binder + // calls. When both are held the order is registrationLock, then `lock`. + private final @NotNull AutoClosableReentrantLock registrationLock = + new AutoClosableReentrantLock(); private volatile @Nullable NetworkCallback networkCallback; private static final @NotNull AutoClosableReentrantLock connectivityManagerLock = @@ -200,7 +204,7 @@ private void ensureNetworkCallbackRegistered() { return; // Already registered } - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { + try (final @NotNull ISentryLifecycleToken ignored = registrationLock.acquire()) { if (networkCallback != null) { return; } @@ -457,7 +461,8 @@ public boolean addConnectionStatusObserver(final @NotNull IConnectionStatusObser try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { connectionStatusObservers.add(observer); } - ensureNetworkCallbackRegistered(); + //noinspection Convert2MethodRef + submitSafe(() -> ensureNetworkCallbackRegistered()); // Network callback is already registered during initialization return networkCallback != null; @@ -472,11 +477,7 @@ public void removeConnectionStatusObserver(final @NotNull IConnectionStatusObser } private void unregisterNetworkCallback(final boolean clearObservers) { - try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) { - if (clearObservers) { - connectionStatusObservers.clear(); - } - + try (final @NotNull ISentryLifecycleToken ignored = registrationLock.acquire()) { final @Nullable NetworkCallback callbackRef = networkCallback; networkCallback = null; @@ -484,10 +485,16 @@ private void unregisterNetworkCallback(final boolean clearObservers) { unregisterNetworkCallback(context, options.getLogger(), callbackRef); } cellularNetworkTechnologyProvider.unregister(); - // Clear cached state - cachedNetworkCapabilities = null; - currentNetwork = null; - cacheFreshUntil = Deadline.passed(ticker); + + try (final @NotNull ISentryLifecycleToken ignored2 = lock.acquire()) { + if (clearObservers) { + connectionStatusObservers.clear(); + } + // Clear cached state + cachedNetworkCapabilities = null; + currentNetwork = null; + cacheFreshUntil = Deadline.passed(ticker); + } } options.getLogger().log(SentryLevel.DEBUG, "Network callback unregistered"); } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt index 56a2a12928c..84652bef351 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProviderTest.kt @@ -26,9 +26,11 @@ import io.sentry.android.core.AppState import io.sentry.android.core.BuildInfoProvider import io.sentry.android.core.ContextUtils import io.sentry.android.core.SystemEventsBreadcrumbsIntegration +import io.sentry.test.DeferredExecutorService import io.sentry.test.ImmediateExecutorService import io.sentry.time.TestMonotonicTicker import java.util.concurrent.TimeUnit.MINUTES +import kotlin.concurrent.thread import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test @@ -45,6 +47,7 @@ import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.argThat import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.clearInvocations +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.mock import org.mockito.kotlin.mockingDetails @@ -275,6 +278,39 @@ class AndroidConnectionStatusProviderTest { provider.close() } + @Test + @Config(sdk = [Build.VERSION_CODES.S]) + fun `Adding an observer does not wait for the callbacks to be registered`() { + val telephonyManager = mock() + whenever(contextMock.getSystemService(eq(Context.TELEPHONY_SERVICE))) + .thenReturn(telephonyManager) + whenever(buildInfo.sdkInfoVersion).thenReturn(Build.VERSION_CODES.S) + val executor = DeferredExecutorService() + options.executorService = executor + val provider = AndroidConnectionStatusProvider(contextMock, options, buildInfo, ticker) + + // Both registrations are binder calls, so another thread adding an observer while either of + // them is running must not wait for it. + val observersAdded = mutableListOf() + val addObserverFromOtherThread = { + val other = thread { provider.addConnectionStatusObserver {} } + other.join(5000) + observersAdded.add(!other.isAlive) + } + doAnswer { addObserverFromOtherThread() } + .whenever(connectivityManager) + .registerDefaultNetworkCallback(any()) + doAnswer { addObserverFromOtherThread() } + .whenever(telephonyManager) + .registerTelephonyCallback(any(), any()) + + executor.runAll() + + assertEquals(listOf(true, true), observersAdded) + options.executorService = ImmediateExecutorService() + provider.close() + } + @Test fun `When on cellular with a known technology, the effective type is the generation`() { whenever(networkCapabilities.hasTransport(eq(TRANSPORT_WIFI))).thenReturn(false)