Try creating sockets again if network change occurs after bind failed.
If the network interface appears active, but binding the sockets fails,
then it won't produce any candidates even though it's never marked as
"network failed". So this was causing nothing to happen once a network
change event occurs and the interface becomes usable again.
So, this CL adds the condition that we only disable gathering of local
ports if we don't have them already.
See bug for more details.
BUG=webrtc:8256
Review-Url: https://codereview.webrtc.org/3015543002
Cr-Commit-Position: refs/heads/master@{#20007}
diff --git a/p2p/base/relayport.cc b/p2p/base/relayport.cc
index e3b7a7f..0742b54 100644
--- a/p2p/base/relayport.cc
+++ b/p2p/base/relayport.cc
@@ -500,12 +500,9 @@
LOG(LS_WARNING) << "Unknown protocol (" << ra->proto << ")";
}
- if (!socket) {
- LOG(LS_WARNING) << "Socket creation failed";
- }
-
// If we failed to get a socket, move on to the next protocol.
if (!socket) {
+ LOG(LS_WARNING) << "Socket creation failed";
port()->thread()->Post(RTC_FROM_HERE, this, kMessageConnectTimeout);
return;
}
diff --git a/p2p/client/basicportallocator.cc b/p2p/client/basicportallocator.cc
index e006380..f4c1fbd 100644
--- a/p2p/client/basicportallocator.cc
+++ b/p2p/client/basicportallocator.cc
@@ -1105,9 +1105,29 @@
// Else turn off the stuff that we've already got covered.
- // Every config implicitly specifies local, so turn that off right away.
- *flags |= PORTALLOCATOR_DISABLE_UDP;
- *flags |= PORTALLOCATOR_DISABLE_TCP;
+ // Every config implicitly specifies local, so turn that off right away if we
+ // already have a port of the corresponding type. Look for a port that
+ // matches this AllocationSequence's network, is the right protocol, and
+ // hasn't encountered an error.
+ // TODO(deadbeef): This doesn't take into account that there may be another
+ // AllocationSequence that's ABOUT to allocate a UDP port, but hasn't yet.
+ // This can happen if, say, there's a network change event right before an
+ // application-triggered ICE restart. Hopefully this problem will just go
+ // away if we get rid of the gathering "phases" though, which is planned.
+ if (std::any_of(session_->ports_.begin(), session_->ports_.end(),
+ [this](const BasicPortAllocatorSession::PortData& p) {
+ return p.port()->Network() == network_ &&
+ p.port()->GetProtocol() == PROTO_UDP && !p.error();
+ })) {
+ *flags |= PORTALLOCATOR_DISABLE_UDP;
+ }
+ if (std::any_of(session_->ports_.begin(), session_->ports_.end(),
+ [this](const BasicPortAllocatorSession::PortData& p) {
+ return p.port()->Network() == network_ &&
+ p.port()->GetProtocol() == PROTO_TCP && !p.error();
+ })) {
+ *flags |= PORTALLOCATOR_DISABLE_TCP;
+ }
if (config_ && config) {
if (config_->StunServers() == config->StunServers()) {
diff --git a/p2p/client/basicportallocator_unittest.cc b/p2p/client/basicportallocator_unittest.cc
index 52c8334..beebe97 100644
--- a/p2p/client/basicportallocator_unittest.cc
+++ b/p2p/client/basicportallocator_unittest.cc
@@ -752,17 +752,15 @@
EXPECT_EQ(0x12345602U, candidates_[0].address().ip());
}
-// Test that high cost networks are filtered if the flag
-// PORTALLOCATOR_DISABLE_COSTLY_NETWORKS is set.
-TEST_F(BasicPortAllocatorTest, TestGatherLowCostNetworkOnly) {
- SocketAddress addr_wifi(IPAddress(0x12345600U), 0);
- SocketAddress addr_cellular(IPAddress(0x12345601U), 0);
- SocketAddress addr_unknown1(IPAddress(0x12345602U), 0);
- SocketAddress addr_unknown2(IPAddress(0x12345603U), 0);
- // If both Wi-Fi and cellular interfaces are present, only gather on the Wi-Fi
- // interface.
- AddInterface(addr_wifi, "test_wlan0", rtc::ADAPTER_TYPE_WIFI);
- AddInterface(addr_cellular, "test_cell0", rtc::ADAPTER_TYPE_CELLULAR);
+// Test that when the PORTALLOCATOR_DISABLE_COSTLY_NETWORKS flag is set and
+// both Wi-Fi and cell interfaces are available, only Wi-Fi is used.
+TEST_F(BasicPortAllocatorTest,
+ WifiUsedInsteadOfCellWhenCostlyNetworksDisabled) {
+ SocketAddress wifi(IPAddress(0x12345600U), 0);
+ SocketAddress cell(IPAddress(0x12345601U), 0);
+ AddInterface(wifi, "test_wlan0", rtc::ADAPTER_TYPE_WIFI);
+ AddInterface(cell, "test_cell0", rtc::ADAPTER_TYPE_CELLULAR);
+ // Disable all but UDP candidates to make the test simpler.
allocator().set_flags(cricket::PORTALLOCATOR_DISABLE_STUN |
cricket::PORTALLOCATOR_DISABLE_RELAY |
cricket::PORTALLOCATOR_DISABLE_TCP |
@@ -771,35 +769,84 @@
session_->StartGettingPorts();
EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
kDefaultAllocationTimeout, fake_clock);
+ // Should only get one Wi-Fi candidate.
EXPECT_EQ(1U, candidates_.size());
- EXPECT_TRUE(addr_wifi.EqualIPs(candidates_[0].address()));
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", wifi);
+}
- // If both cellular and unknown interfaces are present, only gather on the
- // unknown interfaces.
- candidates_.clear();
- candidate_allocation_done_ = false;
- RemoveInterface(addr_wifi);
- AddInterface(addr_unknown1, "test_unknown0", rtc::ADAPTER_TYPE_UNKNOWN);
- AddInterface(addr_unknown2, "test_unknown1", rtc::ADAPTER_TYPE_UNKNOWN);
+// Test that when the PORTALLOCATOR_DISABLE_COSTLY_NETWORKS flag is set and
+// both "unknown" and cell interfaces are available, only the unknown are used.
+// The unknown interface may be something that ultimately uses Wi-Fi, so we do
+// this to be on the safe side.
+TEST_F(BasicPortAllocatorTest,
+ UnknownInterfaceUsedInsteadOfCellWhenCostlyNetworksDisabled) {
+ SocketAddress cell(IPAddress(0x12345601U), 0);
+ SocketAddress unknown1(IPAddress(0x12345602U), 0);
+ SocketAddress unknown2(IPAddress(0x12345603U), 0);
+ AddInterface(cell, "test_cell0", rtc::ADAPTER_TYPE_CELLULAR);
+ AddInterface(unknown1, "test_unknown0", rtc::ADAPTER_TYPE_UNKNOWN);
+ AddInterface(unknown2, "test_unknown1", rtc::ADAPTER_TYPE_UNKNOWN);
+ // Disable all but UDP candidates to make the test simpler.
+ allocator().set_flags(cricket::PORTALLOCATOR_DISABLE_STUN |
+ cricket::PORTALLOCATOR_DISABLE_RELAY |
+ cricket::PORTALLOCATOR_DISABLE_TCP |
+ cricket::PORTALLOCATOR_DISABLE_COSTLY_NETWORKS);
+ EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
session_->StartGettingPorts();
EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
kDefaultAllocationTimeout, fake_clock);
+ // Should only get two candidates, none of which is cell.
EXPECT_EQ(2U, candidates_.size());
- EXPECT_TRUE((addr_unknown1.EqualIPs(candidates_[0].address()) &&
- addr_unknown2.EqualIPs(candidates_[1].address())) ||
- (addr_unknown1.EqualIPs(candidates_[1].address()) &&
- addr_unknown2.EqualIPs(candidates_[0].address())));
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", unknown1);
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", unknown2);
+}
- // If Wi-Fi, cellular, unknown interfaces are all present, only gather on the
- // Wi-Fi interface.
- candidates_.clear();
- candidate_allocation_done_ = false;
- AddInterface(addr_wifi, "test_wlan0", rtc::ADAPTER_TYPE_WIFI);
+// Test that when the PORTALLOCATOR_DISABLE_COSTLY_NETWORKS flag is set and
+// there are a mix of Wi-Fi, "unknown" and cell interfaces, only the Wi-Fi
+// interface is used.
+TEST_F(BasicPortAllocatorTest,
+ WifiUsedInsteadOfUnknownOrCellWhenCostlyNetworksDisabled) {
+ SocketAddress wifi(IPAddress(0x12345600U), 0);
+ SocketAddress cellular(IPAddress(0x12345601U), 0);
+ SocketAddress unknown1(IPAddress(0x12345602U), 0);
+ SocketAddress unknown2(IPAddress(0x12345603U), 0);
+ AddInterface(wifi, "test_wlan0", rtc::ADAPTER_TYPE_WIFI);
+ AddInterface(cellular, "test_cell0", rtc::ADAPTER_TYPE_CELLULAR);
+ AddInterface(unknown1, "test_unknown0", rtc::ADAPTER_TYPE_UNKNOWN);
+ AddInterface(unknown2, "test_unknown1", rtc::ADAPTER_TYPE_UNKNOWN);
+ // Disable all but UDP candidates to make the test simpler.
+ allocator().set_flags(cricket::PORTALLOCATOR_DISABLE_STUN |
+ cricket::PORTALLOCATOR_DISABLE_RELAY |
+ cricket::PORTALLOCATOR_DISABLE_TCP |
+ cricket::PORTALLOCATOR_DISABLE_COSTLY_NETWORKS);
+ EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
session_->StartGettingPorts();
EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
kDefaultAllocationTimeout, fake_clock);
+ // Should only get one Wi-Fi candidate.
EXPECT_EQ(1U, candidates_.size());
- EXPECT_TRUE(addr_wifi.EqualIPs(candidates_[0].address()));
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", wifi);
+}
+
+// Test that if the PORTALLOCATOR_DISABLE_COSTLY_NETWORKS flag is set, but the
+// only interface available is cellular, it ends up used anyway. A costly
+// connection is always better than no connection.
+TEST_F(BasicPortAllocatorTest,
+ CellUsedWhenCostlyNetworksDisabledButThereAreNoOtherInterfaces) {
+ SocketAddress cellular(IPAddress(0x12345601U), 0);
+ AddInterface(cellular, "test_cell0", rtc::ADAPTER_TYPE_CELLULAR);
+ // Disable all but UDP candidates to make the test simpler.
+ allocator().set_flags(cricket::PORTALLOCATOR_DISABLE_STUN |
+ cricket::PORTALLOCATOR_DISABLE_RELAY |
+ cricket::PORTALLOCATOR_DISABLE_TCP |
+ cricket::PORTALLOCATOR_DISABLE_COSTLY_NETWORKS);
+ EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+ session_->StartGettingPorts();
+ EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
+ kDefaultAllocationTimeout, fake_clock);
+ // Make sure we got the cell candidate.
+ EXPECT_EQ(1U, candidates_.size());
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", cellular);
}
// Test that no more than allocator.max_ipv6_networks() IPv6 networks are used
@@ -953,6 +1000,46 @@
EXPECT_EQ(0U, ports_.size());
}
+// Similar to the above tests, but tests a situation when sockets can't be
+// bound to a network interface, then after a network change event can be.
+// Related bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=8256
+TEST_F(BasicPortAllocatorTest, CandidatesRegatheredAfterBindingFails) {
+ // Only test local ports to simplify test.
+ ResetWithNoServersOrNat();
+ // Provide a situation where the interface appears to be available, but
+ // binding the sockets fails. See bug for description of when this can
+ // happen.
+ std::string if_name("test_net0");
+ AddInterface(kClientAddr, if_name);
+ fss_->set_tcp_sockets_enabled(false);
+ fss_->set_udp_sockets_enabled(false);
+ EXPECT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP));
+ session_->StartGettingPorts();
+ ASSERT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
+ kDefaultAllocationTimeout, fake_clock);
+ // Make sure we actually prevented candidates from being gathered (other than
+ // a single TCP active candidate, since that doesn't require creating a
+ // socket).
+ ASSERT_EQ(1U, candidates_.size());
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "tcp", kClientAddr);
+ candidate_allocation_done_ = false;
+
+ // Now simulate the interface coming up, with the newfound ability to bind
+ // sockets.
+ fss_->set_tcp_sockets_enabled(true);
+ fss_->set_udp_sockets_enabled(true);
+ AddInterface(kClientAddr, if_name);
+ ASSERT_TRUE_SIMULATED_WAIT(candidate_allocation_done_,
+ kDefaultAllocationTimeout, fake_clock);
+ // Should get UDP and TCP candidate.
+ ASSERT_EQ(2U, candidates_.size());
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "udp", kClientAddr);
+ // TODO(deadbeef): This is actually the same active TCP candidate as before.
+ // We should extend this test to also verify that a server candidate is
+ // gathered.
+ EXPECT_PRED4(HasCandidate, candidates_, "local", "tcp", kClientAddr);
+}
+
// Verify candidates with default step delay of 1sec.
TEST_F(BasicPortAllocatorTest, TestGetAllPortsWithOneSecondStepDelay) {
AddInterface(kClientAddr);
