Reland "sigslot: make PortInterface::SignalRoleConflict a CallbackList"
This is a reland of commit 1c0ba91c855f26868a6864ff048ca7ebf03ce724
PS1: original reland
PS2+: changes
Original change's description:
> sigslot: make PortInterface::SignalRoleConflict a CallbackLst
>
> Bug: webrtc:42222066
> Change-Id: I15dab9209a58fec8c42b4096dc5682e46a844e19
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/403220
> Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@meta.com>
> Cr-Commit-Position: refs/heads/main@{#45406}
Bug: webrtc:42222066
Change-Id: Ia45c36f8346d8d16da72c05a3409a78942ba8fbd
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/406041
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@meta.com>
Cr-Commit-Position: refs/heads/main@{#45476}
diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn
index af0a607..67927db 100644
--- a/p2p/BUILD.gn
+++ b/p2p/BUILD.gn
@@ -650,6 +650,7 @@
"../rtc_base:socket_address",
"../rtc_base/network:sent_packet",
"../rtc_base/third_party/sigslot",
+ "//third_party/abseil-cpp/absl/functional:any_invocable",
"//third_party/abseil-cpp/absl/strings:string_view",
]
}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 662c990..ce58ee8 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -1603,7 +1603,7 @@
error_code == STUN_ERROR_UNAUTHORIZED) {
// Recoverable error, retry
} else if (error_code == STUN_ERROR_ROLE_CONFLICT) {
- port_->SignalRoleConflict(port_.get());
+ port_->NotifyRoleConflict();
} else if (request->msg()->type() == GOOG_PING_REQUEST) {
// Race, retry.
} else {
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 6662732..9d6643b 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -911,15 +911,14 @@
ports_.push_back(port);
port->SignalUnknownAddress.connect(this,
&P2PTransportChannel::OnUnknownAddress);
+ port->SignalSentPacket.connect(this, &P2PTransportChannel::OnSentPacket);
+
port->SubscribePortDestroyed(
[this](PortInterface* port) { OnPortDestroyed(port); });
-
- port->SignalRoleConflict.connect(this, &P2PTransportChannel::OnRoleConflict);
- port->SignalSentPacket.connect(this, &P2PTransportChannel::OnSentPacket);
+ port->SubscribeRoleConflict([this]() { NotifyRoleConflict(); });
// Attempt to create a connection from this new port to all of the remote
// candidates that we were given so far.
-
std::vector<RemoteCandidate>::iterator iter;
for (iter = remote_candidates_.begin(); iter != remote_candidates_.end();
++iter) {
@@ -1123,7 +1122,7 @@
}
}
-void P2PTransportChannel::OnRoleConflict(PortInterface* /* port */) {
+void P2PTransportChannel::NotifyRoleConflict() {
SignalRoleConflict(this); // STUN ping will be sent when SetRole is called
// from Transport.
}
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index 6ca91e0..dd4cba3 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -362,7 +362,7 @@
// When pruning a port, move it from `ports_` to `pruned_ports_`.
// Returns true if the port is found and removed from `ports_`.
bool PrunePort(PortInterface* port);
- void OnRoleConflict(PortInterface* port);
+ void NotifyRoleConflict();
void OnConnectionStateChange(Connection* connection);
void OnReadPacket(Connection* connection, const ReceivedIpPacket& packet);
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index 30756bc..05d3b2b 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -131,6 +131,7 @@
tiebreaker_(0),
shared_socket_(shared_socket),
network_cost_(args.network->GetCost(env_.field_trials())),
+ role_conflict_callback_(nullptr),
weak_factory_(this) {
RTC_DCHECK_RUN_ON(thread_);
RTC_DCHECK(factory_ != nullptr);
@@ -701,7 +702,7 @@
case ICEROLE_CONTROLLING:
if (ICEROLE_CONTROLLING == remote_ice_role) {
if (remote_tiebreaker >= tiebreaker_) {
- SignalRoleConflict(this);
+ NotifyRoleConflict();
} else {
// Send Role Conflict (487) error response.
SendBindingErrorResponse(stun_msg, addr, STUN_ERROR_ROLE_CONFLICT,
@@ -713,7 +714,7 @@
case ICEROLE_CONTROLLED:
if (ICEROLE_CONTROLLED == remote_ice_role) {
if (remote_tiebreaker < tiebreaker_) {
- SignalRoleConflict(this);
+ NotifyRoleConflict();
} else {
// Send Role Conflict (487) error response.
SendBindingErrorResponse(stun_msg, addr, STUN_ERROR_ROLE_CONFLICT,
@@ -1040,4 +1041,22 @@
std::move(callback)(status);
}
+void Port::SubscribeRoleConflict(absl::AnyInvocable<void()> callback) {
+ RTC_DCHECK_RUN_ON(thread_);
+ RTC_DCHECK(callback);
+ RTC_DCHECK(!role_conflict_callback_);
+ RTC_DCHECK(SignalRoleConflict.is_empty());
+ role_conflict_callback_ = std::move(callback);
+}
+
+void Port::NotifyRoleConflict() {
+ RTC_DCHECK_RUN_ON(thread_);
+ if (role_conflict_callback_) {
+ RTC_DCHECK(SignalRoleConflict.is_empty());
+ role_conflict_callback_();
+ } else {
+ SignalRoleConflict(this);
+ }
+}
+
} // namespace webrtc
diff --git a/p2p/base/port.h b/p2p/base/port.h
index 015a2d3..833f118 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -392,6 +392,10 @@
void GetStunStats(std::optional<StunStats>* /* stats */) override {}
+ // Signals for ICE role conflicts.
+ void SubscribeRoleConflict(absl::AnyInvocable<void()> callback) override;
+ void NotifyRoleConflict() override;
+
protected:
void UpdateNetworkCost() override;
@@ -563,6 +567,8 @@
CallbackList<Port*, const Candidate&> candidate_ready_callback_list_
RTC_GUARDED_BY(thread_);
+ absl::AnyInvocable<void()> role_conflict_callback_ RTC_GUARDED_BY(thread_);
+
// Keep as the last member variable.
WeakPtrFactory<Port> weak_factory_ RTC_GUARDED_BY(thread_);
};
diff --git a/p2p/base/port_interface.h b/p2p/base/port_interface.h
index 1484a14..23f808c 100644
--- a/p2p/base/port_interface.h
+++ b/p2p/base/port_interface.h
@@ -19,6 +19,7 @@
#include <string>
#include <vector>
+#include "absl/functional/any_invocable.h"
#include "absl/strings/string_view.h"
#include "api/candidate.h"
#include "api/packet_socket_factory.h"
@@ -124,7 +125,10 @@
std::function<void(PortInterface*)> callback) = 0;
// Signaled when Port discovers ice role conflict with the peer.
+ // TODO: bugs.webrtc.org/42222066 - remove slot.
sigslot::signal1<PortInterface*> SignalRoleConflict;
+ virtual void SubscribeRoleConflict(absl::AnyInvocable<void()> callback) = 0;
+ virtual void NotifyRoleConflict() = 0;
// Normally, packets arrive through a connection (or they result signaling of
// unknown address). Calling this method turns off delivery of packets
diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc
index 60aa47d..b4e0cec 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -879,7 +879,7 @@
.ice_username_fragment = username,
.ice_password = password};
auto port = std::make_unique<TestPort>(args, 0, 0);
- port->SignalRoleConflict.connect(this, &PortTest::OnRoleConflict);
+ port->SubscribeRoleConflict([this]() { OnRoleConflict(); });
return port;
}
std::unique_ptr<TestPort> CreateTestPort(const SocketAddress& addr,
@@ -903,11 +903,21 @@
.ice_username_fragment = username,
.ice_password = password};
auto port = std::make_unique<TestPort>(args, 0, 0);
- port->SignalRoleConflict.connect(this, &PortTest::OnRoleConflict);
+ port->SubscribeRoleConflict([this]() { OnRoleConflict(); });
return port;
}
- void OnRoleConflict(PortInterface* port) { role_conflict_ = true; }
+ std::unique_ptr<TestPort> CreateRawTestPort() {
+ Port::PortParametersRef args = {
+ .env = env_,
+ .network_thread = &main_,
+ .socket_factory = &socket_factory_,
+ .network = MakeNetwork(kLocalAddr1),
+ };
+ return std::make_unique<TestPort>(args, 0, 0);
+ }
+
+ void OnRoleConflict() { role_conflict_ = true; }
bool role_conflict() const { return role_conflict_; }
void ConnectToSignalDestroyed(PortInterface* port) {
@@ -4026,6 +4036,32 @@
EXPECT_TRUE(port->GetConnection(address) != nullptr);
}
+#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+class DeathChannel : public sigslot::has_slots<> {
+ public:
+ explicit DeathChannel(std::unique_ptr<Port> port) : port_(std::move(port)) {}
+ void IgnoredSlot(PortInterface* /* port */) {}
+ void AddSignal() {
+ port_->SignalRoleConflict.connect(this, &DeathChannel::IgnoredSlot);
+ }
+ void AddCallback() {
+ port_->SubscribeRoleConflict([]() {});
+ }
+
+ private:
+ std::unique_ptr<Port> port_;
+};
+
+class PortDeathTest : public PortTest {};
+
+TEST_F(PortDeathTest, AddSignalThenCallback) {
+ DeathChannel dc(CreateRawTestPort());
+ dc.AddSignal();
+ EXPECT_DEATH(dc.AddCallback(), "");
+}
+
+#endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+
// TODO(webrtc:11463) : Move Connection tests into separate unit test
// splitting out shared test code as needed.
