Fix issues with NetworkMonitor singleton when used by multiple clients.

When you create multiple "PeerConnectionFactory"s, they end up using
the same NetworkMonitor singleton. But the second one's
"AndroidNetworkMonitor" class (in C++) wasn't getting the expected
network list update, and as a result it wasn't binding sockets to
networks successfully, acting as if the networks didn't exist.

The solution is just to move "updateActiveNetworkList" to
"startMonitoring". This CL also does some other minor
cleanup/refactoring, and fixes a more corner-casey issue where, if the
first PeerConnection is destroyed, the second one would stop receiving
network updates.

BUG=webrtc:7946

Review-Url: https://codereview.webrtc.org/2990693002
Cr-Commit-Position: refs/heads/master@{#19156}

webrtc/sdk/android/api/org/webrtc/NetworkMonitor.java

@@ -70,36 +70,40 @@ public class NetworkMonitor {
     return instance;
   }
 
-  /**
-   * Enables auto detection of the current network state based on notifications from the system.
-   * Note that passing true here requires the embedding app have the platform ACCESS_NETWORK_STATE
-   * permission.
-   *
-   * @param shouldAutoDetect true if the NetworkMonitor should listen for system changes in
-   *  network connectivity.
-   */
-  public static void setAutoDetectConnectivityState(boolean shouldAutoDetect) {
-    getInstance().setAutoDetectConnectivityStateInternal(shouldAutoDetect);
-  }
-
   private static void assertIsTrue(boolean condition) {
     if (!condition) {
       throw new AssertionError("Expected to be true");
     }
   }
 
-  // Called by the native code.
+  /**
+   * Called by the native code.
+   *
+   * Enables auto detection of the current network state based on notifications
+   * from the system. Note that this requires the embedding app have the
+   * platform ACCESS_NETWORK_STATE permission.
+   */
   private void startMonitoring(long nativeObserver) {
     Logging.d(TAG, "Start monitoring from native observer " + nativeObserver);
     nativeNetworkObservers.add(nativeObserver);
-    setAutoDetectConnectivityStateInternal(true);
+    if (autoDetector == null) {
+      createAutoDetector();
+    }
+    // The observers expect a network list update after they call startMonitoring.
+    final NetworkMonitorAutoDetect.NetworkState networkState =
+        autoDetector.getCurrentNetworkState();
+    updateCurrentConnectionType(NetworkMonitorAutoDetect.getConnectionType(networkState));
+    updateObserverActiveNetworkList(nativeObserver);
   }
 
   // Called by the native code.
   private void stopMonitoring(long nativeObserver) {
     Logging.d(TAG, "Stop monitoring from native observer " + nativeObserver);
-    setAutoDetectConnectivityStateInternal(false);
     nativeNetworkObservers.remove(nativeObserver);
+    if (nativeNetworkObservers.isEmpty()) {
+      autoDetector.destroy();
+      autoDetector = null;
+    }
   }
 
   // Called by the native code to determine if network binding is supported
@@ -121,19 +125,7 @@ public class NetworkMonitor {
     return autoDetector == null ? INVALID_NET_ID : autoDetector.getDefaultNetId();
   }
 
-  private void destroyAutoDetector() {
-    if (autoDetector != null) {
-      autoDetector.destroy();
-      autoDetector = null;
-    }
-  }
-
-  private void setAutoDetectConnectivityStateInternal(boolean shouldAutoDetect) {
-    if (!shouldAutoDetect) {
-      destroyAutoDetector();
-      return;
-    }
-    if (autoDetector == null) {
+  private void createAutoDetector() {
     autoDetector = new NetworkMonitorAutoDetect(new NetworkMonitorAutoDetect.Observer() {
 
       @Override
@@ -151,11 +143,6 @@ public class NetworkMonitor {
         notifyObserversOfNetworkDisconnect(networkHandle);
       }
     }, ContextUtils.getApplicationContext());
-      final NetworkMonitorAutoDetect.NetworkState networkState =
-          autoDetector.getCurrentNetworkState();
-      updateCurrentConnectionType(NetworkMonitorAutoDetect.getConnectionType(networkState));
-      updateActiveNetworkList();
-    }
   }
 
   private void updateCurrentConnectionType(ConnectionType newConnectionType) {
@@ -187,7 +174,7 @@ public class NetworkMonitor {
     }
   }
 
-  private void updateActiveNetworkList() {
+  private void updateObserverActiveNetworkList(long nativeObserver) {
     List<NetworkInformation> networkInfoList = autoDetector.getActiveNetworkList();
     if (networkInfoList == null || networkInfoList.size() == 0) {
       return;
@@ -195,10 +182,8 @@ public class NetworkMonitor {
 
     NetworkInformation[] networkInfos = new NetworkInformation[networkInfoList.size()];
     networkInfos = networkInfoList.toArray(networkInfos);
-    for (long nativeObserver : nativeNetworkObservers) {
     nativeNotifyOfActiveNetworkList(nativeObserver, networkInfos);
   }
-  }
 
   /**
    * Adds an observer for any connection type changes.
@@ -242,7 +227,12 @@ public class NetworkMonitor {
   }
 
   // For testing only.
-  public static NetworkMonitorAutoDetect getAutoDetectorForTest() {
+  static void createAutoDetectorForTest() {
+    getInstance().createAutoDetector();
+  }
+
+  // For testing only.
+  static NetworkMonitorAutoDetect getAutoDetectorForTest() {
     return getInstance().autoDetector;
   }
 }

webrtc/sdk/android/instrumentationtests/src/org/webrtc/NetworkMonitorTest.java

@@ -41,6 +41,10 @@ import org.junit.runner.RunWith;
 
 /**
  * Tests for org.webrtc.NetworkMonitor.
+ *
+ * TODO(deadbeef): These tests don't cover the interaction between
+ * NetworkManager.java and androidnetworkmonitor_jni.cc, which is how this
+ * class is used in practice in WebRTC.
  */
 @SuppressLint("NewApi")
 @RunWith(BaseJUnit4ClassRunner.class)
@@ -162,7 +166,7 @@ public class NetworkMonitorTest {
   private void createTestMonitor() {
     Context context = InstrumentationRegistry.getTargetContext();
     NetworkMonitor.resetInstanceForTests();
-    NetworkMonitor.setAutoDetectConnectivityState(true);
+    NetworkMonitor.createAutoDetectorForTest();
     receiver = NetworkMonitor.getAutoDetectorForTest();
     assertNotNull(receiver);
 

webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.h

@@ -65,6 +65,7 @@ class AndroidNetworkMonitor : public rtc::NetworkMonitorBase,
   rtc::AdapterType GetAdapterType(const std::string& if_name) override;
   void OnNetworkConnected(const NetworkInformation& network_info);
   void OnNetworkDisconnected(NetworkHandle network_handle);
+  // Always expected to be called on the network thread.
   void SetNetworkInfos(const std::vector<NetworkInformation>& network_infos);
 
  private: