From 5cd2568776460995020d2016b8a248aafefdd7e1 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Wed, 8 Nov 2023 06:22:36 -0800 Subject: [PATCH] Fix foreground service crash with state tracking. --- .../jobs/AttachmentCompressionJob.java | 14 +- .../securesms/jobs/AttachmentUploadJob.kt | 7 +- .../service/AttachmentProgressService.kt | 22 +-- .../service/SafeForegroundService.kt | 133 +++++++++++++++--- 4 files changed, 136 insertions(+), 40 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java index f5cf9858de..6faa30de6b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentCompressionJob.java @@ -206,7 +206,9 @@ public final class AttachmentCompressionJob extends BaseJob { } try (AttachmentProgressService.Controller notification = AttachmentProgressService.start(context, context.getString(R.string.AttachmentUploadJob_compressing_video_start))) { - notification.setIndeterminate(true); + if (notification != null) { + notification.setIndeterminate(true); + } try (MediaDataSource dataSource = attachmentDatabase.mediaDataSourceFor(attachment.getAttachmentId(), false)) { if (dataSource == null) { @@ -232,7 +234,9 @@ public final class AttachmentCompressionJob extends BaseJob { try { try (OutputStream outputStream = ModernEncryptingPartOutputStream.createFor(attachmentSecret, file, true).second) { transcoder.transcode(percent -> { - notification.setProgress(percent / 100f); + if (notification != null) { + notification.setProgress(percent / 100f); + } eventBus.postSticky(new PartProgressEvent(attachment, PartProgressEvent.Type.COMPRESSION, 100, @@ -261,7 +265,9 @@ public final class AttachmentCompressionJob extends BaseJob { Log.i(TAG, "Compressing with android in-memory muxer"); try (MediaStream mediaStream = transcoder.transcode(percent -> { - notification.setProgress(percent/100f); + if (notification != null) { + notification.setProgress(percent / 100f); + } eventBus.postSticky(new PartProgressEvent(attachment, PartProgressEvent.Type.COMPRESSION, 100, @@ -289,7 +295,7 @@ public final class AttachmentCompressionJob extends BaseJob { throw new UndeliverableMessageException("Failed to transcode and cannot skip due to editing", e); } } - } catch (UnableToStartException | IOException | MmsException e) { + } catch (IOException | MmsException e) { throw new UndeliverableMessageException("Failed to transcode", e); } return attachment; diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.kt index ddb4291203..ab22d2dd80 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/AttachmentUploadJob.kt @@ -145,12 +145,7 @@ class AttachmentUploadJob private constructor( private fun getAttachmentNotificationIfNeeded(attachment: Attachment): AttachmentProgressService.Controller? { return if (attachment.size >= FOREGROUND_LIMIT) { - try { - AttachmentProgressService.start(context, context.getString(R.string.AttachmentUploadJob_uploading_media)) - } catch (e: UnableToStartException) { - Log.w(TAG, "Unable to start foreground service", e) - null - } + AttachmentProgressService.start(context, context.getString(R.string.AttachmentUploadJob_uploading_media)) } else { null } diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/AttachmentProgressService.kt b/app/src/main/java/org/thoughtcrime/securesms/service/AttachmentProgressService.kt index af8ca46040..a340d0ff1d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/AttachmentProgressService.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/service/AttachmentProgressService.kt @@ -14,13 +14,11 @@ 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.UnableToStartException import org.thoughtcrime.securesms.notifications.NotificationChannels import org.thoughtcrime.securesms.notifications.NotificationIds import java.util.concurrent.CopyOnWriteArraySet import java.util.concurrent.locks.ReentrantLock import kotlin.concurrent.withLock -import kotlin.jvm.Throws /** * A service to show attachment progress. In order to ensure we only show one status notification, @@ -55,23 +53,27 @@ class AttachmentProgressService : SafeForegroundService() { * use to update the notification. * * Important: This could fail to start! We do our best to start the service regardless of context, - * but it will fail on some devices, throwing an [UnableToStartException] if it does so. + * but it will fail on some devices. If this happens, the returned [Controller] will be null. */ @JvmStatic - @Throws(UnableToStartException::class) - fun start(context: Context, title: String): Controller { + fun start(context: Context, title: String): Controller? { controllerLock.withLock { - if (controllers.isEmpty()) { + val started = if (controllers.isEmpty()) { Log.i(TAG, "[start] First controller. Starting.") SafeForegroundService.start(context, AttachmentProgressService::class.java) } else { Log.i(TAG, "[start] No need to start the service again. Already have an active controller.") + true } - val controller = Controller(context, title) - controllers += controller - onControllersChanged(context) - return controller + return if (started) { + val controller = Controller(context, title) + controllers += controller + onControllersChanged(context) + controller + } else { + null + } } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/service/SafeForegroundService.kt b/app/src/main/java/org/thoughtcrime/securesms/service/SafeForegroundService.kt index eb7d2b2391..187b9104e3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/service/SafeForegroundService.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/service/SafeForegroundService.kt @@ -14,7 +14,9 @@ import androidx.core.app.ServiceCompat import org.signal.core.util.logging.Log import org.thoughtcrime.securesms.jobs.ForegroundServiceUtil import org.thoughtcrime.securesms.jobs.UnableToStartException -import kotlin.jvm.Throws +import java.util.concurrent.locks.ReentrantLock +import javax.annotation.CheckReturnValue +import kotlin.concurrent.withLock /** * A simple parent class meant to encourage the safe usage of foreground services. @@ -29,17 +31,48 @@ abstract class SafeForegroundService : Service() { private const val ACTION_START = "start" private const val ACTION_STOP = "stop" + private var states: MutableMap, State> = mutableMapOf() + private val stateLock = ReentrantLock() + /** * Safety starts the target foreground service. - * Important: This operation can fail. If it does, [UnableToStartException] is thrown. + * @return False if we tried to start the service but failed, otherwise true. */ - @Throws(UnableToStartException::class) - fun start(context: Context, serviceClass: Class) { - val intent = Intent(context, serviceClass).apply { - action = ACTION_START - } + @CheckReturnValue + fun start(context: Context, serviceClass: Class): Boolean { + stateLock.withLock { + val state = currentState(serviceClass) - ForegroundServiceUtil.startWhenCapable(context, intent) + Log.d(TAG, "[start] Current state: $state") + + return when (state) { + State.STARTING, + State.NEEDS_RESTART -> { + Log.d(TAG, "[start] No need to start the service again. Current state: $state") + true + } + State.STOPPED -> { + Log.d(TAG, "[start] Starting service.") + states[serviceClass] = State.STARTING + try { + ForegroundServiceUtil.startWhenCapable( + context = context, + intent = Intent(context, serviceClass).apply { action = ACTION_START } + ) + true + } catch (e: UnableToStartException) { + Log.w(TAG, "[start] Failed to start the service!") + states[serviceClass] = State.STOPPED + false + } + } + State.STOPPING -> { + Log.d(TAG, "[start] Attempted to start while the service is stopping. Enqueueing a restart.") + states[serviceClass] = State.NEEDS_RESTART + true + } + } + } } /** @@ -48,16 +81,42 @@ abstract class SafeForegroundService : Service() { * a start is pending, preventing the posting of a foreground notification. */ fun stop(context: Context, serviceClass: Class) { - val intent = Intent(context, serviceClass).apply { - action = ACTION_STOP - } + stateLock.withLock { + val state = currentState(serviceClass) - try { - ForegroundServiceUtil.startWhenCapable(context, intent) - } catch (e: UnableToStartException) { - Log.w(TAG, "Failed to start service class $serviceClass", e) + Log.d(TAG, "[stop] Current state: $state") + + when (state) { + State.STARTING -> { + Log.d(TAG, "[stop] Stopping service.") + states[serviceClass] = State.STOPPING + try { + ForegroundServiceUtil.startWhenCapable( + context = context, + intent = Intent(context, serviceClass).apply { action = ACTION_STOP } + ) + } catch (e: UnableToStartException) { + Log.w(TAG, "Failed to start service class $serviceClass", e) + states[serviceClass] = State.STOPPED + } + } + + State.STOPPED, + State.STOPPING -> { + Log.d(TAG, "[stop] No need to stop the service. Current state: $state") + } + + State.NEEDS_RESTART -> { + Log.i(TAG, "[stop] Clearing pending restart.") + states[serviceClass] = State.STOPPING + } + } } } + + private fun currentState(clazz: Class): State { + return states.getOrPut(clazz) { State.STOPPED } + } } override fun onCreate() { @@ -74,10 +133,10 @@ abstract class SafeForegroundService : Service() { when (val action = intent.action) { ACTION_START -> { - onServiceStarted(intent) + onServiceStartCommandReceived(intent) } ACTION_STOP -> { - onServiceStopped(intent) + onServiceStopCommandReceived(intent) ServiceCompat.stopForeground(this, ServiceCompat.STOP_FOREGROUND_REMOVE) stopSelf() } @@ -88,8 +147,28 @@ abstract class SafeForegroundService : Service() { } override fun onDestroy() { - Log.d(tag, "[onDestroy]") super.onDestroy() + + stateLock.withLock { + val state = currentState(javaClass) + + Log.d(tag, "[onDestroy] Current state: $state") + when (state) { + State.STOPPED, + State.STARTING, + State.STOPPING -> { + states[javaClass] = State.STOPPED + } + + State.NEEDS_RESTART -> { + Log.i(TAG, "[onDestroy] Restarting service!") + states[javaClass] = State.STOPPED + if (!start(this, javaClass)) { + Log.w(TAG, "[onDestroy] Failed to restart service.") + } + } + } + } } override fun onBind(intent: Intent?): IBinder? = null @@ -104,8 +183,22 @@ abstract class SafeForegroundService : Service() { abstract fun getForegroundNotification(intent: Intent): Notification /** Event listener for when the service is started via an intent. */ - open fun onServiceStarted(intent: Intent) = Unit + open fun onServiceStartCommandReceived(intent: Intent) = Unit /** Event listener for when the service is stopped via an intent. */ - open fun onServiceStopped(intent: Intent) = Unit + open fun onServiceStopCommandReceived(intent: Intent) = Unit + + private enum class State { + /** The service is not running. */ + STOPPED, + + /** We told the service to start (via an intent). It may or may not be actually running yet. */ + STARTING, + + /** We told the service to stop (via an intent), but it's still technically running. */ + STOPPING, + + /** We requested that the service be started while it was in the process of stopping. */ + NEEDS_RESTART + } }