admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert of Update ICE role on all ports, not just ones used for new connections. (patchset #3 id:40001 of https://codereview.webrtc.org/2053043003/ )

Browse Source 
Reason for revert:
Speculative revert: breaks video quality tests on Win and Mac (???): https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Tester/builds/31209

Original issue's description:
> Update ICE role on all ports, not just ones used for new connections.
>
> Previously, if the ICE role changed, SetIceRole was only called on
> the ports from the most recent ICE generation. However, STUN pings
> may still be sent and received by older generation ports, so they
> should receive an updated role as well.
>
> This was previously triggering an ASSERT, because a P2PTransportChannel
> expects the ICE role of each of its ports to match its own role.
>
> Committed: https://crrev.com/370544594e18deb7f560f961295c8cf3f0a679f1
> Cr-Commit-Position: refs/heads/master@{#13226}

TBR=pthatcher@webrtc.org,honghaiz@webrtc.org,deadbeef@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.webrtc.org/2078423004
Cr-Commit-Position: refs/heads/master@{#13240}
This commit is contained in:
phoglund 2016-06-21 08:29:21 -07:00 committed by Commit bot
parent 03153f1032
commit 8d2248ff30
3 changed files with 13 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
webrtc/p2p/base/p2ptransportchannel.cc
View File

@ -279,7 +279,6 @@ void P2PTransportChannel::AddAllocatorSession(
// We now only want to apply new candidates that we receive to the ports
// created by this new session because these are replacing those of the
// previous sessions.
removed_ports_.insert(removed_ports_.end(), ports_.begin(), ports_.end());
ports_.clear();
allocator_sessions_.push_back(std::move(session));
@ -306,20 +305,16 @@ void P2PTransportChannel::SetIceRole(IceRole ice_role) {
ASSERT(worker_thread_ == rtc::Thread::Current());
if (ice_role_ != ice_role) {
ice_role_ = ice_role;
for (PortInterface* port : ports_) {
port->SetIceRole(ice_role);
}
// Update role on removed ports as well, because they may still have
// connections alive that should be using the correct role.
for (PortInterface* port : removed_ports_) {
port->SetIceRole(ice_role);
for (std::vector<PortInterface *>::iterator it = ports_.begin();
it != ports_.end(); ++it) {
(*it)->SetIceRole(ice_role);
}
}
}
void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) {
ASSERT(worker_thread_ == rtc::Thread::Current());
if (!ports_.empty() || !removed_ports_.empty()) {
if (!ports_.empty()) {
LOG(LS_ERROR)
<< "Attempt to change tiebreaker after Port has been allocated.";
return;
@ -972,13 +967,13 @@ int P2PTransportChannel::SetOption(rtc::Socket::Option opt, int value) {
it->second = value;
}
for (PortInterface* port : ports_) {
int val = port->SetOption(opt, value);
for (size_t i = 0; i < ports_.size(); ++i) {
int val = ports_[i]->SetOption(opt, value);
if (val < 0) {
// Because this also occurs deferred, probably no point in reporting an
// error
LOG(WARNING) << "SetOption(" << opt << ", " << value
<< ") failed: " << port->GetError();
LOG(WARNING) << "SetOption(" << opt << ", " << value << ") failed: "
<< ports_[i]->GetError();
}
}
return 0;
@ -1518,9 +1513,10 @@ void P2PTransportChannel::OnPortDestroyed(PortInterface* port) {
ASSERT(worker_thread_ == rtc::Thread::Current());
// Remove this port from the list (if we didn't drop it already).
ports_.erase(std::remove(ports_.begin(), ports_.end(), port));
removed_ports_.erase(
std::remove(removed_ports_.begin(), removed_ports_.end(), port));
std::vector<PortInterface*>::iterator iter =
std::find(ports_.begin(), ports_.end(), port);
if (iter != ports_.end())
ports_.erase(iter);
LOG(INFO) << "Removed port from p2p socket: "
<< static_cast<int>(ports_.size()) << " remaining";
@ -1537,7 +1533,6 @@ void P2PTransportChannel::OnPortNetworkInactive(PortInterface* port) {
if (it == ports_.end()) {
return;
}
removed_ports_.push_back(*it);
ports_.erase(it);
LOG(INFO) << "Removed port due to inactive networks: " << ports_.size()
<< " remaining";

9
webrtc/p2p/base/p2ptransportchannel.h
View File

@ -297,14 +297,7 @@ class P2PTransportChannel : public TransportChannelImpl,
bool incoming_only_;
int error_;
std::vector<std::unique_ptr<PortAllocatorSession>> allocator_sessions_;
// |ports_| contains ports that are used to form new connections when
// new remote candidates are added.
std::vector<PortInterface*> ports_;
// |removed_ports_| contains ports that have been removed from |ports_| and
// are not being used to form new connections, but that aren't yet destroyed.
// They may have existing connections, and they still fire signals such as
// SignalUnknownAddress.
std::vector<PortInterface*> removed_ports_;
std::vector<PortInterface *> ports_;
// |connections_| is a sorted list with the first one always be the
// |best_connection_| when it's not nullptr. The combination of

52
webrtc/p2p/base/p2ptransportchannel_unittest.cc
View File

@ -2693,58 +2693,6 @@ TEST_F(P2PTransportChannelPingTest, TestStopPortAllocatorSessions) {
EXPECT_TRUE(!ch.allocator_session()->IsGettingPorts());
}
// Test that the ICE role is updated even on ports with inactive networks when
// doing continual gathering. These ports may still have connections that need
// a correct role, in case the network becomes active before the connection is
// destroyed.
TEST_F(P2PTransportChannelPingTest,
TestIceRoleUpdatedOnPortAfterSignalNetworkInactive) {
cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
cricket::P2PTransportChannel ch(
"test channel", cricket::ICE_CANDIDATE_COMPONENT_DEFAULT, &pa);
// Starts with ICEROLE_CONTROLLING.
PrepareChannel(&ch);
cricket::IceConfig config = CreateIceConfig(1000, true);
ch.SetIceConfig(config);
ch.Connect();
ch.MaybeStartGathering();
ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1));
cricket::Connection* conn = WaitForConnectionTo(&ch, "1.1.1.1", 1);
ASSERT_TRUE(conn != nullptr);
// Make the fake port signal that its network is inactive, then change the
// ICE role and expect it to be updated.
conn->port()->SignalNetworkInactive(conn->port());
ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, conn->port()->GetIceRole());
}
// Test that the ICE role is updated even on ports with inactive networks.
// These ports may still have connections that need a correct role, for the
// pings sent by those connections until they're replaced by newer-generation
// connections.
TEST_F(P2PTransportChannelPingTest, TestIceRoleUpdatedOnPortAfterIceRestart) {
cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
cricket::P2PTransportChannel ch(
"test channel", cricket::ICE_CANDIDATE_COMPONENT_DEFAULT, &pa);
// Starts with ICEROLE_CONTROLLING.
PrepareChannel(&ch);
ch.Connect();
ch.MaybeStartGathering();
ch.AddRemoteCandidate(CreateHostCandidate("1.1.1.1", 1, 1));
cricket::Connection* conn = WaitForConnectionTo(&ch, "1.1.1.1", 1);
ASSERT_TRUE(conn != nullptr);
// Do an ICE restart, change the role, and expect the old port to have its
// role updated.
ch.SetIceCredentials(kIceUfrag[1], kIcePwd[1]);
ch.MaybeStartGathering();
ch.SetIceRole(cricket::ICEROLE_CONTROLLED);
EXPECT_EQ(cricket::ICEROLE_CONTROLLED, conn->port()->GetIceRole());
}
class P2PTransportChannelMostLikelyToWorkFirstTest
: public P2PTransportChannelPingTest {
public: