From cfba4ffe31424f43b12d348e22c76a75008b5e62 Mon Sep 17 00:00:00 2001 From: Taylor Date: Fri, 31 Jul 2020 22:39:26 +0000 Subject: [PATCH] Revert "Reland "Pass NetworkMonitorFactory through PeerConnectionFactory."" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 7ded73351870bfb45160fa6b9db71a94fe49397b. Reason for revert: Found more code calling NetworkMonitorFactory::SetFactory... Original change's description: > Reland "Pass NetworkMonitorFactory through PeerConnectionFactory." > > This is a reland of 003c9be817817ed0e3aef3f50c78ae5cb31bc0ff > > Original change's description: > > Pass NetworkMonitorFactory through PeerConnectionFactory. > > > > Previously the instance was set through a static method, which was > > really only done because it was difficult to add new > > PeerConnectionFactory construction arguments at the time. > > > > Now that we have PeerConnectionFactoryDependencies it's easy to clean > > this up. > > > > I'm doing this because I plan to add a NetworkMonitor implementation > > for iOS, and don't want to inherit this ugliness. > > > > Bug: webrtc:9883 > > Change-Id: Id94dc061ab1c7186b81af8547393a6e336ff04c2 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180241 > > Reviewed-by: Harald Alvestrand > > Reviewed-by: Sami Kalliomäki > > Commit-Queue: Taylor > > Cr-Commit-Position: refs/heads/master@{#31815} > > TBR=hta@webrtc.org, sakal@webrtc.org > > Bug: webrtc:9883 > Change-Id: Ibf69a22e8f94226908636c7d50ff9eda65bd4129 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180720 > Reviewed-by: Taylor > Commit-Queue: Taylor > Cr-Commit-Position: refs/heads/master@{#31822} TBR=deadbeef@webrtc.org,sakal@webrtc.org,hta@webrtc.org Change-Id: Iae51b94072cec9abc021eed4e51d1fbeee998adc No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9883 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180721 Reviewed-by: Taylor Commit-Queue: Taylor Cr-Commit-Position: refs/heads/master@{#31823} --- api/DEPS | 2 +- api/peer_connection_interface.h | 6 +- pc/peer_connection_factory.cc | 6 +- pc/peer_connection_factory.h | 1 - rtc_base/BUILD.gn | 4 - rtc_base/network.cc | 22 ++--- rtc_base/network.h | 34 +++---- rtc_base/network_monitor.cc | 25 +++++ rtc_base/network_monitor.h | 20 ++++ rtc_base/network_monitor_factory.cc | 18 ---- rtc_base/network_monitor_factory.h | 37 ------- rtc_base/network_unittest.cc | 98 +++++++++---------- sdk/android/native_api/base/network_monitor.h | 2 +- .../peerconnection/peer_connection_factory.cc | 5 +- .../peerconnection/peer_connection_factory.h | 3 +- .../peer_connection_factory_unittest.cc | 2 +- sdk/android/src/jni/android_network_monitor.h | 1 - .../src/jni/pc/owned_factory_and_threads.cc | 8 ++ .../src/jni/pc/owned_factory_and_threads.h | 8 +- .../src/jni/pc/peer_connection_factory.cc | 26 +++-- .../src/jni/pc/peer_connection_factory.h | 3 +- 21 files changed, 155 insertions(+), 176 deletions(-) delete mode 100644 rtc_base/network_monitor_factory.cc delete mode 100644 rtc_base/network_monitor_factory.h diff --git a/api/DEPS b/api/DEPS index 4b93438c3e..4fc132bdc0 100644 --- a/api/DEPS +++ b/api/DEPS @@ -128,7 +128,7 @@ specific_include_rules = { "+media/base/media_config.h", "+media/base/media_engine.h", "+p2p/base/port_allocator.h", - "+rtc_base/network_monitor_factory.h", + "+rtc_base/network.h", "+rtc_base/rtc_certificate.h", "+rtc_base/rtc_certificate_generator.h", "+rtc_base/socket_address.h", diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h index b1beb844e1..52d0d8789a 100644 --- a/api/peer_connection_interface.h +++ b/api/peer_connection_interface.h @@ -113,7 +113,7 @@ // inject a PacketSocketFactory and/or NetworkManager, and not expose // PortAllocator in the PeerConnection api. #include "p2p/base/port_allocator.h" // nogncheck -#include "rtc_base/network_monitor_factory.h" +#include "rtc_base/network.h" #include "rtc_base/rtc_certificate.h" #include "rtc_base/rtc_certificate_generator.h" #include "rtc_base/socket_address.h" @@ -1326,10 +1326,6 @@ struct RTC_EXPORT PeerConnectionFactoryDependencies final { std::unique_ptr network_state_predictor_factory; std::unique_ptr network_controller_factory; - // This will only be used if CreatePeerConnection is called without a - // |port_allocator|, causing the default allocator and network manager to be - // used. - std::unique_ptr network_monitor_factory; std::unique_ptr neteq_factory; std::unique_ptr trials; }; diff --git a/pc/peer_connection_factory.cc b/pc/peer_connection_factory.cc index d79e438152..d3b7fcda8d 100644 --- a/pc/peer_connection_factory.cc +++ b/pc/peer_connection_factory.cc @@ -74,7 +74,6 @@ PeerConnectionFactory::PeerConnectionFactory( worker_thread_(dependencies.worker_thread), signaling_thread_(dependencies.signaling_thread), task_queue_factory_(std::move(dependencies.task_queue_factory)), - network_monitor_factory_(std::move(dependencies.network_monitor_factory)), media_engine_(std::move(dependencies.media_engine)), call_factory_(std::move(dependencies.call_factory)), event_log_factory_(std::move(dependencies.event_log_factory)), @@ -132,10 +131,7 @@ bool PeerConnectionFactory::Initialize() { RTC_DCHECK(signaling_thread_->IsCurrent()); rtc::InitRandom(rtc::Time32()); - // If network_monitor_factory_ is non-null, it will be used to create a - // network monitor while on the network thread. - default_network_manager_.reset( - new rtc::BasicNetworkManager(network_monitor_factory_.get())); + default_network_manager_.reset(new rtc::BasicNetworkManager()); if (!default_network_manager_) { return false; } diff --git a/pc/peer_connection_factory.h b/pc/peer_connection_factory.h index 3932562d22..58859a0296 100644 --- a/pc/peer_connection_factory.h +++ b/pc/peer_connection_factory.h @@ -113,7 +113,6 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface { const std::unique_ptr task_queue_factory_; Options options_; std::unique_ptr channel_manager_; - const std::unique_ptr network_monitor_factory_; std::unique_ptr default_network_manager_; std::unique_ptr default_socket_factory_; std::unique_ptr media_engine_; diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 64548e6f80..73bca85efa 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -705,8 +705,6 @@ rtc_source_set("threading") { # "message_handler.h", # "network_monitor.cc", # "network_monitor.h", - # "network_monitor_factory.cc", - # "network_monitor_factory.h", # "physical_socket_server.cc", # "physical_socket_server.h", # "signal_thread.cc", @@ -855,8 +853,6 @@ rtc_library("rtc_base") { "network_constants.h", "network_monitor.cc", "network_monitor.h", - "network_monitor_factory.cc", - "network_monitor_factory.h", "network_route.cc", "network_route.h", "null_socket_server.cc", diff --git a/rtc_base/network.cc b/rtc_base/network.cc index 3b05c68172..64aee4bdae 100644 --- a/rtc_base/network.cc +++ b/rtc_base/network.cc @@ -470,16 +470,12 @@ Network* NetworkManagerBase::GetNetworkFromAddress( return nullptr; } -BasicNetworkManager::BasicNetworkManager() {} - -BasicNetworkManager::BasicNetworkManager( - NetworkMonitorFactory* network_monitor_factory) - : network_monitor_factory_(network_monitor_factory) {} +BasicNetworkManager::BasicNetworkManager() + : thread_(nullptr), sent_first_update_(false), start_count_(0) {} BasicNetworkManager::~BasicNetworkManager() {} void BasicNetworkManager::OnNetworksChanged() { - RTC_DCHECK_RUN_ON(thread_); RTC_LOG(LS_INFO) << "Network change was observed"; UpdateNetworksOnce(); } @@ -803,8 +799,6 @@ bool BasicNetworkManager::IsIgnoredNetwork(const Network& network) const { void BasicNetworkManager::StartUpdating() { thread_ = Thread::Current(); - // Redundant but necessary for thread annotations. - RTC_DCHECK_RUN_ON(thread_); if (start_count_) { // If network interfaces are already discovered and signal is sent, // we should trigger network signal immediately for the new clients @@ -819,7 +813,7 @@ void BasicNetworkManager::StartUpdating() { } void BasicNetworkManager::StopUpdating() { - RTC_DCHECK_RUN_ON(thread_); + RTC_DCHECK(Thread::Current() == thread_); if (!start_count_) return; @@ -832,11 +826,12 @@ void BasicNetworkManager::StopUpdating() { } void BasicNetworkManager::StartNetworkMonitor() { - if (network_monitor_factory_ == nullptr) { + NetworkMonitorFactory* factory = NetworkMonitorFactory::GetFactory(); + if (factory == nullptr) { return; } if (!network_monitor_) { - network_monitor_.reset(network_monitor_factory_->CreateNetworkMonitor()); + network_monitor_.reset(factory->CreateNetworkMonitor()); if (!network_monitor_) { return; } @@ -854,7 +849,6 @@ void BasicNetworkManager::StopNetworkMonitor() { } void BasicNetworkManager::OnMessage(Message* msg) { - RTC_DCHECK_RUN_ON(thread_); switch (msg->message_id) { case kUpdateNetworksMessage: { UpdateNetworksContinually(); @@ -870,6 +864,7 @@ void BasicNetworkManager::OnMessage(Message* msg) { } IPAddress BasicNetworkManager::QueryDefaultLocalAddress(int family) const { + RTC_DCHECK(thread_ == Thread::Current()); RTC_DCHECK(thread_->socketserver() != nullptr); RTC_DCHECK(family == AF_INET || family == AF_INET6); @@ -898,6 +893,8 @@ void BasicNetworkManager::UpdateNetworksOnce() { if (!start_count_) return; + RTC_DCHECK(Thread::Current() == thread_); + NetworkList list; if (!CreateNetworks(false, &list)) { SignalError(); @@ -921,7 +918,6 @@ void BasicNetworkManager::UpdateNetworksContinually() { } void BasicNetworkManager::DumpNetworks() { - RTC_DCHECK_RUN_ON(thread_); NetworkList list; GetNetworks(&list); RTC_LOG(LS_INFO) << "NetworkManager detected " << list.size() << " networks:"; diff --git a/rtc_base/network.h b/rtc_base/network.h index 26ef628d8a..a67d2a2339 100644 --- a/rtc_base/network.h +++ b/rtc_base/network.h @@ -23,11 +23,8 @@ #include "rtc_base/mdns_responder_interface.h" #include "rtc_base/message_handler.h" #include "rtc_base/network_monitor.h" -#include "rtc_base/network_monitor_factory.h" -#include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/system/rtc_export.h" #include "rtc_base/third_party/sigslot/sigslot.h" -#include "rtc_base/thread_annotations.h" #if defined(WEBRTC_POSIX) struct ifaddrs; @@ -224,7 +221,6 @@ class RTC_EXPORT BasicNetworkManager : public NetworkManagerBase, public sigslot::has_slots<> { public: BasicNetworkManager(); - explicit BasicNetworkManager(NetworkMonitorFactory* network_monitor_factory); ~BasicNetworkManager() override; void StartUpdating() override; @@ -238,9 +234,7 @@ class RTC_EXPORT BasicNetworkManager : public NetworkManagerBase, // Sets the network ignore list, which is empty by default. Any network on the // ignore list will be filtered from network enumeration results. - // Should be called only before initialization. void set_network_ignore_list(const std::vector& list) { - RTC_DCHECK(thread_ == nullptr); network_ignore_list_ = list; } @@ -250,45 +244,41 @@ class RTC_EXPORT BasicNetworkManager : public NetworkManagerBase, void ConvertIfAddrs(ifaddrs* interfaces, IfAddrsConverter* converter, bool include_ignored, - NetworkList* networks) const RTC_RUN_ON(thread_); + NetworkList* networks) const; #endif // defined(WEBRTC_POSIX) // Creates a network object for each network available on the machine. - bool CreateNetworks(bool include_ignored, NetworkList* networks) const - RTC_RUN_ON(thread_); + bool CreateNetworks(bool include_ignored, NetworkList* networks) const; // Determines if a network should be ignored. This should only be determined // based on the network's property instead of any individual IP. - bool IsIgnoredNetwork(const Network& network) const RTC_RUN_ON(thread_); + bool IsIgnoredNetwork(const Network& network) const; // This function connects a UDP socket to a public address and returns the // local address associated it. Since it binds to the "any" address // internally, it returns the default local address on a multi-homed endpoint. - IPAddress QueryDefaultLocalAddress(int family) const RTC_RUN_ON(thread_); + IPAddress QueryDefaultLocalAddress(int family) const; private: friend class NetworkTest; // Creates a network monitor and listens for network updates. - void StartNetworkMonitor() RTC_RUN_ON(thread_); + void StartNetworkMonitor(); // Stops and removes the network monitor. - void StopNetworkMonitor() RTC_RUN_ON(thread_); + void StopNetworkMonitor(); // Called when it receives updates from the network monitor. void OnNetworksChanged(); // Updates the networks and reschedules the next update. - void UpdateNetworksContinually() RTC_RUN_ON(thread_); + void UpdateNetworksContinually(); // Only updates the networks; does not reschedule the next update. - void UpdateNetworksOnce() RTC_RUN_ON(thread_); + void UpdateNetworksOnce(); - Thread* thread_ = nullptr; - bool sent_first_update_ = true; - int start_count_ = 0; + Thread* thread_; + bool sent_first_update_; + int start_count_; std::vector network_ignore_list_; - NetworkMonitorFactory* network_monitor_factory_ RTC_GUARDED_BY(thread_) = - nullptr; - std::unique_ptr network_monitor_ - RTC_GUARDED_BY(thread_); + std::unique_ptr network_monitor_; }; // Represents a Unix-type network interface, with a name and single address. diff --git a/rtc_base/network_monitor.cc b/rtc_base/network_monitor.cc index 8fd5f786d3..4eb52901f3 100644 --- a/rtc_base/network_monitor.cc +++ b/rtc_base/network_monitor.cc @@ -18,6 +18,11 @@ namespace { const uint32_t UPDATE_NETWORKS_MESSAGE = 1; + +// This is set by NetworkMonitorFactory::SetFactory and the caller of +// NetworkMonitorFactory::SetFactory must be responsible for calling +// ReleaseFactory to destroy the factory. +rtc::NetworkMonitorFactory* network_monitor_factory = nullptr; } // namespace namespace rtc { @@ -43,4 +48,24 @@ AdapterType NetworkMonitorBase::GetVpnUnderlyingAdapterType( return ADAPTER_TYPE_UNKNOWN; } +NetworkMonitorFactory::NetworkMonitorFactory() {} +NetworkMonitorFactory::~NetworkMonitorFactory() {} + +void NetworkMonitorFactory::SetFactory(NetworkMonitorFactory* factory) { + if (network_monitor_factory != nullptr) { + delete network_monitor_factory; + } + network_monitor_factory = factory; +} + +void NetworkMonitorFactory::ReleaseFactory(NetworkMonitorFactory* factory) { + if (factory == network_monitor_factory) { + SetFactory(nullptr); + } +} + +NetworkMonitorFactory* NetworkMonitorFactory::GetFactory() { + return network_monitor_factory; +} + } // namespace rtc diff --git a/rtc_base/network_monitor.h b/rtc_base/network_monitor.h index d0f342480f..ed4464db55 100644 --- a/rtc_base/network_monitor.h +++ b/rtc_base/network_monitor.h @@ -98,6 +98,26 @@ class NetworkMonitorBase : public NetworkMonitorInterface, Thread* worker_thread_; }; +/* + * NetworkMonitorFactory creates NetworkMonitors. + */ +class NetworkMonitorFactory { + public: + // This is not thread-safe; it should be called once (or once per audio/video + // call) during the call initialization. + static void SetFactory(NetworkMonitorFactory* factory); + + static void ReleaseFactory(NetworkMonitorFactory* factory); + static NetworkMonitorFactory* GetFactory(); + + virtual NetworkMonitorInterface* CreateNetworkMonitor() = 0; + + virtual ~NetworkMonitorFactory(); + + protected: + NetworkMonitorFactory(); +}; + } // namespace rtc #endif // RTC_BASE_NETWORK_MONITOR_H_ diff --git a/rtc_base/network_monitor_factory.cc b/rtc_base/network_monitor_factory.cc deleted file mode 100644 index 9fac4d95a0..0000000000 --- a/rtc_base/network_monitor_factory.cc +++ /dev/null @@ -1,18 +0,0 @@ -/* - * Copyright 2020 The WebRTC Project Authors. All rights reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include "rtc_base/network_monitor_factory.h" - -namespace rtc { - -NetworkMonitorFactory::NetworkMonitorFactory() {} -NetworkMonitorFactory::~NetworkMonitorFactory() {} - -} // namespace rtc diff --git a/rtc_base/network_monitor_factory.h b/rtc_base/network_monitor_factory.h deleted file mode 100644 index dadcd4aa8a..0000000000 --- a/rtc_base/network_monitor_factory.h +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright 2020 The WebRTC Project Authors. All rights reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef RTC_BASE_NETWORK_MONITOR_FACTORY_H_ -#define RTC_BASE_NETWORK_MONITOR_FACTORY_H_ - -namespace rtc { - -// Forward declaring this so it's not part of the API surface; it's only -// expected to be used by Android/iOS SDK code. -class NetworkMonitorInterface; - -/* - * NetworkMonitorFactory creates NetworkMonitors. - * Note that CreateNetworkMonitor is expected to be called on the network - * thread with the returned object only being used on that thread thereafter. - */ -class NetworkMonitorFactory { - public: - virtual NetworkMonitorInterface* CreateNetworkMonitor() = 0; - - virtual ~NetworkMonitorFactory(); - - protected: - NetworkMonitorFactory(); -}; - -} // namespace rtc - -#endif // RTC_BASE_NETWORK_MONITOR_FACTORY_H_ diff --git a/rtc_base/network_unittest.cc b/rtc_base/network_unittest.cc index 29a89ba1be..cd693563e7 100644 --- a/rtc_base/network_unittest.cc +++ b/rtc_base/network_unittest.cc @@ -19,7 +19,6 @@ #include "rtc_base/checks.h" #include "rtc_base/net_helpers.h" #include "rtc_base/network_monitor.h" -#include "rtc_base/network_monitor_factory.h" #if defined(WEBRTC_POSIX) #include #include @@ -101,27 +100,18 @@ class NetworkTest : public ::testing::Test, public sigslot::has_slots<> { bool IsIgnoredNetwork(BasicNetworkManager& network_manager, const Network& network) { - RTC_DCHECK_RUN_ON(network_manager.thread_); return network_manager.IsIgnoredNetwork(network); } - IPAddress QueryDefaultLocalAddress(BasicNetworkManager& network_manager, - int family) { - RTC_DCHECK_RUN_ON(network_manager.thread_); - return network_manager.QueryDefaultLocalAddress(family); - } - NetworkManager::NetworkList GetNetworks( const BasicNetworkManager& network_manager, bool include_ignored) { - RTC_DCHECK_RUN_ON(network_manager.thread_); NetworkManager::NetworkList list; network_manager.CreateNetworks(include_ignored, &list); return list; } FakeNetworkMonitor* GetNetworkMonitor(BasicNetworkManager& network_manager) { - RTC_DCHECK_RUN_ON(network_manager.thread_); return static_cast( network_manager.network_monitor_.get()); } @@ -146,7 +136,6 @@ class NetworkTest : public ::testing::Test, public sigslot::has_slots<> { struct ifaddrs* interfaces, bool include_ignored, NetworkManager::NetworkList* networks) { - RTC_DCHECK_RUN_ON(network_manager.thread_); // Use the base IfAddrsConverter for test cases. std::unique_ptr ifaddrs_converter(new IfAddrsConverter()); network_manager.ConvertIfAddrs(interfaces, ifaddrs_converter.get(), @@ -258,8 +247,6 @@ class NetworkTest : public ::testing::Test, public sigslot::has_slots<> { class TestBasicNetworkManager : public BasicNetworkManager { public: - TestBasicNetworkManager(NetworkMonitorFactory* network_monitor_factory) - : BasicNetworkManager(network_monitor_factory) {} using BasicNetworkManager::QueryDefaultLocalAddress; using BasicNetworkManager::set_default_local_addresses; }; @@ -281,7 +268,6 @@ TEST_F(NetworkTest, TestIsIgnoredNetworkIgnoresIPsStartingWith0) { Network ipv4_network2("test_eth1", "Test Network Adapter 2", IPAddress(0x010000U), 24, ADAPTER_TYPE_ETHERNET); BasicNetworkManager network_manager; - network_manager.StartUpdating(); EXPECT_FALSE(IsIgnoredNetwork(network_manager, ipv4_network1)); EXPECT_TRUE(IsIgnoredNetwork(network_manager, ipv4_network2)); } @@ -292,18 +278,14 @@ TEST_F(NetworkTest, TestIgnoreList) { 24); Network include_me("include_me", "Include me please!", IPAddress(0x12345600U), 24); - BasicNetworkManager default_network_manager; - default_network_manager.StartUpdating(); - EXPECT_FALSE(IsIgnoredNetwork(default_network_manager, ignore_me)); - EXPECT_FALSE(IsIgnoredNetwork(default_network_manager, include_me)); - - BasicNetworkManager ignoring_network_manager; + BasicNetworkManager network_manager; + EXPECT_FALSE(IsIgnoredNetwork(network_manager, ignore_me)); + EXPECT_FALSE(IsIgnoredNetwork(network_manager, include_me)); std::vector ignore_list; ignore_list.push_back("ignore_me"); - ignoring_network_manager.set_network_ignore_list(ignore_list); - ignoring_network_manager.StartUpdating(); - EXPECT_TRUE(IsIgnoredNetwork(ignoring_network_manager, ignore_me)); - EXPECT_FALSE(IsIgnoredNetwork(ignoring_network_manager, include_me)); + network_manager.set_network_ignore_list(ignore_list); + EXPECT_TRUE(IsIgnoredNetwork(network_manager, ignore_me)); + EXPECT_FALSE(IsIgnoredNetwork(network_manager, include_me)); } // Test is failing on Windows opt: b/11288214 @@ -667,7 +649,6 @@ TEST_F(NetworkTest, TestMultiplePublicNetworksOnOneInterfaceMerge) { // Test that DumpNetworks does not crash. TEST_F(NetworkTest, TestCreateAndDumpNetworks) { BasicNetworkManager manager; - manager.StartUpdating(); NetworkManager::NetworkList list = GetNetworks(manager, true); bool changed; MergeNetworkList(manager, list, &changed); @@ -676,7 +657,6 @@ TEST_F(NetworkTest, TestCreateAndDumpNetworks) { TEST_F(NetworkTest, TestIPv6Toggle) { BasicNetworkManager manager; - manager.StartUpdating(); bool ipv6_found = false; NetworkManager::NetworkList list; list = GetNetworks(manager, true); @@ -773,7 +753,6 @@ TEST_F(NetworkTest, TestConvertIfAddrsNoAddress) { NetworkManager::NetworkList result; BasicNetworkManager manager; - manager.StartUpdating(); CallConvertIfAddrs(manager, &list, true, &result); EXPECT_TRUE(result.empty()); } @@ -789,7 +768,6 @@ TEST_F(NetworkTest, TestConvertIfAddrsMultiAddressesOnOneInterface) { "FFFF:FFFF:FFFF:FFFF::", 0); NetworkManager::NetworkList result; BasicNetworkManager manager; - manager.StartUpdating(); CallConvertIfAddrs(manager, list, true, &result); EXPECT_EQ(1U, result.size()); bool changed; @@ -809,35 +787,46 @@ TEST_F(NetworkTest, TestConvertIfAddrsNotRunning) { NetworkManager::NetworkList result; BasicNetworkManager manager; - manager.StartUpdating(); CallConvertIfAddrs(manager, &list, true, &result); EXPECT_TRUE(result.empty()); } -// Tests that the network type can be determined from the network monitor when -// it would otherwise be unknown. +// Tests that the network type can be updated after the network monitor is +// started. TEST_F(NetworkTest, TestGetAdapterTypeFromNetworkMonitor) { - char if_name[20] = "wifi0"; - std::string ipv6_address = "1000:2000:3000:4000:0:0:0:1"; + char if_name1[20] = "wifi0"; + std::string ipv6_address1 = "1000:2000:3000:4000:0:0:0:1"; + std::string ipv6_address2 = "1000:2000:3000:8000:0:0:0:1"; std::string ipv6_mask = "FFFF:FFFF:FFFF:FFFF::"; - BasicNetworkManager manager_without_monitor; - manager_without_monitor.StartUpdating(); - // A network created without a network monitor will get UNKNOWN type. - ifaddrs* addr_list = InstallIpv6Network(if_name, ipv6_address, ipv6_mask, - manager_without_monitor); - EXPECT_EQ(ADAPTER_TYPE_UNKNOWN, GetAdapterType(manager_without_monitor)); + BasicNetworkManager manager; + // A network created before the network monitor is started will get + // UNKNOWN type. + ifaddrs* addr_list = + InstallIpv6Network(if_name1, ipv6_address1, ipv6_mask, manager); + EXPECT_EQ(ADAPTER_TYPE_UNKNOWN, GetAdapterType(manager)); ReleaseIfAddrs(addr_list); + // Note: Do not call ClearNetworks here in order to test that the type + // of an existing network can be changed after the network monitor starts + // and detects the network type correctly. - // With the fake network monitor the type should be correctly determined. - FakeNetworkMonitorFactory factory; - BasicNetworkManager manager_with_monitor(&factory); - manager_with_monitor.StartUpdating(); + // After the network monitor starts, the type will be updated. + FakeNetworkMonitorFactory* factory = new FakeNetworkMonitorFactory(); + NetworkMonitorFactory::SetFactory(factory); + // This brings up the hook with the network monitor. + manager.StartUpdating(); // Add the same ipv6 address as before but it has the right network type // detected by the network monitor now. - addr_list = InstallIpv6Network(if_name, ipv6_address, ipv6_mask, - manager_with_monitor); - EXPECT_EQ(ADAPTER_TYPE_WIFI, GetAdapterType(manager_with_monitor)); + addr_list = InstallIpv6Network(if_name1, ipv6_address1, ipv6_mask, manager); + EXPECT_EQ(ADAPTER_TYPE_WIFI, GetAdapterType(manager)); ReleaseIfAddrs(addr_list); + ClearNetworks(manager); + + // Add another network with the type inferred from the network monitor. + char if_name2[20] = "cellular0"; + addr_list = InstallIpv6Network(if_name2, ipv6_address2, ipv6_mask, manager); + EXPECT_EQ(ADAPTER_TYPE_CELLULAR, GetAdapterType(manager)); + ReleaseIfAddrs(addr_list); + ClearNetworks(manager); } // Test that the network type can be determined based on name matching in @@ -850,7 +839,6 @@ TEST_F(NetworkTest, TestGetAdapterTypeFromNameMatching) { std::string ipv6_address2 = "1000:2000:3000:8000:0:0:0:1"; std::string ipv6_mask = "FFFF:FFFF:FFFF:FFFF::"; BasicNetworkManager manager; - manager.StartUpdating(); // IPSec interface; name is in form "ipsec". char if_name[20] = "ipsec11"; @@ -1034,10 +1022,11 @@ TEST_F(NetworkTest, TestIPv6Selection) { } TEST_F(NetworkTest, TestNetworkMonitoring) { - FakeNetworkMonitorFactory factory; - BasicNetworkManager manager(&factory); + BasicNetworkManager manager; manager.SignalNetworksChanged.connect(static_cast(this), &NetworkTest::OnNetworksChanged); + FakeNetworkMonitorFactory* factory = new FakeNetworkMonitorFactory(); + NetworkMonitorFactory::SetFactory(factory); manager.StartUpdating(); FakeNetworkMonitor* network_monitor = GetNetworkMonitor(manager); EXPECT_TRUE(network_monitor && network_monitor->started()); @@ -1054,6 +1043,8 @@ TEST_F(NetworkTest, TestNetworkMonitoring) { // Network manager is stopped. manager.StopUpdating(); EXPECT_FALSE(GetNetworkMonitor(manager)->started()); + + NetworkMonitorFactory::ReleaseFactory(factory); } // Fails on Android: https://bugs.chromium.org/p/webrtc/issues/detail?id=4364. @@ -1064,10 +1055,11 @@ TEST_F(NetworkTest, TestNetworkMonitoring) { #endif TEST_F(NetworkTest, MAYBE_DefaultLocalAddress) { IPAddress ip; - FakeNetworkMonitorFactory factory; - TestBasicNetworkManager manager(&factory); + TestBasicNetworkManager manager; manager.SignalNetworksChanged.connect(static_cast(this), &NetworkTest::OnNetworksChanged); + FakeNetworkMonitorFactory* factory = new FakeNetworkMonitorFactory(); + NetworkMonitorFactory::SetFactory(factory); manager.StartUpdating(); EXPECT_TRUE_WAIT(callback_called_, 1000); @@ -1078,12 +1070,12 @@ TEST_F(NetworkTest, MAYBE_DefaultLocalAddress) { EXPECT_TRUE(!networks.empty()); for (const auto* network : networks) { if (network->GetBestIP().family() == AF_INET) { - EXPECT_TRUE(QueryDefaultLocalAddress(manager, AF_INET) != IPAddress()); + EXPECT_TRUE(manager.QueryDefaultLocalAddress(AF_INET) != IPAddress()); } else if (network->GetBestIP().family() == AF_INET6 && !IPIsLoopback(network->GetBestIP())) { // Existence of an IPv6 loopback address doesn't mean it has IPv6 network // enabled. - EXPECT_TRUE(QueryDefaultLocalAddress(manager, AF_INET6) != IPAddress()); + EXPECT_TRUE(manager.QueryDefaultLocalAddress(AF_INET6) != IPAddress()); } } diff --git a/sdk/android/native_api/base/network_monitor.h b/sdk/android/native_api/base/network_monitor.h index d0d354961f..135ebb1e86 100644 --- a/sdk/android/native_api/base/network_monitor.h +++ b/sdk/android/native_api/base/network_monitor.h @@ -15,7 +15,7 @@ #include -#include "rtc_base/network_monitor_factory.h" +#include "rtc_base/network_monitor.h" namespace webrtc { diff --git a/sdk/android/native_api/peerconnection/peer_connection_factory.cc b/sdk/android/native_api/peerconnection/peer_connection_factory.cc index 4e742d1b7a..e6839754ac 100644 --- a/sdk/android/native_api/peerconnection/peer_connection_factory.cc +++ b/sdk/android/native_api/peerconnection/peer_connection_factory.cc @@ -23,10 +23,11 @@ jobject NativeToJavaPeerConnectionFactory( rtc::scoped_refptr pcf, std::unique_ptr network_thread, std::unique_ptr worker_thread, - std::unique_ptr signaling_thread) { + std::unique_ptr signaling_thread, + rtc::NetworkMonitorFactory* network_monitor_factory) { return webrtc::jni::NativeToJavaPeerConnectionFactory( jni, pcf, std::move(network_thread), std::move(worker_thread), - std::move(signaling_thread)); + std::move(signaling_thread), network_monitor_factory); } } // namespace webrtc diff --git a/sdk/android/native_api/peerconnection/peer_connection_factory.h b/sdk/android/native_api/peerconnection/peer_connection_factory.h index 00550a9b12..889d6092e7 100644 --- a/sdk/android/native_api/peerconnection/peer_connection_factory.h +++ b/sdk/android/native_api/peerconnection/peer_connection_factory.h @@ -26,7 +26,8 @@ jobject NativeToJavaPeerConnectionFactory( rtc::scoped_refptr pcf, std::unique_ptr network_thread, std::unique_ptr worker_thread, - std::unique_ptr signaling_thread); + std::unique_ptr signaling_thread, + rtc::NetworkMonitorFactory* network_monitor_factory = nullptr); } // namespace webrtc diff --git a/sdk/android/native_unittests/peerconnection/peer_connection_factory_unittest.cc b/sdk/android/native_unittests/peerconnection/peer_connection_factory_unittest.cc index 75535d052b..54613f9f57 100644 --- a/sdk/android/native_unittests/peerconnection/peer_connection_factory_unittest.cc +++ b/sdk/android/native_unittests/peerconnection/peer_connection_factory_unittest.cc @@ -100,7 +100,7 @@ TEST(PeerConnectionFactoryTest, NativeToJavaPeerConnectionFactory) { jobject java_factory = NativeToJavaPeerConnectionFactory( jni, factory, std::move(network_thread), std::move(worker_thread), - std::move(signaling_thread)); + std::move(signaling_thread), nullptr /* network_monitor_factory */); RTC_LOG(INFO) << java_factory; diff --git a/sdk/android/src/jni/android_network_monitor.h b/sdk/android/src/jni/android_network_monitor.h index 815b72d64d..1d795df991 100644 --- a/sdk/android/src/jni/android_network_monitor.h +++ b/sdk/android/src/jni/android_network_monitor.h @@ -18,7 +18,6 @@ #include "absl/types/optional.h" #include "rtc_base/network_monitor.h" -#include "rtc_base/network_monitor_factory.h" #include "rtc_base/thread_checker.h" #include "sdk/android/src/jni/jni_helpers.h" diff --git a/sdk/android/src/jni/pc/owned_factory_and_threads.cc b/sdk/android/src/jni/pc/owned_factory_and_threads.cc index 5e00ece8ce..e42b117e57 100644 --- a/sdk/android/src/jni/pc/owned_factory_and_threads.cc +++ b/sdk/android/src/jni/pc/owned_factory_and_threads.cc @@ -19,11 +19,19 @@ OwnedFactoryAndThreads::OwnedFactoryAndThreads( std::unique_ptr network_thread, std::unique_ptr worker_thread, std::unique_ptr signaling_thread, + rtc::NetworkMonitorFactory* network_monitor_factory, const rtc::scoped_refptr& factory) : network_thread_(std::move(network_thread)), worker_thread_(std::move(worker_thread)), signaling_thread_(std::move(signaling_thread)), + network_monitor_factory_(network_monitor_factory), factory_(factory) {} +OwnedFactoryAndThreads::~OwnedFactoryAndThreads() { + if (network_monitor_factory_ != nullptr) { + rtc::NetworkMonitorFactory::ReleaseFactory(network_monitor_factory_); + } +} + } // namespace jni } // namespace webrtc diff --git a/sdk/android/src/jni/pc/owned_factory_and_threads.h b/sdk/android/src/jni/pc/owned_factory_and_threads.h index e87879c13f..845d4dbd70 100644 --- a/sdk/android/src/jni/pc/owned_factory_and_threads.h +++ b/sdk/android/src/jni/pc/owned_factory_and_threads.h @@ -33,19 +33,25 @@ class OwnedFactoryAndThreads { std::unique_ptr network_thread, std::unique_ptr worker_thread, std::unique_ptr signaling_thread, + rtc::NetworkMonitorFactory* network_monitor_factory, const rtc::scoped_refptr& factory); - ~OwnedFactoryAndThreads() = default; + ~OwnedFactoryAndThreads(); PeerConnectionFactoryInterface* factory() { return factory_.get(); } rtc::Thread* network_thread() { return network_thread_.get(); } rtc::Thread* signaling_thread() { return signaling_thread_.get(); } rtc::Thread* worker_thread() { return worker_thread_.get(); } + rtc::NetworkMonitorFactory* network_monitor_factory() { + return network_monitor_factory_; + } + void clear_network_monitor_factory() { network_monitor_factory_ = nullptr; } private: const std::unique_ptr network_thread_; const std::unique_ptr worker_thread_; const std::unique_ptr signaling_thread_; + rtc::NetworkMonitorFactory* network_monitor_factory_; const rtc::scoped_refptr factory_; }; diff --git a/sdk/android/src/jni/pc/peer_connection_factory.cc b/sdk/android/src/jni/pc/peer_connection_factory.cc index 2392db2403..9a42a80ef8 100644 --- a/sdk/android/src/jni/pc/peer_connection_factory.cc +++ b/sdk/android/src/jni/pc/peer_connection_factory.cc @@ -138,10 +138,11 @@ ScopedJavaLocalRef NativeToScopedJavaPeerConnectionFactory( rtc::scoped_refptr pcf, std::unique_ptr network_thread, std::unique_ptr worker_thread, - std::unique_ptr signaling_thread) { + std::unique_ptr signaling_thread, + rtc::NetworkMonitorFactory* network_monitor_factory) { OwnedFactoryAndThreads* owned_factory = new OwnedFactoryAndThreads( std::move(network_thread), std::move(worker_thread), - std::move(signaling_thread), pcf); + std::move(signaling_thread), network_monitor_factory, pcf); ScopedJavaLocalRef j_pcf = Java_PeerConnectionFactory_Constructor( env, NativeToJavaPointer(owned_factory)); @@ -171,15 +172,17 @@ PeerConnectionFactoryInterface* PeerConnectionFactoryFromJava(jlong j_p) { // Set in PeerConnectionFactory_initializeAndroidGlobals(). static bool factory_static_initialized = false; + jobject NativeToJavaPeerConnectionFactory( JNIEnv* jni, rtc::scoped_refptr pcf, std::unique_ptr network_thread, std::unique_ptr worker_thread, - std::unique_ptr signaling_thread) { + std::unique_ptr signaling_thread, + rtc::NetworkMonitorFactory* network_monitor_factory) { return NativeToScopedJavaPeerConnectionFactory( jni, pcf, std::move(network_thread), std::move(worker_thread), - std::move(signaling_thread)) + std::move(signaling_thread), network_monitor_factory) .Release(); } @@ -281,9 +284,18 @@ ScopedJavaLocalRef CreatePeerConnectionFactoryForJava( signaling_thread->SetName("signaling_thread", NULL); RTC_CHECK(signaling_thread->Start()) << "Failed to start thread"; + rtc::NetworkMonitorFactory* network_monitor_factory = nullptr; + const absl::optional options = JavaToNativePeerConnectionFactoryOptions(jni, joptions); + // Do not create network_monitor_factory only if the options are + // provided and disable_network_monitor therein is set to true. + if (!(options && options->disable_network_monitor)) { + network_monitor_factory = new AndroidNetworkMonitorFactory(); + rtc::NetworkMonitorFactory::SetFactory(network_monitor_factory); + } + PeerConnectionFactoryDependencies dependencies; dependencies.network_thread = network_thread.get(); dependencies.worker_thread = worker_thread.get(); @@ -298,10 +310,6 @@ ScopedJavaLocalRef CreatePeerConnectionFactoryForJava( dependencies.network_state_predictor_factory = std::move(network_state_predictor_factory); dependencies.neteq_factory = std::move(neteq_factory); - if (!(options && options->disable_network_monitor)) { - dependencies.network_monitor_factory = - std::make_unique(); - } cricket::MediaEngineDependencies media_dependencies; media_dependencies.task_queue_factory = dependencies.task_queue_factory.get(); @@ -328,7 +336,7 @@ ScopedJavaLocalRef CreatePeerConnectionFactoryForJava( return NativeToScopedJavaPeerConnectionFactory( jni, factory, std::move(network_thread), std::move(worker_thread), - std::move(signaling_thread)); + std::move(signaling_thread), network_monitor_factory); } static ScopedJavaLocalRef diff --git a/sdk/android/src/jni/pc/peer_connection_factory.h b/sdk/android/src/jni/pc/peer_connection_factory.h index 5bfdb7a808..904352f425 100644 --- a/sdk/android/src/jni/pc/peer_connection_factory.h +++ b/sdk/android/src/jni/pc/peer_connection_factory.h @@ -24,7 +24,8 @@ jobject NativeToJavaPeerConnectionFactory( rtc::scoped_refptr pcf, std::unique_ptr network_thread, std::unique_ptr worker_thread, - std::unique_ptr signaling_thread); + std::unique_ptr signaling_thread, + rtc::NetworkMonitorFactory* network_monitor_factory = nullptr); } // namespace jni } // namespace webrtc