Merge branch 'fix-service-init-race-condition'

4 files changed, 88 insertions, 63 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index fcfcd5be5b..0ea79cf321 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -48,6 +48,7 @@ Line wrap the file at 100 chars.                                              Th
 #### Android
 - Fix crash when removing the service from foreground on Android versions below API level 24.
 - Fix crash that happened in certain situations when retrieving the relay list.
+- Fix crash caused by initialization race condition.
 
 
 ## [2020.1] - 2020-02-10
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/ConnectionProxy.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/ConnectionProxy.kt
index 5a05a261cd..16b34b137d 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/ConnectionProxy.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/ConnectionProxy.kt
@@ -4,7 +4,6 @@ import android.content.Context
 import android.content.Intent
 import android.net.VpnService
 import kotlinx.coroutines.CompletableDeferred
-import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.Job
@@ -18,15 +17,13 @@ import net.mullvad.talpid.util.EventNotifier
 
 val ANTICIPATED_STATE_TIMEOUT_MS = 1500L
 
-class ConnectionProxy(val context: Context, val daemon: Deferred<MullvadDaemon>) {
+class ConnectionProxy(val context: Context, val daemon: MullvadDaemon) {
     var mainActivity: MainActivity? = null
 
     private var activeAction: Job? = null
     private var resetAnticipatedStateJob: Job? = null
 
-    private val attachListenerJob = attachListener()
     private val fetchInitialStateJob = fetchInitialState()
-
     private val initialState: TunnelState = TunnelState.Disconnected()
 
     var state = initialState
@@ -47,6 +44,14 @@ class ConnectionProxy(val context: Context, val daemon: Deferred<MullvadDaemon>)
     var onStateChange = EventNotifier(state)
     var vpnPermission = CompletableDeferred<Boolean>()
 
+    init {
+        daemon.onTunnelStateChange = { newState ->
+            synchronized(this) {
+                state = newState
+            }
+        }
+    }
+
     fun connect() {
         if (anticipateConnectingState()) {
             cancelActiveAction()
@@ -55,7 +60,7 @@ class ConnectionProxy(val context: Context, val daemon: Deferred<MullvadDaemon>)
 
             activeAction = GlobalScope.launch(Dispatchers.Default) {
                 if (vpnPermission.await()) {
-                    daemon.await().connect()
+                    daemon.connect()
                 }
             }
         }
@@ -65,7 +70,7 @@ class ConnectionProxy(val context: Context, val daemon: Deferred<MullvadDaemon>)
         if (anticipateDisconnectingState()) {
             cancelActiveAction()
             activeAction = GlobalScope.launch(Dispatchers.Default) {
-                daemon.await().disconnect()
+                daemon.disconnect()
             }
         }
     }
@@ -75,10 +80,11 @@ class ConnectionProxy(val context: Context, val daemon: Deferred<MullvadDaemon>)
     }
 
     fun onDestroy() {
+        daemon.onTunnelStateChange = null
+
         onUiStateChange.unsubscribeAll()
         onStateChange.unsubscribeAll()
-        attachListenerJob.cancel()
-        detachListener()
+
         fetchInitialStateJob.cancel()
         cancelActiveAction()
     }
@@ -152,7 +158,7 @@ class ConnectionProxy(val context: Context, val daemon: Deferred<MullvadDaemon>)
     }
 
     private fun fetchInitialState() = GlobalScope.launch(Dispatchers.Default) {
-        val currentState = daemon.await().getState()
+        val currentState = daemon.getState()
 
         synchronized(this) {
             if (state === initialState) {
@@ -160,16 +166,4 @@ class ConnectionProxy(val context: Context, val daemon: Deferred<MullvadDaemon>)
             }
         }
     }
-
-    private fun attachListener() = GlobalScope.launch(Dispatchers.Default) {
-        daemon.await().onTunnelStateChange = { newState ->
-            synchronized(this) {
-                state = newState
-            }
-        }
-    }
-
-    private fun detachListener() = GlobalScope.launch(Dispatchers.Default) {
-        daemon.await().onTunnelStateChange = null
-    }
 }
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 34ccb59bfd..b352db1fce 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/service/ForegroundNotificationManager.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/service/ForegroundNotificationManager.kt
@@ -16,6 +16,7 @@ import net.mullvad.mullvadvpn.dataproxy.ConnectionProxy
 import net.mullvad.mullvadvpn.model.TunnelState
 import net.mullvad.mullvadvpn.ui.MainActivity
 import net.mullvad.talpid.tunnel.ActionAfterDisconnect
+import net.mullvad.talpid.util.EventNotifier
 
 val CHANNEL_ID = "vpn_tunnel_status"
 val FOREGROUND_NOTIFICATION_ID: Int = 1
@@ -23,16 +24,42 @@ val KEY_CONNECT_ACTION = "connect_action"
 val KEY_DISCONNECT_ACTION = "disconnect_action"
 val PERMISSION_TUNNEL_ACTION = "net.mullvad.mullvadvpn.permission.TUNNEL_ACTION"
 
-class ForegroundNotificationManager(val service: Service, val connectionProxy: ConnectionProxy) {
+class ForegroundNotificationManager(
+    val service: MullvadVpnService,
+    val serviceNotifier: EventNotifier<ServiceInstance?>
+) {
     private val notificationManager =
         service.getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager
 
-    private val listenerId = connectionProxy.onStateChange.subscribe { state ->
-        tunnelState = state
+    private val listenerId = serviceNotifier.subscribe { newServiceInstance ->
+        serviceInstance = newServiceInstance
     }
 
+    private var serviceInstance: ServiceInstance? = null
+        set(value) {
+            synchronized(this) {
+                if (value != null) {
+                    connectionProxy = value.connectionProxy.apply {
+                        onStateChange.subscribe { state ->
+                            tunnelState = state
+                        }
+                    }
+                } else {
+                    connectionProxy = null
+                    connectionListenerId?.let { listenerId ->
+                        field?.connectionProxy?.onStateChange?.unsubscribe(listenerId)
+                    }
+                }
+
+                field = value
+            }
+        }
+
     private val badgeColor = service.resources.getColor(R.color.colorPrimary)
 
+    private var connectionListenerId: Int? = null
+    private var connectionProxy: ConnectionProxy? = null
+
     private var onForeground = false
     private var reconnecting = false
     private var showingReconnecting = false
@@ -130,19 +157,16 @@ class ForegroundNotificationManager(val service: Service, val connectionProxy: C
 
     private val connectReceiver = object : BroadcastReceiver() {
         override fun onReceive(context: Context, intent: Intent) {
-            onConnect?.invoke()
+            connectionProxy?.connect()
         }
     }
 
     private val disconnectReceiver = object : BroadcastReceiver() {
         override fun onReceive(context: Context, intent: Intent) {
-            onDisconnect?.invoke()
+            connectionProxy?.disconnect()
         }
     }
 
-    var onConnect: (() -> Unit)? = null
-    var onDisconnect: (() -> Unit)? = null
-
     var loggedIn = false
         set(value) {
             field = value
@@ -172,7 +196,7 @@ class ForegroundNotificationManager(val service: Service, val connectionProxy: C
     }
 
     fun onDestroy() {
-        connectionProxy.onStateChange.unsubscribe(listenerId)
+        serviceNotifier.unsubscribe(listenerId)
 
         service.apply {
             unregisterReceiver(connectReceiver)
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt
index 1623400336..b903d35045 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt
@@ -4,25 +4,37 @@ import android.content.Intent
 import android.net.VpnService
 import android.os.Binder
 import android.os.IBinder
-import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
-import kotlinx.coroutines.async
-import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.launch
 import net.mullvad.mullvadvpn.dataproxy.ConnectionProxy
 import net.mullvad.talpid.TalpidVpnService
 import net.mullvad.talpid.util.EventNotifier
 
 class MullvadVpnService : TalpidVpnService() {
     private val binder = LocalBinder()
+    private val serviceNotifier = EventNotifier<ServiceInstance?>(null)
 
     private var isStopping = false
 
-    private lateinit var daemon: Deferred<MullvadDaemon>
-    private lateinit var connectionProxy: ConnectionProxy
+    private var connectionProxy: ConnectionProxy? = null
+    private var daemon: MullvadDaemon? = null
+    private var startDaemonJob: Job? = null
+
     private lateinit var notificationManager: ForegroundNotificationManager
 
-    private var serviceNotifier = EventNotifier<ServiceInstance?>(null)
+    var shouldConnect = false
+        set(value) {
+            field = value
+
+            if (value == true) {
+                daemon?.apply {
+                    connect()
+                    field = false
+                }
+            }
+        }
 
     private var bindCount = 0
         set(value) {
@@ -33,14 +45,12 @@ class MullvadVpnService : TalpidVpnService() {
     private var isBound = false
         set(value) {
             field = value
-
-            if (this::notificationManager.isInitialized) {
-                notificationManager.lockedToForeground = value
-            }
+            notificationManager.lockedToForeground = value
         }
 
     override fun onCreate() {
         super.onCreate()
+        notificationManager = ForegroundNotificationManager(this, serviceNotifier)
         setUp()
     }
 
@@ -71,6 +81,7 @@ class MullvadVpnService : TalpidVpnService() {
 
     override fun onDestroy() {
         tearDown()
+        notificationManager.onDestroy()
         super.onDestroy()
     }
 
@@ -85,27 +96,29 @@ class MullvadVpnService : TalpidVpnService() {
 
     override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
         val startResult = super.onStartCommand(intent, flags, startId)
+
         if (intent?.getAction() == VpnService.SERVICE_INTERFACE) {
-            runBlocking { daemon.await().connect() }
+            shouldConnect = true
         }
+
         return startResult
     }
 
     private fun setUp() {
-        daemon = startDaemon()
-        connectionProxy = ConnectionProxy(this, daemon)
-        notificationManager = startNotificationManager()
+        startDaemonJob?.cancel()
+        startDaemonJob = startDaemon()
     }
 
-    private fun startDaemon() = GlobalScope.async(Dispatchers.Default) {
+    private fun startDaemon() = GlobalScope.launch(Dispatchers.Default) {
         ApiRootCaFile().extract(application)
 
-        val daemon = MullvadDaemon(this@MullvadVpnService).apply {
+        val newDaemon = MullvadDaemon(this@MullvadVpnService).apply {
             onSettingsChange.subscribe { settings ->
                 notificationManager.loggedIn = settings?.accountToken != null
             }
 
             onDaemonStopped = {
+                connectionProxy?.onDestroy()
                 serviceNotifier.notify(null)
 
                 if (!isStopping) {
@@ -114,17 +127,16 @@ class MullvadVpnService : TalpidVpnService() {
             }
         }
 
-        serviceNotifier.notify(ServiceInstance(daemon, connectionProxy, connectivityListener))
+        val newConnectionProxy = ConnectionProxy(this@MullvadVpnService, newDaemon).apply {
+            if (shouldConnect) {
+                connect()
+            }
+        }
 
-        daemon
-    }
+        daemon = newDaemon
+        connectionProxy = newConnectionProxy
 
-    private fun startNotificationManager(): ForegroundNotificationManager {
-        return ForegroundNotificationManager(this, connectionProxy).apply {
-            onConnect = { connectionProxy.connect() }
-            onDisconnect = { connectionProxy.disconnect() }
-            lockedToForeground = isBound
-        }
+        serviceNotifier.notify(ServiceInstance(newDaemon, newConnectionProxy, connectivityListener))
     }
 
     private fun stop() {
@@ -134,18 +146,12 @@ class MullvadVpnService : TalpidVpnService() {
     }
 
     private fun stopDaemon() {
-        if (daemon.isCompleted) {
-            runBlocking { daemon.await().shutdown() }
-        } else {
-            daemon.cancel()
-        }
+        startDaemonJob?.cancel()
+        daemon?.shutdown()
     }
 
     private fun tearDown() {
         stopDaemon()
-
-        connectionProxy.onDestroy()
-        notificationManager.onDestroy()
     }
 
     private fun restart() {