From d65ae4a8ba50a327e2aa84619fa19207bdc34e3f Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Sun, 8 Oct 2017 11:46:15 -0700 Subject: [PATCH] Fixing DCHECK in turnport.cc and doing some related cleanup. Namely: * Changing destruction_timestamp_ to rtc::Optional, instead of using 0 as a magic value. * Adding some comments. * Adding a log statement that would have helped debugging the issue that hit this DCHECK. * Getting rid of a 2-line method called in one place, which was not really helping code readability. Bug: None Change-Id: I5fb1ce60edea29cab0c2a8c97e735f26c08aba62 Reviewed-on: https://webrtc-review.googlesource.com/7440 Commit-Queue: Taylor Brandstetter Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#20196} --- p2p/base/p2ptransportchannel.cc | 5 +-- p2p/base/turnport.cc | 55 +++++++++++++++++++++------------ p2p/base/turnport.h | 1 - p2p/base/turnport_unittest.cc | 13 ++++++++ 4 files changed, 52 insertions(+), 22 deletions(-) diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc index c02bac7e66..0b3b073859 100644 --- a/p2p/base/p2ptransportchannel.cc +++ b/p2p/base/p2ptransportchannel.cc @@ -334,8 +334,9 @@ void P2PTransportChannel::SetIceParameters(const IceParameters& ice_params) { void P2PTransportChannel::SetRemoteIceParameters( const IceParameters& ice_params) { RTC_DCHECK(network_thread_ == rtc::Thread::Current()); - LOG(LS_INFO) << "Remote supports ICE renomination ? " - << ice_params.renomination; + LOG(LS_INFO) << "Received remote ICE parameters: ufrag=" << ice_params.ufrag + << ", renomination " + << (ice_params.renomination ? "enabled" : "disabled"); IceParameters* current_ice = remote_ice(); if (!current_ice || *current_ice != ice_params) { // Keep the ICE credentials so that newer connections diff --git a/p2p/base/turnport.cc b/p2p/base/turnport.cc index 0e68c4631b..bf1351bcfb 100644 --- a/p2p/base/turnport.cc +++ b/p2p/base/turnport.cc @@ -13,6 +13,7 @@ #include #include +#include "api/optional.h" #include "p2p/base/common.h" #include "p2p/base/stun.h" #include "rtc_base/asyncpacketsocket.h" @@ -147,10 +148,15 @@ class TurnEntry : public sigslot::has_slots<> { const rtc::SocketAddress& address() const { return ext_addr_; } BindState state() const { return state_; } - int64_t destruction_timestamp() { return destruction_timestamp_; } - void set_destruction_timestamp(int64_t destruction_timestamp) { - destruction_timestamp_ = destruction_timestamp; + // If the destruction timestamp is set, that means destruction has been + // scheduled (will occur TURN_PERMISSION_TIMEOUT after it's scheduled). + rtc::Optional destruction_timestamp() { + return destruction_timestamp_; } + void set_destruction_timestamp(int64_t destruction_timestamp) { + destruction_timestamp_.emplace(destruction_timestamp); + } + void reset_destruction_timestamp() { destruction_timestamp_.reset(); } // Helper methods to send permission and channel bind requests. void SendCreatePermissionRequest(int delay); @@ -174,11 +180,11 @@ class TurnEntry : public sigslot::has_slots<> { int channel_id_; rtc::SocketAddress ext_addr_; BindState state_; - // A non-zero value indicates that this entry is scheduled to be destroyed. - // It is also used as an ID of the event scheduling. When the destruction - // event actually fires, the TurnEntry will be destroyed only if the - // timestamp here matches the one in the firing event. - int64_t destruction_timestamp_ = 0; + // An unset value indicates that this entry is scheduled to be destroyed. It + // is also used as an ID of the event scheduling. When the destruction event + // actually fires, the TurnEntry will be destroyed only if the timestamp here + // matches the one in the firing event. + rtc::Optional destruction_timestamp_; }; TurnPort::TurnPort(rtc::Thread* thread, @@ -1040,9 +1046,21 @@ void TurnPort::CreateOrRefreshEntry(const rtc::SocketAddress& addr) { entry = new TurnEntry(this, next_channel_number_++, addr); entries_.push_back(entry); } else { - // The channel binding request for the entry will be refreshed automatically - // until the entry is destroyed. - CancelEntryDestruction(entry); + if (entry->destruction_timestamp()) { + // Destruction should have only been scheduled (indicated by + // destruction_timestamp being set) if there were no connections using + // this address. + RTC_DCHECK(!GetConnection(addr)); + // Resetting the destruction timestamp will ensure that any queued + // destruction tasks, when executed, will see that the timestamp doesn't + // match and do nothing. We do this because (currently) there's not a + // convenient way to cancel queued tasks. + entry->reset_destruction_timestamp(); + } else { + // The only valid reason for destruction not being scheduled is that + // there's still one connection. + RTC_DCHECK(GetConnection(addr)); + } } } @@ -1057,8 +1075,12 @@ void TurnPort::DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp) { if (!EntryExists(entry)) { return; } - bool cancelled = timestamp != entry->destruction_timestamp(); - if (!cancelled) { + // The destruction timestamp is used to manage pending destructions. Proceed + // with destruction if it's set, and matches the timestamp from the posted + // task. Note that CreateOrRefreshEntry will unset the timestamp, canceling + // destruction. + if (entry->destruction_timestamp() && + timestamp == *entry->destruction_timestamp()) { DestroyEntry(entry); } } @@ -1073,7 +1095,7 @@ void TurnPort::HandleConnectionDestroyed(Connection* conn) { } void TurnPort::ScheduleEntryDestruction(TurnEntry* entry) { - RTC_DCHECK(entry->destruction_timestamp() == 0); + RTC_DCHECK(!entry->destruction_timestamp().has_value()); int64_t timestamp = rtc::TimeMillis(); entry->set_destruction_timestamp(timestamp); invoker_.AsyncInvokeDelayed( @@ -1082,11 +1104,6 @@ void TurnPort::ScheduleEntryDestruction(TurnEntry* entry) { TURN_PERMISSION_TIMEOUT); } -void TurnPort::CancelEntryDestruction(TurnEntry* entry) { - RTC_DCHECK(entry->destruction_timestamp() != 0); - entry->set_destruction_timestamp(0); -} - bool TurnPort::SetEntryChannelId(const rtc::SocketAddress& address, int channel_id) { TurnEntry* entry = FindEntry(address); diff --git a/p2p/base/turnport.h b/p2p/base/turnport.h index 895055d5b1..f8919836cb 100644 --- a/p2p/base/turnport.h +++ b/p2p/base/turnport.h @@ -267,7 +267,6 @@ class TurnPort : public Port { // in |entry|. void DestroyEntryIfNotCancelled(TurnEntry* entry, int64_t timestamp); void ScheduleEntryDestruction(TurnEntry* entry); - void CancelEntryDestruction(TurnEntry* entry); // Marks the connection with remote address |address| failed and // pruned (a.k.a. write-timed-out). Returns true if a connection is found. diff --git a/p2p/base/turnport_unittest.cc b/p2p/base/turnport_unittest.cc index d6082efd58..0bc4b31413 100644 --- a/p2p/base/turnport_unittest.cc +++ b/p2p/base/turnport_unittest.cc @@ -1423,6 +1423,19 @@ TEST_F(TurnPortTest, TestTurnTLSReleaseAllocation) { TestTurnReleaseAllocation(PROTO_TLS); } +// Test that nothing bad happens if we try to create a connection to the same +// remote address twice. Previously there was a bug that caused this to hit a +// DCHECK. +TEST_F(TurnPortTest, CanCreateTwoConnectionsToSameAddress) { + CreateTurnPort(kTurnUsername, kTurnPassword, kTurnUdpProtoAddr); + PrepareTurnAndUdpPorts(PROTO_UDP); + Connection* conn1 = turn_port_->CreateConnection(udp_port_->Candidates()[0], + Port::ORIGIN_MESSAGE); + Connection* conn2 = turn_port_->CreateConnection(udp_port_->Candidates()[0], + Port::ORIGIN_MESSAGE); + EXPECT_NE(conn1, conn2); +} + // This test verifies any FD's are not leaked after TurnPort is destroyed. // https://code.google.com/p/webrtc/issues/detail?id=2651 #if defined(WEBRTC_LINUX) && !defined(WEBRTC_ANDROID)