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}
diff --git a/p2p/base/dtls_transport_unittest.cc b/p2p/base/dtls_transport_unittest.cc
index f870c1e..93653c1 100644
--- a/p2p/base/dtls_transport_unittest.cc
+++ b/p2p/base/dtls_transport_unittest.cc
@@ -92,6 +92,8 @@
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,
diff --git a/p2p/base/fake_ice_transport.h b/p2p/base/fake_ice_transport.h
index 788299b..285bfff 100644
--- a/p2p/base/fake_ice_transport.h
+++ b/p2p/base/fake_ice_transport.h
@@ -147,6 +147,10 @@
// 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 @@
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 @@
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;
diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h
index 48f12f9..46717cc 100644
--- a/p2p/base/ice_transport_internal.h
+++ b/p2p/base/ice_transport_internal.h
@@ -254,6 +254,8 @@
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);
diff --git a/p2p/base/mock_ice_transport.h b/p2p/base/mock_ice_transport.h
index b7b986e..ef6bdce 100644
--- a/p2p/base/mock_ice_transport.h
+++ b/p2p/base/mock_ice_transport.h
@@ -57,6 +57,7 @@
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 {}
diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index dcea5b0..241b423 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -162,6 +162,7 @@
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 @@
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 @@
// 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);
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index 2750af1..f7472df 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -128,6 +128,7 @@
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 @@
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_);
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc
index 29bdd99..79ca2a5 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -134,6 +134,9 @@
{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 @@
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 @@
channel->SetRemoteIceParameters(remote_ice);
}
channel->SetIceRole(GetEndpoint(endpoint)->ice_role());
+ channel->SetIceTiebreaker(GetEndpoint(endpoint)->GetIceTiebreaker());
return channel;
}
@@ -556,6 +562,9 @@
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 @@
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, 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 @@
// 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 @@
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 @@
// 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.
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 505ed50..7152826 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -58,6 +58,7 @@
}),
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 @@
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;
}
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index da0b921..cf2b411 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -507,6 +507,7 @@
cricket::IceConfig ice_config_;
cricket::IceRole ice_role_ = cricket::ICEROLE_CONTROLLING;
+ uint64_t ice_tiebreaker_;
rtc::scoped_refptr<rtc::RTCCertificate> certificate_;
BundleManager bundles_;
