diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt index 29fca0b41..894d4e912 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt @@ -50,7 +50,8 @@ internal class OneSignalImp( private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO, ) : IOneSignal, IServiceProvider { - private val suspendCompletion = CompletableDeferred() + @Volatile + private var suspendCompletion = CompletableDeferred() @Volatile private var initState: InitState = InitState.NOT_STARTED @@ -295,9 +296,8 @@ internal class OneSignalImp( context: Context, appId: String, ): Boolean { - Logging.log(LogLevel.DEBUG, "Calling deprecated initWithContextSuspend(context: $context, appId: $appId)") + Logging.log(LogLevel.DEBUG, "initWithContext(context: $context, appId: $appId)") - // do not do this again if already initialized or init is in progress synchronized(initLock) { if (initState.isSDKAccessible()) { Logging.log(LogLevel.DEBUG, "initWithContext: SDK already initialized or in progress") @@ -305,24 +305,33 @@ internal class OneSignalImp( } initState = InitState.IN_PROGRESS + suspendCompletion = CompletableDeferred() } - // FeatureManager depends on ConfigModelStore/PreferencesService which requires appContext. - // Ensure app context is available before evaluating feature gates. - ensureApplicationServiceStarted(context) + initFailureException = IllegalStateException("OneSignal initWithContext failed.") - if (isBackgroundThreadingEnabled) { - // init in background and return immediately to ensure non-blocking + // Once initState is published as IN_PROGRESS, init MUST reach a terminal state — otherwise + // waiters of suspendCompletion (accessors via getServiceWithFeatureGate, login/logout via + // requireInitForOperation) would block forever, and subsequent initWithContext calls would + // short-circuit because IN_PROGRESS counts as "accessible". Catch synchronous failures + // (e.g. ApplicationService.start's `applicationContext as Application` cast failing in + // restricted hosts) and transition to FAILED before rethrowing so the caller can recover. + try { + // FeatureManager depends on ConfigModelStore/PreferencesService which requires appContext. + // Ensure app context is available before evaluating feature gates. + ensureApplicationServiceStarted(context) + + // Always dispatch init asynchronously so this method never blocks the caller. + // Callers that need to wait (accessors, login, logout) will block via suspendCompletion. suspendifyOnIO { internalInit(context, appId) } - return true - } - - // Legacy FF-OFF behavior intentionally blocks caller thread until initialization completes. - return runBlocking(runtimeIoDispatcher) { - internalInit(context, appId) + } catch (e: Exception) { + initFailureException?.addSuppressed(e) + completeInit(InitState.FAILED) + throw e } + return true } /** @@ -333,48 +342,57 @@ internal class OneSignalImp( return initWithContextSuspend(context, null) } + @Suppress("TooGenericExceptionCaught") private fun internalInit( context: Context, appId: String?, ): Boolean { - // Check whether current Android user is accessible. - // Return early if it is inaccessible, as we are unable to complete initialization without access - // to device storage like SharedPreferences. - if (!AndroidUtils.isAndroidUserUnlocked(context)) { - Logging.warn("initWithContext called when device storage is locked, no user data is accessible!") - initState = InitState.FAILED - notifyInitComplete() - return false - } - - initEssentials(context) - - val startupService = bootstrapServices() - - // Now that the IoC container is ready, subscribe the Otel lifecycle - // manager to config store events so it reacts to fresh remote config. - otelManager?.subscribeToConfigStore(services.getService()) - - val result = resolveAppId(appId, configModel, preferencesService) - if (result.failed) { - val message = "suspendInitInternal: no appId provided or found in local storage. Please pass a valid appId to initWithContext()." - val exception = IllegalStateException(message) - // attach the real crash cause to the init failure exception that will be throw shortly after - initFailureException?.addSuppressed(exception) - Logging.warn(message) - initState = InitState.FAILED - notifyInitComplete() - return false - } - configModel.appId = result.appId!! // safe because failed is false - val forceCreateUser = result.forceCreateUser - - updateConfig() - userSwitcher.initUser(forceCreateUser) - startupService.scheduleStart() - initState = InitState.SUCCESS - notifyInitComplete() - return true + try { + // Check whether current Android user is accessible. + // Return early if it is inaccessible, as we are unable to complete initialization without access + // to device storage like SharedPreferences. + if (!AndroidUtils.isAndroidUserUnlocked(context)) { + Logging.warn("initWithContext called when device storage is locked, no user data is accessible!") + completeInit(InitState.FAILED) + return false + } + + initEssentials(context) + + val startupService = bootstrapServices() + + // Now that the IoC container is ready, subscribe the Otel lifecycle + // manager to config store events so it reacts to fresh remote config. + otelManager?.subscribeToConfigStore(services.getService()) + + val result = resolveAppId(appId, configModel, preferencesService) + if (result.failed) { + val message = "suspendInitInternal: no appId provided or found in local storage. Please pass a valid appId to initWithContext()." + val exception = IllegalStateException(message) + // attach the real crash cause to the init failure exception that will be throw shortly after + initFailureException?.addSuppressed(exception) + Logging.warn(message) + completeInit(InitState.FAILED) + return false + } + configModel.appId = result.appId!! // safe because failed is false + val forceCreateUser = result.forceCreateUser + + updateConfig() + userSwitcher.initUser(forceCreateUser) + startupService.scheduleStart() + completeInit(InitState.SUCCESS) + return true + } catch (e: Exception) { + // Defense-in-depth: suspendifyOnIO swallows exceptions internally, so any unhandled + // throw from a step above (a service throwing during bootstrap, a dependency failing + // to resolve, etc.) would otherwise leave suspendCompletion uncompleted and the SDK + // wedged in IN_PROGRESS forever — accessors would deadlock on suspendCompletion.await(). + Logging.error("Unexpected exception during OneSignal initialization", e) + initFailureException?.addSuppressed(e) + completeInit(InitState.FAILED) + throw e + } } override fun login( @@ -386,9 +404,7 @@ internal class OneSignalImp( if (isBackgroundThreadingEnabled) { waitForInit(operationName = "login") } else { - if (!isInitialized) { - throw IllegalStateException("Must call 'initWithContext' before 'login'") - } + requireInitForOperation("login") } val context = loginHelper.switchUser(externalId, jwtBearerToken) ?: return @@ -410,9 +426,7 @@ internal class OneSignalImp( if (isBackgroundThreadingEnabled) { waitForInit(operationName = "logout") } else { - if (!isInitialized) { - throw IllegalStateException("Must call 'initWithContext' before 'logout'") - } + requireInitForOperation("logout") } val context = logoutHelper.switchUser() ?: return @@ -436,6 +450,59 @@ internal class OneSignalImp( override fun getAllServices(c: Class): List = services.getAllServices(c) + /** + * Ensures initialization is complete before proceeding with an operation. + * Blocks if init is in progress; throws immediately if not started or failed. + */ + private fun requireInitForOperation(operationName: String) { + when (initState) { + InitState.NOT_STARTED -> + throw IllegalStateException("Must call 'initWithContext' before '$operationName'") + InitState.IN_PROGRESS -> { + warnIfBlockingOnMainThread(operationName) + waitForInit(operationName = operationName) + } + InitState.FAILED -> + throw initFailureException + ?: IllegalStateException("Initialization failed before '$operationName'") + InitState.SUCCESS -> {} + } + } + + /** + * Make the legacy-mode (FF-off) main-thread blocking behavior explicit. Pre-#2605 there was + * no IN_PROGRESS window observable from accessors — [initWithContext] ran [internalInit] + * synchronously via `runBlocking`, so any blocking happened inside `initWithContext` itself. + * Now that init dispatches asynchronously, the first accessor on the main thread can block on + * [runBlocking] inside [waitForInit]/[waitAndReturn] until init completes. Total ANR risk is + * roughly equivalent to pre-#2605, just shifted in time, but the block is no longer obviously + * located in `initWithContext` — so we log a warning that points callers to the suspend API + * or a background thread when they care about UI responsiveness. + * + * FF-on mode already accepts the ANR-vs-throw trade-off (see [waitUntilInitInternal]); the + * warning here is only useful as a behavior-change signal for legacy mode. + */ + @Suppress("TooGenericExceptionCaught") + private fun warnIfBlockingOnMainThread(operationName: String?) { + if (isBackgroundThreadingEnabled) return + val onMain = + try { + AndroidUtils.isRunningOnMainThread() + } catch (e: RuntimeException) { + // Looper.getMainLooper() may be unavailable in test environments — skip the warning. + return + } + if (!onMain) return + val target = operationName?.let { "'$it'" } ?: "this OneSignal API" + Logging.warn( + "Calling $target on the main thread while OneSignal initialization is still in progress. " + + "This will block the UI thread until init completes (ANR risk on slow devices). " + + "Prefer calling from a background thread, or use the suspend API " + + "(OneSignal.initWithContextSuspend, OneSignal.getUser(), OneSignal.loginSuspend(), etc.) " + + "from a coroutine.", + ) + } + /** * Blocking version that waits for initialization to complete. * Uses runBlocking to bridge to the suspend implementation. @@ -450,10 +517,23 @@ internal class OneSignalImp( } /** - * Notifies both blocking and suspend callers that initialization is complete + * Atomically transitions [initState] to a terminal state (SUCCESS or FAILED) + * and completes [suspendCompletion] so any waiters unblock. + * + * Both the state write and the completion run under [initLock]. This closes a + * race where another caller could observe the terminal state in between the two + * writes, call `initWithContext` to flip back to IN_PROGRESS, and replace + * [suspendCompletion] with a fresh deferred — at which point this thread's + * completion would prematurely unblock waiters of the *second* init. */ - private fun notifyInitComplete() { - suspendCompletion.complete(Unit) + private fun completeInit(terminalState: InitState) { + require(terminalState == InitState.SUCCESS || terminalState == InitState.FAILED) { + "completeInit requires a terminal state, got $terminalState" + } + synchronized(initLock) { + initState = terminalState + suspendCompletion.complete(Unit) + } } /** @@ -533,14 +613,18 @@ internal class OneSignalImp( } private fun getServiceWithFeatureGate(getter: () -> T): T { - return if (isBackgroundThreadingEnabled) { - waitAndReturn(getter) - } else { - if (isInitialized) { - getter() - } else { - throw IllegalStateException("Must call 'initWithContext' before use") + if (isBackgroundThreadingEnabled) { + return waitAndReturn(getter) + } + return when (initState) { + InitState.SUCCESS -> getter() + InitState.IN_PROGRESS -> { + warnIfBlockingOnMainThread(operationName = null) + waitAndReturn(getter) } + InitState.FAILED -> throw initFailureException + ?: IllegalStateException("Initialization failed. Cannot proceed.") + InitState.NOT_STARTED -> throw IllegalStateException("Must call 'initWithContext' before use") } } @@ -645,10 +729,10 @@ internal class OneSignalImp( } initState = InitState.IN_PROGRESS + suspendCompletion = CompletableDeferred() } val result = internalInit(context, appId) - // initState is already set correctly in internalInit, no need to overwrite it result } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitTests.kt index 724784856..6039387b8 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitTests.kt @@ -7,7 +7,9 @@ import androidx.test.core.app.ApplicationProvider.getApplicationContext import br.com.colman.kotest.android.extensions.robolectric.RobolectricTest import com.onesignal.common.AndroidUtils import com.onesignal.core.internal.preferences.PreferenceOneSignalKeys.PREFS_LEGACY_APP_ID +import com.onesignal.debug.ILogListener import com.onesignal.debug.LogLevel +import com.onesignal.debug.OneSignalLogEvent import com.onesignal.debug.internal.logging.Logging import com.onesignal.internal.OneSignalImp import io.kotest.assertions.throwables.shouldThrow @@ -461,6 +463,123 @@ class SDKInitTests : FunSpec({ os.user.externalId shouldBe "" } + test("initWithContext recovers when ensureApplicationServiceStarted throws synchronously") { + // Given: a context whose applicationContext is NOT an Application instance. + // ApplicationService.start does `context.applicationContext as Application`, which + // will throw ClassCastException — simulates restricted hosts (Robolectric custom + // application factories, instrumentation context wrappers, multi-process content + // provider init order). Without recovery, initState would be left at IN_PROGRESS + // forever and accessors would deadlock on suspendCompletion.await(). + val baseContext = getApplicationContext() + val brokenContext = + object : ContextWrapper(baseContext) { + override fun getApplicationContext(): Context = ContextWrapper(baseContext) + } + val os = OneSignalImp() + + // When: synchronous failure during the bootstrap step. + shouldThrow { + os.initWithContext(brokenContext, "appId") + } + + // Then: SDK transitions to FAILED, accessor throws with the cast as suppressed cause. + val ex = + shouldThrow { + os.user.onesignalId + } + ex.message shouldBe "OneSignal initWithContext failed." + ex.suppressed.any { it is ClassCastException } shouldBe true + + // And: a retry with a working context succeeds (FAILED is not "accessible", + // so the retry legitimately re-enters init). + os.initWithContext(baseContext, "appId") shouldBe true + waitForInitialization(os) + } + + test("initWithContext recovers when async internalInit throws unexpectedly") { + // Given: AndroidUtils.isAndroidUserUnlocked throws unexpectedly inside the async init, + // simulating any unexpected runtime exception during initialization (a service throwing + // during bootstrap, a dependency failing to resolve, etc.). suspendifyOnIO swallows + // exceptions internally, so without the catch in internalInit the deferred would never + // complete and accessors would deadlock on suspendCompletion.await(). + val context = getApplicationContext() + val os = OneSignalImp() + val cause = RuntimeException("simulated init crash") + + mockkObject(AndroidUtils) + every { AndroidUtils.isAndroidUserUnlocked(any()) } throws cause + + try { + // When: public initWithContext returns true (the throw happens asynchronously). + os.initWithContext(context, "appId") shouldBe true + + // Then: the async catch transitions to FAILED and completes the deferred, so + // accessors throw promptly instead of blocking forever. + val ex = + shouldThrow { + os.user.onesignalId + } + ex.message shouldBe "OneSignal initWithContext failed." + ex.suppressed.any { it === cause } shouldBe true + } finally { + unmockkObject(AndroidUtils) + } + } + + test("legacy mode logs guidance when accessor on main thread blocks during in-progress init") { + // Given: pre-#2605, legacy mode threw if the SDK wasn't ready; the blocking happened + // inside initWithContext itself (synchronous runBlocking). Now that init dispatches + // asynchronously, the first accessor on the main thread can block instead. Total ANR + // risk is roughly equivalent — just shifted in time — but it's no longer obviously + // located in initWithContext, so we log a warning that points callers to the suspend API. + val trigger = CountDownLatch(1) + val context = getApplicationContext() + val blockingPrefContext = BlockingPrefsContext(context, trigger) + val os = OneSignalImp() + val warnings = mutableListOf() + val listener = + ILogListener { event: OneSignalLogEvent -> + if (event.level == LogLevel.WARN) warnings.add(event.entry) + } + + mockkObject(AndroidUtils) + every { AndroidUtils.isAndroidUserUnlocked(any()) } returns true + every { AndroidUtils.isRunningOnMainThread() } returns true + + os.debug.addLogListener(listener) + try { + // When: init kicks off but stalls inside internalInit on shared-prefs access. + os.initWithContext(blockingPrefContext, "appId") + os.isInitialized shouldBe false + + // Trigger the would-block accessor from a background thread (so the test thread + // doesn't actually hang on runBlocking) — the warning logged from inside + // warnIfBlockingOnMainThread is what we're verifying. + val accessorThread = + Thread { + runCatching { os.user.onesignalId } + } + accessorThread.start() + accessorThread.join(500) + + // The accessor is blocked waiting for init; release prefs so it can complete. + trigger.countDown() + accessorThread.join(2_000) + accessorThread.isAlive shouldBe false + + // Then: a warning must have been emitted with actionable guidance. + val match = warnings.firstOrNull { it.contains("OneSignal initialization is still in progress") } + match shouldNotBe null + (match!!.contains("main thread")) shouldBe true + (match.contains("suspend API")) shouldBe true + } finally { + os.debug.removeLogListener(listener) + // Ensure any waiter is released even if assertions failed early. + if (trigger.count > 0) trigger.countDown() + unmockkObject(AndroidUtils) + } + } + test("login should throw exception when initWithContext is never called") { // Given val oneSignalImp = OneSignalImp() diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt index bc6695d41..f1b63effe 100644 --- a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt @@ -17,6 +17,10 @@ class FeatureManagerTests : FunSpec({ ThreadingMode.useBackgroundThreading = false } + afterEach { + ThreadingMode.useBackgroundThreading = false + } + fun stubConfigModel(model: ConfigModel) { every { model.sdkRemoteFeatureFlags } returns emptyList() every { model.sdkRemoteFeatureFlagMetadata } returns null diff --git a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt index d740fe13d..4709d4ad4 100644 --- a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt +++ b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt @@ -5,6 +5,7 @@ import com.onesignal.common.threading.suspendifyOnIO import com.onesignal.debug.LogLevel import com.onesignal.debug.internal.logging.Logging import com.onesignal.mocks.AndroidMockHelper +import com.onesignal.mocks.IOMockHelper import com.onesignal.mocks.MockHelper import com.onesignal.notifications.INotificationReceivedEvent import com.onesignal.notifications.INotificationWillDisplayEvent @@ -96,6 +97,8 @@ private class Mocks { } class NotificationGenerationProcessorTests : FunSpec({ + listener(IOMockHelper) + beforeAny { Logging.logLevel = LogLevel.NONE @@ -279,16 +282,20 @@ class NotificationGenerationProcessorTests : FunSpec({ test("processNotificationData allows the will display callback to prevent default behavior twice") { // Given val mocks = Mocks() + // Bump the callback timeout from the suite default (10ms). Top-level launchOnIO falls + // through to GlobalScope.launch(Dispatchers.IO) (it's not stubbed by IOMockHelper), + // and on slow CI runners the IO scheduler can take longer than 10ms to dispatch the + // callback, causing withTimeout to cancel before discard is ever set. + every { mocks.notificationGenerationProcessor getProperty "EXTERNAL_CALLBACKS_TIMEOUT" } answers { 1_000L } coEvery { mocks.notificationDisplayer.displayNotification(any()) } returns true coEvery { mocks.notificationLifecycleService.externalRemoteNotificationReceived(any()) } just runs coEvery { mocks.notificationLifecycleService.externalNotificationWillShowInForeground(any()) } coAnswers { val willDisplayEvent = firstArg() willDisplayEvent.preventDefault(false) suspendifyOnIO { - delay(100) + // Second preventDefault(true) wakes the waiter with false; avoid notification.display() + // which would wake(true) and overwrite the conflated channel (CI flake on fast runners). willDisplayEvent.preventDefault(true) - delay(100) - willDisplayEvent.notification.display() } } @@ -304,15 +311,13 @@ class NotificationGenerationProcessorTests : FunSpec({ test("processNotificationData allows the received event callback to prevent default behavior twice") { // Given val mocks = Mocks() + every { mocks.notificationGenerationProcessor getProperty "EXTERNAL_CALLBACKS_TIMEOUT" } answers { 1_000L } coEvery { mocks.notificationDisplayer.displayNotification(any()) } returns true coEvery { mocks.notificationLifecycleService.externalRemoteNotificationReceived(any()) } coAnswers { val receivedEvent = firstArg() receivedEvent.preventDefault(false) suspendifyOnIO { - delay(100) receivedEvent.preventDefault(true) - delay(100) - receivedEvent.notification.display() } } diff --git a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/listeners/DeviceRegistrationListenerTests.kt b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/listeners/DeviceRegistrationListenerTests.kt index 188cc66cb..300917777 100644 --- a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/listeners/DeviceRegistrationListenerTests.kt +++ b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/listeners/DeviceRegistrationListenerTests.kt @@ -252,7 +252,7 @@ class DeviceRegistrationListenerTests : FunSpec({ permission = true, pushModel = uninitializedPushModel(), pushTokenResponse = - PushTokenResponse(NEW_TOKEN, SubscriptionStatus.SUBSCRIBED), + PushTokenResponse(NEW_TOKEN, SubscriptionStatus.SUBSCRIBED), ) // Permission flips off between gate evaluation and the IO callback. every { harness.notificationsManager.permission } returns true andThen false diff --git a/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/IOMockHelper.kt b/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/IOMockHelper.kt index 177980ab1..f9a4df94c 100644 --- a/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/IOMockHelper.kt +++ b/OneSignalSDK/onesignal/testhelpers/src/main/java/com/onesignal/mocks/IOMockHelper.kt @@ -2,6 +2,7 @@ package com.onesignal.mocks import com.onesignal.common.threading.OneSignalDispatchers import com.onesignal.common.threading.suspendifyOnIO +import com.onesignal.common.threading.suspendifyOnMain import io.kotest.core.listeners.AfterSpecListener import io.kotest.core.listeners.BeforeSpecListener import io.kotest.core.listeners.BeforeTestListener @@ -30,7 +31,8 @@ import java.util.concurrent.atomic.AtomicInteger * (e.g., delay(50)) to wait for async work to finish. * * This helper avoids that by: - * - Mocking `suspendifyOnIO`, `launchOnIO`, and `launchOnDefault` so their blocks run immediately + * - Mocking `suspendifyOnIO`, `suspendifyOnMain`, and `OneSignalDispatchers.launchOnIO` / + * `launchOnDefault` so their blocks run immediately * - Completing a `CompletableDeferred` when the async block finishes * - Providing `awaitIO()` so tests can explicitly wait for all async work without sleeps * @@ -114,6 +116,11 @@ object IOMockHelper : BeforeSpecListener, AfterSpecListener, BeforeTestListener, trackAsyncWork(block) } + every { suspendifyOnMain(any Unit>()) } answers { + val block = firstArg Unit>() + trackAsyncWork(block) + } + every { OneSignalDispatchers.launchOnIO(any Unit>()) } answers { val block = firstArg Unit>() trackAsyncWork(block)