Merge branch 'fix-restart-race-conditions'

9 files changed, 159 insertions, 47 deletions

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 6d8747f915..19c357a599 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/service/MullvadVpnService.kt
@@ -72,6 +72,8 @@ class MullvadVpnService : TalpidVpnService() {
             }
 
             onDaemonStopped = {
+                serviceNotifier.notify(null)
+
                 if (!isStopping) {
                     restart()
                 }
@@ -105,7 +107,6 @@ class MullvadVpnService : TalpidVpnService() {
     }
 
     private fun tearDown() {
-        serviceNotifier.notify(null)
         stopDaemon()
 
         connectionProxy.onDestroy()
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AccountFragment.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AccountFragment.kt
index a03b99d9f9..572a2a0b65 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AccountFragment.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/AccountFragment.kt
@@ -26,7 +26,7 @@ class AccountFragment : ServiceDependentFragment(OnNoService.GoBack) {
 
     private var updateViewJob: Job? = null
 
-    override fun onCreateView(
+    override fun onSafelyCreateView(
         inflater: LayoutInflater,
         container: ViewGroup?,
         savedInstanceState: Bundle?
@@ -50,18 +50,14 @@ class AccountFragment : ServiceDependentFragment(OnNoService.GoBack) {
         return view
     }
 
-    override fun onResume() {
-        super.onResume()
-
+    override fun onSafelyResume() {
         accountCache.onAccountDataChange = { accountNumber, accountExpiry ->
             updateViewJob = updateView(accountNumber, accountExpiry)
         }
     }
 
-    override fun onPause() {
+    override fun onSafelyPause() {
         accountCache.onAccountDataChange = null
-
-        super.onPause()
     }
 
     private fun updateView(accountNumber: String?, accountExpiry: DateTime?) =
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ConnectFragment.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ConnectFragment.kt
index a6b076b03b..92ebdc102a 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ConnectFragment.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ConnectFragment.kt
@@ -36,7 +36,7 @@ class ConnectFragment : ServiceDependentFragment(OnNoService.GoToLaunchScreen) {
             savedInstanceState?.getBoolean(KEY_IS_TUNNEL_INFO_EXPANDED, false) ?: false
     }
 
-    override fun onCreateView(
+    override fun onSafelyCreateView(
         inflater: LayoutInflater,
         container: ViewGroup?,
         savedInstanceState: Bundle?
@@ -70,9 +70,7 @@ class ConnectFragment : ServiceDependentFragment(OnNoService.GoToLaunchScreen) {
         return view
     }
 
-    override fun onResume() {
-        super.onResume()
-
+    override fun onSafelyResume() {
         locationInfo.isTunnelInfoExpanded = isTunnelInfoExpanded
 
         notificationBanner.onResume()
@@ -96,7 +94,7 @@ class ConnectFragment : ServiceDependentFragment(OnNoService.GoToLaunchScreen) {
         }
     }
 
-    override fun onPause() {
+    override fun onSafelyPause() {
         keyStatusListener.onKeyStatusChange = null
         locationInfoCache.onNewLocation = null
         relayListListener.onRelayListChange = null
@@ -109,17 +107,13 @@ class ConnectFragment : ServiceDependentFragment(OnNoService.GoToLaunchScreen) {
         notificationBanner.onPause()
 
         isTunnelInfoExpanded = locationInfo.isTunnelInfoExpanded
-
-        super.onPause()
     }
 
-    override fun onDestroyView() {
+    override fun onSafelyDestroyView() {
         switchLocationButton.onDestroy()
-
-        super.onDestroyView()
     }
 
-    override fun onSaveInstanceState(state: Bundle) {
+    override fun onSafelySaveInstanceState(state: Bundle) {
         isTunnelInfoExpanded = locationInfo.isTunnelInfoExpanded
         state.putBoolean(KEY_IS_TUNNEL_INFO_EXPANDED, isTunnelInfoExpanded)
     }
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/LoginFragment.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/LoginFragment.kt
index 15bbf26389..434584fb87 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/LoginFragment.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/LoginFragment.kt
@@ -31,7 +31,7 @@ class LoginFragment : ServiceDependentFragment(OnNoService.GoToLaunchScreen) {
     private var loginJob: Deferred<Boolean>? = null
     private var advanceToNextScreenJob: Job? = null
 
-    override fun onCreateView(
+    override fun onSafelyCreateView(
         inflater: LayoutInflater,
         container: ViewGroup?,
         savedInstanceState: Bundle?
@@ -54,17 +54,15 @@ class LoginFragment : ServiceDependentFragment(OnNoService.GoToLaunchScreen) {
         return view
     }
 
-    override fun onResume() {
-        super.onResume()
+    override fun onSafelyResume() {
         advanceToNextScreenJob = GlobalScope.launch(Dispatchers.Main) {
             loggedIn.join()
             openConnectScreen()
         }
     }
 
-    override fun onPause() {
+    override fun onSafelyPause() {
         advanceToNextScreenJob?.cancel()
-        super.onPause()
     }
 
     private fun createAccount() {
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 c1502fc300..6e0314612e 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/MainActivity.kt
@@ -26,7 +26,6 @@ class MainActivity : FragmentActivity() {
     private var serviceConnection: ServiceConnection? = null
     private var serviceConnectionSubscription: Int? = null
     private var shouldConnect = false
-    private var shouldStopService = false
 
     private val serviceConnectionManager = object : android.content.ServiceConnection {
         override fun onServiceConnected(className: ComponentName, binder: IBinder) {
@@ -93,10 +92,6 @@ class MainActivity : FragmentActivity() {
     override fun onStop() {
         serviceNotifier.unsubscribeAll()
 
-        if (shouldStopService) {
-            service?.apply { stop() }
-        }
-
         unbindService(serviceConnectionManager)
 
         super.onStop()
@@ -134,7 +129,7 @@ class MainActivity : FragmentActivity() {
     }
 
     fun quit() {
-        shouldStopService = true
+        service?.stop()
         finishAndRemoveTask()
     }
 
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/SelectLocationFragment.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/SelectLocationFragment.kt
index dc229dc7c0..8f95f2ce79 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/SelectLocationFragment.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/SelectLocationFragment.kt
@@ -41,7 +41,7 @@ class SelectLocationFragment : ServiceDependentFragment(OnNoService.GoToLaunchSc
         }
     }
 
-    override fun onCreateView(
+    override fun onSafelyCreateView(
         inflater: LayoutInflater,
         container: ViewGroup?,
         savedInstanceState: Bundle?
@@ -58,24 +58,18 @@ class SelectLocationFragment : ServiceDependentFragment(OnNoService.GoToLaunchSc
         return view
     }
 
-    override fun onResume() {
-        super.onResume()
-
+    override fun onSafelyResume() {
         relayListListener.onRelayListChange = { relayList, selectedItem ->
             updateRelayListJob = updateRelayList(relayList, selectedItem)
         }
     }
 
-    override fun onPause() {
+    override fun onSafelyPause() {
         relayListListener.onRelayListChange = null
-
-        super.onPause()
     }
 
-    override fun onDestroyView() {
+    override fun onSafelyDestroyView() {
         updateRelayListJob?.cancel()
-
-        super.onDestroyView()
     }
 
     fun close() {
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ServiceDependentFragment.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ServiceDependentFragment.kt
index 96ea5ec1f5..1bafd1beeb 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ServiceDependentFragment.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/ServiceDependentFragment.kt
@@ -1,8 +1,13 @@
 package net.mullvad.mullvadvpn.ui
 
+import android.os.Bundle
+import android.view.LayoutInflater
+import android.view.View
+import android.view.ViewGroup
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.GlobalScope
 import kotlinx.coroutines.launch
+import net.mullvad.mullvadvpn.R
 import net.mullvad.mullvadvpn.dataproxy.AccountCache
 import net.mullvad.mullvadvpn.dataproxy.AppVersionInfoCache
 import net.mullvad.mullvadvpn.dataproxy.ConnectionProxy
@@ -13,11 +18,22 @@ import net.mullvad.mullvadvpn.dataproxy.SettingsListener
 import net.mullvad.mullvadvpn.service.MullvadDaemon
 import net.mullvad.talpid.ConnectivityListener
 
-open class ServiceDependentFragment(val onNoService: OnNoService) : ServiceAwareFragment() {
+abstract class ServiceDependentFragment(val onNoService: OnNoService) : ServiceAwareFragment() {
     enum class OnNoService {
         GoBack, GoToLaunchScreen
     }
 
+    enum class State {
+        Uninitialized,
+        Initialized,
+        Active,
+        Paused,
+        LostConnection,
+        WaitingForReconnection,
+    }
+
+    private var state = State.Uninitialized
+
     lateinit var accountCache: AccountCache
         private set
 
@@ -46,6 +62,8 @@ open class ServiceDependentFragment(val onNoService: OnNoService) : ServiceAware
         private set
 
     override fun onNewServiceConnection(serviceConnection: ServiceConnection) {
+        // This method is always either called first or after an `onNoServiceConnection`, so the
+        // initialization of the fields doesn't have to be synchronized
         accountCache = serviceConnection.accountCache
         appVersionInfoCache = serviceConnection.appVersionInfoCache
         connectionProxy = serviceConnection.connectionProxy
@@ -55,9 +73,122 @@ open class ServiceDependentFragment(val onNoService: OnNoService) : ServiceAware
         locationInfoCache = serviceConnection.locationInfoCache
         relayListListener = serviceConnection.relayListListener
         settingsListener = serviceConnection.settingsListener
+
+        synchronized(this) {
+            when (state) {
+                State.Uninitialized -> state = State.Initialized
+                State.WaitingForReconnection -> state = State.Paused
+            }
+        }
     }
 
     override fun onNoServiceConnection() {
+        synchronized(this) {
+            when (state) {
+                State.Uninitialized -> {
+                    state = State.LostConnection
+                    leaveFragment()
+                }
+                State.Active -> {
+                    state = State.LostConnection
+                    leaveFragment()
+                }
+                State.Paused -> state = State.WaitingForReconnection
+                else -> {}
+            }
+        }
+    }
+
+    override fun onCreateView(
+        inflater: LayoutInflater,
+        container: ViewGroup?,
+        savedInstanceState: Bundle?
+    ): View {
+        synchronized(this) {
+            when (state) {
+                State.Initialized, State.Active, State.Paused -> {
+                    return onSafelyCreateView(inflater, container, savedInstanceState)
+                }
+                State.Uninitialized, State.LostConnection, State.WaitingForReconnection -> {
+                    return inflater.inflate(R.layout.missing_service, container, false)
+                }
+            }
+        }
+    }
+
+    override fun onResume() {
+        super.onResume()
+
+        synchronized(this) {
+            when (state) {
+                State.Initialized, State.Paused -> {
+                    state = State.Active
+                    onSafelyResume()
+                }
+                State.WaitingForReconnection -> {
+                    state = State.LostConnection
+                    leaveFragment()
+                }
+                else -> {}
+            }
+        }
+    }
+
+    override fun onSaveInstanceState(instanceState: Bundle) {
+        synchronized(this) {
+            when (state) {
+                State.Initialized, State.Paused, State.Active -> {
+                    onSafelySaveInstanceState(instanceState)
+                }
+                else -> {}
+            }
+        }
+    }
+
+    override fun onPause() {
+        synchronized(this) {
+            when (state) {
+                State.Initialized, State.Active -> {
+                    onSafelyPause()
+                    state = State.Paused
+                }
+                else -> {}
+            }
+        }
+
+        super.onPause()
+    }
+
+    override fun onDestroyView() {
+        synchronized(this) {
+            when (state) {
+                State.Initialized, State.Paused, State.Active -> onSafelyDestroyView()
+                else -> {}
+            }
+        }
+
+        super.onDestroyView()
+    }
+
+    abstract fun onSafelyCreateView(
+        inflater: LayoutInflater,
+        container: ViewGroup?,
+        savedInstanceState: Bundle?
+    ): View
+
+    open fun onSafelyResume() {
+    }
+
+    open fun onSafelySaveInstanceState(state: Bundle) {
+    }
+
+    open fun onSafelyPause() {
+    }
+
+    open fun onSafelyDestroyView() {
+    }
+
+    private fun leaveFragment() {
         GlobalScope.launch(Dispatchers.Main) {
             when (onNoService) {
                 OnNoService.GoBack -> parentActivity.onBackPressed()
diff --git a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/WireguardKeyFragment.kt b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/WireguardKeyFragment.kt
index 5614fb1f82..74c8b5882c 100644
--- a/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/WireguardKeyFragment.kt
+++ b/android/src/main/kotlin/net/mullvad/mullvadvpn/ui/WireguardKeyFragment.kt
@@ -48,7 +48,7 @@ class WireguardKeyFragment : ServiceDependentFragment(OnNoService.GoToLaunchScre
     private lateinit var verifyButton: Button
     private lateinit var verifySpinner: ProgressBar
 
-    override fun onCreateView(
+    override fun onSafelyCreateView(
         inflater: LayoutInflater,
         container: ViewGroup?,
         savedInstanceState: Bundle?
@@ -280,7 +280,7 @@ class WireguardKeyFragment : ServiceDependentFragment(OnNoService.GoToLaunchScre
         }
     }
 
-    override fun onPause() {
+    override fun onSafelyPause() {
         tunnelStateListener?.let { listener ->
             connectionProxy.onUiStateChange.unsubscribe(listener)
         }
@@ -291,12 +291,9 @@ class WireguardKeyFragment : ServiceDependentFragment(OnNoService.GoToLaunchScre
         validatingKey = false
         generatingKey = false
         urlController.onPause()
-        super.onPause()
     }
 
-    override fun onResume() {
-        super.onResume()
-
+    override fun onSafelyResume() {
         tunnelStateListener = connectionProxy.onUiStateChange.subscribe { uiState ->
             tunnelState = uiState
             updateViewsJob?.cancel()
diff --git a/android/src/main/res/layout/missing_service.xml b/android/src/main/res/layout/missing_service.xml
new file mode 100644
index 0000000000..2292301f18
--- /dev/null
+++ b/android/src/main/res/layout/missing_service.xml
@@ -0,0 +1,6 @@
+<FrameLayout
+        xmlns:android="http://schemas.android.com/apk/res/android"
+        android:layout_width="match_parent"
+        android:layout_height="match_parent"
+        >
+</FrameLayout>