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}

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

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 @@ 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<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 @@ 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<int64_t> 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<void>(
@@ -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);

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.

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)