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}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 1ef42cc..94e5911 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -262,14 +262,17 @@
}
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::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 @@
// information required for logging needs access to `port_`.
port_.reset();
+ // Clear any pending requests (or responses).
+ requests_.Clear();
+
return true;
}
@@ -846,6 +853,7 @@
// 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::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 @@
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 @@
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::OnConnectionRequestErrorResponse(ConnectionRequest* request,
StunMessage* response) {
+ RTC_DCHECK(port_) << ToDebugId()
+ << ": port_ null in OnConnectionRequestErrorResponse";
if (!port_)
return;
@@ -1610,6 +1624,8 @@
void Connection::MaybeUpdateLocalCandidate(StunRequest* request,
StunMessage* response) {
+ RTC_DCHECK(port_) << ToDebugId()
+ << ": port_ null in MaybeUpdateLocalCandidate";
if (!port_)
return;
@@ -1754,6 +1770,7 @@
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;
diff --git a/p2p/base/connection.h b/p2p/base/connection.h
index 8e439855..1ea9180 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -111,6 +111,7 @@
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;
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index a90ab63..afd998c 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -188,8 +188,8 @@
Port::~Port() {
RTC_DCHECK_RUN_ON(thread_);
- CancelPendingTasks();
DestroyAllConnections();
+ CancelPendingTasks();
}
const absl::string_view Port::Type() const {
diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc
index 25d387c..15c1e6a 100644
--- a/p2p/base/stun_request.cc
+++ b/p2p/base/stun_request.cc
@@ -133,31 +133,39 @@
}
}
- 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 {
diff --git a/p2p/base/stun_request_unittest.cc b/p2p/base/stun_request_unittest.cc
index 6831d9f..2f88ab1 100644
--- a/p2p/base/stun_request_unittest.cc
+++ b/p2p/base/stun_request_unittest.cc
@@ -54,15 +54,15 @@
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 @@
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
