admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "p2p: reduce visibility of ICE tiebreaker further"

Browse Source 
This reverts commit b5df2ba10db3cd04febcde8727e782457708f2fa.

Reason for revert: Breaks downstream

Original change's description:
> p2p: reduce visibility of ICE tiebreaker further
>
> since the tie breaker is owned by the allocator now.
>
> BUG=webrtc:42224914
>
> Change-Id: I76bd5ae714fb2a6df38e014991242f390ae87e6a
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/351180
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
> Commit-Queue: Philipp Hancke <phancke@meta.com>
> Cr-Commit-Position: refs/heads/main@{#42371}

Bug: webrtc:42224914
Change-Id: Ic9d5ee229738575910bd33dee278f6049be81205
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/351680
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Auto-Submit: Björn Terelius <terelius@webrtc.org>
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42374}
This commit is contained in:
Björn Terelius 2024-05-24 10:17:38 +00:00 committed by WebRTC LUCI CQ
parent 4e86f14209
commit dbbb6cabc3
9 changed files with 78 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
p2p/base/dtls_transport_unittest.cc
View File

@ -92,6 +92,8 @@ class DtlsTestClient : public sigslot::has_slots<> {
fake_ice_transport_->SetAsync(true);
fake_ice_transport_->SetAsyncDelay(async_delay_ms);
fake_ice_transport_->SetIceRole(role);
fake_ice_transport_->SetIceTiebreaker((role == ICEROLE_CONTROLLING) ? 1
: 2);
// Hook the raw packets so that we can verify they are encrypted.
fake_ice_transport_->RegisterReceivedPacketCallback(
this, [&](rtc::PacketTransportInternal* transport,

9
p2p/base/fake_ice_transport.h
View File

@ -147,6 +147,10 @@ class FakeIceTransport : public IceTransportInternal {
// Fake IceTransportInternal implementation.
const std::string& transport_name() const override { return name_; }
int component() const override { return component_; }
uint64_t IceTiebreaker() const {
RTC_DCHECK_RUN_ON(network_thread_);
return tiebreaker_;
}
IceMode remote_ice_mode() const {
RTC_DCHECK_RUN_ON(network_thread_);
return remote_ice_mode_;
@ -208,6 +212,10 @@ class FakeIceTransport : public IceTransportInternal {
RTC_DCHECK_RUN_ON(network_thread_);
return role_;
}
void SetIceTiebreaker(uint64_t tiebreaker) override {
RTC_DCHECK_RUN_ON(network_thread_);
tiebreaker_ = tiebreaker;
}
void SetIceParameters(const IceParameters& ice_params) override {
RTC_DCHECK_RUN_ON(network_thread_);
ice_parameters_ = ice_params;
@ -398,6 +406,7 @@ class FakeIceTransport : public IceTransportInternal {
Candidates remote_candidates_ RTC_GUARDED_BY(network_thread_);
IceConfig ice_config_ RTC_GUARDED_BY(network_thread_);
IceRole role_ RTC_GUARDED_BY(network_thread_) = ICEROLE_UNKNOWN;
uint64_t tiebreaker_ RTC_GUARDED_BY(network_thread_) = 0;
IceParameters ice_parameters_ RTC_GUARDED_BY(network_thread_);
IceParameters remote_ice_parameters_ RTC_GUARDED_BY(network_thread_);
IceMode remote_ice_mode_ RTC_GUARDED_BY(network_thread_) = ICEMODE_FULL;

2
p2p/base/ice_transport_internal.h
View File

@ -254,6 +254,8 @@ class RTC_EXPORT IceTransportInternal : public rtc::PacketTransportInternal {
virtual void SetIceRole(IceRole role) = 0;
virtual void SetIceTiebreaker(uint64_t tiebreaker) = 0;
virtual void SetIceCredentials(absl::string_view ice_ufrag,
absl::string_view ice_pwd);

1
p2p/base/mock_ice_transport.h
View File

@ -57,6 +57,7 @@ class MockIceTransport : public IceTransportInternal {
const std::string& transport_name() const override { return transport_name_; }
int component() const override { return 0; }
void SetIceRole(IceRole role) override {}
void SetIceTiebreaker(uint64_t tiebreaker) override {}
// The ufrag and pwd in `ice_params` must be set
// before candidate gathering can start.
void SetIceParameters(const IceParameters& ice_params) override {}

14
p2p/base/p2p_transport_channel.cc
View File

@ -162,6 +162,7 @@ P2PTransportChannel::P2PTransportChannel(
error_(0),
remote_ice_mode_(ICEMODE_FULL),
ice_role_(ICEROLE_UNKNOWN),
ice_tiebreaker_(0),
gathering_state_(kIceGatheringNew),
weak_ping_interval_(GetWeakPingIntervalInFieldTrial(field_trials)),
config_(RECEIVING_TIMEOUT,
@ -315,6 +316,17 @@ IceRole P2PTransportChannel::GetIceRole() const {
return ice_role_;
}
void P2PTransportChannel::SetIceTiebreaker(uint64_t tiebreaker) {
RTC_DCHECK_RUN_ON(network_thread_);
if (!ports_.empty() || !pruned_ports_.empty()) {
RTC_LOG(LS_ERROR)
<< "Attempt to change tiebreaker after Port has been allocated.";
return;
}
ice_tiebreaker_ = tiebreaker;
}
IceTransportState P2PTransportChannel::GetState() const {
RTC_DCHECK_RUN_ON(network_thread_);
return state_;
@ -914,7 +926,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession* session,
// if one is pending.
port->SetIceRole(ice_role_);
port->SetIceTiebreaker(allocator_->ice_tiebreaker());
port->SetIceTiebreaker(ice_tiebreaker_);
ports_.push_back(port);
port->SignalUnknownAddress.connect(this,
&P2PTransportChannel::OnUnknownAddress);

2
p2p/base/p2p_transport_channel.h
View File

@ -128,6 +128,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal,
bool receiving() const override;
void SetIceRole(IceRole role) override;
IceRole GetIceRole() const override;
void SetIceTiebreaker(uint64_t tiebreaker) override;
void SetIceParameters(const IceParameters& ice_params) override;
void SetRemoteIceParameters(const IceParameters& ice_params) override;
void SetRemoteIceMode(IceMode mode) override;
@ -438,6 +439,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal,
RTC_GUARDED_BY(network_thread_);
IceMode remote_ice_mode_ RTC_GUARDED_BY(network_thread_);
IceRole ice_role_ RTC_GUARDED_BY(network_thread_);
uint64_t ice_tiebreaker_ RTC_GUARDED_BY(network_thread_);
IceGatheringState gathering_state_ RTC_GUARDED_BY(network_thread_);
std::unique_ptr<webrtc::BasicRegatheringController> regathering_controller_
RTC_GUARDED_BY(network_thread_);

72
p2p/base/p2p_transport_channel_unittest.cc
View File

@ -134,6 +134,9 @@ const cricket::IceParameters kIceParams[4] = {
{kIceUfrag[2], kIcePwd[2], false},
{kIceUfrag[3], kIcePwd[3], false}};
const uint64_t kLowTiebreaker = 11111;
const uint64_t kHighTiebreaker = 22222;
cricket::IceConfig CreateIceConfig(
int receiving_timeout,
cricket::ContinualGatheringPolicy continual_gathering_policy,
@ -374,6 +377,8 @@ class P2PTransportChannelTestBase : public ::testing::Test,
void SetIceRole(IceRole role) { role_ = role; }
IceRole ice_role() { return role_; }
void SetIceTiebreaker(uint64_t tiebreaker) { tiebreaker_ = tiebreaker; }
uint64_t GetIceTiebreaker() { return tiebreaker_; }
void OnRoleConflict(bool role_conflict) { role_conflict_ = role_conflict; }
bool role_conflict() { return role_conflict_; }
void SetAllocationStepDelay(uint32_t delay) {
@ -482,6 +487,7 @@ class P2PTransportChannelTestBase : public ::testing::Test,
channel->SetRemoteIceParameters(remote_ice);
}
channel->SetIceRole(GetEndpoint(endpoint)->ice_role());
channel->SetIceTiebreaker(GetEndpoint(endpoint)->GetIceTiebreaker());
return channel;
}
@ -556,6 +562,9 @@ class P2PTransportChannelTestBase : public ::testing::Test,
void SetIceRole(int endpoint, IceRole role) {
GetEndpoint(endpoint)->SetIceRole(role);
}
void SetIceTiebreaker(int endpoint, uint64_t tiebreaker) {
GetEndpoint(endpoint)->SetIceTiebreaker(tiebreaker);
}
bool GetRoleConflict(int endpoint) {
return GetEndpoint(endpoint)->role_conflict();
}
@ -769,6 +778,31 @@ class P2PTransportChannelTestBase : public ::testing::Test,
EXPECT_EQ(1u, RemoteCandidate(ep1_ch1())->generation());
}
void TestSignalRoleConflict() {
rtc::ScopedFakeClock clock;
// Default EP1 is in controlling state.
SetIceTiebreaker(0, kLowTiebreaker);
SetIceRole(1, ICEROLE_CONTROLLING);
SetIceTiebreaker(1, kHighTiebreaker);
// Creating channels with both channels role set to CONTROLLING.
CreateChannels();
// Since both the channels initiated with controlling state and channel2
// has higher tiebreaker value, channel1 should receive SignalRoleConflict.
EXPECT_TRUE_SIMULATED_WAIT(GetRoleConflict(0), kShortTimeout, clock);
EXPECT_FALSE(GetRoleConflict(1));
EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()),
kShortTimeout, clock);
EXPECT_TRUE(ep1_ch1()->selected_connection() &&
ep2_ch1()->selected_connection());
TestSendRecv(&clock);
DestroyChannels();
}
void TestPacketInfoIsSet(rtc::PacketInfo info) {
EXPECT_NE(info.packet_type, rtc::PacketType::kUnknown);
EXPECT_NE(info.protocol, rtc::PacketInfoProtocolType::kUnknown);
@ -1805,36 +1839,13 @@ TEST_F(P2PTransportChannelTest, TestTcpConnectionTcptypeSet) {
}
TEST_F(P2PTransportChannelTest, TestIceRoleConflict) {
rtc::ScopedFakeClock clock;
AddAddress(0, kPublicAddrs[0]);
AddAddress(1, kPublicAddrs[1]);
// Creating channels with both channels role set to CONTROLLING.
SetIceRole(0, ICEROLE_CONTROLLING);
SetIceRole(1, ICEROLE_CONTROLLING);
CreateChannels();
bool first_endpoint_has_lower_tiebreaker =
GetEndpoint(0)->allocator_->ice_tiebreaker() <
GetEndpoint(1)->allocator_->ice_tiebreaker();
// Since both the channels initiated with controlling state, the channel with
// the lower tiebreaker should receive SignalRoleConflict.
EXPECT_TRUE_SIMULATED_WAIT(
GetRoleConflict(first_endpoint_has_lower_tiebreaker ? 0 : 1),
kShortTimeout, clock);
EXPECT_FALSE(GetRoleConflict(first_endpoint_has_lower_tiebreaker ? 1 : 0));
EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()),
kShortTimeout, clock);
EXPECT_TRUE(ep1_ch1()->selected_connection() &&
ep2_ch1()->selected_connection());
TestSendRecv(&clock);
DestroyChannels();
TestSignalRoleConflict();
}
// Tests that the ice configs (protocol and role) can be passed down to ports.
// Tests that the ice configs (protocol, tiebreaker and role) can be passed
// down to ports.
TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) {
rtc::ScopedFakeClock clock;
AddAddress(0, kPublicAddrs[0]);
@ -1843,7 +1854,9 @@ TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) {
// Give the first connection the higher tiebreaker so its role won't
// change unless we tell it to.
SetIceRole(0, ICEROLE_CONTROLLING);
SetIceTiebreaker(0, kHighTiebreaker);
SetIceRole(1, ICEROLE_CONTROLLING);
SetIceTiebreaker(1, kLowTiebreaker);
CreateChannels();
@ -1852,13 +1865,18 @@ TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) {
const std::vector<PortInterface*> ports_before = ep1_ch1()->ports();
for (size_t i = 0; i < ports_before.size(); ++i) {
EXPECT_EQ(ICEROLE_CONTROLLING, ports_before[i]->GetIceRole());
EXPECT_EQ(kHighTiebreaker, ports_before[i]->IceTiebreaker());
}
ep1_ch1()->SetIceRole(ICEROLE_CONTROLLED);
ep1_ch1()->SetIceTiebreaker(kLowTiebreaker);
const std::vector<PortInterface*> ports_after = ep1_ch1()->ports();
for (size_t i = 0; i < ports_after.size(); ++i) {
EXPECT_EQ(ICEROLE_CONTROLLED, ports_before[i]->GetIceRole());
// SetIceTiebreaker after ports have been created will fail. So expect the
// original value.
EXPECT_EQ(kHighTiebreaker, ports_before[i]->IceTiebreaker());
}
EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()),
@ -2244,7 +2262,9 @@ TEST_F(P2PTransportChannelTest,
// With conflicting ICE roles, endpoint 1 has the higher tie breaker and will
// send a binding error response.
SetIceRole(0, ICEROLE_CONTROLLING);
SetIceTiebreaker(0, kHighTiebreaker);
SetIceRole(1, ICEROLE_CONTROLLING);
SetIceTiebreaker(1, kLowTiebreaker);
// We want the remote TURN candidate to show up as prflx. To do this we need
// to configure the server to accept packets from an address we haven't
// explicitly installed permission for.

2
pc/jsep_transport_controller.cc
View File

@ -58,6 +58,7 @@ JsepTransportController::JsepTransportController(
}),
config_(std::move(config)),
active_reset_srtp_params_(config.active_reset_srtp_params),
ice_tiebreaker_(port_allocator ? port_allocator->ice_tiebreaker() : 0),
bundles_(config.bundle_policy) {
// The `transport_observer` is assumed to be non-null.
RTC_DCHECK(config_.transport_observer);
@ -403,6 +404,7 @@ JsepTransportController::CreateIceTransport(const std::string& transport_name,
transport_name, component, std::move(init));
RTC_DCHECK(transport);
transport->internal()->SetIceRole(ice_role_);
transport->internal()->SetIceTiebreaker(ice_tiebreaker_);
transport->internal()->SetIceConfig(ice_config_);
return transport;
}

1
pc/jsep_transport_controller.h
View File

@ -507,6 +507,7 @@ class JsepTransportController : public sigslot::has_slots<> {
cricket::IceConfig ice_config_;
cricket::IceRole ice_role_ = cricket::ICEROLE_CONTROLLING;
uint64_t ice_tiebreaker_;
rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
BundleManager bundles_;