From 02293096f9689fee3d32defa77dca227cc1eee90 Mon Sep 17 00:00:00 2001 From: Jonas Oreland Date: Tue, 12 Apr 2022 10:54:19 +0200 Subject: [PATCH] Reset all maps in AndroidNetworkMonitor Start()/Stop() This cl/ fixes another race condition with the recent additions to NetworkMonitorAutoDetect (getAllNetworksFromCache). The getAllNetworksFromCache-feature uses the by the Android team preferred way of enumerating networks, i.e to register network listeners. Th recent fix to add IsAdapterAvailable, https://webrtc-review.googlesource.com/c/src/+/257400 contained a bug in that the adapter_type_by_name_ map was not reset either on disconnect or Start/Stop. This cl/ addresses that including unit test. It also de-obfuscates NetworkMonitor so that it always calls NotifyOfActiveNetworkList on startMonitoring even if list.size() == 0. This should not matter but makes code easier to understand. Bug: webrtc:13741 Change-Id: I438b877eebf769a8b2e7292b697ef1c0a349b24f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258721 Reviewed-by: Harald Alvestrand Commit-Queue: Jonas Oreland Cr-Commit-Position: refs/heads/main@{#36530} --- .../api/org/webrtc/NetworkMonitor.java | 5 +- .../android_network_monitor_unittest.cc | 63 +++++++++++++++++++ .../src/jni/android_network_monitor.cc | 31 +++++++-- sdk/android/src/jni/android_network_monitor.h | 7 +++ 4 files changed, 100 insertions(+), 6 deletions(-) diff --git a/sdk/android/api/org/webrtc/NetworkMonitor.java b/sdk/android/api/org/webrtc/NetworkMonitor.java index 173f76e87e..0bc461df18 100644 --- a/sdk/android/api/org/webrtc/NetworkMonitor.java +++ b/sdk/android/api/org/webrtc/NetworkMonitor.java @@ -138,10 +138,11 @@ public class NetworkMonitor { startMonitoring( applicationContext != null ? applicationContext : ContextUtils.getApplicationContext(), fieldTrialsString); - // The native observers expect a network list update after they call startMonitoring. + synchronized (nativeNetworkObservers) { nativeNetworkObservers.add(nativeObserver); } + // The native observer expects a network list update after startMonitoring. updateObserverActiveNetworkList(nativeObserver); // currentConnectionType was updated in startMonitoring(). // Need to notify the native observers here. @@ -271,7 +272,7 @@ public class NetworkMonitor { networkInfoList = (networkChangeDetector == null) ? null : networkChangeDetector.getActiveNetworkList(); } - if (networkInfoList == null || networkInfoList.size() == 0) { + if (networkInfoList == null) { return; } diff --git a/sdk/android/native_unittests/android_network_monitor_unittest.cc b/sdk/android/native_unittests/android_network_monitor_unittest.cc index 129168cce2..455e4f2e19 100644 --- a/sdk/android/native_unittests/android_network_monitor_unittest.cc +++ b/sdk/android/native_unittests/android_network_monitor_unittest.cc @@ -11,6 +11,7 @@ #include "sdk/android/src/jni/android_network_monitor.h" #include "rtc_base/ip_address.h" +#include "rtc_base/logging.h" #include "sdk/android/native_unittests/application_context_provider.h" #include "sdk/android/src/jni/jni_helpers.h" #include "test/gtest.h" @@ -60,6 +61,10 @@ class AndroidNetworkMonitorTest : public ::testing::Test { network_monitor_->Stop(); } + void Disconnect(jni::NetworkHandle handle) { + network_monitor_->OnNetworkDisconnected_n(handle); + } + protected: test::ScopedKeyValueConfig field_trials_; std::unique_ptr network_monitor_; @@ -169,5 +174,63 @@ TEST_F(AndroidNetworkMonitorTest, TestUnderlyingVpnType) { network_monitor_->GetVpnUnderlyingAdapterType("v4-wlan0")); } +// Verify that Disconnect makes interface unavailable. +TEST_F(AndroidNetworkMonitorTest, Disconnect) { + network_monitor_->Start(); + + jni::NetworkHandle ipv4_handle = 100; + rtc::IPAddress ipv4_address(kTestIpv4Address); + jni::NetworkInformation net_info = + CreateNetworkInformation("wlan0", ipv4_handle, ipv4_address); + net_info.type = jni::NETWORK_WIFI; + network_monitor_->SetNetworkInfos({net_info}); + + EXPECT_TRUE(network_monitor_->IsAdapterAvailable("wlan0")); + EXPECT_TRUE(network_monitor_ + ->FindNetworkHandleFromAddressOrName(ipv4_address, "v4-wlan0") + .has_value()); + EXPECT_EQ(network_monitor_->GetAdapterType("v4-wlan0"), + rtc::ADAPTER_TYPE_WIFI); + + // Check that values are reset on disconnect(). + Disconnect(ipv4_handle); + EXPECT_FALSE(network_monitor_->IsAdapterAvailable("wlan0")); + EXPECT_FALSE( + network_monitor_ + ->FindNetworkHandleFromAddressOrName(ipv4_address, "v4-wlan0") + .has_value()); + EXPECT_EQ(network_monitor_->GetAdapterType("v4-wlan0"), + rtc::ADAPTER_TYPE_UNKNOWN); +} + +// Verify that Stop() resets all caches. +TEST_F(AndroidNetworkMonitorTest, Reset) { + network_monitor_->Start(); + + jni::NetworkHandle ipv4_handle = 100; + rtc::IPAddress ipv4_address(kTestIpv4Address); + jni::NetworkInformation net_info = + CreateNetworkInformation("wlan0", ipv4_handle, ipv4_address); + net_info.type = jni::NETWORK_WIFI; + network_monitor_->SetNetworkInfos({net_info}); + + EXPECT_TRUE(network_monitor_->IsAdapterAvailable("wlan0")); + EXPECT_TRUE(network_monitor_ + ->FindNetworkHandleFromAddressOrName(ipv4_address, "v4-wlan0") + .has_value()); + EXPECT_EQ(network_monitor_->GetAdapterType("v4-wlan0"), + rtc::ADAPTER_TYPE_WIFI); + + // Check that values are reset on Stop(). + network_monitor_->Stop(); + EXPECT_FALSE(network_monitor_->IsAdapterAvailable("wlan0")); + EXPECT_FALSE( + network_monitor_ + ->FindNetworkHandleFromAddressOrName(ipv4_address, "v4-wlan0") + .has_value()); + EXPECT_EQ(network_monitor_->GetAdapterType("v4-wlan0"), + rtc::ADAPTER_TYPE_UNKNOWN); +} + } // namespace test } // namespace webrtc diff --git a/sdk/android/src/jni/android_network_monitor.cc b/sdk/android/src/jni/android_network_monitor.cc index 59a9204fe2..4807c78105 100644 --- a/sdk/android/src/jni/android_network_monitor.cc +++ b/sdk/android/src/jni/android_network_monitor.cc @@ -243,6 +243,7 @@ void AndroidNetworkMonitor::Start() { if (started_) { return; } + reset(); started_ = true; surface_cellular_types_ = field_trials_.IsEnabled("WebRTC-SurfaceCellularTypes"); @@ -265,6 +266,15 @@ void AndroidNetworkMonitor::Start() { env, field_trials_.Lookup("WebRTC-NetworkMonitorAutoDetect"))); } +void AndroidNetworkMonitor::reset() { + RTC_DCHECK_RUN_ON(network_thread_); + network_handle_by_address_.clear(); + network_info_by_handle_.clear(); + adapter_type_by_name_.clear(); + vpn_underlying_adapter_type_by_name_.clear(); + network_preference_by_adapter_type_.clear(); +} + void AndroidNetworkMonitor::Stop() { RTC_DCHECK_RUN_ON(network_thread_); if (!started_) { @@ -281,8 +291,7 @@ void AndroidNetworkMonitor::Stop() { Java_NetworkMonitor_stopMonitoring(env, j_network_monitor_, jlongFromPointer(this)); - network_handle_by_address_.clear(); - network_info_by_handle_.clear(); + reset(); } // The implementation is largely taken from UDPSocketPosix::BindToNetwork in @@ -417,6 +426,7 @@ void AndroidNetworkMonitor::OnNetworkConnected_n( for (const rtc::IPAddress& address : network_info.ip_addresses) { network_handle_by_address_[address] = network_info.handle; } + RTC_CHECK(adapter_type_by_name_.size() == network_info_by_handle_.size()); InvokeNetworksChangedCallback(); } @@ -472,8 +482,12 @@ void AndroidNetworkMonitor::OnNetworkDisconnected_n(NetworkHandle handle) { for (const rtc::IPAddress& address : iter->second.ip_addresses) { network_handle_by_address_.erase(address); } + adapter_type_by_name_.erase(iter->second.interface_name); + vpn_underlying_adapter_type_by_name_.erase(iter->second.interface_name); network_info_by_handle_.erase(iter); } + + RTC_CHECK(adapter_type_by_name_.size() == network_info_by_handle_.size()); } void AndroidNetworkMonitor::OnNetworkPreference_n( @@ -491,8 +505,17 @@ void AndroidNetworkMonitor::OnNetworkPreference_n( void AndroidNetworkMonitor::SetNetworkInfos( const std::vector& network_infos) { RTC_DCHECK_RUN_ON(network_thread_); - network_handle_by_address_.clear(); - network_info_by_handle_.clear(); + + // We expect this method to be called once directly after startMonitoring. + // All the caches should be empty. + RTC_CHECK(network_handle_by_address_.empty()); + RTC_CHECK(network_info_by_handle_.empty()); + RTC_CHECK(adapter_type_by_name_.empty()); + RTC_CHECK(vpn_underlying_adapter_type_by_name_.empty()); + RTC_CHECK(network_preference_by_adapter_type_.empty()); + + // ...but reset just in case. + reset(); RTC_LOG(LS_INFO) << "Android network monitor found " << network_infos.size() << " networks"; for (const NetworkInformation& network : network_infos) { diff --git a/sdk/android/src/jni/android_network_monitor.h b/sdk/android/src/jni/android_network_monitor.h index dd661acdcf..a8356df9be 100644 --- a/sdk/android/src/jni/android_network_monitor.h +++ b/sdk/android/src/jni/android_network_monitor.h @@ -29,6 +29,10 @@ #include "sdk/android/src/jni/jni_helpers.h" namespace webrtc { +namespace test { +class AndroidNetworkMonitorTest; +} // namespace test + namespace jni { typedef int64_t NetworkHandle; @@ -120,6 +124,7 @@ class AndroidNetworkMonitor : public rtc::NetworkMonitorInterface { absl::string_view ifname) const; private: + void reset(); void OnNetworkConnected_n(const NetworkInformation& network_info); void OnNetworkDisconnected_n(NetworkHandle network_handle); void OnNetworkPreference_n(NetworkType type, @@ -163,6 +168,8 @@ class AndroidNetworkMonitor : public rtc::NetworkMonitorInterface { RTC_PT_GUARDED_BY(network_thread_) = nullptr; const FieldTrialsView& field_trials_; + + friend class webrtc::test::AndroidNetworkMonitorTest; }; class AndroidNetworkMonitorFactory : public rtc::NetworkMonitorFactory {