[StunRequestManager] Use unique_ptr in RequestMap

This removes the circular delete/remove pattern between the StunRequest
and StunRequestManager whereby deleting a request would call the
manager from the dtor to do a lookup+erase from the map. Instead
the operation of removing from the map, equals delete and is controlled
by the manager (owner) class.

Bug: webrtc:13892
Change-Id: I1920ac4b98636d38b851f4c2057026bfbd392584
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/258660
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36532}

p2p/base/stun_request.cc

@@ -15,6 +15,7 @@
 #include <utility>
 #include <vector>
 
+#include "absl/memory/memory.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/helpers.h"
 #include "rtc_base/logging.h"
@@ -44,13 +45,7 @@ const int STUN_MAX_RTO = 8000;  // milliseconds, or 5 doublings
 
 StunRequestManager::StunRequestManager(rtc::Thread* thread) : thread_(thread) {}
 
-StunRequestManager::~StunRequestManager() {
-  while (requests_.begin() != requests_.end()) {
-    StunRequest* request = requests_.begin()->second;
-    requests_.erase(requests_.begin());
-    delete request;
-  }
-}
+StunRequestManager::~StunRequestManager() = default;
 
 void StunRequestManager::Send(StunRequest* request) {
   SendDelayed(request, 0);
@@ -59,31 +54,31 @@ void StunRequestManager::Send(StunRequest* request) {
 void StunRequestManager::SendDelayed(StunRequest* request, int delay) {
   RTC_DCHECK_RUN_ON(thread_);
   RTC_DCHECK_EQ(this, request->manager());
-  RTC_DCHECK(requests_.find(request->id()) == requests_.end());
   request->Construct();
-  requests_[request->id()] = request;
+  auto [iter, was_inserted] =
+      requests_.emplace(request->id(), absl::WrapUnique(request));
+  RTC_DCHECK(was_inserted);
   if (delay > 0) {
-    thread_->PostDelayed(RTC_FROM_HERE, delay, request, MSG_STUN_SEND, NULL);
+    thread_->PostDelayed(RTC_FROM_HERE, delay, iter->second.get(),
+                         MSG_STUN_SEND, NULL);
   } else {
-    thread_->Send(RTC_FROM_HERE, request, MSG_STUN_SEND, NULL);
+    thread_->Send(RTC_FROM_HERE, iter->second.get(), MSG_STUN_SEND, NULL);
   }
 }
 
 void StunRequestManager::FlushForTest(int msg_type) {
   RTC_DCHECK_RUN_ON(thread_);
-  for (const auto& kv : requests_) {
-    StunRequest* request = kv.second;
+  for (const auto& [unused, request] : requests_) {
     if (msg_type == kAllRequests || msg_type == request->type()) {
-      thread_->Clear(request, MSG_STUN_SEND);
-      thread_->Send(RTC_FROM_HERE, request, MSG_STUN_SEND, NULL);
+      thread_->Clear(request.get(), MSG_STUN_SEND);
+      thread_->Send(RTC_FROM_HERE, request.get(), MSG_STUN_SEND, NULL);
     }
   }
 }
 
 bool StunRequestManager::HasRequestForTest(int msg_type) {
   RTC_DCHECK_RUN_ON(thread_);
-  for (const auto& kv : requests_) {
-    StunRequest* request = kv.second;
+  for (const auto& [unused, request] : requests_) {
     if (msg_type == kAllRequests || msg_type == request->type()) {
       return true;
     }
@@ -91,28 +86,9 @@ bool StunRequestManager::HasRequestForTest(int msg_type) {
   return false;
 }
 
-void StunRequestManager::Remove(StunRequest* request) {
-  RTC_DCHECK_RUN_ON(thread_);
-  RTC_DCHECK(request->manager() == this);
-  RequestMap::iterator iter = requests_.find(request->id());
-  if (iter != requests_.end()) {
-    RTC_DCHECK(iter->second == request);
-    requests_.erase(iter);
-    thread_->Clear(request);
-  }
-}
-
 void StunRequestManager::Clear() {
   RTC_DCHECK_RUN_ON(thread_);
-  std::vector<StunRequest*> requests;
-  for (RequestMap::iterator i = requests_.begin(); i != requests_.end(); ++i)
-    requests.push_back(i->second);
-
-  for (uint32_t i = 0; i < requests.size(); ++i) {
-    // StunRequest destructor calls Remove() which deletes requests
-    // from `requests_`.
-    delete requests[i];
-  }
+  requests_.clear();
 }
 
 bool StunRequestManager::CheckResponse(StunMessage* msg) {
@@ -124,7 +100,7 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) {
     return false;
   }
 
-  StunRequest* request = iter->second;
+  StunRequest* request = iter->second.get();
 
   // Now that we know the request, we can see if the response is
   // integrity-protected or not.
@@ -137,14 +113,15 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) {
     msg->ValidateMessageIntegrity(request->msg()->password());
   }
 
+  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.";
-    delete request;
-    return false;
+    success = false;
   } else if (msg->type() == GetStunSuccessResponseType(request->type())) {
     if (!msg->IntegrityOk() && !skip_integrity_checking) {
       return false;
@@ -159,8 +136,8 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) {
     return false;
   }
 
-  delete request;
-  return true;
+  requests_.erase(iter);
+  return success;
 }
 
 bool StunRequestManager::empty() const {
@@ -199,6 +176,11 @@ bool StunRequestManager::CheckResponse(const char* data, size_t size) {
   return CheckResponse(response.get());
 }
 
+void StunRequestManager::OnRequestTimedOut(StunRequest* request) {
+  RTC_DCHECK_RUN_ON(thread_);
+  requests_.erase(request->id());
+}
+
 StunRequest::StunRequest(StunRequestManager& manager)
     : manager_(manager),
       msg_(new StunMessage()),
@@ -219,7 +201,6 @@ StunRequest::StunRequest(StunRequestManager& manager,
 }
 
 StunRequest::~StunRequest() {
-  manager_.Remove(this);
   manager_.network_thread()->Clear(this);
 }
 
@@ -250,7 +231,7 @@ void StunRequest::OnMessage(rtc::Message* pmsg) {
 
   if (timeout_) {
     OnTimeout();
-    delete this;
+    manager_.OnRequestTimedOut(this);
     return;
   }