From 9af888a595ec8819f8fe6702a1c28f694fc9f28b Mon Sep 17 00:00:00 2001 From: Clark Date: Tue, 11 Jul 2023 16:05:30 -0400 Subject: [PATCH] Refactor FcmFetchManager to make foreground service clearer. --- .../gcm/FcmFetchForegroundService.kt | 71 ++++++++++++- .../securesms/gcm/FcmFetchManager.kt | 99 +++---------------- .../securesms/gcm/FcmReceiveService.java | 34 ++----- .../keyvalue/MiscellaneousValues.java | 9 -- .../securesms/util/FeatureFlags.java | 8 -- 5 files changed, 91 insertions(+), 130 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchForegroundService.kt b/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchForegroundService.kt index f611abf939..319dfb9f17 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchForegroundService.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchForegroundService.kt @@ -11,6 +11,7 @@ import org.signal.core.util.PendingIntentFlags import org.signal.core.util.logging.Log import org.thoughtcrime.securesms.MainActivity import org.thoughtcrime.securesms.R +import org.thoughtcrime.securesms.jobs.ForegroundServiceUtil import org.thoughtcrime.securesms.notifications.NotificationChannels import org.thoughtcrime.securesms.notifications.NotificationIds import org.thoughtcrime.securesms.util.WakeLockUtil @@ -27,6 +28,7 @@ class FcmFetchForegroundService : Service() { private const val WAKELOCK_TAG = "FcmForegroundService" private const val KEY_STOP_SELF = "stop_self" + private const val MAX_BLOCKING_TIME_MS = 500L private val WAKELOCK_TIMEOUT = FcmFetchManager.WEBSOCKET_DRAIN_TIMEOUT @@ -36,11 +38,76 @@ class FcmFetchForegroundService : Service() { * The safest thing to do is to just tell it to start so it can call [startForeground] and then stop itself. * Fun. */ - fun buildStopIntent(context: Context): Intent { + private fun buildStopIntent(context: Context): Intent { return Intent(context, FcmFetchForegroundService::class.java).apply { putExtra(KEY_STOP_SELF, true) } } + + enum class State { + STOPPED, + STARTED, + STOPPING, + RESTARTING + } + + private var foregroundServiceState: State = State.STOPPED + + fun startServiceIfNecessary(context: Context) { + synchronized(this) { + when (foregroundServiceState) { + State.STOPPING -> foregroundServiceState = State.RESTARTING + State.STOPPED -> { + foregroundServiceState = try { + startForegroundFetchService(context) + State.STARTED + } catch (e: IllegalStateException) { + Log.e(TAG, "Failed to start foreground service", e) + State.STOPPED + } + } + else -> Log.i(TAG, "Already started foreground service") + } + } + } + + fun stopServiceIfNecessary(context: Context) { + synchronized(this) { + when (foregroundServiceState) { + State.STARTED -> { + foregroundServiceState = State.STOPPING + try { + context.startService(buildStopIntent(context)) + } catch (e: IllegalStateException) { + Log.w(TAG, "Failed to stop the foreground service, assuming already stopped", e) + foregroundServiceState = State.STOPPED + } + } + State.RESTARTING -> foregroundServiceState = State.STOPPED + else -> Log.i(TAG, "No service to stop") + } + } + } + + private fun onServiceDestroyed(context: Context) { + synchronized(this) { + Log.i(TAG, "Fcm fetch service destroyed") + when (foregroundServiceState) { + State.RESTARTING -> { + foregroundServiceState = State.STOPPED + Log.i(TAG, "Restarting service.") + startServiceIfNecessary(context) + } + else -> { + foregroundServiceState = State.STOPPED + } + } + } + } + + private fun startForegroundFetchService(context: Context) { + ForegroundServiceUtil.startWhenCapableOrThrow(context, Intent(context, FcmFetchForegroundService::class.java), MAX_BLOCKING_TIME_MS) + } } override fun onCreate() { @@ -81,7 +148,7 @@ class FcmFetchForegroundService : Service() { override fun onDestroy() { Log.i(TAG, "onDestroy()") WakeLockUtil.release(wakeLock, WAKELOCK_TAG) - FcmFetchManager.onDestroyForegroundFetchService() + onServiceDestroyed(this) wakeLock = null } diff --git a/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchManager.kt b/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchManager.kt index af00babbb4..d72329f679 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmFetchManager.kt @@ -3,11 +3,9 @@ package org.thoughtcrime.securesms.gcm import android.content.Context import android.content.Intent import android.os.Build -import android.os.PowerManager import org.signal.core.util.concurrent.SignalExecutors import org.signal.core.util.logging.Log import org.thoughtcrime.securesms.dependencies.ApplicationDependencies -import org.thoughtcrime.securesms.jobs.ForegroundServiceUtil import org.thoughtcrime.securesms.jobs.PushNotificationReceiveJob import org.thoughtcrime.securesms.messages.WebSocketStrategy import org.thoughtcrime.securesms.util.SignalLocalMetrics @@ -33,7 +31,6 @@ import kotlin.time.Duration.Companion.minutes object FcmFetchManager { private val TAG = Log.tag(FcmFetchManager::class.java) - private const val MAX_BLOCKING_TIME_MS = 500L private val EXECUTOR = SerialMonoLifoExecutor(SignalExecutors.UNBOUNDED) val WEBSOCKET_DRAIN_TIMEOUT = 5.minutes.inWholeMilliseconds @@ -41,57 +38,22 @@ object FcmFetchManager { @Volatile private var activeCount = 0 - @Volatile - private var startedForeground = false - - @Volatile - private var stoppingForeground = false - - @Volatile - private var startForegroundOnDestroy = false - - private var wakeLock: PowerManager.WakeLock? = null + /** + * @return True if a service was successfully started, otherwise false. + */ + @JvmStatic + fun startBackgroundService(context: Context) { + Log.i(TAG, "Starting in the background.") + context.startService(Intent(context, FcmFetchBackgroundService::class.java)) + } /** * @return True if a service was successfully started, otherwise false. */ @JvmStatic - fun enqueue(context: Context, foreground: Boolean): Boolean { - synchronized(this) { - try { - if (foreground) { - Log.i(TAG, "Starting in the foreground.") - if (!startedForeground) { - if (!stoppingForeground) { - ForegroundServiceUtil.startWhenCapableOrThrow(context, Intent(context, FcmFetchForegroundService::class.java), MAX_BLOCKING_TIME_MS) - startedForeground = true - } else { - Log.w(TAG, "Foreground service currently stopping, enqueuing a start after destroy") - startForegroundOnDestroy = true - } - } else { - Log.i(TAG, "Already started foreground service") - } - } else { - Log.i(TAG, "Starting in the background.") - context.startService(Intent(context, FcmFetchBackgroundService::class.java)) - } - - val performedReplace = EXECUTOR.enqueue { fetch(context) } - - if (performedReplace) { - Log.i(TAG, "Already have one running and one enqueued. Ignoring.") - } else { - activeCount++ - Log.i(TAG, "Incrementing active count to $activeCount") - } - } catch (e: Exception) { - Log.w(TAG, "Failed to start service!", e) - return false - } - } - - return true + fun startForegroundService(context: Context) { + Log.i(TAG, "Starting in the foreground.") + FcmFetchForegroundService.startServiceIfNecessary(context) } private fun fetch(context: Context) { @@ -109,37 +71,20 @@ object FcmFetchManager { if (activeCount <= 0) { Log.i(TAG, "No more active. Stopping.") context.stopService(Intent(context, FcmFetchBackgroundService::class.java)) - - if (startedForeground) { - try { - context.startService(FcmFetchForegroundService.buildStopIntent(context)) - stoppingForeground = true - } catch (e: IllegalStateException) { - Log.w(TAG, "Failed to stop the foreground notification!", e) - } - - startedForeground = false - } + FcmFetchForegroundService.stopServiceIfNecessary(context) } } } @JvmStatic - fun tryLegacyFallback(context: Context) { + fun enqueueFetch(context: Context) { synchronized(this) { - if (startedForeground || stoppingForeground) { - if (stoppingForeground) { - startForegroundOnDestroy = true - Log.i(TAG, "Legacy fallback: foreground service is stopping, but trying to run in background anyways.") - } - } val performedReplace = EXECUTOR.enqueue { fetch(context) } - if (performedReplace) { - Log.i(TAG, "Legacy fallback: already have one running and one enqueued. Ignoring.") + Log.i(TAG, "Already have one running and one enqueued. Ignoring.") } else { activeCount++ - Log.i(TAG, "Legacy fallback: Incrementing active count to $activeCount") + Log.i(TAG, "Incrementing active count to $activeCount") } } } @@ -162,18 +107,4 @@ object FcmFetchManager { return success } - - fun onDestroyForegroundFetchService() { - synchronized(this) { - stoppingForeground = false - if (startForegroundOnDestroy) { - startForegroundOnDestroy = false - try { - enqueue(ApplicationDependencies.getApplication(), true) - } catch (e: Exception) { - Log.e(TAG, "Failed to restart foreground service after onDestroy") - } - } - } - } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmReceiveService.java b/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmReceiveService.java index 51a6d6cd8f..550a48b7b9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmReceiveService.java +++ b/app/src/main/java/org/thoughtcrime/securesms/gcm/FcmReceiveService.java @@ -15,18 +15,14 @@ import org.thoughtcrime.securesms.jobs.FcmRefreshJob; import org.thoughtcrime.securesms.jobs.SubmitRateLimitPushChallengeJob; import org.thoughtcrime.securesms.keyvalue.SignalStore; import org.thoughtcrime.securesms.registration.PushChallengeRequest; -import org.thoughtcrime.securesms.util.FeatureFlags; import org.thoughtcrime.securesms.util.NetworkUtil; import java.util.Locale; -import java.util.concurrent.TimeUnit; public class FcmReceiveService extends FirebaseMessagingService { private static final String TAG = Log.tag(FcmReceiveService.class); - private static final long FCM_FOREGROUND_INTERVAL = TimeUnit.MINUTES.toMillis(3); - @Override public void onMessageReceived(RemoteMessage remoteMessage) { Log.i(TAG, String.format(Locale.US, @@ -78,37 +74,21 @@ public class FcmReceiveService extends FirebaseMessagingService { } private static void handleReceivedNotification(Context context, @Nullable RemoteMessage remoteMessage) { - boolean enqueueSuccessful = false; - boolean highPriority = false; try { - highPriority = remoteMessage != null && remoteMessage.getPriority() == RemoteMessage.PRIORITY_HIGH; + boolean highPriority = remoteMessage != null && remoteMessage.getPriority() == RemoteMessage.PRIORITY_HIGH; - long timeSinceLastRefresh = System.currentTimeMillis() - SignalStore.misc().getLastFcmForegroundServiceTime(); + Log.d(TAG, String.format(Locale.US, "[handleReceivedNotification] API: %s, RemoteMessagePriority: %s", Build.VERSION.SDK_INT, remoteMessage != null ? remoteMessage.getPriority() : "n/a")); - Log.d(TAG, String.format(Locale.US, "[handleReceivedNotification] API: %s, FeatureFlag: %s, RemoteMessagePriority: %s, TimeSinceLastRefresh: %s ms", Build.VERSION.SDK_INT, FeatureFlags.useFcmForegroundService(), remoteMessage != null ? remoteMessage.getPriority() : "n/a", timeSinceLastRefresh)); - - if (highPriority && FeatureFlags.useFcmForegroundService()) { - enqueueSuccessful = FcmFetchManager.enqueue(context, true); - SignalStore.misc().setLastFcmForegroundServiceTime(System.currentTimeMillis()); - } else if (highPriority && Build.VERSION.SDK_INT >= 31 && timeSinceLastRefresh > FCM_FOREGROUND_INTERVAL) { - enqueueSuccessful = FcmFetchManager.enqueue(context, true); - SignalStore.misc().setLastFcmForegroundServiceTime(System.currentTimeMillis()); - } else if (highPriority || Build.VERSION.SDK_INT < 26 || remoteMessage == null) { - enqueueSuccessful = FcmFetchManager.enqueue(context, false); + if (highPriority) { + FcmFetchManager.startForegroundService(context); + } else if (Build.VERSION.SDK_INT < 26) { + FcmFetchManager.startBackgroundService(context); } } catch (Exception e) { Log.w(TAG, "Failed to start service.", e); - enqueueSuccessful = false; } - if (!enqueueSuccessful) { - if (highPriority) { - Log.w(TAG, "Unable to start service though high priority push. Falling back to legacy approach."); - } else { - Log.d(TAG, "Low priority push, trying legacy fallback"); - } - FcmFetchManager.tryLegacyFallback(context); - } + FcmFetchManager.enqueueFetch(context); } private static void handleRegistrationPushChallenge(@NonNull String challenge) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.java b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.java index df6de1a080..87160543c6 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.java +++ b/app/src/main/java/org/thoughtcrime/securesms/keyvalue/MiscellaneousValues.java @@ -29,7 +29,6 @@ public final class MiscellaneousValues extends SignalStoreValues { private static final String LAST_GV2_PROFILE_CHECK_TIME = "misc.last_gv2_profile_check_time"; private static final String CDS_TOKEN = "misc.cds_token"; private static final String CDS_BLOCKED_UNTIL = "misc.cds_blocked_until"; - private static final String LAST_FCM_FOREGROUND_TIME = "misc.last_fcm_foreground_time"; private static final String LAST_FOREGROUND_TIME = "misc.last_foreground_time"; private static final String PNI_INITIALIZED_DEVICES = "misc.pni_initialized_devices"; private static final String SMS_PHASE_1_START_MS = "misc.sms_export.phase_1_start.3"; @@ -217,14 +216,6 @@ public final class MiscellaneousValues extends SignalStoreValues { return getLong(CDS_BLOCKED_UNTIL, 0); } - public long getLastFcmForegroundServiceTime() { - return getLong(LAST_FCM_FOREGROUND_TIME, 0); - } - - public void setLastFcmForegroundServiceTime(long time) { - putLong(LAST_FCM_FOREGROUND_TIME, time); - } - public long getLastForegroundTime() { return getLong(LAST_FOREGROUND_TIME, 0); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java index 5e2716ec55..e27c9e58fb 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/FeatureFlags.java @@ -86,7 +86,6 @@ public final class FeatureFlags { private static final String USE_AEC3 = "android.calling.useAec3"; private static final String PAYMENTS_COUNTRY_BLOCKLIST = "global.payments.disabledRegions"; public static final String PHONE_NUMBER_PRIVACY = "android.pnp"; - private static final String USE_FCM_FOREGROUND_SERVICE = "android.useFcmForegroundService.4"; private static final String STORIES_AUTO_DOWNLOAD_MAXIMUM = "android.stories.autoDownloadMaximum"; private static final String TELECOM_MANUFACTURER_ALLOWLIST = "android.calling.telecomAllowList"; private static final String TELECOM_MODEL_BLOCKLIST = "android.calling.telecomModelBlockList"; @@ -145,7 +144,6 @@ public final class FeatureFlags { USE_HARDWARE_AEC_IF_OLD, USE_AEC3, PAYMENTS_COUNTRY_BLOCKLIST, - USE_FCM_FOREGROUND_SERVICE, STORIES_AUTO_DOWNLOAD_MAXIMUM, TELECOM_MANUFACTURER_ALLOWLIST, TELECOM_MODEL_BLOCKLIST, @@ -220,7 +218,6 @@ public final class FeatureFlags { USE_HARDWARE_AEC_IF_OLD, USE_AEC3, PAYMENTS_COUNTRY_BLOCKLIST, - USE_FCM_FOREGROUND_SERVICE, TELECOM_MANUFACTURER_ALLOWLIST, TELECOM_MODEL_BLOCKLIST, CAMERAX_MODEL_BLOCKLIST, @@ -488,11 +485,6 @@ public final class FeatureFlags { return getBoolean(USE_AEC3, true); } - /** Whether or not we show a foreground service on every high-priority FCM push. */ - public static boolean useFcmForegroundService() { - return getBoolean(USE_FCM_FOREGROUND_SERVICE, false); - } - /** * Prefetch count for stories from a given user. */