From 13bbd24df170ec40b57e1ac28ea0b929d4317ac7 Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Mon, 23 Mar 2026 18:07:25 +0100 Subject: [PATCH] Fix wakelock not stopping early when notifications are disabled (#6424) If notifications for a device are disabled when there is no connection with the HS, the push registration will still exist, so the device can still receive push notifications. In that cases, we were running into an issue where the wakelock for push notifications was started immediately after receiving a push but was never stopped and it ran for 3 minutes until its timeout, keeping the device awake for no reason. This patch changes `DefaultPushHandler` so if we don't need the wakelock it returns `false` and we can stop the wakelock early. --- .../push/impl/push/DefaultPushHandler.kt | 14 ++-- .../push/test/test/FakePushHandler.kt | 6 +- .../pushproviders/api/PushHandler.kt | 12 +++- .../VectorFirebaseMessagingService.kt | 8 ++- .../VectorFirebaseMessagingServiceTest.kt | 65 ++++++++++++++++++- .../VectorUnifiedPushMessagingReceiver.kt | 8 ++- .../VectorUnifiedPushMessagingReceiverTest.kt | 57 +++++++++++++++- 7 files changed, 157 insertions(+), 13 deletions(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt index c39caa5781..44cf6edefc 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt @@ -64,7 +64,7 @@ class DefaultPushHandler( * @param pushData the data received in the push. * @param providerInfo the provider info. */ - override suspend fun handle(pushData: PushData, providerInfo: String) { + override suspend fun handle(pushData: PushData, providerInfo: String): Boolean { // Start measuring how long it takes to display a notification from when the push is received Timber.d("Calculating push-to-notification for event ${pushData.eventId}") val parent = analyticsService.startLongRunningTransaction(AnalyticsLongRunningTransaction.PushToNotification(pushData.eventId.value)) @@ -81,9 +81,10 @@ class DefaultPushHandler( } // Diagnostic Push - if (pushData.eventId == DefaultTestPush.TEST_EVENT_ID) { + return if (pushData.eventId == DefaultTestPush.TEST_EVENT_ID) { pushHistoryService.onDiagnosticPush(providerInfo) diagnosticPushHandler.handlePush() + false } else { handleInternal(pushData, providerInfo) } @@ -100,7 +101,7 @@ class DefaultPushHandler( * @param pushData Object containing message data. * @param providerInfo the provider info. */ - private suspend fun handleInternal(pushData: PushData, providerInfo: String) { + private suspend fun handleInternal(pushData: PushData, providerInfo: String): Boolean { try { if (buildMeta.lowPrivacyLoggingEnabled) { Timber.tag(loggerTag.value).d("## handleInternal() : $pushData") @@ -117,13 +118,13 @@ class DefaultPushHandler( roomId = pushData.roomId, reason = "Unable to get userId from client secret", ) - return + return false } val areNotificationsEnabled = userPushStoreFactory.getOrCreate(userId).getNotificationEnabledForDevice().first() if (!areNotificationsEnabled) { Timber.w("Push notification received when push notifications are disabled.") - return + return false } val pushRequest = PushRequest( @@ -144,8 +145,11 @@ class DefaultPushHandler( Timber.d("No pending worker for push notifications found") workManagerScheduler.submit(syncPendingNotificationsRequestFactory.create(userId)) } + + return true } catch (e: Exception) { Timber.tag(loggerTag.value).e(e, "## handleInternal() failed") + return false } } } diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt index a7e476be9c..27f1af5baa 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt @@ -13,11 +13,11 @@ import io.element.android.libraries.pushproviders.api.PushHandler import io.element.android.tests.testutils.lambda.lambdaError class FakePushHandler( - private val handleResult: (PushData, String) -> Unit = { _, _ -> lambdaError() }, + private val handleResult: (PushData, String) -> Boolean = { _, _ -> lambdaError() }, private val handleInvalidResult: (String, String) -> Unit = { _, _ -> lambdaError() }, ) : PushHandler { - override suspend fun handle(pushData: PushData, providerInfo: String) { - handleResult(pushData, providerInfo) + override suspend fun handle(pushData: PushData, providerInfo: String): Boolean { + return handleResult(pushData, providerInfo) } override suspend fun handleInvalid(providerInfo: String, data: String) { diff --git a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt index d3105111c5..49b578070b 100644 --- a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt +++ b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt @@ -9,11 +9,21 @@ package io.element.android.libraries.pushproviders.api interface PushHandler { + /** + * Handle a push received from the provider. + * + * @param pushData the data of the push, containing the client secret and the push content. + * @param providerInfo an identifier of the provider that sent the push, for logging and debugging purposes. + * @return `true` if the push was handled successfully and is now enqueued for processing, false otherwise. + */ suspend fun handle( pushData: PushData, providerInfo: String, - ) + ): Boolean + /** + * Handle an invalid push received from the provider. + */ suspend fun handleInvalid( providerInfo: String, data: String, 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 6c479b92c1..67da09f1ab 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 @@ -58,11 +58,17 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() { "$it: ${message.data[it]}" }, ) + pushHandlingWakeLock.unlock() } else { - pushHandler.handle( + val handled = pushHandler.handle( pushData = pushData, providerInfo = FirebaseConfig.NAME, ) + + // If we failed to handle the push, we should release the wakelock early to avoid keeping the device awake for too long. + if (!handled) { + 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 81bf19e666..87ca74730c 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 @@ -29,6 +29,7 @@ import kotlinx.coroutines.test.runTest import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner +import kotlin.time.Duration @RunWith(RobolectricTestRunner::class) class VectorFirebaseMessagingServiceTest { @@ -56,7 +57,7 @@ class VectorFirebaseMessagingServiceTest { @Test fun `test receiving valid data`() = runTest { - val lambda = lambdaRecorder { _, _ -> } + val lambda = lambdaRecorder { _, _ -> true } val vectorFirebaseMessagingService = createVectorFirebaseMessagingService( pushHandler = FakePushHandler(handleResult = lambda) ) @@ -78,6 +79,68 @@ class VectorFirebaseMessagingServiceTest { ) } + @Test + fun `test pushHandler returning true locks and does not unlock the wakelock so it continues running`() = runTest { + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorFirebaseMessagingService = createVectorFirebaseMessagingService( + pushHandler = FakePushHandler(handleResult = { _, _ -> true }), + 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) + }, + ) + ) + + // The wakelock should be locked but not unlocked + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After handling the push, the wakelock should still not be unlocked + unlockLambda.assertions().isNeverCalled() + } + + @Test + fun `test pushHandler returning false locks and unlocks the wakelock early`() = 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) + }, + ) + ) + + // The wakelock should be locked but not unlocked + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After handling the push, the wakelock should be unlocked + unlockLambda.assertions().isCalledOnce() + } + @Test fun `test new token is forwarded to the handler`() = runTest { val lambda = lambdaRecorder { } diff --git a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt index 59a2f654dd..363400ba13 100644 --- a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt +++ b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt @@ -71,11 +71,17 @@ class VectorUnifiedPushMessagingReceiver : MessagingReceiver() { providerInfo = "${UnifiedPushConfig.NAME} - $instance", data = String(message.content), ) + pushHandlingWakeLock.unlock() } else { - pushHandler.handle( + val handled = pushHandler.handle( pushData = pushData, providerInfo = "${UnifiedPushConfig.NAME} - $instance", ) + + // If we failed to handle the push, we should release the wakelock early to avoid keeping the device awake for too long. + if (!handled) { + pushHandlingWakeLock.unlock() + } } } } diff --git a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt index 10de44d4ff..ef81c647b3 100644 --- a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt +++ b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt @@ -39,6 +39,7 @@ import org.unifiedpush.android.connector.FailedReason import org.unifiedpush.android.connector.data.PublicKeySet import org.unifiedpush.android.connector.data.PushEndpoint import org.unifiedpush.android.connector.data.PushMessage +import kotlin.time.Duration @RunWith(RobolectricTestRunner::class) class VectorUnifiedPushMessagingReceiverTest { @@ -76,7 +77,7 @@ class VectorUnifiedPushMessagingReceiverTest { @Test fun `onMessage valid invokes the push handler`() = runTest { val context = InstrumentationRegistry.getInstrumentation().context - val pushHandlerResult = lambdaRecorder { _, _ -> } + val pushHandlerResult = lambdaRecorder { _, _ -> true } val vectorUnifiedPushMessagingReceiver = createVectorUnifiedPushMessagingReceiver( pushHandler = FakePushHandler( handleResult = pushHandlerResult @@ -101,6 +102,60 @@ class VectorUnifiedPushMessagingReceiverTest { ) } + @Test + fun `pushHandler returning true locks the wake lock but does not unlock it so it continues to run`() = runTest { + val context = InstrumentationRegistry.getInstrumentation().context + val pushHandlerResult = lambdaRecorder { _, _ -> true } + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorUnifiedPushMessagingReceiver = createVectorUnifiedPushMessagingReceiver( + pushHandler = FakePushHandler( + handleResult = pushHandlerResult + ), + pushHandlingWakeLock = FakePushHandlingWakeLock( + lock = lockLambda, + unlock = unlockLambda, + ), + ) + vectorUnifiedPushMessagingReceiver.onMessage(context, aPushMessage(), A_SECRET) + + // The wakelock should be locked but not unlocked, so it should continue to run + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After waiting for any possible timeout, the lock is still locked + unlockLambda.assertions().isNeverCalled() + } + + @Test + fun `pushHandler returning false locks and unlocks the wakelock early`() = runTest { + val context = InstrumentationRegistry.getInstrumentation().context + val pushHandlerResult = lambdaRecorder { _, _ -> false } + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorUnifiedPushMessagingReceiver = createVectorUnifiedPushMessagingReceiver( + pushHandler = FakePushHandler( + handleResult = pushHandlerResult + ), + pushHandlingWakeLock = FakePushHandlingWakeLock( + lock = lockLambda, + unlock = unlockLambda, + ), + ) + vectorUnifiedPushMessagingReceiver.onMessage(context, aPushMessage(), A_SECRET) + + // The wakelock should be locked but not unlocked, so it should continue to run + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After waiting for a bit, the lock should have been unlocked since the handler returned false + unlockLambda.assertions().isCalledOnce() + } + @Test fun `onMessage invalid invokes the push handler invalid method`() = runTest { val context = InstrumentationRegistry.getInstrumentation().context