Add RTC_DCHECK that port_ is always valid in Connection.

This patch is a follow up to https://webrtc-review.googlesource.com/c/src/+/260943
which made it possible to destroy a Port before a Connection (on that
port).

This patch "reverses" this, by adding RTC_DHECK, and fixes the Port
destructor to release the Connections before invalidating the WeakPtr<>.

Currently there are no known occurrences where a Connection is destroyed after it's Port is. But prior to the change in Port destructor, a bunch unit tests failed on the newly added DCHECKs.

In addition:
a) modify StunReqquestManager to remove entry from hash before calling callback. This makes it possible for callback to modify (clear) hash.
b) clear pending requests when disconnecting from port, "should not be needed", depends on a)
c) add a getter for pending_delete()

Bug: webrtc:13892, webrtc:13865
Change-Id: I5d18f2db8d93b7cc25d18bd620063589ee9257c9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/322861
Reviewed-by: Per Kjellander <perkj@webrtc.org>
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40907}

p2p/base/connection.cc

@@ -262,14 +262,17 @@ const Candidate& Connection::remote_candidate() const {
 }
 
 const rtc::Network* Connection::network() const {
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in network()";
   return port()->Network();
 }
 
 int Connection::generation() const {
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in generation()";
   return port()->generation();
 }
 
 uint64_t Connection::priority() const {
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in priority()";
   if (!port_)
     return 0;
 
@@ -806,13 +809,14 @@ void Connection::Prune() {
 
 void Connection::Destroy() {
   RTC_DCHECK_RUN_ON(network_thread_);
-  RTC_DCHECK(port_) << "Calling Destroy() twice?";
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in Destroy()";
   if (port_)
     port_->DestroyConnection(this);
 }
 
 bool Connection::Shutdown() {
   RTC_DCHECK_RUN_ON(network_thread_);
+  RTC_DCHECK(port_) << ToDebugId() << ": Calling Shutdown() twice?";
   if (!port_)
     return false;  // already shut down.
 
@@ -832,6 +836,9 @@ bool Connection::Shutdown() {
   // information required for logging needs access to `port_`.
   port_.reset();
 
+  // Clear any pending requests (or responses).
+  requests_.Clear();
+
   return true;
 }
 
@@ -846,6 +853,7 @@ void Connection::FailAndPrune() {
   // will be nulled.
   // In such a case, there's a chance that the Port object gets
   // deleted before the Connection object ends up being deleted.
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in FailAndPrune()";
   if (!port_)
     return;
 
@@ -882,6 +890,7 @@ void Connection::set_selected(bool selected) {
 
 void Connection::UpdateState(int64_t now) {
   RTC_DCHECK_RUN_ON(network_thread_);
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in UpdateState()";
   if (!port_)
     return;
 
@@ -958,6 +967,7 @@ int64_t Connection::last_ping_sent() const {
 void Connection::Ping(int64_t now,
                       std::unique_ptr<StunByteStringAttribute> delta) {
   RTC_DCHECK_RUN_ON(network_thread_);
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in Ping()";
   if (!port_)
     return;
 
@@ -1251,11 +1261,13 @@ std::string Connection::ToDebugId() const {
 
 uint32_t Connection::ComputeNetworkCost() const {
   // TODO(honghaiz): Will add rtt as part of the network cost.
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in ComputeNetworkCost()";
   return port()->network_cost() + remote_candidate_.network_cost();
 }
 
 std::string Connection::ToString() const {
   RTC_DCHECK_RUN_ON(network_thread_);
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in ToString()";
   constexpr absl::string_view CONNECT_STATE_ABBREV[2] = {
       "-",  // not connected (false)
       "C",  // connected (true)
@@ -1457,6 +1469,8 @@ void Connection::OnConnectionRequestResponse(StunRequest* request,
 
 void Connection::OnConnectionRequestErrorResponse(ConnectionRequest* request,
                                                   StunMessage* response) {
+  RTC_DCHECK(port_) << ToDebugId()
+                    << ": port_ null in OnConnectionRequestErrorResponse";
   if (!port_)
     return;
 
@@ -1610,6 +1624,8 @@ ConnectionInfo Connection::stats() {
 
 void Connection::MaybeUpdateLocalCandidate(StunRequest* request,
                                            StunMessage* response) {
+  RTC_DCHECK(port_) << ToDebugId()
+                    << ": port_ null in MaybeUpdateLocalCandidate";
   if (!port_)
     return;
 
@@ -1754,6 +1770,7 @@ ProxyConnection::ProxyConnection(rtc::WeakPtr<Port> port,
 int ProxyConnection::Send(const void* data,
                           size_t size,
                           const rtc::PacketOptions& options) {
+  RTC_DCHECK(port_) << ToDebugId() << ": port_ null in Send()";
   if (!port_)
     return SOCKET_ERROR;
 

p2p/base/connection.h

@@ -111,6 +111,7 @@ class RTC_EXPORT Connection : public CandidatePairInterface {
   bool connected() const;
   bool weak() const;
   bool active() const;
+  bool pending_delete() const { return !port_; }
 
   // A connection is dead if it can be safely deleted.
   bool dead(int64_t now) const;

p2p/base/port.cc

@@ -188,8 +188,8 @@ void Port::Construct() {
 
 Port::~Port() {
   RTC_DCHECK_RUN_ON(thread_);
-  CancelPendingTasks();
   DestroyAllConnections();
+  CancelPendingTasks();
 }
 
 const absl::string_view Port::Type() const {

p2p/base/stun_request.cc

@@ -133,31 +133,39 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) {
     }
   }
 
-  bool success = true;
-
   if (!msg->GetNonComprehendedAttributes().empty()) {
     // If a response contains unknown comprehension-required attributes, it's
     // simply discarded and the transaction is considered failed. See RFC5389
     // sections 7.3.3 and 7.3.4.
     RTC_LOG(LS_ERROR) << ": Discarding response due to unknown "
                          "comprehension-required attribute.";
-    success = false;
+    requests_.erase(iter);
+    return false;
   } else if (msg->type() == GetStunSuccessResponseType(request->type())) {
     if (!msg->IntegrityOk() && !skip_integrity_checking) {
       return false;
     }
-    request->OnResponse(msg);
+    // Erase element from hash before calling callback. This ensures
+    // that the callback can modify the StunRequestManager any way it
+    // sees fit.
+    std::unique_ptr<StunRequest> owned_request = std::move(iter->second);
+    requests_.erase(iter);
+    owned_request->OnResponse(msg);
+    return true;
   } else if (msg->type() == GetStunErrorResponseType(request->type())) {
-    request->OnErrorResponse(msg);
+    // Erase element from hash before calling callback. This ensures
+    // that the callback can modify the StunRequestManager any way it
+    // sees fit.
+    std::unique_ptr<StunRequest> owned_request = std::move(iter->second);
+    requests_.erase(iter);
+    owned_request->OnErrorResponse(msg);
+    return true;
   } else {
     RTC_LOG(LS_ERROR) << "Received response with wrong type: " << msg->type()
                       << " (expecting "
                       << GetStunSuccessResponseType(request->type()) << ")";
     return false;
   }
-
-  requests_.erase(iter);
-  return success;
 }
 
 bool StunRequestManager::empty() const {

p2p/base/stun_request_unittest.cc

@@ -54,15 +54,15 @@ class StunRequestTest : public ::testing::Test {
     request_count_++;
   }
 
-  void OnResponse(StunMessage* res) {
+  virtual void OnResponse(StunMessage* res) {
     response_ = res;
     success_ = true;
   }
-  void OnErrorResponse(StunMessage* res) {
+  virtual void OnErrorResponse(StunMessage* res) {
     response_ = res;
     failure_ = true;
   }
-  void OnTimeout() { timeout_ = true; }
+  virtual void OnTimeout() { timeout_ = true; }
 
  protected:
   rtc::AutoThread main_thread_;
@@ -216,4 +216,42 @@ TEST_F(StunRequestTest, TestUnrecognizedComprehensionRequiredAttribute) {
   EXPECT_FALSE(timeout_);
 }
 
+class StunRequestReentranceTest : public StunRequestTest {
+ public:
+  void OnResponse(StunMessage* res) override {
+    manager_.Clear();
+    StunRequestTest::OnResponse(res);
+  }
+  void OnErrorResponse(StunMessage* res) override {
+    manager_.Clear();
+    StunRequestTest::OnErrorResponse(res);
+  }
+};
+
+TEST_F(StunRequestReentranceTest, TestSuccess) {
+  auto* request = new StunRequestThunker(manager_, this);
+  std::unique_ptr<StunMessage> res =
+      request->CreateResponseMessage(STUN_BINDING_RESPONSE);
+  manager_.Send(request);
+  EXPECT_TRUE(manager_.CheckResponse(res.get()));
+
+  EXPECT_TRUE(response_ == res.get());
+  EXPECT_TRUE(success_);
+  EXPECT_FALSE(failure_);
+  EXPECT_FALSE(timeout_);
+}
+
+TEST_F(StunRequestReentranceTest, TestError) {
+  auto* request = new StunRequestThunker(manager_, this);
+  std::unique_ptr<StunMessage> res =
+      request->CreateResponseMessage(STUN_BINDING_ERROR_RESPONSE);
+  manager_.Send(request);
+  EXPECT_TRUE(manager_.CheckResponse(res.get()));
+
+  EXPECT_TRUE(response_ == res.get());
+  EXPECT_FALSE(success_);
+  EXPECT_TRUE(failure_);
+  EXPECT_FALSE(timeout_);
+}
+
 }  // namespace cricket