Merge branch 'avoid-race-condition-when-updating-notification'

2 files changed, 61 insertions, 22 deletions

diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/service/ForegroundNotificationManager.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/service/ForegroundNotificationManager.kt
index c402881488..2daf1477ba 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/service/ForegroundNotificationManager.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/service/ForegroundNotificationManager.kt
@@ -8,6 +8,11 @@ import android.content.Intent
 import android.content.IntentFilter
 import android.os.Build
 import kotlin.properties.Delegates.observable
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.GlobalScope
+import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.channels.actor
+import kotlinx.coroutines.channels.sendBlocking
 import net.mullvad.mullvadvpn.model.TunnelState
 import net.mullvad.mullvadvpn.service.notifications.TunnelStateNotification
 import net.mullvad.talpid.util.EventNotifier
@@ -18,6 +23,15 @@ class ForegroundNotificationManager(
     val serviceNotifier: EventNotifier<ServiceInstance?>,
     val keyguardManager: KeyguardManager
 ) {
+    private sealed class UpdaterMessage {
+        class UpdateNotification : UpdaterMessage()
+        class UpdateAction : UpdaterMessage()
+        class AcknowledgeStartForegroundService : UpdaterMessage()
+        class NewTunnelState(val newState: TunnelState) : UpdaterMessage()
+    }
+
+    private val updater = runUpdater()
+
     private val tunnelStateNotification = TunnelStateNotification(service)
 
     private val deviceLockListener = object : BroadcastReceiver() {
@@ -36,15 +50,16 @@ class ForegroundNotificationManager(
 
     private var tunnelStateEvents
     by autoSubscribable<TunnelState>(this, TunnelState.Disconnected()) { newState ->
-        tunnelStateNotification.tunnelState = newState
-        updateNotification()
+        updater.sendBlocking(UpdaterMessage.NewTunnelState(newState))
     }
 
     private var deviceIsUnlocked by observable(!keyguardManager.isDeviceLocked) { _, _, _ ->
-        updateNotificationAction()
+        updater.sendBlocking(UpdaterMessage.UpdateAction())
     }
 
-    private var loggedIn by observable(false) { _, _, _ -> updateNotificationAction() }
+    private var loggedIn by observable(false) { _, _, _ ->
+        updater.sendBlocking(UpdaterMessage.UpdateAction())
+    }
 
     private val tunnelState
         get() = tunnelStateEvents?.latestEvent ?: TunnelState.Disconnected()
@@ -55,7 +70,9 @@ class ForegroundNotificationManager(
     var onForeground = false
         private set
 
-    var lockedToForeground by observable(false) { _, _, _ -> updateNotification() }
+    var lockedToForeground by observable(false) { _, _, _ ->
+        updater.sendBlocking(UpdaterMessage.UpdateNotification())
+    }
 
     init {
         serviceNotifier.subscribe(this) { newServiceInstance ->
@@ -73,7 +90,7 @@ class ForegroundNotificationManager(
             )
         }
 
-        updateNotification()
+        updater.sendBlocking(UpdaterMessage.UpdateNotification())
     }
 
     fun onDestroy() {
@@ -84,17 +101,40 @@ class ForegroundNotificationManager(
 
         service.unregisterReceiver(deviceLockListener)
 
-        tunnelStateNotification.visible = false
+        updater.close()
     }
 
     fun acknowledgeStartForegroundService() {
+        updater.sendBlocking(UpdaterMessage.AcknowledgeStartForegroundService())
+    }
+
+    private fun runUpdater() = GlobalScope.actor<UpdaterMessage>(
+        Dispatchers.Main,
+        Channel.UNLIMITED
+    ) {
+        for (message in channel) {
+            when (message) {
+                is UpdaterMessage.UpdateNotification -> updateNotification()
+                is UpdaterMessage.UpdateAction -> updateNotificationAction()
+                is UpdaterMessage.AcknowledgeStartForegroundService -> {
+                    doAcknowledgeStartForegroundService()
+                }
+                is UpdaterMessage.NewTunnelState -> {
+                    tunnelStateNotification.tunnelState = message.newState
+                    updateNotification()
+                }
+            }
+        }
+
+        tunnelStateNotification.visible = false
+    }
+
+    private fun doAcknowledgeStartForegroundService() {
         // When sending start commands to the service, it is necessary to request the service to be
         // on the foreground. With such request, when the service is started it must be placed on
         // the foreground with a call to startForeground before a timeout expires, otherwise Android
         // kills the app.
-        synchronized(this) {
-            showOnForeground()
-        }
+        showOnForeground()
 
         // Restore the notification to its correct state.
         updateNotification()
@@ -110,19 +150,17 @@ class ForegroundNotificationManager(
     }
 
     private fun updateNotification() {
-        synchronized(this) {
-            if (shouldBeOnForeground != onForeground) {
-                if (shouldBeOnForeground) {
-                    showOnForeground()
+        if (shouldBeOnForeground != onForeground) {
+            if (shouldBeOnForeground) {
+                showOnForeground()
+            } else {
+                if (Build.VERSION.SDK_INT >= 24) {
+                    service.stopForeground(Service.STOP_FOREGROUND_DETACH)
                 } else {
-                    if (Build.VERSION.SDK_INT >= 24) {
-                        service.stopForeground(Service.STOP_FOREGROUND_DETACH)
-                    } else {
-                        service.stopForeground(false)
-                    }
-
-                    onForeground = false
+                    service.stopForeground(false)
                 }
+
+                onForeground = false
             }
         }
     }
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
index 1cb61da920..0128df069a 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
@@ -71,6 +71,7 @@ class MainActivity : FragmentActivity() {
         override fun onServiceDisconnected(className: ComponentName) {
             android.util.Log.d("mullvad", "UI lost the connection to the service")
             service?.serviceNotifier?.unsubscribe(this@MainActivity)
+            service = null
             serviceConnection = null
             serviceNotifier.notify(null)
         }
@@ -112,7 +113,6 @@ class MainActivity : FragmentActivity() {
         }
 
         bindService(intent, serviceConnectionManager, 0)
-        service?.isUiVisible = true
     }
 
     override fun onActivityResult(requestCode: Int, resultCode: Int, resultData: Intent?) {
@@ -123,6 +123,7 @@ class MainActivity : FragmentActivity() {
         android.util.Log.d("mullvad", "Stoping main activity")
         isUiVisible = false
         service?.isUiVisible = false
+        service = null
         unbindService(serviceConnectionManager)
 
         super.onStop()