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

Reland "p2p: reduce visibility of ICE tiebreaker further"

Browse Source 
This reverts commit dbbb6cabc3d25faa61864ad1610bcfdd2cbedf23.

Reason for revert: Fixed downstream issues (in p2)

Original change's description:
> Revert "p2p: reduce visibility of ICE tiebreaker further"
>
> 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}

Bug: webrtc:42224914
Change-Id: Iea2678ef21aba990bc8b95e5275157c0dba5fa77
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/351661
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42378}
This commit is contained in:
Harald Alvestrand 2024-05-24 14:58:13 +00:00 committed by WebRTC LUCI CQ
parent 5d3e6805f2
commit 3214fbc725
9 changed files with 31 additions and 77 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,8 +92,6 @@ 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,10 +147,6 @@ 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_;
@ -212,10 +208,6 @@ 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;
@ -406,7 +398,6 @@ 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;

5
p2p/base/ice_transport_internal.h
View File

@ -254,7 +254,10 @@ class RTC_EXPORT IceTransportInternal : public rtc::PacketTransportInternal {
virtual void SetIceRole(IceRole role) = 0;
virtual void SetIceTiebreaker(uint64_t tiebreaker) = 0;
// Default implementation in order to allow downstream usage deletion.
// TODO: bugs.webrtc.org/42224914 - Remove when all downstream overrides are
// gone.
virtual void SetIceTiebreaker(uint64_t tiebreaker) { RTC_CHECK_NOTREACHED(); }
virtual void SetIceCredentials(absl::string_view ice_ufrag,
absl::string_view ice_pwd);

1
p2p/base/mock_ice_transport.h
View File

@ -57,7 +57,6 @@ 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,7 +162,6 @@ 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,
@ -316,17 +315,6 @@ 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_;
@ -926,7 +914,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession* session,
// if one is pending.
port->SetIceRole(ice_role_);
port->SetIceTiebreaker(ice_tiebreaker_);
port->SetIceTiebreaker(allocator_->ice_tiebreaker());
ports_.push_back(port);
port->SignalUnknownAddress.connect(this,
&P2PTransportChannel::OnUnknownAddress);

2
p2p/base/p2p_transport_channel.h
View File

@ -128,7 +128,6 @@ 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;
@ -439,7 +438,6 @@ 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,9 +134,6 @@ 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,
@ -377,8 +374,6 @@ 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) {
@ -487,7 +482,6 @@ class P2PTransportChannelTestBase : public ::testing::Test,
channel->SetRemoteIceParameters(remote_ice);
}
channel->SetIceRole(GetEndpoint(endpoint)->ice_role());
channel->SetIceTiebreaker(GetEndpoint(endpoint)->GetIceTiebreaker());
return channel;
}
@ -562,9 +556,6 @@ 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();
}
@ -778,31 +769,6 @@ 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);
@ -1839,13 +1805,36 @@ TEST_F(P2PTransportChannelTest, TestTcpConnectionTcptypeSet) {
}
TEST_F(P2PTransportChannelTest, TestIceRoleConflict) {
rtc::ScopedFakeClock clock;
AddAddress(0, kPublicAddrs[0]);
AddAddress(1, kPublicAddrs[1]);
TestSignalRoleConflict();
// 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();
}
// Tests that the ice configs (protocol, tiebreaker and role) can be passed
// down to ports.
// Tests that the ice configs (protocol and role) can be passed down to ports.
TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) {
rtc::ScopedFakeClock clock;
AddAddress(0, kPublicAddrs[0]);
@ -1854,9 +1843,7 @@ 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();
@ -1865,18 +1852,13 @@ 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()),
@ -2262,9 +2244,7 @@ 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,7 +58,6 @@ 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);
@ -404,7 +403,6 @@ 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,7 +507,6 @@ 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_;