Merge pull request #111 from Jigsaw-Code/bemasc-anr

Don't block the UI thread during bootstrap

Android/app/src/main/java/app/intra/DnsVpnService.java

@@ -53,13 +53,35 @@ public class DnsVpnService extends VpnService implements NetworkManager.NetworkL
   private static final String LOG_TAG = "DnsVpnService";
   private static final int SERVICE_ID = 1; // Only has to be unique within this app.
   private static final String CHANNEL_ID = "vpn";
+  private static final String NO_PENDING_CONNECTION = "This value is not a possible URL.";
 
+  // The network manager is populated in onStartCommand.  Its main function is to enable delayed
+  // initialization if the network is initially disconnected.
   private NetworkManager networkManager;
+
+  // The state of the device's network access, recording the latest update from networkManager.
+  private boolean networkConnected = false;
+
+  // The VPN adapter runs within this service and is responsible for establishing the VPN and
+  // passing packets to the network.  vpnAdapter is only null before startup and after shutdown,
+  // but it may be atomically replaced by restartVpn().
   private VpnAdapter vpnAdapter = null;
+
+  // The server connection represents a DNS query transport.  It is null before shutdown, during a
+  // reconfiguration, and if connection bootstrap fails.
   private ServerConnection serverConnection = null;
-  private boolean networkConnected = false;
+
+  // The URL of the DNS server.  null and "" are special values indicating the default server.
+  // This value can change if the user changes their configuration after starting the VPN.
   private String url = null;
 
+  // The URL of a pending connection attempt, or a special value if there is no pending connection
+  // attempt.  This value is only used within updateServerConnection(), where it serves to avoid
+  // the creation of duplicate outstanding server connection attempts.  Whenever pendingUrl
+  // indicates a pending connection, serverConnection should be null to avoid sending queries to the
+  // previously selected server.
+  private String pendingUrl = NO_PENDING_CONNECTION;
+
   private FirebaseAnalytics firebaseAnalytics;
 
   public boolean isOn() {
@@ -166,11 +188,33 @@ ServerConnection getServerConnection() {
   }
 
   @WorkerThread
-  private synchronized void updateServerConnection() {
-    if (serverConnection != null && TextUtils.equals(url, serverConnection.getUrl())) {
-      return;
+  private void updateServerConnection() {
+    // This method consists of three steps:
+    // 1. a synchronized block to check if an update is necessary and set a flag (pendingUrl)
+    // 2. an unsynchronized section to perform the update without holding the lock
+    // 3. a synchronized block to unset the flag and confirm the update
+
+    // Step 1: Check if an update is necessary and set a flag (pendingUrl).
+    synchronized (this) {
+      if (serverConnection != null && TextUtils.equals(url, serverConnection.getUrl())) {
+        // Connection state is consistent.  No need for an update.
+        return;
+      }
+
+      if (TextUtils.equals(url, pendingUrl)) {
+        // There's a pending update for this URL already.
+        return;
+      }
+
+      // Indicate that there is a pending update.
+      pendingUrl = url;
+
+      // Stop using the old connection if present.
+      serverConnection = null;
     }
 
+    // Step 2: Perform the update (which blocks on network activity) without holding the lock.
+
     // Inform the controller that we are starting a new connection.
     DnsVpnController controller = DnsVpnController.getInstance();
     controller.onConnectionStateChanged(this, ServerConnection.State.NEW);
@@ -179,15 +223,16 @@ private synchronized void updateServerConnection() {
     // the current DNS configuration.
     Bundle bootstrap = new Bundle();
     long beforeBootstrap = SystemClock.elapsedRealtime();
+    final ServerConnection newConnection;
     if (url == null || url.isEmpty()) {
       // Use the Google Resolver
       AssetManager assets = this.getApplicationContext().getAssets();
-      serverConnection = GoogleServerConnection.get(new GoogleServerDatabase(this, assets));
+      newConnection = GoogleServerConnection.get(new GoogleServerDatabase(this, assets));
     } else {
-      serverConnection = StandardServerConnection.get(url);
+      newConnection = StandardServerConnection.get(url);
     }
 
-    if (serverConnection != null) {
+    if (newConnection != null) {
       controller.onConnectionStateChanged(this, ServerConnection.State.WORKING);
 
       // Measure bootstrap delay.
@@ -198,6 +243,22 @@ private synchronized void updateServerConnection() {
       controller.onConnectionStateChanged(this, ServerConnection.State.FAILING);
       firebaseAnalytics.logEvent(Names.BOOTSTRAP_FAILED.name(), bootstrap);
     }
+
+    // Step 3: Unset the flag and confirm the update.
+    synchronized (this) {
+      pendingUrl = NO_PENDING_CONNECTION;
+
+      if (serverConnection != null && TextUtils.equals(url, serverConnection.getUrl())) {
+        // Connection state has somehow become consistent, so an update is no longer needed.
+        return;
+      }
+
+      if (newConnection == null || TextUtils.equals(url, newConnection.getUrl())) {
+        // Current connection state is not consistent, but newConnection is consistent with the
+        // current URL, so perform the update.
+        serverConnection = newConnection;
+      }
+    }
   }
 
   /**
@@ -211,9 +272,6 @@ private synchronized void startVpn() {
       return;
     }
 
-    // The server connection setup process may rely on DNS, so it has to occur before we set up
-    // the VPN.
-    updateServerConnection();
     startVpnAdapter();
 
     DnsVpnController.getInstance().onStartComplete(this, vpnAdapter != null);
@@ -438,6 +496,7 @@ public void onNetworkConnected(NetworkInfo networkInfo) {
     new Thread(
         new Runnable() {
           public void run() {
+            updateServerConnection();
             startVpn();
           }
         }, "startVpn-onNetworkConnected")

Android/app/src/main/java/app/intra/GoogleServerConnection.java

@@ -17,6 +17,7 @@
 
 import android.util.Log;
 
+import androidx.annotation.WorkerThread;
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
@@ -56,6 +57,7 @@ public class GoogleServerConnection implements ServerConnection {
    * work. Blocks for the duration of bootstrap if it has not yet succeeded. This method performs
    * network activity, so it cannot be called on the application's main thread.
    */
+  @WorkerThread
   public static GoogleServerConnection get(GoogleServerDatabase db) {
     GoogleServerConnection s = new GoogleServerConnection(db);
     DualStackResult redirects = s.bootstrap();