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 <deadbeef@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20196}diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc
index c02bac7..0b3b073 100644
--- a/p2p/base/p2ptransportchannel.cc
+++ b/p2p/base/p2ptransportchannel.cc
@@ -334,8 +334,9 @@
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 0e68c46..bf1351b 100644
--- a/p2p/base/turnport.cc
+++ b/p2p/base/turnport.cc
@@ -13,6 +13,7 @@
#include <algorithm>
#include <functional>
+#include "api/optional.h"
#include "p2p/base/common.h"
#include "p2p/base/stun.h"
#include "rtc_base/asyncpacketsocket.h"
@@ -147,10 +148,15 @@
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<int64_t> 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 @@
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<int64_t> destruction_timestamp_;
};
TurnPort::TurnPort(rtc::Thread* thread,
@@ -1040,9 +1046,21 @@
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 @@
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::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<void>(
@@ -1082,11 +1104,6 @@
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 895055d..f891983 100644
--- a/p2p/base/turnport.h
+++ b/p2p/base/turnport.h
@@ -267,7 +267,6 @@
// 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 d6082ef..0bc4b31 100644
--- a/p2p/base/turnport_unittest.cc
+++ b/p2p/base/turnport_unittest.cc
@@ -1423,6 +1423,19 @@
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)