Propagate content name to ports at construction time, add sanity checks
This sets the Port::content_name (i.e. mid) at construction time and
removes one call to `set_content_name()`. One call to
`set_content_name()` still remains, which is when sessions are pooled
and reused. Because of that, Port::content_name_ can still not be const.
However, this CL improves constness in P2PTransportChannel and adds
several RTC_DCHECKs to ensure the correctness of the content_name field,
including when it belongs to a pooled and not-pooled sessions.
Bug: none
Change-Id: I9730c2ff13efb33d6ad43f9ca152d8fe93929550
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/407281
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#45513}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 1afd41b..577e4b0 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -287,16 +287,19 @@
}
const Network* Connection::network() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(port_) << ToDebugId() << ": port_ null in network()";
return port()->Network();
}
int Connection::generation() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(port_) << ToDebugId() << ": port_ null in generation()";
return port()->generation();
}
uint64_t Connection::priority() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(port_) << ToDebugId() << ": port_ null in priority()";
if (!port_)
return 0;
@@ -1391,6 +1394,7 @@
}
uint32_t Connection::ComputeNetworkCost() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
// 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();
diff --git a/p2p/base/connection.h b/p2p/base/connection.h
index 55de3f9..1cb0c20 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -117,7 +117,10 @@
bool connected() const;
bool weak() const;
bool active() const;
- bool pending_delete() const { return !port_; }
+ bool pending_delete() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ return !port_;
+ }
// A connection is dead if it can be safely deleted.
bool dead(Timestamp now) const;
@@ -391,8 +394,14 @@
void SendResponseMessage(const StunMessage& response);
// An accessor for unit tests.
- PortInterface* PortForTest() { return port_.get(); }
- const PortInterface* PortForTest() const { return port_.get(); }
+ PortInterface* PortForTest() {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ return port_.get();
+ }
+ const PortInterface* PortForTest() const {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ return port_.get();
+ }
std::unique_ptr<IceMessage> BuildPingRequestForTest() {
RTC_DCHECK_RUN_ON(network_thread_);
@@ -478,7 +487,10 @@
void set_connected(bool value);
// The local port where this connection sends and receives packets.
- PortInterface* port() { return port_.get(); }
+ PortInterface* port() {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ return port_.get();
+ }
const Environment& env() { return env_; }
ConnectionInfo& mutable_stats() { return stats_; }
@@ -511,7 +523,7 @@
// TODO(tommi): This ^^^ should be fixed.
TaskQueueBase* const network_thread_;
const uint32_t id_;
- WeakPtr<PortInterface> port_;
+ WeakPtr<PortInterface> port_ RTC_GUARDED_BY(network_thread_);
Candidate local_candidate_ RTC_GUARDED_BY(network_thread_);
Candidate remote_candidate_;
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index 82d854f..db6c987 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -866,17 +866,16 @@
ice_parameters_.ufrag,
ice_parameters_.pwd);
if (pooled_session) {
+ PortAllocatorSession* raw_session = pooled_session.get();
AddAllocatorSession(std::move(pooled_session));
- PortAllocatorSession* raw_pooled_session =
- allocator_sessions_.back().get();
+ RTC_DCHECK_EQ(raw_session, allocator_sessions_.back().get());
// Process the pooled session's existing candidates/ports, if they exist.
- OnCandidatesReady(raw_pooled_session,
- raw_pooled_session->ReadyCandidates());
- for (PortInterface* port : allocator_sessions_.back()->ReadyPorts()) {
- OnPortReady(raw_pooled_session, port);
+ OnCandidatesReady(raw_session, raw_session->ReadyCandidates());
+ for (PortInterface* port : raw_session->ReadyPorts()) {
+ OnPortReady(raw_session, port);
}
- if (allocator_sessions_.back()->CandidatesAllocationDone()) {
- OnCandidatesAllocationDone(raw_pooled_session);
+ if (raw_session->CandidatesAllocationDone()) {
+ OnCandidatesAllocationDone(raw_session);
}
} else {
AddAllocatorSession(allocator_->CreateSession(
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index 7f087ba..8ce03f6 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -443,9 +443,9 @@
void GoogDeltaAckReceived(RTCErrorOr<const StunUInt64Attribute*>);
const Environment env_;
- std::string transport_name_ RTC_GUARDED_BY(network_thread_);
+ const std::string transport_name_ RTC_GUARDED_BY(network_thread_);
int component_ RTC_GUARDED_BY(network_thread_);
- PortAllocator* allocator_ RTC_GUARDED_BY(network_thread_);
+ PortAllocator* const allocator_ RTC_GUARDED_BY(network_thread_);
AsyncDnsResolverFactoryInterface* const async_dns_resolver_factory_
RTC_GUARDED_BY(network_thread_);
const std::unique_ptr<AsyncDnsResolverFactoryInterface>
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index c7c1de2..ea6b045 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -121,6 +121,7 @@
network_(args.network),
min_port_(min_port),
max_port_(max_port),
+ content_name_(args.content_name),
component_(ICE_CANDIDATE_COMPONENT_DEFAULT),
generation_(0),
ice_username_fragment_(args.ice_username_fragment),
diff --git a/p2p/base/port.h b/p2p/base/port.h
index 1a237be..134d9eb 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -169,6 +169,7 @@
const ::webrtc::Network* network;
absl::string_view ice_username_fragment;
absl::string_view ice_password;
+ absl::string_view content_name;
LocalNetworkAccessPermissionFactoryInterface* lna_permission_factory =
nullptr;
};
diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc
index ba95687..cd7bd07 100644
--- a/p2p/base/port_allocator.cc
+++ b/p2p/base/port_allocator.cc
@@ -241,6 +241,7 @@
auto it =
pooled_sessions_.begin() + std::distance(pooled_sessions_.cbegin(), cit);
std::unique_ptr<PortAllocatorSession> ret = std::move(*it);
+ RTC_DCHECK(ret->pooled());
ret->SetIceParameters(content_name, component, ice_ufrag, ice_pwd);
ret->set_pooled(false);
// According to JSEP, a pooled session should filter candidates only
@@ -256,9 +257,8 @@
auto it = FindPooledSession(ice_credentials);
if (it == pooled_sessions_.end()) {
return nullptr;
- } else {
- return it->get();
}
+ return it->get();
}
std::vector<std::unique_ptr<PortAllocatorSession>>::const_iterator
diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h
index 87490f2..643fb7b 100644
--- a/p2p/base/port_allocator.h
+++ b/p2p/base/port_allocator.h
@@ -303,6 +303,9 @@
int component,
absl::string_view ice_ufrag,
absl::string_view ice_pwd) {
+ RTC_DCHECK(pooled_);
+ RTC_DCHECK(!content_name.empty());
+ RTC_DCHECK(content_name_.empty());
content_name_ = std::string(content_name);
component_ = component;
ice_ufrag_ = std::string(ice_ufrag);
@@ -310,7 +313,12 @@
UpdateIceParametersInternal();
}
- void set_pooled(bool value) { pooled_ = value; }
+ void set_pooled(bool value) {
+ pooled_ = value;
+ if (pooled_) {
+ content_name_.clear();
+ }
+ }
uint32_t flags_;
uint32_t generation_;
diff --git a/p2p/base/port_allocator_unittest.cc b/p2p/base/port_allocator_unittest.cc
index 8bc61da..d7272ba 100644
--- a/p2p/base/port_allocator_unittest.cc
+++ b/p2p/base/port_allocator_unittest.cc
@@ -239,6 +239,7 @@
static_cast<webrtc::FakePortAllocatorSession*>(
allocator_->TakePooledSession(kContentName, 1, kIceUfrag, kIcePwd)
.release()));
+ EXPECT_FALSE(session->pooled());
EXPECT_EQ(1, session->transport_info_update_count());
EXPECT_EQ(kContentName, session->content_name());
EXPECT_EQ(1, session->component());
diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h
index c74feae..0674a5a 100644
--- a/p2p/base/turn_port.h
+++ b/p2p/base/turn_port.h
@@ -93,6 +93,7 @@
.network = args.network,
.ice_username_fragment = args.username,
.ice_password = args.password,
+ .content_name = args.content_name,
.lna_permission_factory = args.lna_permission_factory},
socket, *args.server_address, args.config->credentials,
args.relative_priority, args.config->tls_alpn_protocols,
@@ -116,6 +117,7 @@
.network = args.network,
.ice_username_fragment = args.username,
.ice_password = args.password,
+ .content_name = args.content_name,
.lna_permission_factory = args.lna_permission_factory},
min_port, max_port, *args.server_address, args.config->credentials,
args.relative_priority, args.config->tls_alpn_protocols,
diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc
index ee47628..e288c71 100644
--- a/p2p/client/basic_port_allocator.cc
+++ b/p2p/client/basic_port_allocator.cc
@@ -567,6 +567,7 @@
void BasicPortAllocatorSession::UpdateIceParametersInternal() {
RTC_DCHECK_RUN_ON(network_thread_);
+ RTC_DCHECK(pooled());
for (PortData& port : ports_) {
port.port()->set_content_name(content_name());
port.port()->SetIceParameters(component(), ice_ufrag(), ice_pwd());
@@ -907,18 +908,15 @@
void BasicPortAllocatorSession::AddAllocatedPort(Port* port,
AllocationSequence* seq) {
RTC_DCHECK_RUN_ON(network_thread_);
- if (!port)
- return;
+ RTC_DCHECK_EQ(port->content_name(), content_name());
RTC_LOG(LS_INFO) << "Adding allocated port for " << content_name();
- port->set_content_name(content_name());
port->set_component(component());
port->set_generation(generation());
port->set_send_retransmit_count_attribute(
(flags() & PORTALLOCATOR_ENABLE_STUN_RETRANSMIT_ATTRIBUTE) != 0);
- PortData data(port, seq);
- ports_.push_back(data);
+ ports_.emplace_back(port, seq);
port->SubscribeCandidateReadyCallback(
[this](Port* port, const Candidate& c) { OnCandidateReady(port, c); });
@@ -1453,6 +1451,7 @@
.network = network_,
.ice_username_fragment = session_->username(),
.ice_password = session_->password(),
+ .content_name = session_->content_name(),
.lna_permission_factory =
session_->allocator()->lna_permission_factory()},
udp_socket_.get(), emit_local_candidate_for_anyaddress,
@@ -1465,6 +1464,7 @@
.network = network_,
.ice_username_fragment = session_->username(),
.ice_password = session_->password(),
+ .content_name = session_->content_name(),
.lna_permission_factory =
session_->allocator()->lna_permission_factory()},
session_->allocator()->min_port(), session_->allocator()->max_port(),
@@ -1478,9 +1478,6 @@
// UDPPort.
if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) {
udp_port_ = port.get();
- port->SubscribePortDestroyed(
- [this](PortInterface* port) { OnPortDestroyed(port); });
-
// If STUN is not disabled, setting stun server address to port.
if (!IsFlagSet(PORTALLOCATOR_DISABLE_STUN)) {
if (config_ && !config_->StunServers().empty()) {
@@ -1503,12 +1500,15 @@
}
std::unique_ptr<Port> port = TCPPort::Create(
- {.env = session_->allocator()->env(),
- .network_thread = session_->network_thread(),
- .socket_factory = session_->socket_factory(),
- .network = network_,
- .ice_username_fragment = session_->username(),
- .ice_password = session_->password()},
+ {
+ .env = session_->allocator()->env(),
+ .network_thread = session_->network_thread(),
+ .socket_factory = session_->socket_factory(),
+ .network = network_,
+ .ice_username_fragment = session_->username(),
+ .ice_password = session_->password(),
+ .content_name = session_->content_name(),
+ },
session_->allocator()->min_port(), session_->allocator()->max_port(),
session_->allocator()->allow_tcp_listen());
if (port) {
@@ -1542,6 +1542,7 @@
.network = network_,
.ice_username_fragment = session_->username(),
.ice_password = session_->password(),
+ .content_name = session_->content_name(),
.lna_permission_factory =
session_->allocator()->lna_permission_factory()},
session_->allocator()->min_port(), session_->allocator()->max_port(),
@@ -1611,6 +1612,7 @@
args.network = network_;
args.username = session_->username();
args.password = session_->password();
+ args.content_name = session_->content_name();
args.server_address = &(*relay_port);
args.config = &config;
args.turn_customizer = session_->allocator()->turn_customizer();
@@ -1635,11 +1637,6 @@
}
relay_ports_.push_back(port.get());
- // Listen to the port destroyed signal, to allow AllocationSequence to
- // remove the entry from it's map.
- port->SubscribePortDestroyed(
- [this](PortInterface* port) { OnPortDestroyed(port); });
-
} else {
port = session_->allocator()->relay_port_factory()->Create(
args, session_->allocator()->min_port(),
diff --git a/p2p/client/basic_port_allocator.h b/p2p/client/basic_port_allocator.h
index ced785f..f2242a7 100644
--- a/p2p/client/basic_port_allocator.h
+++ b/p2p/client/basic_port_allocator.h
@@ -201,10 +201,13 @@
// interface. Only TURN ports may be pruned.
};
- PortData() {}
+ PortData() = delete;
+ PortData(PortData&&) = default;
PortData(Port* port, AllocationSequence* seq)
: port_(port), sequence_(seq) {}
+ PortData& operator=(PortData&&) = default;
+
Port* port() const { return port_; }
AllocationSequence* sequence() const { return sequence_; }
bool has_pairable_candidate() const { return has_pairable_candidate_; }
diff --git a/p2p/client/relay_port_factory_interface.h b/p2p/client/relay_port_factory_interface.h
index 63b7c8b..2bcb2e0 100644
--- a/p2p/client/relay_port_factory_interface.h
+++ b/p2p/client/relay_port_factory_interface.h
@@ -40,6 +40,7 @@
const RelayServerConfig* config;
std::string username;
std::string password;
+ std::string content_name;
TurnCustomizer* turn_customizer = nullptr;
// Relative priority of candidates from this TURN server in relation
// to the candidates from other servers. Required because ICE priorities
