From 47251489193c42cb0c5aa9ec1e8f15075167102d Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Mon, 30 Mar 2026 18:44:08 +0200 Subject: [PATCH] Try handling `ForegroundServiceStartNotAllowedException` better (#6483) * Try handling `ForegroundServiceStartNotAllowedException` better The docs mention starting a foreground service when the app is on background is allowed when FCM receives a high priority notification, so we don't do it if the priority is not high. Also, we handle the case where starting the foreground service fails so it doesn't crash the app. --- .../impl/push/FetchPushForegroundService.kt | 38 ++++++++++++++++--- .../VectorFirebaseMessagingService.kt | 14 +++++-- .../VectorFirebaseMessagingServiceTest.kt | 29 ++++++++++++++ 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/FetchPushForegroundService.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/FetchPushForegroundService.kt index d7cc950b99..6dc6bdfa0e 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/FetchPushForegroundService.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/FetchPushForegroundService.kt @@ -17,6 +17,7 @@ import android.os.PowerManager import androidx.core.app.NotificationCompat import dev.zacsweers.metro.Inject import io.element.android.libraries.architecture.bindings +import io.element.android.libraries.core.extensions.runCatchingExceptions import io.element.android.libraries.designsystem.utils.CommonDrawables import io.element.android.libraries.di.annotations.AppCoroutineScope import io.element.android.libraries.push.api.push.PushHandlingWakeLock @@ -57,6 +58,8 @@ class FetchPushForegroundService : Service() { } } + private var isOnForeground = false + override fun onCreate() { Timber.d("Creating FetchPushForegroundService") @@ -71,12 +74,32 @@ class FetchPushForegroundService : Service() { .setVibrate(longArrayOf(0)) .setSound(null) .build() - startForeground(NOTIFICATION_ID, notificationCompat) + + // Try to start the service in foreground. This can fail, even in cases where it's supposed to work according to the docs. + // In those cases we catch the exception and handle the failure so we don't try to start the wakelock or stop the service + // from running in foreground later. + runCatchingExceptions { + startForeground(NOTIFICATION_ID, notificationCompat) + } + .onSuccess { + isOnForeground = true + Timber.d("FetchPushForegroundService started in foreground successfully") + } + .onFailure { + isOnForeground = false + Timber.e(it, "Failed to start FetchPushForegroundService in foreground") + } super.onCreate() } override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { + if (!isOnForeground) { + Timber.w("FetchPushForegroundService is not running in foreground, stopping it to avoid crash") + stopSelf() + return START_NOT_STICKY + } + wakelock.acquire(wakelockTimeout) // The timeout is not automatic before Android 15, so we need to schedule it ourselves @@ -91,18 +114,21 @@ class FetchPushForegroundService : Service() { } override fun stopService(intent: Intent?): Boolean { - wakelock.release() + if (isOnForeground) { + wakelock.release() + stopForeground(STOP_FOREGROUND_REMOVE) + } - stopForeground(STOP_FOREGROUND_REMOVE) return super.stopService(intent) } override fun onTimeout(startId: Int) { super.onTimeout(startId) - Timber.d("Wakelock timeout reached, stopping FetchPushForegroundService") - - coroutineScope.launch { pushHandlingWakeLock.unlock() } + if (isOnForeground) { + Timber.d("Wakelock timeout reached, stopping FetchPushForegroundService") + coroutineScope.launch { pushHandlingWakeLock.unlock() } + } } companion object { diff --git a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt index 67da09f1ab..3961f1f591 100644 --- a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt +++ b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt @@ -10,6 +10,7 @@ package io.element.android.libraries.pushproviders.firebase import com.google.firebase.messaging.FirebaseMessagingService import com.google.firebase.messaging.RemoteMessage +import com.google.firebase.messaging.RemoteMessage.PRIORITY_HIGH import dev.zacsweers.metro.Inject import io.element.android.libraries.architecture.bindings import io.element.android.libraries.core.log.logger.LoggerTag @@ -45,8 +46,11 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() { override fun onMessageReceived(message: RemoteMessage) { Timber.tag(loggerTag.value).w("New Firebase message. Priority: ${message.priority}/${message.originalPriority}") - // Acquire wakelock to ensure the device stays awake while we handle the push and schedule and run the work - pushHandlingWakeLock.lock() + val isHighPriority = message.priority == PRIORITY_HIGH + if (isHighPriority) { + // Acquire wakelock to ensure the device stays awake while we handle the push and schedule and run the work + pushHandlingWakeLock.lock() + } coroutineScope.launch { val pushData = pushParser.parse(message.data) @@ -58,7 +62,9 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() { "$it: ${message.data[it]}" }, ) - pushHandlingWakeLock.unlock() + if (isHighPriority) { + pushHandlingWakeLock.unlock() + } } else { val handled = pushHandler.handle( pushData = pushData, @@ -66,7 +72,7 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() { ) // If we failed to handle the push, we should release the wakelock early to avoid keeping the device awake for too long. - if (!handled) { + if (!handled && isHighPriority) { pushHandlingWakeLock.unlock() } } diff --git a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt index 87ca74730c..798328e626 100644 --- a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt +++ b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt @@ -96,6 +96,7 @@ class VectorFirebaseMessagingServiceTest { putString("event_id", AN_EVENT_ID.value) putString("room_id", A_ROOM_ID.value) putString("cs", A_SECRET) + putString("google.priority", "high") }, ) ) @@ -127,6 +128,7 @@ class VectorFirebaseMessagingServiceTest { putString("event_id", AN_EVENT_ID.value) putString("room_id", A_ROOM_ID.value) putString("cs", A_SECRET) + putString("google.priority", "high") }, ) ) @@ -141,6 +143,33 @@ class VectorFirebaseMessagingServiceTest { unlockLambda.assertions().isCalledOnce() } + @Test + fun `test pushHandler with a remote message with normal priority won't lock the wakelock`() = runTest { + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorFirebaseMessagingService = createVectorFirebaseMessagingService( + pushHandler = FakePushHandler(handleResult = { _, _ -> false }), + pushHandlingWakeLock = FakePushHandlingWakeLock( + lock = lockLambda, + unlock = unlockLambda + ) + ) + vectorFirebaseMessagingService.onMessageReceived( + message = RemoteMessage( + Bundle().apply { + putString("event_id", AN_EVENT_ID.value) + putString("room_id", A_ROOM_ID.value) + putString("cs", A_SECRET) + putString("google.priority", "normal") + }, + ) + ) + + // The wakelock should not be locked + lockLambda.assertions().isNeverCalled() + unlockLambda.assertions().isNeverCalled() + } + @Test fun `test new token is forwarded to the handler`() = runTest { val lambda = lambdaRecorder { }