Fix race conditions in NetworkMonitor.

This change makes the class thread-safe.

Bug: b/73773043
Change-Id: I1ad13e4f15907e3dd1fef1307f9c654e53b69b22
Reviewed-on: https://webrtc-review.googlesource.com/57040
Commit-Queue: Honghai Zhang <honghaiz@webrtc.org>
Reviewed-by: Sami Kalliomäki <sakal@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22238}

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

@@ -20,12 +20,12 @@ import org.webrtc.NetworkMonitorAutoDetect.ConnectionType;
 import org.webrtc.NetworkMonitorAutoDetect.NetworkInformation;
 
 /**
- * Borrowed from Chromium's src/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
+ * Borrowed from Chromium's
+ * src/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
  *
  * <p>Triggers updates to the underlying network state from OS networking events.
  *
- * <p>Thread-safety is ensured for methods that may be called from both native code and java code,
- * including getInstance(), startMonitoring(), and stopMonitoring().
+ * <p>This class is thread-safe.
  */
 public class NetworkMonitor {
   /**
@@ -54,12 +54,13 @@ public class NetworkMonitor {
   // Also guarded by autoDetectorLock.
   private int numMonitors;
 
-  private ConnectionType currentConnectionType = ConnectionType.CONNECTION_UNKNOWN;
+  private volatile ConnectionType currentConnectionType;
 
   private NetworkMonitor() {
     nativeNetworkObservers = new ArrayList<Long>();
     networkObservers = new ArrayList<NetworkObserver>();
     numMonitors = 0;
+    currentConnectionType = ConnectionType.CONNECTION_UNKNOWN;
   }
 
   // TODO(sakal): Remove once downstream dependencies have been updated.
@@ -111,7 +112,9 @@ public class NetworkMonitor {
 
     startMonitoring();
     // The native observers expect a network list update after they call startMonitoring.
-    nativeNetworkObservers.add(nativeObserver);
+    synchronized (nativeNetworkObservers) {
+      nativeNetworkObservers.add(nativeObserver);
+    }
     updateObserverActiveNetworkList(nativeObserver);
     // currentConnectionType was updated in startMonitoring().
     // Need to notify the native observers here.
@@ -132,7 +135,9 @@ public class NetworkMonitor {
   private void stopMonitoring(long nativeObserver) {
     Logging.d(TAG, "Stop monitoring with native observer " + nativeObserver);
     stopMonitoring();
-    nativeNetworkObservers.remove(nativeObserver);
+    synchronized (nativeNetworkObservers) {
+      nativeNetworkObservers.remove(nativeObserver);
+    }
   }
 
   // Returns true if network binding is supported on this platform.
@@ -185,23 +190,35 @@ public class NetworkMonitor {
 
   /** Alerts all observers of a connection change. */
   private void notifyObserversOfConnectionTypeChange(ConnectionType newConnectionType) {
-    for (long nativeObserver : nativeNetworkObservers) {
-      nativeNotifyConnectionTypeChanged(nativeObserver);
+    synchronized (nativeNetworkObservers) {
+      for (Long nativeObserver : nativeNetworkObservers) {
+        nativeNotifyConnectionTypeChanged(nativeObserver);
+      }
     }
-    for (NetworkObserver observer : networkObservers) {
+    // This avoids calling external methods while locking on an object.
+    NetworkObserver[] javaObservers;
+    synchronized (networkObservers) {
+      javaObservers = new NetworkObserver[networkObservers.size()];
+      javaObservers = networkObservers.toArray(javaObservers);
+    }
+    for (NetworkObserver observer : javaObservers) {
       observer.onConnectionTypeChanged(newConnectionType);
     }
   }
 
   private void notifyObserversOfNetworkConnect(NetworkInformation networkInfo) {
-    for (long nativeObserver : nativeNetworkObservers) {
-      nativeNotifyOfNetworkConnect(nativeObserver, networkInfo);
+    synchronized (nativeNetworkObservers) {
+      for (Long nativeObserver : nativeNetworkObservers) {
+        nativeNotifyOfNetworkConnect(nativeObserver, networkInfo);
+      }
     }
   }
 
   private void notifyObserversOfNetworkDisconnect(long networkHandle) {
-    for (long nativeObserver : nativeNetworkObservers) {
-      nativeNotifyOfNetworkDisconnect(nativeObserver, networkHandle);
+    synchronized (nativeNetworkObservers) {
+      for (Long nativeObserver : nativeNetworkObservers) {
+        nativeNotifyOfNetworkDisconnect(nativeObserver, networkHandle);
+      }
     }
   }
 
@@ -230,7 +247,9 @@ public class NetworkMonitor {
   }
 
   public void addObserver(NetworkObserver observer) {
-    networkObservers.add(observer);
+    synchronized (networkObservers) {
+      networkObservers.add(observer);
+    }
   }
 
   /**
@@ -244,7 +263,9 @@ public class NetworkMonitor {
   }
 
   public void removeObserver(NetworkObserver observer) {
-    networkObservers.remove(observer);
+    synchronized (networkObservers) {
+      networkObservers.remove(observer);
+    }
   }
 
   /** Checks if there currently is connectivity. */